refactor(cli): extract sandbox destroy action

[cv]

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
• Next PR: #2896 — refactor(cli): extract sandbox rebuild action

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

• 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

• npx prek run --all-files passes
• npm test passes
• Tests added or updated for new or changed behavior
• 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 (doc changes only)
• New doc pages include SPDX header and frontmatter (new pages only)

---

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

Summary by CodeRabbit

• Bug Fixes
  • Improved handling for sandbox deletion when the sandbox has already been removed.

[copy-pr-bot]

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.

[coderabbitai]

📝 Walkthrough

Walkthrough

Sandbox destruction logic is extracted from the runtime bridge interface and distributed across files into a dedicated sandbox-destroy-action.ts module that handles pre-deletion cleanup, registry updates, gateway teardown, and interactive confirmation. Dependent code is updated to use the new helpers.

Changes

Sandbox Destruction Extraction

Layer / File(s)	Summary
Interface Removal src/lib/nemoclaw-runtime-bridge.ts	NemoClawRuntimeBridge interface no longer declares sandboxDestroy method.
Core Destruction Implementation src/lib/sandbox-destroy-action.ts	New module implements sandbox destruction with service cleanup (NIM containers, Ollama models, stale proxies), outcome detection, registry/image removal, session state updates, and optional gateway teardown. Exports destroySandbox, getSandboxDeleteOutcome, removeSandboxImage, and removeSandboxRegistryEntry.
Runtime Action Delegation src/lib/sandbox-runtime-actions.ts	destroySandbox now delegates to the new action module via dynamic require instead of calling the removed bridge method.
Main Module Integration src/nemoclaw.ts	Imports sandbox-destroy helpers; removes in-file destruction implementations; updates sandboxRebuild to use removeSandboxRegistryEntry instead of inline cleanup.
Tests / Documentation test/image-cleanup.test.ts	Replaces regex-based file inspection with direct unit tests for the new helper functions, verifying image removal ordering, Docker invocation, and "sandbox not found" detection.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 Behold, destruction finds its home,
No bridge to cross, we carve our tome—
Clean helpers dance through docker dreams,
Where sandbox dust dissolves to schemes,
One module, tidy, rules supreme! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 14.29% 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 'refactor(cli): extract sandbox destroy action' is fully related to the main change, which extracts sandbox destruction logic into a dedicated module.
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 refactor/oclif-extract-sandbox-destroy-action

---

_{Review rate limit: 9/10 reviews remaining, refill in 6 minutes.}

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

[prekshivyas]

LGTM with growing scope-hygiene debt.

Extraction is correct. destroySandbox and its helpers move cleanly into src/lib/sandbox-destroy-action.ts. Public surface preserved (--yes/--force, confirmation prompt, exit codes, "already gone" / NIM / Ollama cleanup paths all byte-identical). The new removeSandboxRegistryEntry consolidates the image-then-registry ordering invariant that was previously duplicated at both destroy and rebuild sites — clean win.

Test rework is a strict improvement. test/image-cleanup.test.ts swaps source-shape regex assertions on src/nemoclaw.ts for actual functional tests via dependency injection (removeImage/removeSandbox/getSandbox/dockerRmi seams). Behavior coverage replaces structural coverage. Net +4 lines, real assertions.

But src/nemoclaw.ts is accumulating dead code across the stack. After this PR it carries:

New orphans introduced here (3):

• NEMOCLAW_GATEWAY_NAME (line 117) — only consumer was cleanupGatewayAfterLastSandbox, moved out
• DASHBOARD_FORWARD_PORT (line 118) — same
• dockerRemoveVolumesByPrefix (line 34) — same

Carryover orphans from prior stack PRs that should have been swept (7):

• parseForwardList (#2891)
• probeProviderHealth, buildStatusCommandDeps, GATEWAY_PORT, OLLAMA_PORT, recoverNamedGatewayRuntime, isErrnoException (all from #2893)

Plus two stale comments at lines 362 and 373 referencing sandboxDestroy() which no longer exists in this file.

Each individual PR's "while I'm here" cleanup feels out of scope, but the debt is compounding. Suggest either:

1. A dedicated cleanup PR at the end of this extraction phase, or
2. Letting later extractions opportunistically remove imports as their last consumers leave.

Not blocking — approving so you can keep moving.

Also: self-hosted e2e suite (build-sandbox-images, arm64, test-e2e-sandbox, etc.) was still in flight at review time. Worth confirming green on this hash before merging.

[coderabbitai]

🧹 Nitpick comments (1)

src/nemoclaw.ts (1)

390-390: Keep the lifecycle E2Es in the merge checklist.

This refactor now shares destroy cleanup between direct destroy and rebuild, so I'd still run the recommended sandbox lifecycle workflows on the branch before merging.

As per coding guidelines, "src/nemoclaw.ts: This file contains CLI dispatch, status, recovery, and connect functions. Changes affect sandbox lifecycle commands. E2E test recommendation: sandbox-survival-e2e — gateway restart recovery; sandbox-operations-e2e — process recovery after gateway kill."

🤖 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/nemoclaw.ts` at line 390, The refactor centralizes teardown
(removeSandboxRegistryEntry(sandboxName)) between direct destroy and rebuild, so
add a merge-checklist/action item and an inline comment near
removeSandboxRegistryEntry(sandboxName) to ensure the sandbox lifecycle E2Es are
retained and run before merging: explicitly list and run sandbox-survival-e2e
(gateway restart recovery) and sandbox-operations-e2e (process recovery after
gateway kill) to validate the shared cleanup path; update any project merge
checklist (or CI gating) to require these tests and mention the shared cleanup
in the comment so future changes remember to validate both destroy and rebuild
flows.

🤖 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/nemoclaw.ts`:
- Line 390: The refactor centralizes teardown
(removeSandboxRegistryEntry(sandboxName)) between direct destroy and rebuild, so
add a merge-checklist/action item and an inline comment near
removeSandboxRegistryEntry(sandboxName) to ensure the sandbox lifecycle E2Es are
retained and run before merging: explicitly list and run sandbox-survival-e2e
(gateway restart recovery) and sandbox-operations-e2e (process recovery after
gateway kill) to validate the shared cleanup path; update any project merge
checklist (or CI gating) to require these tests and mention the shared cleanup
in the comment so future changes remember to validate both destroy and rebuild
flows.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 15ab0845-de9a-4a0a-91be-06b80990be44

📥 Commits

Reviewing files that changed from the base of the PR and between 2ac6420 and c3c7f99.

📒 Files selected for processing (5)

• src/lib/nemoclaw-runtime-bridge.ts
• src/lib/sandbox-destroy-action.ts
• src/lib/sandbox-runtime-actions.ts
• src/nemoclaw.ts
• test/image-cleanup.test.ts

💤 Files with no reviewable changes (1)

• src/lib/nemoclaw-runtime-bridge.ts