Skip to content

refactor(cli): extract sandbox destroy helpers#2964

Merged
cv merged 65 commits intomainfrom
refactor/retry-sandbox-destroy-helpers
May 5, 2026
Merged

refactor(cli): extract sandbox destroy helpers#2964
cv merged 65 commits intomainfrom
refactor/retry-sandbox-destroy-helpers

Conversation

@cv
Copy link
Copy Markdown
Contributor

@cv cv commented May 4, 2026

Summary

Extract pure sandbox destroy decision helpers from the destructive destroy action module.

Stack Navigation

Changes

  • Added sandbox-destroy-helpers.ts for delete outcome parsing and cleanup decision helpers.
  • Updated destroy and rebuild actions to import delete outcome parsing from the helper module.
  • Moved/added helper tests for missing-sandbox output, delete outcomes, host-service cleanup decisions, and gateway cleanup decisions.

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

  • Refactor

    • Reorganized sandbox deletion utilities and result interpretation into a new shared helpers module.
    • Decoupled gateway cleanup and host services shutdown decision logic from primary deletion operations.
  • Tests

    • Added comprehensive test coverage for new sandbox destruction helper utilities.

cv added 30 commits May 2, 2026 13:36
@cv cv self-assigned this May 4, 2026
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented May 4, 2026

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
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 4, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: e4fe44d8-d8e8-4701-bf0f-175b5808ec20

📥 Commits

Reviewing files that changed from the base of the PR and between b34e5fd and 3c29e55.

📒 Files selected for processing (5)
  • src/lib/sandbox-destroy-action.ts
  • src/lib/sandbox-destroy-helpers.test.ts
  • src/lib/sandbox-destroy-helpers.ts
  • src/lib/sandbox-rebuild-action.ts
  • test/image-cleanup.test.ts

📝 Walkthrough

Walkthrough

This PR extracts sandbox destruction decision logic from sandbox-destroy-action.ts into a new sandbox-destroy-helpers.ts module, introducing reusable utility functions for interpreting deletion outcomes and determining when to clean up gateway and host services. Comprehensive tests are added for the new helpers, and dependent imports are updated accordingly.

Changes

Sandbox Destroy Logic Refactoring

Layer / File(s) Summary
New Helper Module
src/lib/sandbox-destroy-helpers.ts
Introduces SpawnLikeResult type, isMissingSandboxDeleteOutput(), getSandboxDeleteOutcome(), shouldStopHostServicesAfterDestroy(), and shouldCleanupGatewayAfterDestroy() to centralize delete-outcome interpretation and cleanup decision logic.
Test Coverage
src/lib/sandbox-destroy-helpers.test.ts
Comprehensive Vitest suite validating detection of missing sandbox outputs (including ANSI-colored variants), delete-outcome classification, and boolean decision logic for host services and gateway cleanup across multiple scenarios.
Main Action Refactoring
src/lib/sandbox-destroy-action.ts
Removes in-file SpawnLikeResult type and getSandboxDeleteOutcome() implementation; imports these from sandbox-destroy-helpers; replaces inline gateway-cleanup and host-services-shutdown logic with calls to shouldCleanupGatewayAfterDestroy() and shouldStopHostServicesAfterDestroy().
Import Updates
src/lib/sandbox-rebuild-action.ts, test/image-cleanup.test.ts
Updates imports to fetch getSandboxDeleteOutcome from sandbox-destroy-helpers instead of sandbox-destroy-action; removeSandboxRegistryEntry and removeSandboxImage remain imported from sandbox-destroy-action.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 A rabbit's refactoring delight,
Decision logic, clean and tight,
Helpers extracted, tested true,
Modular code through and through!
No more rabbitholes—just clarity bright.

✨ 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/retry-sandbox-destroy-helpers

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

@cv cv added the v0.0.34 Release target label May 4, 2026
@cv cv requested a review from cjagwani May 5, 2026 00:27
@cv cv added v0.0.35 Release target and removed v0.0.34 Release target labels May 5, 2026
@prekshivyas prekshivyas self-assigned this May 5, 2026
@cv cv requested a review from prekshivyas May 5, 2026 22:43
cv added a commit that referenced this pull request May 5, 2026
## Summary
Extract pure upgrade sandbox classification helpers from the
subprocess-heavy upgrade action module.

## Stack Navigation
- Position: 48 of 60
- Previous PR: [#2962 — refactor(cli): extract maintenance image
helpers](#2962)
- Next PR: [#2964 — refactor(cli): extract sandbox destroy
helpers](#2964)

## Changes
- Added `upgrade-sandboxes-helpers.ts` for confirmation bypass,
stale/unknown classification, and rebuildable/stopped splitting.
- Updated `upgradeSandboxes` to call the extracted helpers while keeping
OpenShell and rebuild orchestration in the action module.
- Added direct helper coverage for stale, unknown, current, running, and
stopped sandbox cases.

## 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

* **Refactor**
* Refactored the sandbox upgrade workflow by introducing modular helper
functions that improve code organization, maintainability, and
readability while preserving all existing behavior and functionality.

* **Tests**
* Added comprehensive test suite covering upgrade confirmation handling,
sandbox version classification, and the intelligent grouping of
sandboxes by their rebuild state and configuration.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
Base automatically changed from refactor/retry-upgrade-sandbox-helpers to main May 5, 2026 23:14
@cv cv marked this pull request as ready for review May 5, 2026 23:15
@cv cv enabled auto-merge (squash) May 5, 2026 23:15
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Copy link
Copy Markdown
Contributor

@prekshivyas prekshivyas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Same pattern as #2962/#2963 — pure helper extraction. 5 files / +147 / -34.

Four helpers in new sandbox-destroy-helpers.ts:

  • isMissingSandboxDeleteOutput (renamed from ...Result for clarity).
  • getSandboxDeleteOutcome (moved out of sandbox-destroy-action.ts).
  • shouldStopHostServicesAfterDestroy — wraps the inline "only sandbox + still registered" gate.
  • shouldCleanupGatewayAfterDestroy — wraps the four-clause final-gateway-cleanup gate.

Inline boolean blobs become named helper calls with structured input objects — strictly more readable, behavior identical. Import paths in sandbox-rebuild-action.ts and test/image-cleanup.test.ts migrated to the new module — no zombie imports.

+70 lines of direct unit tests covering ANSI stripping, three delete-outcome cases, and both decision helpers. Existing test count not reduced (1 line touched in image-cleanup.test.ts is just the import path).

CI: pr.yaml mostly green (lint/dco/check-hash/legacy-path-guard/macos-e2e/changes PASS); wsl-e2e/checks + pr-self-hosted builds still in flight at review time. No failures.

@cv cv merged commit 6943547 into main May 5, 2026
12 checks passed
@prekshivyas prekshivyas deleted the refactor/retry-sandbox-destroy-helpers branch May 5, 2026 23:19
cv added a commit that referenced this pull request May 5, 2026
## Summary
Extract pure sandbox log option and argv helpers from the log streaming
action module.

## Stack Navigation
- Position: 50 of 60
- Previous PR: [#2964 — refactor(cli): extract sandbox destroy
helpers](#2964)
- Next PR: [#2966 — refactor(cli): extract policy channel
helpers](#2966)

## Changes
- Added `sandbox-logs-helpers.ts` for option normalization, timeout
parsing, probe descriptions, signal exit codes, and OpenShell argv
builders.
- Updated `sandbox-logs-action.ts` to import the helper logic and focus
on subprocess orchestration.
- Moved helper tests to target the extracted helper module directly.

## 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>

---------

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v0.0.35 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants