fix(status): recommend start for crashed-sandbox recovery (#7222) - #7225
Conversation
When a sandbox container crashes (Exited 137), the Error-phase recovery hint recommended `rebuild --yes`, promising "workspace state will be preserved." That command aborts: the pre-rebuild backup cannot snapshot a stopped container, so the backup gate refuses to proceed and leaves the sandbox in Error, pushing the user toward `rebuild --force` (which discards workspace state). The command that actually recovers a crashed sandbox without data loss is `<sandbox> start`, which restarts the stopped container and brings its gateway back up — but it was never surfaced. Steer the crashed-container hint (container present, terminal phase) at `start` in both places it is printed: the `status` renderer (status-lookup-rendering.ts) and the shared non-Ready-phase guidance (gateway-state.ts). `rebuild --yes` is kept only as the recreate alternative, with a note that its pre-rebuild backup cannot snapshot a stopped container, so `start` must come first. The `missing`-container branch (where `start` cannot help because the container is gone) keeps the rebuild guidance. Fixes #7222 Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSandbox recovery guidance now distinguishes paused, crashed, and unrecoverable containers, recommending ChangesSandbox recovery guidance
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant SandboxGateway
participant DockerRuntime
participant Operator
SandboxGateway->>DockerRuntime: inspect Error-phase container
DockerRuntime-->>SandboxGateway: return paused or exited container metadata
SandboxGateway->>Operator: print docker unpause, start, or rebuild command
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 9ac77e6 in the TypeScript / code-coverage/cliThe overall coverage in commit 9ac77e6 in the Show a code coverage summary of the most impacted files.
Updated |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/actions/sandbox/status-lookup-rendering.test.ts (1)
19-25: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a present, non-paused runtime fixture for the crashed case.
dockerRuntime: nulldoes not establish the comment’s “container present but not paused” precondition; it exercises the generic no-runtime input. A regression affecting an actually present stopped container could therefore pass this test. Supply a typed fixture withpaused: falseand a container name, keeping a separate null-runtime test only if that behavior is intentional.As per path instructions, tests under
**/*.test.tsmust provide behavioral confidence for the claimed scenario.🤖 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/status-lookup-rendering.test.ts` around lines 19 - 25, Update the crashed-sandbox test for printNonReadySandboxPhaseGuidance to pass a typed dockerRuntime fixture representing a present, non-paused container, including paused: false and a container name. Preserve or add a separate null-runtime test only if generic no-runtime behavior is intentionally covered.Source: Path instructions
🤖 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/actions/sandbox/gateway-state.ts`:
- Around line 592-595: Update ensureLiveSandboxOrExit to detect paused Docker
containers before emitting the generic start/rebuild guidance, and reuse the
existing paused-container guidance helper or runtime query used by
status-lookup-rendering.ts. Preserve the docker unpause instructions for paused
containers, and add a regression test covering a paused container reporting
Phase: Error through this entrypoint.
---
Nitpick comments:
In `@src/lib/actions/sandbox/status-lookup-rendering.test.ts`:
- Around line 19-25: Update the crashed-sandbox test for
printNonReadySandboxPhaseGuidance to pass a typed dockerRuntime fixture
representing a present, non-paused container, including paused: false and a
container name. Preserve or add a separate null-runtime test only if generic
no-runtime behavior is intentionally covered.
🪄 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: 327cf876-0d3d-4d32-acdb-f56891eea294
📒 Files selected for processing (3)
src/lib/actions/sandbox/gateway-state.tssrc/lib/actions/sandbox/status-lookup-rendering.test.tssrc/lib/actions/sandbox/status-lookup-rendering.ts
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/actions/sandbox/gateway-state-hints.test.ts (1)
168-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the recovery outcome rather than the helper call.
The guidance output and exit already prove this behavior; the spy assertion unnecessarily locks the test to
getSandboxDockerRuntime.As per path instructions, tests should “prefer observable outcomes through the public boundary over ... mock-call assertions.”
🤖 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/gateway-state-hints.test.ts` at line 168, Update the test around the recovery scenario to remove the getSandboxDockerRuntimeSpy call assertion and retain assertions on the observable guidance output and exit behavior, using the public boundary rather than mocking implementation details.Source: Path instructions
🤖 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.
Nitpick comments:
In `@src/lib/actions/sandbox/gateway-state-hints.test.ts`:
- Line 168: Update the test around the recovery scenario to remove the
getSandboxDockerRuntimeSpy call assertion and retain assertions on the
observable guidance output and exit behavior, using the public boundary rather
than mocking implementation details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 03148d10-8495-4e16-bee3-799c2b6f5df6
📒 Files selected for processing (3)
src/lib/actions/sandbox/gateway-state-hints.test.tssrc/lib/actions/sandbox/gateway-state.tssrc/lib/actions/sandbox/status-lookup-rendering.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/actions/sandbox/status-lookup-rendering.test.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Maintainer follow-up on exact head 9a69741: I inspected the getSandboxDockerRuntime contract flagged by the Nemotron warning. Container resolution catches registry errors; missing/non-Docker containers return paused=false with no container name; Docker health/paused inspect errors are caught; and unrecognized paused output normalizes to false, so this path fails closed rather than inventing an unpause hint. Exact-head focused validation including docker-health, gateway-state-hints, and status-lookup-rendering passed 37/37 tests. No code change is needed for that warning; the advisor job itself failed on its internal canonical-ledger consistency check. |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
ericksoa
left a comment
There was a problem hiding this comment.
Approved on exact head 26ee70d against current main a5ccdca. Reviewed and remediated the crashed-versus-missing sandbox guidance and exited-container detection paths. Focused local validation: 56/56 tests plus CLI build/typecheck. Both review-advisor passes reported no findings, CodeRabbit has no unresolved major/critical findings, all current CI checks and the selected onboard-repair/onboard-resume E2E jobs are green.
|
🌿 Preview your docs: https://nvidia-preview-pr-7225.docs.buildwithfern.com/nemoclaw |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
✨ Related open issues: |
cjagwani
left a comment
There was a problem hiding this comment.
Approved exact head 9ac77e6 on current main 5b547cd. The current recovery recommendation preserves label-based ownership, skips Docker discovery for non-Docker sandboxes, and is covered by 44 focused tests. All 52 current checks and the selected onboard-repair and onboard-resume E2E targets passed; CodeRabbit and the canonical Advisor review are clean; all commits are GitHub Verified.
<!-- markdownlint-disable MD041 --> ## Summary Add the canonical dated changelog entry for NemoClaw `v0.0.89` before the release plan captures the tag commit. The entry summarizes the user-visible Station preparation, inference, recovery, policy-disclosure, and CLI-containment changes merged since `v0.0.88`. ## Changes - Add `docs/changelog/2026-07-20.mdx` with the exact `## v0.0.89` release heading, parser-safe SPDX comment, summary, and detailed bullets. - Link each shipped theme to the most specific published OpenClaw documentation routes. - Keep internal E2E, advisory-registry, and review-workflow refactors out of the user-facing release summary. Source summary: - #7214, #7241, #7237, #7223, #7204, #7202, #7183, and #7090 -> `docs/changelog/2026-07-20.mdx`: Summarize qualified DGX Station identity, package-state, PackageKit, DKMS, and reboot-handoff fixes. - #7242, #7221, #7186, #7164, and #6874 -> `docs/changelog/2026-07-20.mdx`: Summarize inference endpoint provenance, provider attachment, managed vLLM cleanup and progress, and Ollama selection guidance. - #7225, #7216, #7192, #7136, #7096, and #6910 -> `docs/changelog/2026-07-20.mdx`: Summarize sandbox readiness, recovery guidance, rebuilt model routing, durable Hermes state, and gateway restart behavior. - #7187 -> `docs/changelog/2026-07-20.mdx`: Summarize complete effective messaging-preset egress disclosure before policy mutation. - #7218, #7165, and #7184 -> `docs/changelog/2026-07-20.mdx`: Summarize structured output containment, terminal-safe route display, and Hermes forward cleanup. ## 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 PR adds release-history prose only; the focused changelog contract test validates its required structure and routes. - [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 a `Signed-off-by:` line 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 — `mise exec node@22.23.1 -- npx vitest run test/changelog-docs.test.ts` (6 passed). - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — not applicable to this doc-only release entry. - [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) — `mise exec node@22.23.1 -- npm run docs` completed with 0 errors and 2 existing site-wide warnings. - [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) — the native changelog entry uses the required parser-safe MDX SPDX comment and does not use frontmatter. --- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Expanded DGX Station installation support for qualified GB300 and OTA-upgraded environments. * Preserved selected inference providers, endpoints, model pins, and tuning settings during sandbox creation and rebuilds. * Improved sandbox recovery by validating availability and stability before restarting services. * Added clearer policy output showing the complete effective messaging egress configuration. * **Bug Fixes** * Hardened status and inference route displays by sanitizing terminal control characters. * Improved Hermes uninstall behavior by stopping detached dashboard forwarding. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Add the canonical `## v0.0.90` entry to `docs/changelog/2026-07-20.mdx` before the release tag is planned. The update also corrects the documented custom-image migration window so the compatibility fallback that first ships in v0.0.90 remains available until v0.0.91. ## Changes - Add the v0.0.90 summary and detailed release bullets for managed-image routing, provider-reset recovery, WhatsApp health reporting, and DGX Station guidance. - Keep the newest release first in the shared dated changelog and use root-absolute links to the canonical OpenClaw routes. - Correct `docs/reference/commands.mdx` to state that the legacy image route selector remains supported through v0.0.90 and is removed in v0.0.91. - Release source summary: - [#7264](#7264) -> `docs/resources/prompt-assets/dgx-station.md`, `docs/changelog/2026-07-20.mdx`: Record the versioned Station installer path, Nemotron 3 Ultra 550B default, and explicit DeepSeek override. - [#7261](#7261) -> `docs/get-started/dgx-station-preparation.mdx`, `docs/manage-sandboxes/recover-rebuild-sandboxes.mdx`, `docs/changelog/2026-07-20.mdx`: Include the OpenIB, legacy recovery, and Additional Setup documentation follow-ups. - [#7232](#7232) -> `docs/changelog/2026-07-20.mdx`: Document provider-reset recovery for wrapped OpenShell attachment diagnostics. - [#7189](#7189) -> `docs/reference/commands.mdx`, `docs/changelog/2026-07-20.mdx`: Document the managed-image route-selector rename and correct its one-release migration window. - [#7015](#7015) -> `docs/changelog/2026-07-20.mdx`: Document corrected OpenClaw WhatsApp health reporting. - No additional user-facing page update is needed for [#7193](#7193), [#7110](#7110), [#6783](#6783), or [#7263](#7263) because they change contributor governance, internal CI or release automation, or editorial style without changing supported user behavior. - [#7242](#7242) and [#7225](#7225) are already ancestors of and documented in v0.0.89, so this entry does not duplicate them despite their stale v0.0.90 labels. ## 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 - [x] Existing tests cover changed behavior — justification: `test/changelog-docs.test.ts` validates the dated changelog heading, SPDX form, version order, and published links. - [ ] Tests not applicable — justification: - [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: ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable; this PR does not change `scripts/prepare-dgx-station-host.sh` or runtime behavior. - Station profile/scenario: Not applicable. - Result: Not applicable. - Supporting evidence: Not applicable. ## Verification - [x] PR description includes a `Signed-off-by:` line 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 — `npx vitest run test/changelog-docs.test.ts` (6 passed). - [ ] 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; this is a focused 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) — passed with 0 errors and two unrelated baseline warnings for unauthenticated redirect checks and the existing light-mode contrast ratio. - [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) --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Documentation** - Added release notes for v0.0.90 covering inference routing, credential reset behavior, WhatsApp status detection, and DGX Station coding-agent guidance. - Updated custom Dockerfile guidance to document continued support for the legacy provider argument through v0.0.90. - Clarified that legacy declarations must be renamed to `NEMOCLAW_INFERENCE_PROVIDER_ID` before v0.0.91. - Added and refreshed related documentation links. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Summary
When a sandbox container crashes (Exited 137), the Error-phase recovery hint recommended
rebuild --yes("workspace state will be preserved"), but that command aborts on the pre-rebuild backup of the dead container and pushes the user towardrebuild --force(which discards state). The command that actually recovers the sandbox without data loss —<sandbox> start— was never surfaced. This PR points the crashed-container hint atstart.Closes #7222.
Reproduction
On our Ubuntu 24.04 x86_64 test host (no GPU). The reporter is on DGX Station aarch64; the crashed-sandbox recovery hint is CLI logic and is arch/GPU-independent.
Phase: Ready.nemoclaw <sb> exec -- bash -c 'echo MARK > /sandbox/marker.txt'.docker kill <the openshell-<sb>-... container>(→ Exited 137).nemoclaw <sb> status— observe the recovery hint.nemoclaw <sb> start.Observed on
main(before fix)Observed on
fix/...(after fix)Analysis
For a crashed sandbox the container is still present but its phase is terminal (
Error). Two places print an identical recovery hint recommendingrebuild --yes: thestatusrendererprintNonReadySandboxPhaseGuidance(src/lib/actions/sandbox/status-lookup-rendering.ts) and the shared non-Ready-phase guidance inensureLiveSandboxOrExit(src/lib/actions/sandbox/gateway-state.ts). Butrebuildfirst snapshots the sandbox, and the pre-rebuild backup cannot snapshot a stopped container (openclaw.jsonbackup fails), so the backup gate aborts to prevent data loss.startSandbox(src/lib/actions/sandbox/start.ts) is the crashed-container recovery path — it restarts the stopped container (or unpauses a paused one) and brings the gateway back up "exactly asrecover", preserving/sandbox. So the promised recovery command could not deliver, while the working one was hidden.Fix
Point the crashed-container hint (container present + terminal phase) at
nemoclaw <sb> startin both print sites.rebuild --yesis kept only as the recreate alternative, with a note that its pre-rebuild backup cannot snapshot a stopped container sostartmust come first. Themissing-container branch ingateway-state.ts— wherestartcannot help because the container is gone — deliberately keeps the rebuild/onboard guidance. The paused-container branch (#4495,docker unpause) is unchanged.Tests (
status-lookup-rendering.test.ts) pin the contract: a crashed (Error, non-paused) sandbox is steered tostartand no longer to therebuild --yes ... workspace state will be preservedpromise; the paused branch still recommendsdocker unpauseand neverstart/rebuild; a Ready sandbox prints no guidance.Changes
src/lib/actions/sandbox/status-lookup-rendering.ts: crashed-containerstatushint recommendsstart; export the guidance helper for testing.src/lib/actions/sandbox/gateway-state.ts: same hint change in the shared non-Ready-phase guidance.src/lib/actions/sandbox/status-lookup-rendering.test.ts: new — pins the hint contract and its boundaries.Type of Change
Verification
status-lookup-rendering.test.ts, 3 tests)npm run typecheck:cliandnpm run build:clipassrebuild --yes(which aborts on the dead container); after the fix it recommendsstart, which recovers toPhase: Readywith the workspace marker preserved.AI Disclosure
Signed-off-by: Yanyun Liao yanyunl@nvidia.com
Summary by CodeRabbit
Bug Fixes
Errorcases using Docker runtime state when available.startto preserve workspace state.rebuild --yescaveat when backups can’t snapshot stopped containers, and addeddocker unpause <containerName>instructions for paused containers.Tests
Error, paused,Failed, andReadyscenarios.