fix(destroy): preserve shared gateway by default; opt-in via --cleanup-gateway#3229
Conversation
…p-gateway
`nemoclaw <name> destroy --yes` on the only remaining sandbox previously
tore down the shared OpenShell gateway too, leaving the user to re-run
the full onboard before the next sandbox could come up.
Default behaviour now preserves the gateway when the last sandbox is
destroyed; users opt into the old teardown via `--cleanup-gateway`,
`NEMOCLAW_CLEANUP_GATEWAY=1`, or `{ cleanupGateway: true }`.
Prompt-mode handling:
- explicit `cleanupGateway` set -> honour without prompting
- non-interactive (`NEMOCLAW_NON_INTERACTIVE=1`) -> preserve
- `--yes` / `--force` -> preserve, surface hint
- interactive without `--yes` -> prompt separately
Closes #2166
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
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. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…ve-gateway-on-destroy Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Codex review on #3229 caught three follow-ups: 1. `test/cli.test.ts` still asserted the pre-#2166 last-sandbox destroy path called `forward stop 18789` and `gateway destroy -g nemoclaw`. Flip that test to assert preservation by default, and add two matching opt-in cases: - `alpha destroy -y --cleanup-gateway` exercises the flag path. - `alpha destroy -y` with `NEMOCLAW_CLEANUP_GATEWAY=1` in env exercises the env-var path. 2. `src/commands/sandbox/destroy.ts` (the legacy `nemoclaw <name> destroy` registration) advertised `[--yes|-y|--force]` only; extend to `[--yes|-y|--force] [--cleanup-gateway|--no-cleanup-gateway]` so the new switch is discoverable from `nemoclaw <name> destroy --help`. (The third Codex item — branch behind main — was resolved by the preceding merge commit on this branch, which kept the new `state/onboard-session` and `inference/ollama/proxy` import paths.) Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…x case (#2166) Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/commands/sandbox/destroy.ts`:
- Line 14: The usage string in destroy.ts's static usage currently lists only
"--cleanup-gateway" even though the flag is defined with allowNo: true and thus
supports "--no-cleanup-gateway"; update the static usage declaration (static
usage) to advertise both forms (e.g., include
"--cleanup-gateway|--no-cleanup-gateway") so callers see the no- form; locate
the flag definition that sets allowNo: true for the "--cleanup-gateway" option
and ensure the usage text matches that flag name.
In `@src/lib/domain/lifecycle/options.ts`:
- Around line 51-55: The current cleanupGateway assignment uses options.includes
which gives precedence to "--cleanup-gateway" even if "--no-cleanup-gateway"
appears later; change it to last-flag-wins by examining the positions of the two
flags (or iterating options from the end) and set cleanupGateway accordingly
using options, envCleanupGateway, and the
"--cleanup-gateway"/"--no-cleanup-gateway" tokens (refer to the cleanupGateway
variable and options array); ensure that if neither flag exists you fall back to
envCleanupGateway.
🪄 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: 56586b5e-48bc-4c6a-b9d8-e172fd9cad02
📒 Files selected for processing (7)
src/commands/sandbox/destroy.tssrc/lib/actions/sandbox/destroy.tssrc/lib/commands/sandbox/destroy.tssrc/lib/commands/sandbox/oclif-command-adapters.test.tssrc/lib/domain/lifecycle/options.test.tssrc/lib/domain/lifecycle/options.tstest/cli.test.ts
… --no- variant Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Reviewed current head 351fd6b against current main. CodeRabbit's two actionable items are addressed in this head: static usage advertises --cleanup-gateway|--no-cleanup-gateway, and cleanup-gateway argv parsing is last-flag-wins with env fallback only when neither flag appears.\n\nBehavior review: default last-sandbox destroy now preserves the shared gateway for confirmed paths such as --yes/--force and for the non-interactive gateway decision; explicit opt-in via --cleanup-gateway, NEMOCLAW_CLEANUP_GATEWAY=1, or API cleanupGateway: true still tears down the gateway. Existing shared-gateway cleanup predicates still gate the final cleanup path.\n\nValidated locally on a merge simulation with current origin/main: npm run build:cli; npx vitest run src/lib/domain/lifecycle/options.test.ts src/lib/commands/sandbox/oclif-command-adapters.test.ts test/cli.test.ts; git diff --check.
3 participants
Summary
nemoclaw <name> destroy --yeson the only remaining sandbox tore down the shared OpenShell gateway too, leaving the operator to re-run the full onboard before the next sandbox could come up.--yeswas intended to confirm the sandbox destruction, not the gateway.Default behaviour now preserves the gateway when the last sandbox is destroyed; the old teardown is opt-in via:
--cleanup-gatewayflag onnemoclaw <name> destroy(with--no-cleanup-gatewayto override an env-var default)NEMOCLAW_CLEANUP_GATEWAY=1environment variable{ cleanupGateway: true }in the typed options APIRelated Issue
Closes #2166
Changes
src/lib/domain/lifecycle/options.tscleanupGateway?: booleantoDestroySandboxOptions.NEMOCLAW_CLEANUP_GATEWAYenv (1/true/yesand inverse).src/lib/actions/sandbox/destroy.tscleanupGatewayAfterLastSandbox()on the new option viaresolveCleanupGatewayDecision().NEMOCLAW_NON_INTERACTIVE=1preserves the gateway;--yes/--forcepreserves with a one-line hint; interactive without--yesprompts separately.src/lib/commands/sandbox/destroy.ts--cleanup-gateway(withallowNo) and update usage / examples so it is discoverable from--help.src/lib/domain/lifecycle/options.test.ts— env-var, flag precedence, and value spelling cases.src/lib/commands/sandbox/oclif-command-adapters.test.ts— adapter threads the flag (both directions) through todestroySandbox.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
New Features
--cleanup-gatewayflag to sandbox destroy command to control whether the shared gateway is torn down during sandbox destruction.Bug Fixes
--cleanup-gatewayflag orNEMOCLAW_CLEANUP_GATEWAYenvironment variable.