test(destroy): cover sandbox destroy flow outcomes by cv · Pull Request #5476 · NVIDIA/NemoClaw

cv · 2026-06-15T21:37:20Z

Summary

Adds focused destroySandbox flow coverage for successful teardown and delete-failure safety behavior. The tests exercise gateway selection, live sandbox deletion, host cleanup, registry/session cleanup, gateway cleanup, and the fail-fast path before local state is removed.

Changes

Added src/lib/actions/sandbox/destroy-flow.test.ts with a dist-style harness for destroySandbox.
Covered successful sandbox destroy with NIM/proxy cleanup, OpenShell delete, host service cleanup, registry removal, shields cleanup, and optional gateway cleanup.
Covered failed OpenShell delete behavior to ensure local cleanup and registry removal are skipped.

Type of Change

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

Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes
Targeted tests pass for changed behavior
- npx vitest run src/lib/actions/sandbox/destroy-flow.test.ts --project cli
- npx vitest run src/lib/actions/sandbox/destroy-flow.test.ts test/destroy-cleanup-sandbox-services.test.ts --project cli
- npx @biomejs/biome lint src/lib/actions/sandbox/destroy-flow.test.ts
- npm run typecheck:cli
Full npm test passes (broad runtime changes only)
Tests added or updated for new or changed behavior
No secrets, API keys, or credentials committed
Docs updated for user-facing behavior changes
npm run docs builds without warnings (doc changes only)
Doc pages follow the style guide (doc changes only)
New doc pages include SPDX header and frontmatter (new pages only)

Note: commit and push hooks were skipped for this test-only branch because the broad local hook suite has unrelated pre-existing CLI failures in this checkout; targeted tests and CLI typecheck passed.

Signed-off-by: Carlos Villela cvillela@nvidia.com

Summary by CodeRabbit

Tests
- Added comprehensive test coverage for the sandbox destruction flow, validating the end-to-end success sequence (gateway selection, dependent services shutdown, OpenShell delete invocation, and cleanup) and error handling (propagating failure status and skipping cleanup steps when deletion fails).

coderabbitai · 2026-06-15T21:37:35Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 5666f142-4bb6-4d4b-81f9-7c7ef6aaba38

📥 Commits

Reviewing files that changed from the base of the PR and between d2486b9 and 0754caf.

📒 Files selected for processing (1)

src/lib/actions/sandbox/destroy-flow.test.ts

🚧 Files skipped from review as they are similar to previous changes (1)

src/lib/actions/sandbox/destroy-flow.test.ts

📝 Walkthrough

Walkthrough

Adds a new Vitest test file src/lib/actions/sandbox/destroy-flow.test.ts with 181 lines. It defines a createDestroyHarness helper that stubs OpenShell adapters, sandbox/registry/onboard state, and system operations, then asserts both the success-path sequence and the failure-path behavior of destroySandbox.

destroySandbox flow test suite

Layer / File(s)	Summary
Test harness and lifecycle hooks `src/lib/actions/sandbox/destroy-flow.test.ts`	`createDestroyHarness` clears the `require` cache for the dist destroy module, stubs `resolveOpenshell`, `runOpenshell`, `captureOpenshell`, mocks `getActiveSandboxSessions`, `getSandbox`, `listSandboxes`, `removeSandbox`, and onboard session load/update, and spies on gateway cleanup, NIM stop, stale proxy kill, Ollama unload, tunnel shutdown, and timer kill. Lifecycle hooks spy on `process.exit` before each test and restore all mocks and the module cache after each test.
Success and failure test cases `src/lib/actions/sandbox/destroy-flow.test.ts`	The success test asserts `destroySandbox` resolves and invokes gateway selection, NIM stop, stale proxy kill, OpenShell "sandbox delete" with `ignoreError: true`, Ollama model unload, registry removal, and host/gateway cleanup in order, emits the "Sandbox 'alpha' destroyed" log, and never calls `process.exit`. The failure test asserts `destroySandbox` rejects when OpenShell returns a nonzero status, skips registry removal and gateway cleanup, and calls `process.exit` with the failure code.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

area: sandbox, chore

Poem

🐇 Hop hop, the sandbox must fall,
The harness is built, it mocks them all.
OpenShell delete, NIM stopped with care,
Registry cleared and gateway laid bare.
On failure, exit code rings the bell—
Our bunny tests all, and tests it well! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'test(destroy): cover sandbox destroy flow outcomes' directly and specifically describes the main change: adding test coverage for the sandbox destruction flow with multiple outcome scenarios.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch test/destroy-flow-coverage

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-code-quality · 2026-06-15T21:38:07Z

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the test/destroy-flow-co... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.

