feat(sandbox): add stop and start container lifecycle commands#6748
Conversation
NemoClaw had no way to temporarily free a sandbox's CPU/memory/GPU without destroying it: the action surface offered only destroy, and the deprecated global `nemoclaw stop` controls the tunnel, not the sandbox. OpenShell exposes no native sandbox stop/start verb, so the correct lever is the Docker adapter NemoClaw already owns. `nemoclaw <name> stop` gracefully stops OpenClaw-managed in-sandbox channels (agent-managed gateways shut down with the container's stop signal), then docker-stops every labeled container that is not at rest. Crash-looping `Restarting` containers are stopped too — docker stop is what disarms an armed restart policy — rather than being misreported as already stopped. Workspace volume, registry entry, OpenShell sandbox record, credentials, and the shared gateway/tunnel/NIM (gateway-scoped, serving other sandboxes) are all left untouched. `nemoclaw <name> start` reuses the #4423 docker-driver recovery module (stopped original, gpu-backup-sibling rename) plus a `docker unpause` branch for paused containers — which `docker ps` reports as `Up … (Paused)`, so the recovery classifier alone would no-op and `docker start` would fail — then delegates to the recover body so the gateway and host forwards return exactly as `recover` restores them. Both commands preflight with isDockerRuntimeDown so a daemon outage is reported with the shared #4428 guidance instead of being collapsed into "no container found — run rebuild", which is exactly the guidance that helper exists to prevent. Both gate on the direct-container drivers the privileged-exec path already allows (null/docker/vm) and refuse others. Routing, "Valid actions:", and shell completion derive automatically from the registered oclif commands; the display entries land next to recover, and the deprecated global stop/start shims now point users at the sandbox-scoped commands. Fixes #6026 Signed-off-by: Dongni Yang <dongniy@nvidia.com>
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds ChangesSandbox lifecycle controls
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the branch. TypeScript / code-coverage/cliThe overall coverage in the branch remains at 79%, unchanged from the branch. Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-6748.docs.buildwithfern.com/nemoclaw |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings 1 warning · 0 optional suggestionsWarningsThese merit maintainer attention but do not block by themselves.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/lib/actions/sandbox/stop.ts (1)
12-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider importing the adapter's real Docker types instead of a loose local shape.
DockerOpResult/DockerStopFnre-declare a subset ofdockerStop's actual signature (DockerRunOptions/DockerRunResultpersrc/lib/adapters/docker/container.ts). Since the lazy-require pattern only defers the runtime import, a type-only import (import type { DockerRunOptions, DockerRunResult } from "../../adapters/docker") costs nothing at runtime and keeps this file in sync if the adapter's option/result shape changes.♻️ Proposed refactor
-type DockerOpResult = { status?: number | null }; -type DockerStopFn = (name: string, opts?: Record<string, unknown>) => DockerOpResult; +import type { DockerRunOptions, DockerRunResult } from "../../adapters/docker/container"; +type DockerStopFn = (name: string, opts?: DockerRunOptions) => DockerRunResult;🤖 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/actions/sandbox/stop.ts` around lines 12 - 17, Replace the local DockerOpResult and DockerStopFn shapes in loadDockerStop with type-only imports of DockerRunOptions and DockerRunResult from the Docker adapter, and type the lazy-loaded dockerStop reference using that real adapter signature while preserving deferred runtime loading.src/commands/sandbox/start.test.ts (1)
20-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the
console.errorspy inafterEach, not inline.If an assertion above
error.mockRestore()throws, the spy leaks into later tests in this file.♻️ Proposed fix
+ let errorSpy: ReturnType<typeof vi.spyOn> | undefined; + afterEach(() => { process.exitCode = 0; + errorSpy?.mockRestore(); + errorSpy = undefined; }); it("propagates the action's failure exit code (`#6026`)", async () => { - const error = vi.spyOn(console, "error").mockImplementation(() => {}); + errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); startSandbox.mockResolvedValue({ exitCode: 1, message: " boom" }); await SandboxStartCommand.run(["alpha"], rootDir); expect(process.exitCode).toBe(1); - expect(error).toHaveBeenCalledWith(" boom"); - error.mockRestore(); + expect(errorSpy).toHaveBeenCalledWith(" boom"); });Also applies to: 31-40
🤖 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/commands/sandbox/start.test.ts` around lines 20 - 22, Move restoration of the console.error spy into the existing afterEach cleanup alongside resetting process.exitCode. Remove inline error.mockRestore() calls from the affected tests so cleanup still runs when assertions throw and the spy is restored for subsequent tests.
🤖 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 `@docs/reference/commands.mdx`:
- Line 1142: Update the stopSandbox documentation sentence to state that the
command supports both Docker-driver and vm-driver sandboxes, while retaining the
existing behavior description for unreachable Docker daemons.
In `@src/lib/actions/sandbox/stop.ts`:
- Around line 159-173: Update the dockerStop loop in the sandbox stop flow to
attempt every container in stoppable, rather than returning on the first
non-zero result. Collect each failed container and aggregate their
names/statuses into the final failure message, while preserving a successful
exit when all stops succeed and the existing never-remove behavior.
---
Nitpick comments:
In `@src/commands/sandbox/start.test.ts`:
- Around line 20-22: Move restoration of the console.error spy into the existing
afterEach cleanup alongside resetting process.exitCode. Remove inline
error.mockRestore() calls from the affected tests so cleanup still runs when
assertions throw and the spy is restored for subsequent tests.
In `@src/lib/actions/sandbox/stop.ts`:
- Around line 12-17: Replace the local DockerOpResult and DockerStopFn shapes in
loadDockerStop with type-only imports of DockerRunOptions and DockerRunResult
from the Docker adapter, and type the lazy-loaded dockerStop reference using
that real adapter signature while preserving deferred runtime loading.
🪄 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: 0e2bc15b-ac69-4ebe-ae80-ac2ce55c37bd
📒 Files selected for processing (15)
docs/manage-sandboxes/lifecycle.mdxdocs/reference/commands.mdxsrc/commands/sandbox/start.test.tssrc/commands/sandbox/start.tssrc/commands/sandbox/stop.test.tssrc/commands/sandbox/stop.tssrc/commands/start.tssrc/commands/stop.tssrc/lib/actions/sandbox/start.test.tssrc/lib/actions/sandbox/start.tssrc/lib/actions/sandbox/stop.test.tssrc/lib/actions/sandbox/stop.tssrc/lib/adapters/docker/container.tssrc/lib/cli/public-display-defaults.tstest/package-contract/cli/command-registry.test.ts
Addresses two CodeRabbit findings on #6748: - stopSandbox aborted on the first docker-stop failure, so a sandbox with more than one labeled container (e.g. a gpu-backup sibling) could leave the remaining containers running — the opposite of what stop is for. Attempt every stoppable container and aggregate failures into one message instead of returning on the first. - The stop command reference claimed it "requires a Docker-driver sandbox", but the driver gate also allows the vm driver (refusing only remote drivers like kubernetes). Reword to name both local-container drivers and the remote-driver exclusion. Refs #6026 Signed-off-by: Dongni Yang <dongniy@nvidia.com>
cjagwani
left a comment
There was a problem hiding this comment.
Reviewed exact head c9d01b1.
All required checks are green, contributor compliance passes, the trusted PR Review Advisor recommends merge_as_is, and CodeRabbit has no unresolved major or critical findings.
I also completed the nine-category security review. No vulnerability, credential leak, injection path, SSRF, authorization bypass, dependency risk, policy weakening, or unsafe destructive-state path was found. The advisor's retry-convergence coverage request is a non-blocking test hardening item: after a successful lifecycle mutation, retry discovery reaches the canonical running-container no-op path and probes again. Two additional low-risk follow-ups are worth tracking separately: prefer canonical-original selection before unpausing if a host already contains inconsistent original/backup siblings, and add a live stop/start persistence/isolation scenario.
<!-- markdownlint-disable MD041 --> ## Summary Release-prep documentation for v0.0.82 now summarizes user-facing changes merged since v0.0.81. It also closes stale wording in the stopped-sandbox backup, snapshot-clone, Ollama selection, and custom-policy authoring guidance. ## Changes - Add the `v0.0.82` section to `docs/about/release-notes.mdx` with links to the focused user guides. - Document that snapshot clones receive a destination-owned dashboard port before destructive replacement begins. - Align `backup-all` guidance with eligible stopped Docker-driver sandboxes that NemoClaw starts temporarily. - Describe the running and stopped Ollama menu states without claiming one fixed label. - Document runtime rejection of catch-all hosts in custom policy files. ### Source summary - [#6748](#6748) -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/lifecycle.mdx`, and `docs/reference/commands.mdx`: Summarize non-destructive sandbox `stop` and `start` commands. - [#6723](#6723) -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/backup-restore.mdx`, and `docs/reference/commands.mdx`: Record temporary startup and cleanup for eligible stopped-sandbox backups. - [#6749](#6749) -> `docs/about/release-notes.mdx` and `docs/manage-sandboxes/backup-restore.mdx`: Document destination-owned dashboard ports for snapshot clones. - [#6764](#6764) -> `docs/about/release-notes.mdx`: Summarize installer handling of route-only onboarding placeholders. - [#6771](#6771) -> `docs/about/release-notes.mdx`, `docs/inference/set-up-vllm.mdx`, `docs/inference/choose-inference-provider.mdx`, `docs/reference/commands.mdx`, and `docs/reference/platform-support.mdx`: Summarize managed-vLLM storage gates, immutable image digests, and the explicit override boundary. - [#6759](#6759) -> `docs/about/release-notes.mdx`: Record early, actionable OpenShell gateway-port conflict diagnostics. - [#6753](#6753) -> `docs/about/release-notes.mdx` and `docs/inference/set-up-ollama.mdx`: Document truthful running and stopped Ollama menu states. - [#6776](#6776) -> `docs/about/release-notes.mdx`: Summarize proxy-independent loopback readiness checks. - [#6769](#6769) -> `docs/about/release-notes.mdx`: Record compatible endpoint and agent guidance when Chat Completions is unavailable. - [#6730](#6730) -> `docs/about/release-notes.mdx`: Summarize bounded reuse of an eligible successful Chat Completions check. - [#6768](#6768) -> `docs/about/release-notes.mdx`: Record route-reservation repair during resumed onboarding. - [#6742](#6742) -> `docs/about/release-notes.mdx`: Summarize pre-mutation resolution of secret-free sandbox create intent. - [#6721](#6721) -> `docs/about/release-notes.mdx` and `docs/get-started/quickstart-langchain-deepagents-code.mdx`: Record bounded cleanup of completed managed Deep Agents headless sessions. - [#6731](#6731) -> `docs/about/release-notes.mdx` and `docs/network-policy/customize-network-policy.mdx`: Document runtime rejection of catch-all custom-policy destinations. - [#6729](#6729) -> `docs/about/release-notes.mdx` and `docs/get-started/prerequisites.mdx`: Record the Node.js 22.19 minimum. - [#6735](#6735) -> `docs/about/release-notes.mdx` and `docs/reference/platform-support.mdx`: Summarize the Ubuntu 26.04 userspace contract without claiming pending host or live validation. - [#6775](#6775) -> `docs/about/release-notes.mdx` and `docs/resources/community-contributions.mdx`: Route independent solutions outside canonical supported-product documentation. - [#6740](#6740) -> `docs/about/release-notes.mdx`: Summarize the semantic dependency-upgrade contributor workflow. - [#6777](#6777) -> `docs/about/release-notes.mdx` and `docs/CONTRIBUTING.md`: Summarize the route-safe documentation-refactor workflow. - [#6741](#6741) -> `docs/about/release-notes.mdx` and `docs/security/openclaw-2026.6.10-dependency-review.md`: Summarize reviewed npm archive verification and audit enforcement. - [#6739](#6739) -> `docs/about/release-notes.mdx` and `docs/security/openclaw-2026.6.10-dependency-review.md`: Record the locked offline dependency graph for the managed OpenClaw WeChat runtime. - [#6737](#6737) -> `docs/about/release-notes.mdx`: Record removal of the messaging build plan from final OpenClaw and Hermes image environments. - [#6733](#6733) -> `docs/about/release-notes.mdx`: Summarize cached plugin dependency layers for source and blueprint rebuilds. ### Skipped from docs-skip - None. No commit or changed path in `v0.0.81..origin/main` matched `openclaw-sandbox-permissive.yaml` or `config-show`, and the drafted content contains none of the configured skip terms. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: This is a documentation-only release-prep update; behavior is protected by the merged source PRs, and the documentation build validates the changed routes and agent variants. - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — tests are not applicable for this documentation-only change. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: not run for this documentation-only change. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — 0 errors; two pre-existing Fern warnings remain. - [x] 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) — no new pages. --- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated release notes with improvements to sandbox recovery, onboarding, session management, policy validation, storage checks, and system requirements. * Clarified Ollama setup instructions and status labels. * Documented safer snapshot restoration, including dedicated ports and protection against destructive failures. * Expanded `backup-all` coverage to include eligible stopped sandboxes. * Added guidance rejecting broad or catch-all network destinations in custom policies. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Summary
NemoClaw sandboxes could only be destroyed, never temporarily stopped: the action surface offered
connect/exec/rebuild/destroy/recover, and the deprecated globalnemoclaw stopcontrols the tunnel, not the sandbox container. This PR addsnemoclaw <name> stopandnemoclaw <name> start, which stop and restart the sandbox's Docker container while preserving the workspace volume, registry entry, OpenShell sandbox record, and credentials — so users can free CPU/memory/GPU without losing their workspace.Related Issue
Fixes #6026
Changes
sandbox:stopaction (src/lib/actions/sandbox/stop.ts): gracefully stops OpenClaw-managed in-sandbox channels via the existingstopSandboxChannels, thendocker stops every labeled container that is not at rest. Crash-loopingRestarting (N)containers are stopped too (docker stop is what disarms an armed restart policy) instead of being misreported as already stopped. Idempotent on already-stopped sandboxes. The shared host gateway, tunnel, and NIM container are gateway-scoped and deliberately untouched.sandbox:startaction (src/lib/actions/sandbox/start.ts): reuses the [DGX Spark][Sandbox] post-reboot first 'nemoclaw <name> status' destroys sandbox via "Removed stale local registry entry" #4423 docker-driver recovery module (stopped original, gpu-backup-sibling rename), adds adocker unpausebranch for paused containers (docker psreports them asUp … (Paused), so the recovery classifier alone would no-op whiledocker startfails), then delegates to therecoverbody (connectSandboxprobe) so the gateway and host forwards return exactly asrecoverrestores them.isDockerRuntimeDown/printDockerRuntimeDownGuidance([Ubuntu 24.04][CLI] Docker restart leaves sandbox stuck in Provisioning and connect times out #4428) so a Docker daemon outage is reported as a host runtime problem instead of "no container found — run rebuild". Both gate on the direct-container drivers the privileged-exec path already allows (null/docker/vm) and refuse others.dockerUnpauseprimitive insrc/lib/adapters/docker/container.ts, mirroringdockerStop/dockerStart(consumer: the paused-container branch instartSandbox; protected bysrc/lib/actions/sandbox/start.test.ts).src/commands/sandbox/stop.ts/start.ts(recover.ts pattern); routing,Valid actions:text, and shell completion derive automatically from the registered command IDs.public-display-defaults.tsnext torecover; package-contract counts updated (58→60 commands, 33→35 action tokens).stop/startdeprecation messages now point at the sandbox-scoped commands (the issue's Actual Result shows this exact confusion).docs/reference/commands.mdxsections for both commands (dump-commands parity gate), and a "Stop and Start a Sandbox" section indocs/manage-sandboxes/lifecycle.mdx.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project cli src/lib/actions/sandbox/stop.test.ts src/lib/actions/sandbox/start.test.ts src/commands/sandbox/stop.test.ts src/commands/sandbox/start.test.ts src/lib/adapters/docker/index.test.ts→ 52 passed;npm run build:cli && npx vitest run --project package-contract→ 302 passed;npm run typecheck:clicleannpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Dongni Yang dongniy@nvidia.com
Summary by CodeRabbit
New Features
nemoclaw <sandbox-name> stopandnemoclaw <sandbox-name> start.Documentation
Bug Fixes
Tests