File	`main`	`test/destroy-flow-co...` 0754caf	+/-
`nemoclaw/src/se...cret-scanner.ts`	—	100%	—
`nemoclaw/src/commands/slash.ts`	—	100%	—
`nemoclaw/src/li...bprocess-env.ts`	—	100%	—
`nemoclaw/src/bl...eprint/state.ts`	—	98%	—
`nemoclaw/src/onboard/config.ts`	—	98%	—
`nemoclaw/src/bl...int/snapshot.ts`	—	97%	—
`nemoclaw/src/bl...print/runner.ts`	—	95%	—
`nemoclaw/src/co...ration-state.ts`	—	94%	—
`nemoclaw/src/bl...ate-networks.ts`	—	94%	—
`nemoclaw/src/index.ts`	—	94%	—

TypeScript / code-coverage/cli

The overall coverage in the test/destroy-flow-co... branch is 45%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.

File	`main`	`test/destroy-flow-co...` 0754caf	+/-
`src/lib/state/o...oard-session.ts`	—	90%	—
`src/lib/inference/local.ts`	—	76%	—
`src/lib/sandbox/config.ts`	—	72%	—
`src/lib/onboard/preflight.ts`	—	64%	—
`src/lib/actions...dbox/rebuild.ts`	—	60%	—
`src/lib/state/sandbox.ts`	—	55%	—
`src/lib/actions...licy-channel.ts`	—	52%	—
`src/lib/onboard...er-gpu-patch.ts`	—	50%	—
`src/lib/policy/index.ts`	—	49%	—
`src/lib/onboard.ts`	—	17%	—

_{Updated June 15, 2026 21:58 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.}

github-actions · 2026-06-15T21:38:10Z

E2E Advisor Recommendation

Required E2E: None
Optional E2E: None

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

None. No E2E is recommended because this is a tests-only change adding unit/flow coverage for sandbox destroy behavior without changing runtime code or CI/E2E configuration.

Optional E2E

None.

New E2E recommendations

None.

github-actions · 2026-06-15T21:38:11Z

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: None
Optional Vitest E2E scenarios: None

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

None. No Vitest E2E scenario dispatch is required: the PR only adds a non-scenario Vitest test outside test/e2e-scenario/ and does not change the shared Vitest scenario workflow, registry, runtime support, fixtures, or live scenario machinery.

Optional Vitest E2E scenarios

None.

Relevant changed files

None.

github-actions · 2026-06-15T21:39:49Z

PR Review Advisor

Findings: 0 needs attention, 1 worth checking, 0 nice ideas
Since last review: 0 prior items resolved, 0 still apply, 1 new item found

Review findings

🛠️ Needs attention

None.

🔎 Worth checking

Mock filesystem cleanup before exercising the successful destroy path (src/lib/actions/sandbox/destroy-flow.test.ts:142): The successful-flow test calls the real compiled `destroySandbox`, and the harness does not replace the `fs.rmSync` calls used by `cleanupSandboxServices` and `cleanupShieldsDestroyArtifacts`. With sandbox name `alpha`, that path can attempt to remove `/tmp/nemoclaw-services-alpha` and `$HOME/.nemoclaw/state/shields-alpha.json` / `shields-timer-alpha.json` on the developer or CI host. Even with `force: true`, tests for sandbox lifecycle code should not depend on or mutate real host state.
- Recommendation: Spy/mock the `node:fs` `rmSync` used by the compiled destroy module, or run the SUT with a temporary HOME/state directory and assert the expected cleanup paths. Keep the OpenShell and registry mocks as-is, but make host filesystem deletion hermetic.
- Evidence: The test invokes `harness.destroySandbox("alpha", { yes: true, cleanupGateway: true })`; nearby production code in `destroy.ts` calls `fs.rmSync(`/tmp/nemoclaw-services-${sandboxName}`, ...)` and removes shields files under `resolveNemoclawStateDir()` during this success path.

🌱 Nice ideas

None.

Consider writing more tests for

**Mocked behavioral coverage** — Verify successful destroy performs sandbox service and shields cleanup through mocked `fs.rmSync` paths without touching the real `/tmp` or `$HOME/.nemoclaw/state`.. The behavioral coverage is appropriate for a test-only PR, but the success path should be made hermetic by mocking or redirecting filesystem cleanup before it exercises real host deletion calls.
**Mocked behavioral coverage** — Verify the delete-failure path does not call mocked filesystem cleanup before `process.exit(7)`.. The behavioral coverage is appropriate for a test-only PR, but the success path should be made hermetic by mocking or redirecting filesystem cleanup before it exercises real host deletion calls.
**Mock filesystem cleanup before exercising the successful destroy path** — Spy/mock the `node:fs` `rmSync` used by the compiled destroy module, or run the SUT with a temporary HOME/state directory and assert the expected cleanup paths. Keep the OpenShell and registry mocks as-is, but make host filesystem deletion hermetic.

Since last review details

Current findings:

Mock filesystem cleanup before exercising the successful destroy path (src/lib/actions/sandbox/destroy-flow.test.ts:142): The successful-flow test calls the real compiled `destroySandbox`, and the harness does not replace the `fs.rmSync` calls used by `cleanupSandboxServices` and `cleanupShieldsDestroyArtifacts`. With sandbox name `alpha`, that path can attempt to remove `/tmp/nemoclaw-services-alpha` and `$HOME/.nemoclaw/state/shields-alpha.json` / `shields-timer-alpha.json` on the developer or CI host. Even with `force: true`, tests for sandbox lifecycle code should not depend on or mutate real host state.
- Recommendation: Spy/mock the `node:fs` `rmSync` used by the compiled destroy module, or run the SUT with a temporary HOME/state directory and assert the expected cleanup paths. Keep the OpenShell and registry mocks as-is, but make host filesystem deletion hermetic.
- Evidence: The test invokes `harness.destroySandbox("alpha", { yes: true, cleanupGateway: true })`; nearby production code in `destroy.ts` calls `fs.rmSync(`/tmp/nemoclaw-services-${sandboxName}`, ...)` and removes shields files under `resolveNemoclawStateDir()` during this success path.

Workflow run details

This is an automated advisory review. A human maintainer must make the final merge decision.

coderabbitai

Actionable comments posted: 1

🤖 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/destroy-flow.test.ts`:
- Around line 76-82: The mock implementation for runOpenshellSpy returns an
object with an output property, but the getSandboxDeleteOutcome function expects
a SpawnLikeResult with stdout and stderr properties. Update the mock return
statements in the runOpenshellSpy implementation to return stdout and stderr
instead of output. Change the first return statement to return { status:
options.deleteStatus ?? 0, stdout: options.deleteOutput ?? "", stderr: "" } and
the fallback return statement to return { status: 0, stdout: "", stderr: "" }.

🪄 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: 84125866-888d-4da6-8309-b50fe3dcfe88

📥 Commits

Reviewing files that changed from the base of the PR and between 543bbd4 and d2486b9.

📒 Files selected for processing (1)

src/lib/actions/sandbox/destroy-flow.test.ts

cv · 2026-06-15T21:55:25Z

Addressed review feedback in the latest commit:

Updated the runOpenshell delete mock to return stdout/stderr rather than an output property, matching getSandboxDeleteOutcome's SpawnLikeResult contract.

Validation rerun:

npx vitest run src/lib/actions/sandbox/destroy-flow.test.ts test/destroy-cleanup-sandbox-services.test.ts --project cli
npm run typecheck:cli

test(destroy): cover sandbox destroy flow outcomes

d2486b9

cv self-assigned this Jun 15, 2026

coderabbitai Bot reviewed Jun 15, 2026

View reviewed changes

Comment thread src/lib/actions/sandbox/destroy-flow.test.ts

test(destroy): align delete mock shape

0754caf

cv added the v0.0.66 label Jun 15, 2026

cv merged commit a9a6eea into main Jun 15, 2026
44 checks passed

cv deleted the test/destroy-flow-coverage branch June 15, 2026 23:42

coderabbitai Bot mentioned this pull request Jul 10, 2026

fix(destroy): release final gateway on macOS #6597

Merged

21 tasks

wscurran added area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery chore Build, CI, dependency, or tooling maintenance labels Aug 1, 2026

This was referenced Aug 3, 2026

fix(sandbox): continue destroy when pre-delete shields hardening fails (#7727) #7886

Merged

fix(sandbox): report gateway authority migration instead of crashing #8118

Merged

fix(uninstall): treat an already-removed sandbox as removable state #8247

Merged

Uh oh!

Conversation

cv commented Jun 15, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Type of Change

Verification

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Suggested labels

Poem

❌ Failed checks (1 warning)

Uh oh!

github-code-quality Bot commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Overview

TypeScript / code-coverage/plugin

TypeScript / code-coverage/cli

Uh oh!

github-actions Bot commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

E2E Advisor Recommendation

E2E Recommendation Advisor

Required E2E

Optional E2E

New E2E recommendations

Uh oh!

github-actions Bot commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Vitest E2E Scenario Recommendation

Vitest E2E Scenario Advisor

Required Vitest E2E scenarios

Optional Vitest E2E scenarios

Relevant changed files

Uh oh!

github-actions Bot commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Advisor

🛠️ Needs attention

🔎 Worth checking

🌱 Nice ideas

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cv commented Jun 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cv commented Jun 15, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 15, 2026 •

edited

Loading

github-code-quality Bot commented Jun 15, 2026 •

edited

Loading

github-actions Bot commented Jun 15, 2026 •

edited

Loading

github-actions Bot commented Jun 15, 2026 •

edited

Loading

github-actions Bot commented Jun 15, 2026 •

edited

Loading