Skip to content

fix(sandbox): let destroy --force clean up when the OpenShell gateway is down (#6046) - #6050

Merged
cv merged 4 commits into
mainfrom
fix/6046-destroy-force-gateway-down
Jul 3, 2026
Merged

fix(sandbox): let destroy --force clean up when the OpenShell gateway is down (#6046)#6050
cv merged 4 commits into
mainfrom
fix/6046-destroy-force-gateway-down

Conversation

@jason-ma-nv

@jason-ma-nv jason-ma-nv commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

When the OpenShell gateway is not listening on 127.0.0.1:8080, every gateway call — including the final sandbox delete — returns a connection-refused/transport error. destroy treated that as fatal with no bypass (neither --force nor --yes helped, since both only skip the confirmation prompt), so there was no supported way to remove a sandbox while the gateway was down. This makes --force fall back to local cleanup.

Related Issue

Fixes #6046

Changes

  • src/lib/domain/sandbox/destroy.ts: add isGatewayUnreachableDeleteOutput() and surface gatewayUnreachable from getSandboxDeleteOutcome() — classifying gateway-transport failures (connection refused / os error 61|111 / tcp connect error / …) separately from real delete rejections.
  • src/lib/actions/sandbox/destroy.ts:
    • With --force + gateway-unreachable: fall back to local cleanup (remove the registry entry and local artifacts) with a clear warning that the sandbox may still exist if the gateway returns. Gateway teardown is intentionally skipped (the gateway-side delete was not confirmed).
    • Without --force: still fails, but now points at the recovery paths (<name> status to start the gateway, or --force).
    • Real (non-transport) delete errors stay fatal, unchanged.
  • Tests: domain classification unit tests; CLI E2E (destroy --force removes the local record when the fake gateway delete returns connection-refused; destroy -y without --force fails with the recovery hint and preserves the registry entry).

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)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: --force is already documented for destroy; this extends its effect (gateway-down fallback) without a new flag/command surface.
  • 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: sandbox-destroy/data-lifecycle path. The fallback is gated behind explicit --force AND a gateway-transport classification; it only removes the local record (no gateway teardown), warns that the sandbox may persist, and leaves real delete errors fatal. The pre-existing alreadyGone / real-error paths are unchanged and still covered (37 destroy/rebuild tests pass).
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes
  • Targeted tests pass for changed behavior
  • Full npm test passes (broad runtime changes only)
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • 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)

Signed-off-by: Jason Ma jama@nvidia.com

Summary by CodeRabbit

  • Bug Fixes
    • Improved sandbox delete handling when the remote gateway is unreachable by detecting connection/transport failures and surfacing that outcome to callers.
    • alpha destroy --force now safely performs local cleanup even when gateway-side deletion can’t be reached, while avoiding shared host teardown.
    • Non-forced runs now provide clearer recovery messaging, including guidance to retry with --force when the gateway is unavailable.
    • Warning text now clarifies that local sandbox removal happened without confirming gateway-side deletion.
  • Tests
    • Added CLI regression coverage for the gateway-unreachable alpha destroy scenario.

… is down (#6046)

When the gateway is not listening, every gateway call (including the final sandbox delete) returns a connection-refused/transport error. destroy treated that as fatal with no bypass, so a sandbox could not be removed while the gateway was down — unlike status/doctor, which auto-start it.

Classify gateway-transport delete failures (isGatewayUnreachableDeleteOutput) separately from real rejections. Under --force, fall back to local cleanup (remove the registry entry/local artifacts) with a clear warning that the sandbox may persist if the gateway returns; gateway teardown is skipped since the delete was not confirmed. Without --force, destroy still fails but now points at starting the gateway or re-running with --force. Real (non-transport) delete errors stay fatal.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Jason Ma <jama@nvidia.com>
@jason-ma-nv jason-ma-nv self-assigned this Jun 30, 2026
@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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: 4d73114c-481f-4e5e-884d-84476f99b134

📥 Commits

Reviewing files that changed from the base of the PR and between 4c79215 and a2a23a1.

📒 Files selected for processing (3)
  • src/lib/actions/sandbox/destroy-flow.test.ts
  • src/lib/actions/sandbox/destroy.ts
  • src/lib/actions/sandbox/snapshot.test.ts
💤 Files with no reviewable changes (3)
  • src/lib/actions/sandbox/snapshot.test.ts
  • src/lib/actions/sandbox/destroy-flow.test.ts
  • src/lib/actions/sandbox/destroy.ts

📝 Walkthrough

Walkthrough

Adds gateway-unreachable detection to sandbox delete outcomes and updates destroy so --force can complete local cleanup when the gateway is down. Tests cover the new outcome classification, destroy flow, and CLI behavior.

Changes

Gateway-Unreachable Destroy Handling

Layer / File(s) Summary
Delete outcome classification
src/lib/domain/sandbox/destroy.ts, src/lib/domain/sandbox/destroy.test.ts
Adds isGatewayUnreachableDeleteOutput and extends getSandboxDeleteOutcome to return gatewayUnreachable; unit tests cover the helper and the new outcome shape.
Forced local cleanup path
src/lib/actions/sandbox/destroy.ts
Consumes gatewayUnreachable, adds forcedLocalCleanup for --force when the gateway is unreachable, and changes failure handling to warn and continue local cleanup instead of exiting.
CLI and flow coverage
test/cli/destroy-gateway-unreachable.test.ts, src/lib/actions/sandbox/destroy-flow.test.ts, src/lib/actions/sandbox/snapshot.test.ts, test/image-cleanup.test.ts
Adds CLI regression coverage for gateway-down delete behavior, updates the destroy harness for last-sandbox cleanup assertions, and adjusts mocks/assertions to include gatewayUnreachable: false.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant destroySandbox
  participant getSandboxDeleteOutcome
  participant registry
  participant tunnelServices
  CLI->>destroySandbox: destroy --force
  destroySandbox->>getSandboxDeleteOutcome: inspect openshell delete result
  getSandboxDeleteOutcome-->>destroySandbox: gatewayUnreachable=true
  destroySandbox->>registry: remove local sandbox record
  destroySandbox->>tunnelServices: skip shared teardown on forcedLocalCleanup
  destroySandbox-->>CLI: success or failure with guidance
Loading

Suggested reviewers: cjagwani, ericksoa

🚥 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 clearly states the sandbox destroy --force fix for gateway-down cleanup and matches the main change.
Linked Issues check ✅ Passed The PR implements the requested fallback by completing local cleanup when the gateway is unreachable, which satisfies #6046.
Out of Scope Changes check ✅ Passed The extra test harness and mock updates support the destroy behavior change and do not appear unrelated.
✨ 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 fix/6046-destroy-force-gateway-down

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

@github-code-quality

github-code-quality Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the fix/6046-destroy-for... 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 fix/6046-destroy-for... a2a23a1 +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/blueprint/ssrf.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 fix/6046-destroy-for... branch is 69%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/6046-destroy-for... a2a23a1 +/-
src/lib/actions...dbox/rebuild.ts 82%
src/lib/actions...all/run-plan.ts 80%
src/lib/state/o...oard-session.ts 80%
src/lib/shields/index.ts 75%
src/lib/state/sandbox.ts 73%
src/lib/onboard...er-gpu-patch.ts 69%
src/lib/onboard/preflight.ts 69%
src/lib/policy/index.ts 67%
src/lib/actions...licy-channel.ts 59%
src/lib/onboard.ts 20%

Updated July 03, 2026 04:31 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: sandbox-operations
Optional E2E: sandbox-survival, tunnel-lifecycle

Dispatch hint: sandbox-operations

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • sandbox-operations (high): Required because this PR changes sandbox destroy lifecycle behavior. The existing sandbox-operations E2E exercises real OpenShell/Docker boundaries, multi-sandbox lifecycle, gateway recovery, and confirms nemoclaw <sandbox> destroy --yes removes both NemoClaw and OpenShell entries.

Optional E2E

  • sandbox-survival (high): Useful adjacent confidence because the destroy change interacts with gateway stop/start/recovery and final registry cleanup; sandbox-survival validates a full install/onboard, gateway restart survival, and final destroy/list state.
  • tunnel-lifecycle (high): Optional because the code path protects shared host services from stopAll() on forced local cleanup; tunnel-lifecycle gives extra coverage around host tunnel/service lifecycle if maintainers want confidence beyond the targeted destroy E2E.

New E2E recommendations

  • destroy with unreachable OpenShell gateway (medium): No existing live E2E appears to explicitly create a registered sandbox, make the OpenShell gateway unreachable, run nemoclaw <sandbox> destroy --force, and assert local registry removal without shared host-service or gateway teardown. The PR adds CLI-level coverage, but the specific real OpenShell boundary is not covered by an existing E2E job.
    • Suggested test: Add a live E2E target such as destroy-gateway-unreachable that provisions a sandbox, stops/kills the OpenShell gateway, runs nemoclaw <sandbox> destroy --force, verifies the local registry entry is removed, and verifies shared host services/gateway are not torn down for unconfirmed live delete.

Dispatch hint

  • Workflow: .github/workflows/e2e.yaml
  • jobs input: sandbox-operations

@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

E2E Target Recommendation

Required E2E targets: sandbox-operations
Optional E2E targets: sandbox-survival

Dispatch required E2E targets:

  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=sandbox-operations

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E targets

  • sandbox-operations: The PR changes core sandbox destroy/delete outcome handling, including gateway-unreachable classification and host-service/gateway teardown gating. The sandbox-operations live job is the smallest wired E2E job that explicitly exercises live sandbox destroy, post-destroy NemoClaw/OpenShell listing, and gateway recovery boundaries.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=sandbox-operations

Optional E2E targets

  • sandbox-survival: Adjacent coverage for destroy after a real install and gateway restart; useful because the change is specifically about destroy behavior when gateway state is abnormal, but sandbox-operations is the primary targeted coverage.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=sandbox-survival

Relevant changed files

  • src/lib/actions/sandbox/destroy.ts
  • src/lib/domain/sandbox/destroy.ts

@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Source-of-truth review needed: Forced local cleanup when `sandbox delete` reports gateway-unreachable.
Open items: 0 required · 6 warnings · 0 suggestions · 8 test follow-ups
Since last review: 0 prior items resolved · 4 still apply · 0 new items found

Action checklist

  • PRA-1 Resolve or justify: Source-of-truth review needed: Forced local cleanup when `sandbox delete` reports gateway-unreachable
  • PRA-2 Resolve or justify: Source-of-truth review needed: Tolerant parsing of gateway-unreachable delete output
  • PRA-3 Resolve or justify: Gateway-down regression only fails final delete, not all gateway calls in test/cli/destroy-gateway-unreachable.test.ts:21
  • PRA-4 Resolve or justify: Gateway-unreachable matching is broad for a local source-of-truth deletion path in src/lib/domain/sandbox/destroy.ts:30
  • PRA-5 Resolve or justify: Source-of-truth review for forced local cleanup still needs completion in src/lib/actions/sandbox/destroy.ts:417
  • PRA-6 Resolve or justify: New fallback policy remains embedded in the destroy monolith in src/lib/actions/sandbox/destroy.ts:417
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Gateway-down regression only fails final delete, not all gateway calls
  • PRA-T6 Add or justify test follow-up: Acceptance clause
  • PRA-T7 Add or justify test follow-up: Acceptance clause
  • PRA-T8 Add or justify test follow-up: Acceptance clause

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-3 Resolve/justify tests test/cli/destroy-gateway-unreachable.test.ts:21 Update the fake OpenShell used by this CLI regression so gateway-dependent pre-delete operations used by the current destroy path, such as `sandbox exec` for wipe-state and `sandbox provider detach`, return the same connection-refused transport failure while `destroy --force` still removes the local registry record. Keep only intentionally local or non-gateway commands successful.
PRA-4 Resolve/justify security src/lib/domain/sandbox/destroy.ts:30 Either narrow the classifier to the concrete local-gateway-not-listening forms from the issue, such as `connection refused`, `os error 61|111`, `tcp connect error`, and known `127.0.0.1:8080` connect failures, or add table-driven tests and comments justifying why each broader phrase is safe to treat as `--force` local-cleanup eligible.
PRA-5 Resolve/justify architecture src/lib/actions/sandbox/destroy.ts:417 Document the source-of-truth decision next to the fallback or in an extracted helper: the invalid state, source boundary, source-fix constraint, regression test, and removal condition. Include why this PR chooses `--force` local cleanup instead of the linked issue's alternate recovery option of starting the gateway like `status`/`doctor`.
PRA-6 Resolve/justify architecture src/lib/actions/sandbox/destroy.ts:417 Shrink the action by extracting the delete-failure policy into a small pure helper near the existing domain destroy helpers, returning values such as `forcedLocalCleanup`, `deleteSucceededOrAlreadyGone`, and `proceedWithLocalCleanup`, with unit coverage for each branch.
Review findings by urgency: 0 required fixes, 6 items to resolve/justify, 0 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: Forced local cleanup when `sandbox delete` reports gateway-unreachable

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: The current CLI regression only fails final `sandbox delete`; it does not reproduce the issue's state where every provider detach and final delete returns connection refused.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `src/lib/actions/sandbox/destroy.ts` adds `forcedLocalCleanup` and warns the sandbox may still exist, but the source-fix constraint and removal condition are missing.

PRA-2 Resolve/justify — Source-of-truth review needed: Tolerant parsing of gateway-unreachable delete output

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Domain tests cover a few positives and simple negatives, but not the broader regex alternatives or ambiguous non-local transport failures.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `isGatewayUnreachableDeleteOutput()` accepts broad phrases including `transport error`, `failed to connect to`, timeout/deadline, and reset.

PRA-3 Resolve/justify — Gateway-down regression only fails final delete, not all gateway calls

  • Location: test/cli/destroy-gateway-unreachable.test.ts:21
  • Category: tests
  • Problem: The linked issue states that every gateway call returns connection refused when the OpenShell gateway is down, including provider detaches and the final delete. The new CLI fake exits nonzero only for `sandbox delete` and exits 0 for all other OpenShell commands, so it proves the new final-delete fallback but not that `destroy --force` still reaches local cleanup when earlier gateway-dependent best-effort calls also fail.
  • Impact: A real gateway-down destroy could still abort before the `forcedLocalCleanup` branch if a pre-delete gateway operation changes from best-effort to fatal or regresses, leaving users with the original no-bypass failure despite the new final-delete fallback.
  • Recommended action: Update the fake OpenShell used by this CLI regression so gateway-dependent pre-delete operations used by the current destroy path, such as `sandbox exec` for wipe-state and `sandbox provider detach`, return the same connection-refused transport failure while `destroy --force` still removes the local registry record. Keep only intentionally local or non-gateway commands successful.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `GATEWAY_DOWN_OPENSHELL` in `test/cli/destroy-gateway-unreachable.test.ts` and compare its handled commands with the call sequence in `src/lib/actions/sandbox/destroy.ts`, `wipeSandboxState()`, and `runSandboxProviderPreDeleteCleanup()`.
  • Missing regression test: Add a CLI regression named like `destroy --force removes the local record when sandbox exec, provider detach, and sandbox delete all return connection refused`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `GATEWAY_DOWN_OPENSHELL` in `test/cli/destroy-gateway-unreachable.test.ts` and compare its handled commands with the call sequence in `src/lib/actions/sandbox/destroy.ts`, `wipeSandboxState()`, and `runSandboxProviderPreDeleteCleanup()`.
  • Evidence: `test/cli/destroy-gateway-unreachable.test.ts:21-28` handles `sandbox list`, fails `sandbox delete`, then `exit 0` for everything else; `src/lib/actions/sandbox/destroy.ts` selects the gateway, wipes state, detaches providers, and only then runs `sandbox delete`.

PRA-4 Resolve/justify — Gateway-unreachable matching is broad for a local source-of-truth deletion path

  • Location: src/lib/domain/sandbox/destroy.ts:30
  • Category: security
  • Problem: `isGatewayUnreachableDeleteOutput()` treats generic phrases such as `transport error`, `failed to connect to`, `connect timed out`, `deadline has elapsed`, and `connection reset` as gateway-unreachable delete failures. The fallback is gated by explicit `--force`, but the classifier can still convert ambiguous delete failures into local registry removal when the live gateway-side delete was not confirmed.
  • Impact: A user could drop NemoClaw's local source of truth for a sandbox after an ambiguous transport-layer or remote rejection message that is not actually the local `127.0.0.1:8080` OpenShell gateway being down, leaving an orphaned live sandbox and making follow-up cleanup harder.
  • Recommended action: Either narrow the classifier to the concrete local-gateway-not-listening forms from the issue, such as `connection refused`, `os error 61|111`, `tcp connect error`, and known `127.0.0.1:8080` connect failures, or add table-driven tests and comments justifying why each broader phrase is safe to treat as `--force` local-cleanup eligible.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the regex in `isGatewayUnreachableDeleteOutput()` and compare each alternative against the linked issue logs and the `forcedLocalCleanup` branch in `destroySandbox()`.
  • Missing regression test: Add domain tests proving non-transport delete rejections and ambiguous timeout/deadline/reset messages do not set `gatewayUnreachable` unless they include the intended local gateway-down signal, plus positive tests for every intentionally supported OpenShell gateway-down wording.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the regex in `isGatewayUnreachableDeleteOutput()` and compare each alternative against the linked issue logs and the `forcedLocalCleanup` branch in `destroySandbox()`.
  • Evidence: The regex at `src/lib/domain/sandbox/destroy.ts:30` includes `transport error`, `failed to connect to`, `connect(?:ion)? timed out`, `deadline has elapsed`, and `connection reset`, while the concrete issue failure is `Connection refused (os error 61)` against `127.0.0.1:8080`.

PRA-5 Resolve/justify — Source-of-truth review for forced local cleanup still needs completion

  • Location: src/lib/actions/sandbox/destroy.ts:417
  • Category: architecture
  • Problem: The fallback comments now identify that the gateway-side delete is unconfirmed and shared services must not be stopped, but the localized workaround still does not fully document why destroy should not or cannot auto-recover the gateway in this PR, which upstream/runtime boundary owns the source fix, which regression reproduces the full real invalid state, and when the local-only fallback can be removed.
  • Impact: A localized recovery path can become permanent and hide the true invalid state: NemoClaw may remove registry, shields, and timer artifacts for a sandbox that still exists on the gateway, without a clear owner or removal condition for the workaround.
  • Recommended action: Document the source-of-truth decision next to the fallback or in an extracted helper: the invalid state, source boundary, source-fix constraint, regression test, and removal condition. Include why this PR chooses `--force` local cleanup instead of the linked issue's alternate recovery option of starting the gateway like `status`/`doctor`.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the forced-local-cleanup comments and helper boundaries around `src/lib/actions/sandbox/destroy.ts:417-492` for all five source-of-truth elements.
  • Missing regression test: The existing CLI regression proves final `sandbox delete` connection-refused plus `--force` removes the local record, and `destroy -y` preserves it with a hint; it does not yet reproduce every gateway call failing, which should be the regression named in the source-of-truth note.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the forced-local-cleanup comments and helper boundaries around `src/lib/actions/sandbox/destroy.ts:417-492` for all five source-of-truth elements.
  • Evidence: `src/lib/actions/sandbox/destroy.ts` warns that the sandbox may still exist if the gateway comes back and skips shared-service/gateway teardown, but no removal condition or source-fix constraint is recorded.

PRA-6 Resolve/justify — New fallback policy remains embedded in the destroy monolith

  • Location: src/lib/actions/sandbox/destroy.ts:417
  • Category: architecture
  • Problem: The high-risk sandbox destroy action grew with inline outcome classification, forced-local-cleanup branching, failure messaging, host-service cleanup decision wiring, and gateway-teardown safety conditions. This file is an active lifecycle hotspot with overlapping work touching it.
  • Impact: Keeping the recovery policy inline makes later sandbox/gateway changes more likely to miss important safety conditions, such as requiring explicit `--force`, preserving real delete failures as fatal, or preventing host-service/gateway teardown when gateway-side deletion was not confirmed.
  • Recommended action: Shrink the action by extracting the delete-failure policy into a small pure helper near the existing domain destroy helpers, returning values such as `forcedLocalCleanup`, `deleteSucceededOrAlreadyGone`, and `proceedWithLocalCleanup`, with unit coverage for each branch.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read the forced-local-cleanup branch in `src/lib/actions/sandbox/destroy.ts` and confirm whether the safety policy is expressed as a tested pure helper rather than inline lifecycle code.
  • Missing regression test: Add helper-level tests for confirmed delete, already-gone delete, gateway-unreachable with `--force`, gateway-unreachable without `--force`, and non-transport delete errors.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read the forced-local-cleanup branch in `src/lib/actions/sandbox/destroy.ts` and confirm whether the safety policy is expressed as a tested pure helper rather than inline lifecycle code.
  • Evidence: Synthetic drift shows `src/lib/actions/sandbox/destroy.ts` grew by 46 lines to 545 lines; the new policy is still inline around the delete outcome handling and cleanup decisions.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

  • None.
Simplification opportunities: 1 possible cut

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

  • PRA-6 shrink (src/lib/actions/sandbox/destroy.ts:417): Inline forced-local-cleanup decisioning and derived cleanup booleans inside `destroySandbox`.
    • Replacement: A small pure helper in `src/lib/domain/sandbox/destroy.ts` that derives the delete/cleanup policy from normalized options and delete outcome.
    • Safety boundary: Do not remove explicit `--force` gating, confirmed-delete checks, redaction boundaries, shields/timer cleanup ordering, or the shared host-service/gateway teardown guard.
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — destroy --force removes the local registry record when sandbox exec, sandbox provider detach, and sandbox delete all return connection refused. The changed behavior is in sandbox/runtime lifecycle code and changes when NemoClaw removes local source-of-truth records after an unconfirmed gateway-side delete. Existing unit and CLI tests are useful but do not yet reproduce the full gateway-down path from the linked issue.
  • PRA-T2 Runtime validation — isGatewayUnreachableDeleteOutput rejects generic timeout/deadline/reset/failed-to-connect diagnostics unless they match the intended local OpenShell gateway-down signal. The changed behavior is in sandbox/runtime lifecycle code and changes when NemoClaw removes local source-of-truth records after an unconfirmed gateway-side delete. Existing unit and CLI tests are useful but do not yet reproduce the full gateway-down path from the linked issue.
  • PRA-T3 Runtime validation — destroy --force preserves the local registry record for non-gateway delete rejections that include ambiguous transport wording. The changed behavior is in sandbox/runtime lifecycle code and changes when NemoClaw removes local source-of-truth records after an unconfirmed gateway-side delete. Existing unit and CLI tests are useful but do not yet reproduce the full gateway-down path from the linked issue.
  • PRA-T4 Runtime validation — delete cleanup policy helper returns fatal, confirmed-delete, already-gone, and forced-local-cleanup decisions without stopping host services for unconfirmed deletes. The changed behavior is in sandbox/runtime lifecycle code and changes when NemoClaw removes local source-of-truth records after an unconfirmed gateway-side delete. Existing unit and CLI tests are useful but do not yet reproduce the full gateway-down path from the linked issue.
  • PRA-T5 Gateway-down regression only fails final delete, not all gateway calls — Update the fake OpenShell used by this CLI regression so gateway-dependent pre-delete operations used by the current destroy path, such as `sandbox exec` for wipe-state and `sandbox provider detach`, return the same connection-refused transport failure while `destroy --force` still removes the local registry record. Keep only intentionally local or non-gateway commands successful.
  • PRA-T6 Acceptance clause — Verbose (`RUST_LOG=debug`) output proves every call — each provider detach **and** the final delete — connects to `127.0.0.1:8080` (the gateway), **not** to the messaging-bridge / devtunnel endpoints. — add test evidence or identify existing coverage. The production path still runs wipe-state and provider detach before final delete, but the new CLI fake succeeds every OpenShell command except `sandbox delete`, so it does not reproduce provider-detach or wipe calls failing against the gateway.
  • PRA-T7 Acceptance clause — When the gateway process is not running, all of them get `Connection refused (os error 61)`. — add test evidence or identify existing coverage. Domain tests cover several connection-refused strings and the CLI test covers final delete returning `tcp connect error: Connection refused (os error 61)`, but no regression makes every gateway-dependent OpenShell call return that failure.
  • PRA-T8 Acceptance clause — Unlike `nemoclaw <name> status` / `doctor`, which auto-start the gateway, `destroy` assumes the gateway is already up and does **not** start/recover it. — add test evidence or identify existing coverage. The PR chooses the `--force` local-cleanup alternative rather than gateway recovery. The changed code does not start or recover the gateway, and it does not document why that source fix is not done here.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: Forced local cleanup when `sandbox delete` reports gateway-unreachable

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: The current CLI regression only fails final `sandbox delete`; it does not reproduce the issue's state where every provider detach and final delete returns connection refused.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `src/lib/actions/sandbox/destroy.ts` adds `forcedLocalCleanup` and warns the sandbox may still exist, but the source-fix constraint and removal condition are missing.

PRA-2 Resolve/justify — Source-of-truth review needed: Tolerant parsing of gateway-unreachable delete output

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Domain tests cover a few positives and simple negatives, but not the broader regex alternatives or ambiguous non-local transport failures.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `isGatewayUnreachableDeleteOutput()` accepts broad phrases including `transport error`, `failed to connect to`, timeout/deadline, and reset.

PRA-3 Resolve/justify — Gateway-down regression only fails final delete, not all gateway calls

  • Location: test/cli/destroy-gateway-unreachable.test.ts:21
  • Category: tests
  • Problem: The linked issue states that every gateway call returns connection refused when the OpenShell gateway is down, including provider detaches and the final delete. The new CLI fake exits nonzero only for `sandbox delete` and exits 0 for all other OpenShell commands, so it proves the new final-delete fallback but not that `destroy --force` still reaches local cleanup when earlier gateway-dependent best-effort calls also fail.
  • Impact: A real gateway-down destroy could still abort before the `forcedLocalCleanup` branch if a pre-delete gateway operation changes from best-effort to fatal or regresses, leaving users with the original no-bypass failure despite the new final-delete fallback.
  • Recommended action: Update the fake OpenShell used by this CLI regression so gateway-dependent pre-delete operations used by the current destroy path, such as `sandbox exec` for wipe-state and `sandbox provider detach`, return the same connection-refused transport failure while `destroy --force` still removes the local registry record. Keep only intentionally local or non-gateway commands successful.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `GATEWAY_DOWN_OPENSHELL` in `test/cli/destroy-gateway-unreachable.test.ts` and compare its handled commands with the call sequence in `src/lib/actions/sandbox/destroy.ts`, `wipeSandboxState()`, and `runSandboxProviderPreDeleteCleanup()`.
  • Missing regression test: Add a CLI regression named like `destroy --force removes the local record when sandbox exec, provider detach, and sandbox delete all return connection refused`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `GATEWAY_DOWN_OPENSHELL` in `test/cli/destroy-gateway-unreachable.test.ts` and compare its handled commands with the call sequence in `src/lib/actions/sandbox/destroy.ts`, `wipeSandboxState()`, and `runSandboxProviderPreDeleteCleanup()`.
  • Evidence: `test/cli/destroy-gateway-unreachable.test.ts:21-28` handles `sandbox list`, fails `sandbox delete`, then `exit 0` for everything else; `src/lib/actions/sandbox/destroy.ts` selects the gateway, wipes state, detaches providers, and only then runs `sandbox delete`.

PRA-4 Resolve/justify — Gateway-unreachable matching is broad for a local source-of-truth deletion path

  • Location: src/lib/domain/sandbox/destroy.ts:30
  • Category: security
  • Problem: `isGatewayUnreachableDeleteOutput()` treats generic phrases such as `transport error`, `failed to connect to`, `connect timed out`, `deadline has elapsed`, and `connection reset` as gateway-unreachable delete failures. The fallback is gated by explicit `--force`, but the classifier can still convert ambiguous delete failures into local registry removal when the live gateway-side delete was not confirmed.
  • Impact: A user could drop NemoClaw's local source of truth for a sandbox after an ambiguous transport-layer or remote rejection message that is not actually the local `127.0.0.1:8080` OpenShell gateway being down, leaving an orphaned live sandbox and making follow-up cleanup harder.
  • Recommended action: Either narrow the classifier to the concrete local-gateway-not-listening forms from the issue, such as `connection refused`, `os error 61|111`, `tcp connect error`, and known `127.0.0.1:8080` connect failures, or add table-driven tests and comments justifying why each broader phrase is safe to treat as `--force` local-cleanup eligible.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the regex in `isGatewayUnreachableDeleteOutput()` and compare each alternative against the linked issue logs and the `forcedLocalCleanup` branch in `destroySandbox()`.
  • Missing regression test: Add domain tests proving non-transport delete rejections and ambiguous timeout/deadline/reset messages do not set `gatewayUnreachable` unless they include the intended local gateway-down signal, plus positive tests for every intentionally supported OpenShell gateway-down wording.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the regex in `isGatewayUnreachableDeleteOutput()` and compare each alternative against the linked issue logs and the `forcedLocalCleanup` branch in `destroySandbox()`.
  • Evidence: The regex at `src/lib/domain/sandbox/destroy.ts:30` includes `transport error`, `failed to connect to`, `connect(?:ion)? timed out`, `deadline has elapsed`, and `connection reset`, while the concrete issue failure is `Connection refused (os error 61)` against `127.0.0.1:8080`.

PRA-5 Resolve/justify — Source-of-truth review for forced local cleanup still needs completion

  • Location: src/lib/actions/sandbox/destroy.ts:417
  • Category: architecture
  • Problem: The fallback comments now identify that the gateway-side delete is unconfirmed and shared services must not be stopped, but the localized workaround still does not fully document why destroy should not or cannot auto-recover the gateway in this PR, which upstream/runtime boundary owns the source fix, which regression reproduces the full real invalid state, and when the local-only fallback can be removed.
  • Impact: A localized recovery path can become permanent and hide the true invalid state: NemoClaw may remove registry, shields, and timer artifacts for a sandbox that still exists on the gateway, without a clear owner or removal condition for the workaround.
  • Recommended action: Document the source-of-truth decision next to the fallback or in an extracted helper: the invalid state, source boundary, source-fix constraint, regression test, and removal condition. Include why this PR chooses `--force` local cleanup instead of the linked issue's alternate recovery option of starting the gateway like `status`/`doctor`.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the forced-local-cleanup comments and helper boundaries around `src/lib/actions/sandbox/destroy.ts:417-492` for all five source-of-truth elements.
  • Missing regression test: The existing CLI regression proves final `sandbox delete` connection-refused plus `--force` removes the local record, and `destroy -y` preserves it with a hint; it does not yet reproduce every gateway call failing, which should be the regression named in the source-of-truth note.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the forced-local-cleanup comments and helper boundaries around `src/lib/actions/sandbox/destroy.ts:417-492` for all five source-of-truth elements.
  • Evidence: `src/lib/actions/sandbox/destroy.ts` warns that the sandbox may still exist if the gateway comes back and skips shared-service/gateway teardown, but no removal condition or source-fix constraint is recorded.

PRA-6 Resolve/justify — New fallback policy remains embedded in the destroy monolith

  • Location: src/lib/actions/sandbox/destroy.ts:417
  • Category: architecture
  • Problem: The high-risk sandbox destroy action grew with inline outcome classification, forced-local-cleanup branching, failure messaging, host-service cleanup decision wiring, and gateway-teardown safety conditions. This file is an active lifecycle hotspot with overlapping work touching it.
  • Impact: Keeping the recovery policy inline makes later sandbox/gateway changes more likely to miss important safety conditions, such as requiring explicit `--force`, preserving real delete failures as fatal, or preventing host-service/gateway teardown when gateway-side deletion was not confirmed.
  • Recommended action: Shrink the action by extracting the delete-failure policy into a small pure helper near the existing domain destroy helpers, returning values such as `forcedLocalCleanup`, `deleteSucceededOrAlreadyGone`, and `proceedWithLocalCleanup`, with unit coverage for each branch.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read the forced-local-cleanup branch in `src/lib/actions/sandbox/destroy.ts` and confirm whether the safety policy is expressed as a tested pure helper rather than inline lifecycle code.
  • Missing regression test: Add helper-level tests for confirmed delete, already-gone delete, gateway-unreachable with `--force`, gateway-unreachable without `--force`, and non-transport delete errors.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read the forced-local-cleanup branch in `src/lib/actions/sandbox/destroy.ts` and confirm whether the safety policy is expressed as a tested pure helper rather than inline lifecycle code.
  • Evidence: Synthetic drift shows `src/lib/actions/sandbox/destroy.ts` grew by 46 lines to 545 lines; the new policy is still inline around the delete outcome handling and cleanup decisions.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Blocked

Merge posture: Do not merge until addressed
Primary next action: Fix PRA-3: False-positive gateway-down classification enables --force policy bypass; then add or justify PRA-T1.
Open items: 4 required · 9 warnings · 2 suggestions · 8 test follow-ups
Since last review: 1 prior item resolved · 7 still apply · 3 new items found

Action checklist

  • PRA-3 Fix: False-positive gateway-down classification enables --force policy bypass in src/lib/domain/sandbox/destroy.ts:24
  • PRA-4 Fix: Monolith growth: destroy.ts grew 46 lines to 545 lines; forced-local-cleanup not extracted in src/lib/actions/sandbox/destroy.ts:1
  • PRA-5 Fix: Recovery hint suggests 'nemoclaw <name> status' to start gateway — incorrect command in src/lib/actions/sandbox/destroy.ts:464
  • PRA-6 Fix: Missing negative regression tests for gateway-up rejection errors in src/lib/domain/sandbox/destroy.test.ts:37
  • PRA-1 Resolve or justify: Source-of-truth review needed: src/lib/actions/sandbox/destroy.ts:340-375 (gateway-unreachable fallback / forcedLocalCleanup)
  • PRA-2 Resolve or justify: Source-of-truth review needed: src/lib/actions/sandbox/destroy.ts:88 (isNonInteractive duplication)
  • PRA-7 Resolve or justify: Source-of-truth review needed: gateway-unreachable fallback is localized workaround in src/lib/actions/sandbox/destroy.ts:340
  • PRA-8 Resolve or justify: isNonInteractive() duplicated — only checks env var, missing CLI flag in src/lib/actions/sandbox/destroy.ts:88
  • PRA-9 Resolve or justify: isNonInteractive() duplicated across 4+ files — source-of-truth drift risk in src/lib/actions/sandbox/destroy.ts:88
  • PRA-10 Resolve or justify: Fake openshell is fragile — silently passes unhandled commands in test/cli/destroy-gateway-unreachable.test.ts:1
  • PRA-11 Resolve or justify: Gateway teardown correctly skipped on forced cleanup — untested in E2E in test/cli/destroy-gateway-unreachable.test.ts:1
  • PRA-12 Resolve or justify: --non-interactive flag inconsistency: destroy.ts only checks env var in src/lib/actions/sandbox/destroy.ts:88
  • PRA-13 Resolve or justify: Regex 'connection reset' specifically catches 'connection reset by peer' (gateway-up rejection) in src/lib/domain/sandbox/destroy.ts:24
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Runtime validation
  • PRA-T6 Add or justify test follow-up: Missing negative regression tests for gateway-up rejection errors
  • PRA-T7 Add or justify test follow-up: Fake openshell is fragile — silently passes unhandled commands
  • PRA-T8 Add or justify test follow-up: Gateway teardown correctly skipped on forced cleanup — untested in E2E
  • PRA-14 In-scope improvement: Lift isNonInteractive() to src/lib/core/non-interactive.ts in src/lib/actions/sandbox/destroy.ts:88
  • PRA-15 In-scope improvement: Single-use PATH construction in E2E test — extract to constant in test/cli/destroy-gateway-unreachable.test.ts:71

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-3 Required security src/lib/domain/sandbox/destroy.ts:24 Tighten regex to require connection-refused + localhost/127.0.0.1 context, or add explicit negative patterns for common gateway-up rejection messages (quota exceeded, authentication failed, sandbox busy, validation error, connection reset by peer). At minimum, add unit test cases proving gateway-up-rejecting errors return false.
PRA-4 Required architecture src/lib/actions/sandbox/destroy.ts:1 Extract the forced-local-cleanup decision and execution into a separate helper (e.g., handleGatewayUnreachableDestroy(sandboxName, deleteResult, normalized, cleanupGatewayName, runOpenshell)). Consider splitting the action into orchestration + discrete steps as was done for wipe-state.
PRA-5 Required acceptance src/lib/actions/sandbox/destroy.ts:464 Change the recovery hint to the correct command: 'openshell gateway start --name <gatewayName>' (using the captured cleanupGatewayName) or 'nemoclaw onboard'.
PRA-6 Required tests src/lib/domain/sandbox/destroy.test.ts:37 Add negative test cases for all gateway-up rejection error types that must return false.
PRA-7 Resolve/justify architecture src/lib/actions/sandbox/destroy.ts:340 Document the source-of-truth analysis in a comment at the fallback site addressing the 5 questions.
PRA-8 Resolve/justify architecture src/lib/actions/sandbox/destroy.ts:88 Lift isNonInteractive() to src/lib/core/non-interactive.ts and update destroy.ts, policy-channel.ts, and inference/ollama/proxy.ts to import from there (see PRA-11).
PRA-9 Resolve/justify architecture src/lib/actions/sandbox/destroy.ts:88 Create src/lib/core/non-interactive.ts exporting isNonInteractive(), update all 4 files to import from there. Low risk, small scope.
PRA-10 Resolve/justify tests test/cli/destroy-gateway-unreachable.test.ts:1 Make fake openshell stricter: fail on unexpected commands, or log them. At minimum, add a comment listing which openshell calls the destroy flow makes so future changes know to update the fake.
PRA-11 Resolve/justify tests test/cli/destroy-gateway-unreachable.test.ts:1 Add E2E test assertion that gateway teardown is not attempted when --force + gateway-down (e.g., verify no 'gateway remove' or 'gateway destroy' calls in fake openshell).
PRA-12 Resolve/justify correctness src/lib/actions/sandbox/destroy.ts:88 Either lift to shared core (PRA-11) which would fix the inconsistency, or update destroy.ts to also check the module-level flag (requires importing from onboard or using a shared source).
PRA-13 Resolve/justify security src/lib/domain/sandbox/destroy.ts:24 Either remove 'connection reset' from the regex, or make it more specific (e.g., 'connection refused|connection reset' with localhost context), or add explicit negative pattern for 'connection reset by peer'.
PRA-14 Improvement correctness src/lib/actions/sandbox/destroy.ts:88 Create src/lib/core/non-interactive.ts, move the canonical implementation there (checking both module flag and env var), update destroy.ts, policy-channel.ts, proxy.ts to import from it.
PRA-15 Improvement architecture test/cli/destroy-gateway-unreachable.test.ts:71 Extract to const testPath = `${localBin}:${process.env.PATH || ''}` at fixture scope.

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

PRA-3 Required — False-positive gateway-down classification enables --force policy bypass

  • Location: src/lib/domain/sandbox/destroy.ts:24
  • Category: security
  • Problem: isGatewayUnreachableDeleteOutput regex matches broad transport patterns including 'connection reset' which catches 'connection reset by peer' — a common gateway-up TCP rejection error. Also matches 'transport error', 'failed to connect to', 'deadline has elapsed' which can appear in gateway-up quota/auth/busy/validation rejections. This allows --force to trigger local cleanup when gateway is actually reachable but rejecting delete for policy/quota/auth reasons, bypassing policy enforcement.
  • Impact: User runs --force thinking gateway is down, but gateway is up and rejecting delete for policy reasons; local cleanup proceeds, gateway-side sandbox remains with policies/quotas/network rules intact, policy enforcement bypassed.
  • Required action: Tighten regex to require connection-refused + localhost/127.0.0.1 context, or add explicit negative patterns for common gateway-up rejection messages (quota exceeded, authentication failed, sandbox busy, validation error, connection reset by peer). At minimum, add unit test cases proving gateway-up-rejecting errors return false.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Run: expect(isGatewayUnreachableDeleteOutput('Error: sandbox delete rejected: quota exceeded')).toBe(false); expect(isGatewayUnreachableDeleteOutput('Authentication failed')).toBe(false); expect(isGatewayUnreachableDeleteOutput('sandbox busy')).toBe(false); expect(isGatewayUnreachableDeleteOutput('validation error: invalid sandbox state')).toBe(false); expect(isGatewayUnreachableDeleteOutput('connection reset by peer')).toBe(false);
  • Missing regression test: Add negative test cases in src/lib/domain/sandbox/destroy.test.ts for gateway-up-but-rejecting-delete errors: quota exceeded, authentication failed, sandbox busy, validation error, connection reset by peer — all should return false.
  • Done when: The required change is committed and verification passes: Run: expect(isGatewayUnreachableDeleteOutput('Error: sandbox delete rejected: quota exceeded')).toBe(false); expect(isGatewayUnreachableDeleteOutput('Authentication failed')).toBe(false); expect(isGatewayUnreachableDeleteOutput('sandbox busy')).toBe(false); expect(isGatewayUnreachableDeleteOutput('validation error: invalid sandbox state')).toBe(false); expect(isGatewayUnreachableDeleteOutput('connection reset by peer')).toBe(false);.
  • Evidence: Regex at line 24: /connection refused|os error (?:61|111)|tcp connect error|error trying to connect|transport error|failed to connect to|connect(?:ion)? timed out|deadline has elapsed|connection reset/i. Current tests only cover positive gateway-down cases and 'permission denied' / 'sandbox not found'.

PRA-4 Required — Monolith growth: destroy.ts grew 46 lines to 545 lines; forced-local-cleanup not extracted

  • Location: src/lib/actions/sandbox/destroy.ts:1
  • Category: architecture
  • Problem: destroy.ts grew from 499 to 545 lines (+46 lines) with the forced-local-cleanup logic embedded in the main destroySandbox function. The file handles confirmation, NIM cleanup, Ollama proxy, gateway selection, state wipe, provider detach, sandbox delete, forced-local-cleanup decision/execution, host services, shields cleanup, registry removal, session cleanup, and gateway teardown — 12+ responsibilities in one async function.
  • Impact: File exceeds monolith threshold; harder to review, test, and maintain. New logic is embedded in a 545-line function instead of being a composable helper. Security-critical paths (gateway-down classification, forced cleanup gating) are harder to audit.
  • Required action: Extract the forced-local-cleanup decision and execution into a separate helper (e.g., handleGatewayUnreachableDestroy(sandboxName, deleteResult, normalized, cleanupGatewayName, runOpenshell)). Consider splitting the action into orchestration + discrete steps as was done for wipe-state.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: wc -l src/lib/actions/sandbox/destroy.ts shows 545 lines. The forced-local-cleanup block (lines ~340-375) is a self-contained decision with its own warnings and side effects.
  • Missing regression test: No test needed — structural concern. Verify by extracting the block and confirming existing E2E tests pass.
  • Done when: The required change is committed and verification passes: wc -l src/lib/actions/sandbox/destroy.ts shows 545 lines. The forced-local-cleanup block (lines ~340-375) is a self-contained decision with its own warnings and side effects.
  • Evidence: Drift context flags +46 lines as blocker severity. File has 12+ logical responsibilities in one async function.

PRA-5 Required — Recovery hint suggests 'nemoclaw <name> status' to start gateway — incorrect command

  • Location: src/lib/actions/sandbox/destroy.ts:464
  • Category: acceptance
  • Problem: The recovery hint tells users 'Start it (run nemoclaw <name> status)' when gateway is unreachable. However, 'nemoclaw <name> status' only queries the gateway via getSandboxStatusPreflight/collectSandboxStatusSnapshot — it does NOT start the gateway. The correct gateway start command is 'openshell gateway start --name <gatewayName>' (as shown in gateway-state.ts:381, status-lookup-rendering.ts:152, 174) or 'nemoclaw onboard' which creates/starts the gateway.
  • Impact: User follows recovery hint, command doesn't start gateway, user remains blocked. Undermines the recovery path UX and wastes user time.
  • Required action: Change the recovery hint to the correct command: 'openshell gateway start --name <gatewayName>' (using the captured cleanupGatewayName) or 'nemoclaw onboard'.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check src/lib/actions/sandbox/status.ts — it only calls getSandboxStatusPreflight and collectSandboxStatusSnapshot, never starts gateway. Check gateway-state.ts:381 for the correct start command.
  • Missing regression test: Add a test verifying the error message contains the correct gateway start command.
  • Done when: The required change is committed and verification passes: Check src/lib/actions/sandbox/status.ts — it only calls getSandboxStatusPreflight and collectSandboxStatusSnapshot, never starts gateway. Check gateway-state.ts:381 for the correct start command.
  • Evidence: Line 464: console.error(` The OpenShell gateway is unreachable. Start it (run '${CLI_NAME} ${sandboxName} status'),`); status.ts has no gateway start logic; gateway-state.ts:381 shows 'openshell gateway start --name ${targetGatewayName}'

PRA-6 Required — Missing negative regression tests for gateway-up rejection errors

  • Location: src/lib/domain/sandbox/destroy.test.ts:37
  • Category: tests
  • Problem: The new test 'detects gateway transport errors vs real failures' only covers positive gateway-down cases and two negative cases ('permission denied', 'sandbox alpha not found'). It does NOT test the critical gateway-up rejection errors that the regex can misclassify: 'quota exceeded', 'authentication failed', 'sandbox busy', 'validation error', 'connection reset by peer' (matches 'connection reset' in regex).
  • Impact: Without these tests, the classification bug (PRA-3) has no automated guard. Regression could be introduced silently.
  • Required action: Add negative test cases for all gateway-up rejection error types that must return false.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Run the test file and verify the new negative cases pass: expect(isGatewayUnreachableDeleteOutput('quota exceeded')).toBe(false); expect(isGatewayUnreachableDeleteOutput('Authentication failed')).toBe(false); etc.
  • Missing regression test: Add negative test cases in src/lib/domain/sandbox/destroy.test.ts for: quota exceeded, authentication failed, sandbox busy, validation error, connection reset by peer — all should return false.
  • Done when: The required change is committed and verification passes: Run the test file and verify the new negative cases pass: expect(isGatewayUnreachableDeleteOutput('quota exceeded')).toBe(false); expect(isGatewayUnreachableDeleteOutput('Authentication failed')).toBe(false); etc.
  • Evidence: Test at line 37 only asserts true for connection refused/tcp connect error/error trying to connect, and false for permission denied/sandbox not found.
Review findings by urgency: 4 required fixes, 9 items to resolve/justify, 2 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/destroy.ts:340-375 (gateway-unreachable fallback / forcedLocalCleanup)

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: test/cli/destroy-gateway-unreachable.test.ts — 'removes the local sandbox record with --force' (covers happy path)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: Lines 340-375 contain the forcedLocalCleanup decision with comment but no source-of-truth documentation

PRA-2 Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/destroy.ts:88 (isNonInteractive duplication)

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: None currently; need test verifying --non-interactive flag works in destroy flow
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: destroy.ts:88 local definition; onboard.ts:675 canonical; proxy.ts:794 local; policy-channel.ts:36 import from onboard

PRA-7 Resolve/justify — Source-of-truth review needed: gateway-unreachable fallback is localized workaround

  • Location: src/lib/actions/sandbox/destroy.ts:340
  • Category: architecture
  • Problem: The gateway-unreachable fallback (lines 340-375) is a localized workaround for gateway-down state. Source-of-truth review needed: what invalid state is handled (gateway process down), where that state is created (gateway crash, host reboot, manual stop), why the source cannot be fixed in this PR (gateway is external process), what regression test proves the source cannot regress (E2E test destroy-gateway-unreachable.test.ts), and when the workaround can be removed (when gateway auto-restart is reliable).
  • Impact: Without documented source-of-truth analysis, the workaround may become permanent technical debt or be extended incorrectly.
  • Recommended action: Document the source-of-truth analysis in a comment at the fallback site addressing the 5 questions.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read lines 340-375 in destroy.ts and evaluate against the 5 source-of-truth questions.
  • Missing regression test: The E2E test destroy-gateway-unreachable.test.ts covers the happy path; add a test for gateway-up rejection misclassification (see PRA-3/PRA-9).
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read lines 340-375 in destroy.ts and evaluate against the 5 source-of-truth questions.
  • Evidence: Lines 340-375 contain the forcedLocalCleanup decision with comment 'When the OpenShell gateway is down, every gateway call...' but no source-of-truth documentation.

PRA-8 Resolve/justify — isNonInteractive() duplicated — only checks env var, missing CLI flag

  • Location: src/lib/actions/sandbox/destroy.ts:88
  • Category: architecture
  • Problem: isNonInteractive() is duplicated in destroy.ts (line 88) with a comment acknowledging it mirrors onboard.ts. The destroy.ts version only checks process.env.NEMOCLAW_NON_INTERACTIVE === '1' while onboard.ts checks BOTH a module-level NON_INTERACTIVE flag (set by --non-interactive CLI flag) AND the env var. This creates a drift risk where --non-interactive flag works in onboard but not destroy.
  • Impact: Inconsistent behavior: --non-interactive flag works for onboard but NOT for destroy, confusing users and CI pipelines.
  • Recommended action: Lift isNonInteractive() to src/lib/core/non-interactive.ts and update destroy.ts, policy-channel.ts, and inference/ollama/proxy.ts to import from there (see PRA-11).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Compare destroy.ts:88 (only env var) vs onboard.ts:675 (module flag OR env var) vs proxy.ts:794 (only env var).
  • Missing regression test: Add a test verifying --non-interactive flag (not just env var) works in destroy flow.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Compare destroy.ts:88 (only env var) vs onboard.ts:675 (module flag OR env var) vs proxy.ts:794 (only env var).
  • Evidence: destroy.ts:88: function isNonInteractive(): boolean { return process.env.NEMOCLAW_NON_INTERACTIVE === '1'; } vs onboard.ts:675: NON_INTERACTIVE || process.env.NEMOCLAW_NON_INTERACTIVE === '1'

PRA-9 Resolve/justify — isNonInteractive() duplicated across 4+ files — source-of-truth drift risk

  • Location: src/lib/actions/sandbox/destroy.ts:88
  • Category: architecture
  • Problem: isNonInteractive() duplicated across 4+ files: onboard.ts (canonical, checks module flag + env), destroy.ts (line 88, only env), policy-channel.ts (line 36, imports from onboard), inference/ollama/proxy.ts (line 794, only env). This is source-of-truth drift risk.
  • Impact: Changes to non-interactive logic must be made in multiple places; inconsistency between implementations.
  • Recommended action: Create src/lib/core/non-interactive.ts exporting isNonInteractive(), update all 4 files to import from there. Low risk, small scope.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -r 'isNonInteractive' src/lib --include='*.ts' | grep -v test | grep -v '.d.ts' shows 4+ definitions/imports.
  • Missing regression test: No new test needed — refactoring. Verify existing tests pass.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -r 'isNonInteractive' src/lib --include='*.ts' | grep -v test | grep -v '.d.ts' shows 4+ definitions/imports.
  • Evidence: Found in onboard.ts, destroy.ts, policy-channel.ts, proxy.ts with different implementations.

PRA-10 Resolve/justify — Fake openshell is fragile — silently passes unhandled commands

  • Location: test/cli/destroy-gateway-unreachable.test.ts:1
  • Category: tests
  • Problem: The fake openshell script (GATEWAY_DOWN_OPENSHELL) silently passes all unhandled commands with 'exit 0'. It only handles 'sandbox list' and 'sandbox delete'. If the destroy flow adds new openshell calls (e.g., 'provider delete', 'gateway' commands), the fake will silently succeed, masking test gaps.
  • Impact: Test gaps go undetected when destroy flow evolves; false confidence in E2E coverage.
  • Recommended action: Make fake openshell stricter: fail on unexpected commands, or log them. At minimum, add a comment listing which openshell calls the destroy flow makes so future changes know to update the fake.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read test/cli/destroy-gateway-unreachable.test.ts lines 14-24 — the fake only handles sandbox list/delete.
  • Missing regression test: Add a test that verifies the fake fails on an unexpected command, or add a comment documenting all expected openshell calls.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read test/cli/destroy-gateway-unreachable.test.ts lines 14-24 — the fake only handles sandbox list/delete.
  • Evidence: GATEWAY_DOWN_OPENSHELL ends with 'exit 0' catching all unmatched commands.

PRA-11 Resolve/justify — Gateway teardown correctly skipped on forced cleanup — untested in E2E

  • Location: test/cli/destroy-gateway-unreachable.test.ts:1
  • Category: tests
  • Problem: Gateway teardown correctly skipped on forced cleanup is tested in the unit test (destroy-flow.test.ts: 'does not stop shared host services when --force cleans up the last sandbox with the gateway down' checks cleanupGatewaySpy.not.toHaveBeenCalled), but NOT tested in the E2E test. The E2E test only asserts registry removal and exit code.
  • Impact: E2E test doesn't verify the critical security property that forced cleanup doesn't tear down gateway for unconfirmed delete.
  • Recommended action: Add E2E test assertion that gateway teardown is not attempted when --force + gateway-down (e.g., verify no 'gateway remove' or 'gateway destroy' calls in fake openshell).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check test/cli/destroy-gateway-unreachable.test.ts — it only asserts registryHasAlpha and exit code.
  • Missing regression test: Add E2E test asserting cleanupGatewayAfterLastSandbox is NOT called when --force + gateway-down.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check test/cli/destroy-gateway-unreachable.test.ts — it only asserts registryHasAlpha and exit code.
  • Evidence: E2E test only checks r.code === 0, r.out contains 'Sandbox destroyed', registryHasAlpha === false.

PRA-12 Resolve/justify — --non-interactive flag inconsistency: destroy.ts only checks env var

  • Location: src/lib/actions/sandbox/destroy.ts:88
  • Category: correctness
  • Problem: isNonInteractive() in destroy.ts only checks process.env.NEMOCLAW_NON_INTERACTIVE === '1' but onboard.ts checks (NON_INTERACTIVE || process.env.NEMOCLAW_NON_INTERACTIVE === '1') where NON_INTERACTIVE is a module-level flag set by the --non-interactive CLI flag. This means --non-interactive flag works for onboard but NOT for destroy, creating inconsistent behavior.
  • Impact: Users expecting --non-interactive to work for destroy (as it does for onboard) will get interactive prompts, breaking CI automation.
  • Recommended action: Either lift to shared core (PRA-11) which would fix the inconsistency, or update destroy.ts to also check the module-level flag (requires importing from onboard or using a shared source).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Compare onboard.ts:668 (let NON_INTERACTIVE = false) and :675 (NON_INTERACTIVE || env) vs destroy.ts:88 (only env).
  • Missing regression test: Add test verifying --non-interactive flag (not just env var) works in destroy flow.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Compare onboard.ts:668 (let NON_INTERACTIVE = false) and :675 (NON_INTERACTIVE || env) vs destroy.ts:88 (only env).
  • Evidence: onboard.ts exports isNonInteractive but destroy.ts redefines it locally with subset logic.

PRA-13 Resolve/justify — Regex 'connection reset' specifically catches 'connection reset by peer' (gateway-up rejection)

  • Location: src/lib/domain/sandbox/destroy.ts:24
  • Category: security
  • Problem: The regex pattern 'connection reset' (without 'by peer') will match 'connection reset by peer' which is a gateway-up TCP-level rejection (the gateway accepted the connection but then reset it). This is distinct from 'connection refused' (gateway not listening). The current test does not cover this case.
  • Impact: A gateway that is UP but resetting connections (e.g., overloaded, crashing mid-request) will be misclassified as gateway-down, enabling --force bypass.
  • Recommended action: Either remove 'connection reset' from the regex, or make it more specific (e.g., 'connection refused|connection reset' with localhost context), or add explicit negative pattern for 'connection reset by peer'.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Run: expect(isGatewayUnreachableDeleteOutput('connection reset by peer')).toBe(false) — currently returns TRUE (bug).
  • Missing regression test: Add test case: expect(isGatewayUnreachableDeleteOutput('connection reset by peer')).toBe(false).
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Run: expect(isGatewayUnreachableDeleteOutput('connection reset by peer')).toBe(false) — currently returns TRUE (bug).
  • Evidence: Regex includes 'connection reset' as alternation; 'connection reset by peer' is a common gateway-up error distinct from 'connection refused'.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

PRA-14 Improvement — Lift isNonInteractive() to src/lib/core/non-interactive.ts

  • Location: src/lib/actions/sandbox/destroy.ts:88
  • Category: correctness
  • Problem: isNonInteractive() should be lifted to src/lib/core/non-interactive.ts as a shared utility. This is an in-scope improvement since the duplication is in the changed file.
  • Impact: Eliminates duplication drift risk; ensures consistent non-interactive behavior across all callers.
  • Suggested action: Create src/lib/core/non-interactive.ts, move the canonical implementation there (checking both module flag and env var), update destroy.ts, policy-channel.ts, proxy.ts to import from it.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check if src/lib/core/ exists and create non-interactive.ts there.
  • Missing regression test: No new test — refactoring. Verify all callers still work.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: 4+ files duplicate or import this logic; core/ directory exists for shared utilities.

PRA-15 Improvement — Single-use PATH construction in E2E test — extract to constant

  • Location: test/cli/destroy-gateway-unreachable.test.ts:71
  • Category: architecture
  • Problem: Single-use PATH construction in E2E test: `${localBin}:${process.env.PATH || ''}` appears inline at lines 71 and 90. Should be extracted to a const at fixture scope for readability.
  • Impact: Minor readability/maintainability issue; duplicated string construction.
  • Suggested action: Extract to const testPath = `${localBin}:${process.env.PATH || ''}` at fixture scope.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read lines 71 and 90 in test/cli/destroy-gateway-unreachable.test.ts.
  • Missing regression test: No test needed — code style.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Two identical inline PATH constructions in the two test cases.
Simplification opportunities: 3 possible cuts, net -51 lines possible

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

  • PRA-4 shrink (src/lib/actions/sandbox/destroy.ts:1): Forced-local-cleanup decision and execution block (lines ~340-375) including forcedLocalCleanup variable, warning logs, and gating logic
    • Replacement: Call to extracted helper handleGatewayUnreachableDestroy(sandboxName, deleteResult, normalized, cleanupGatewayName, runOpenshell)
    • Net: -35 lines
    • Safety boundary: Must preserve: gatewayUnreachable check, forcedLocalCleanup gating on normalized.force, warning messages, cleanupShieldsDestroyArtifacts call, and the critical gating of host service/gateway teardown on deleteSucceededOrAlreadyGone
  • PRA-14 stdlib (src/lib/actions/sandbox/destroy.ts:88): Local isNonInteractive() function in destroy.ts (lines 84-90) and proxy.ts (line 794); import from onboard in policy-channel.ts
    • Replacement: import { isNonInteractive } from '../../core/non-interactive'
    • Net: -15 lines
    • Safety boundary: Must preserve: module-level NON_INTERACTIVE flag check OR env var check; all callers must receive identical behavior
  • PRA-15 shrink (test/cli/destroy-gateway-unreachable.test.ts:71): Inline `${localBin}:${process.env.PATH || ''}` at lines 71 and 90
    • Replacement: const testPath = `${localBin}:${process.env.PATH || ''}` at fixture scope (after localBin definition)
    • Net: -1 lines
    • Safety boundary: Must not change PATH value passed to runWithEnv
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — isGatewayUnreachableDeleteOutput returns false for quota exceeded error. Runtime/sandbox/infrastructure paths need behavioral runtime validation: src/lib/actions/sandbox/destroy.ts, src/lib/domain/sandbox/destroy.ts. The new --force fallback path and gateway-down classification require negative test cases for gateway-up rejection errors and E2E validation of gateway teardown skipping.
  • PRA-T2 Runtime validation — isGatewayUnreachableDeleteOutput returns false for authentication failed error. Runtime/sandbox/infrastructure paths need behavioral runtime validation: src/lib/actions/sandbox/destroy.ts, src/lib/domain/sandbox/destroy.ts. The new --force fallback path and gateway-down classification require negative test cases for gateway-up rejection errors and E2E validation of gateway teardown skipping.
  • PRA-T3 Runtime validation — isGatewayUnreachableDeleteOutput returns false for sandbox busy error. Runtime/sandbox/infrastructure paths need behavioral runtime validation: src/lib/actions/sandbox/destroy.ts, src/lib/domain/sandbox/destroy.ts. The new --force fallback path and gateway-down classification require negative test cases for gateway-up rejection errors and E2E validation of gateway teardown skipping.
  • PRA-T4 Runtime validation — isGatewayUnreachableDeleteOutput returns false for validation error. Runtime/sandbox/infrastructure paths need behavioral runtime validation: src/lib/actions/sandbox/destroy.ts, src/lib/domain/sandbox/destroy.ts. The new --force fallback path and gateway-down classification require negative test cases for gateway-up rejection errors and E2E validation of gateway teardown skipping.
  • PRA-T5 Runtime validation — isGatewayUnreachableDeleteOutput returns false for connection reset by peer. Runtime/sandbox/infrastructure paths need behavioral runtime validation: src/lib/actions/sandbox/destroy.ts, src/lib/domain/sandbox/destroy.ts. The new --force fallback path and gateway-down classification require negative test cases for gateway-up rejection errors and E2E validation of gateway teardown skipping.
  • PRA-T6 Missing negative regression tests for gateway-up rejection errors — Add negative test cases for all gateway-up rejection error types that must return false.
  • PRA-T7 Fake openshell is fragile — silently passes unhandled commands — Make fake openshell stricter: fail on unexpected commands, or log them. At minimum, add a comment listing which openshell calls the destroy flow makes so future changes know to update the fake.
  • PRA-T8 Gateway teardown correctly skipped on forced cleanup — untested in E2E — Add E2E test assertion that gateway teardown is not attempted when --force + gateway-down (e.g., verify no 'gateway remove' or 'gateway destroy' calls in fake openshell).
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/destroy.ts:340-375 (gateway-unreachable fallback / forcedLocalCleanup)

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: test/cli/destroy-gateway-unreachable.test.ts — 'removes the local sandbox record with --force' (covers happy path)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: Lines 340-375 contain the forcedLocalCleanup decision with comment but no source-of-truth documentation

PRA-2 Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/destroy.ts:88 (isNonInteractive duplication)

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: None currently; need test verifying --non-interactive flag works in destroy flow
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: destroy.ts:88 local definition; onboard.ts:675 canonical; proxy.ts:794 local; policy-channel.ts:36 import from onboard

PRA-3 Required — False-positive gateway-down classification enables --force policy bypass

  • Location: src/lib/domain/sandbox/destroy.ts:24
  • Category: security
  • Problem: isGatewayUnreachableDeleteOutput regex matches broad transport patterns including 'connection reset' which catches 'connection reset by peer' — a common gateway-up TCP rejection error. Also matches 'transport error', 'failed to connect to', 'deadline has elapsed' which can appear in gateway-up quota/auth/busy/validation rejections. This allows --force to trigger local cleanup when gateway is actually reachable but rejecting delete for policy/quota/auth reasons, bypassing policy enforcement.
  • Impact: User runs --force thinking gateway is down, but gateway is up and rejecting delete for policy reasons; local cleanup proceeds, gateway-side sandbox remains with policies/quotas/network rules intact, policy enforcement bypassed.
  • Required action: Tighten regex to require connection-refused + localhost/127.0.0.1 context, or add explicit negative patterns for common gateway-up rejection messages (quota exceeded, authentication failed, sandbox busy, validation error, connection reset by peer). At minimum, add unit test cases proving gateway-up-rejecting errors return false.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Run: expect(isGatewayUnreachableDeleteOutput('Error: sandbox delete rejected: quota exceeded')).toBe(false); expect(isGatewayUnreachableDeleteOutput('Authentication failed')).toBe(false); expect(isGatewayUnreachableDeleteOutput('sandbox busy')).toBe(false); expect(isGatewayUnreachableDeleteOutput('validation error: invalid sandbox state')).toBe(false); expect(isGatewayUnreachableDeleteOutput('connection reset by peer')).toBe(false);
  • Missing regression test: Add negative test cases in src/lib/domain/sandbox/destroy.test.ts for gateway-up-but-rejecting-delete errors: quota exceeded, authentication failed, sandbox busy, validation error, connection reset by peer — all should return false.
  • Done when: The required change is committed and verification passes: Run: expect(isGatewayUnreachableDeleteOutput('Error: sandbox delete rejected: quota exceeded')).toBe(false); expect(isGatewayUnreachableDeleteOutput('Authentication failed')).toBe(false); expect(isGatewayUnreachableDeleteOutput('sandbox busy')).toBe(false); expect(isGatewayUnreachableDeleteOutput('validation error: invalid sandbox state')).toBe(false); expect(isGatewayUnreachableDeleteOutput('connection reset by peer')).toBe(false);.
  • Evidence: Regex at line 24: /connection refused|os error (?:61|111)|tcp connect error|error trying to connect|transport error|failed to connect to|connect(?:ion)? timed out|deadline has elapsed|connection reset/i. Current tests only cover positive gateway-down cases and 'permission denied' / 'sandbox not found'.

PRA-4 Required — Monolith growth: destroy.ts grew 46 lines to 545 lines; forced-local-cleanup not extracted

  • Location: src/lib/actions/sandbox/destroy.ts:1
  • Category: architecture
  • Problem: destroy.ts grew from 499 to 545 lines (+46 lines) with the forced-local-cleanup logic embedded in the main destroySandbox function. The file handles confirmation, NIM cleanup, Ollama proxy, gateway selection, state wipe, provider detach, sandbox delete, forced-local-cleanup decision/execution, host services, shields cleanup, registry removal, session cleanup, and gateway teardown — 12+ responsibilities in one async function.
  • Impact: File exceeds monolith threshold; harder to review, test, and maintain. New logic is embedded in a 545-line function instead of being a composable helper. Security-critical paths (gateway-down classification, forced cleanup gating) are harder to audit.
  • Required action: Extract the forced-local-cleanup decision and execution into a separate helper (e.g., handleGatewayUnreachableDestroy(sandboxName, deleteResult, normalized, cleanupGatewayName, runOpenshell)). Consider splitting the action into orchestration + discrete steps as was done for wipe-state.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: wc -l src/lib/actions/sandbox/destroy.ts shows 545 lines. The forced-local-cleanup block (lines ~340-375) is a self-contained decision with its own warnings and side effects.
  • Missing regression test: No test needed — structural concern. Verify by extracting the block and confirming existing E2E tests pass.
  • Done when: The required change is committed and verification passes: wc -l src/lib/actions/sandbox/destroy.ts shows 545 lines. The forced-local-cleanup block (lines ~340-375) is a self-contained decision with its own warnings and side effects.
  • Evidence: Drift context flags +46 lines as blocker severity. File has 12+ logical responsibilities in one async function.

PRA-5 Required — Recovery hint suggests 'nemoclaw <name> status' to start gateway — incorrect command

  • Location: src/lib/actions/sandbox/destroy.ts:464
  • Category: acceptance
  • Problem: The recovery hint tells users 'Start it (run nemoclaw <name> status)' when gateway is unreachable. However, 'nemoclaw <name> status' only queries the gateway via getSandboxStatusPreflight/collectSandboxStatusSnapshot — it does NOT start the gateway. The correct gateway start command is 'openshell gateway start --name <gatewayName>' (as shown in gateway-state.ts:381, status-lookup-rendering.ts:152, 174) or 'nemoclaw onboard' which creates/starts the gateway.
  • Impact: User follows recovery hint, command doesn't start gateway, user remains blocked. Undermines the recovery path UX and wastes user time.
  • Required action: Change the recovery hint to the correct command: 'openshell gateway start --name <gatewayName>' (using the captured cleanupGatewayName) or 'nemoclaw onboard'.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check src/lib/actions/sandbox/status.ts — it only calls getSandboxStatusPreflight and collectSandboxStatusSnapshot, never starts gateway. Check gateway-state.ts:381 for the correct start command.
  • Missing regression test: Add a test verifying the error message contains the correct gateway start command.
  • Done when: The required change is committed and verification passes: Check src/lib/actions/sandbox/status.ts — it only calls getSandboxStatusPreflight and collectSandboxStatusSnapshot, never starts gateway. Check gateway-state.ts:381 for the correct start command.
  • Evidence: Line 464: console.error(` The OpenShell gateway is unreachable. Start it (run '${CLI_NAME} ${sandboxName} status'),`); status.ts has no gateway start logic; gateway-state.ts:381 shows 'openshell gateway start --name ${targetGatewayName}'

PRA-6 Required — Missing negative regression tests for gateway-up rejection errors

  • Location: src/lib/domain/sandbox/destroy.test.ts:37
  • Category: tests
  • Problem: The new test 'detects gateway transport errors vs real failures' only covers positive gateway-down cases and two negative cases ('permission denied', 'sandbox alpha not found'). It does NOT test the critical gateway-up rejection errors that the regex can misclassify: 'quota exceeded', 'authentication failed', 'sandbox busy', 'validation error', 'connection reset by peer' (matches 'connection reset' in regex).
  • Impact: Without these tests, the classification bug (PRA-3) has no automated guard. Regression could be introduced silently.
  • Required action: Add negative test cases for all gateway-up rejection error types that must return false.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Run the test file and verify the new negative cases pass: expect(isGatewayUnreachableDeleteOutput('quota exceeded')).toBe(false); expect(isGatewayUnreachableDeleteOutput('Authentication failed')).toBe(false); etc.
  • Missing regression test: Add negative test cases in src/lib/domain/sandbox/destroy.test.ts for: quota exceeded, authentication failed, sandbox busy, validation error, connection reset by peer — all should return false.
  • Done when: The required change is committed and verification passes: Run the test file and verify the new negative cases pass: expect(isGatewayUnreachableDeleteOutput('quota exceeded')).toBe(false); expect(isGatewayUnreachableDeleteOutput('Authentication failed')).toBe(false); etc.
  • Evidence: Test at line 37 only asserts true for connection refused/tcp connect error/error trying to connect, and false for permission denied/sandbox not found.

PRA-7 Resolve/justify — Source-of-truth review needed: gateway-unreachable fallback is localized workaround

  • Location: src/lib/actions/sandbox/destroy.ts:340
  • Category: architecture
  • Problem: The gateway-unreachable fallback (lines 340-375) is a localized workaround for gateway-down state. Source-of-truth review needed: what invalid state is handled (gateway process down), where that state is created (gateway crash, host reboot, manual stop), why the source cannot be fixed in this PR (gateway is external process), what regression test proves the source cannot regress (E2E test destroy-gateway-unreachable.test.ts), and when the workaround can be removed (when gateway auto-restart is reliable).
  • Impact: Without documented source-of-truth analysis, the workaround may become permanent technical debt or be extended incorrectly.
  • Recommended action: Document the source-of-truth analysis in a comment at the fallback site addressing the 5 questions.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read lines 340-375 in destroy.ts and evaluate against the 5 source-of-truth questions.
  • Missing regression test: The E2E test destroy-gateway-unreachable.test.ts covers the happy path; add a test for gateway-up rejection misclassification (see PRA-3/PRA-9).
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read lines 340-375 in destroy.ts and evaluate against the 5 source-of-truth questions.
  • Evidence: Lines 340-375 contain the forcedLocalCleanup decision with comment 'When the OpenShell gateway is down, every gateway call...' but no source-of-truth documentation.

PRA-8 Resolve/justify — isNonInteractive() duplicated — only checks env var, missing CLI flag

  • Location: src/lib/actions/sandbox/destroy.ts:88
  • Category: architecture
  • Problem: isNonInteractive() is duplicated in destroy.ts (line 88) with a comment acknowledging it mirrors onboard.ts. The destroy.ts version only checks process.env.NEMOCLAW_NON_INTERACTIVE === '1' while onboard.ts checks BOTH a module-level NON_INTERACTIVE flag (set by --non-interactive CLI flag) AND the env var. This creates a drift risk where --non-interactive flag works in onboard but not destroy.
  • Impact: Inconsistent behavior: --non-interactive flag works for onboard but NOT for destroy, confusing users and CI pipelines.
  • Recommended action: Lift isNonInteractive() to src/lib/core/non-interactive.ts and update destroy.ts, policy-channel.ts, and inference/ollama/proxy.ts to import from there (see PRA-11).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Compare destroy.ts:88 (only env var) vs onboard.ts:675 (module flag OR env var) vs proxy.ts:794 (only env var).
  • Missing regression test: Add a test verifying --non-interactive flag (not just env var) works in destroy flow.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Compare destroy.ts:88 (only env var) vs onboard.ts:675 (module flag OR env var) vs proxy.ts:794 (only env var).
  • Evidence: destroy.ts:88: function isNonInteractive(): boolean { return process.env.NEMOCLAW_NON_INTERACTIVE === '1'; } vs onboard.ts:675: NON_INTERACTIVE || process.env.NEMOCLAW_NON_INTERACTIVE === '1'

PRA-9 Resolve/justify — isNonInteractive() duplicated across 4+ files — source-of-truth drift risk

  • Location: src/lib/actions/sandbox/destroy.ts:88
  • Category: architecture
  • Problem: isNonInteractive() duplicated across 4+ files: onboard.ts (canonical, checks module flag + env), destroy.ts (line 88, only env), policy-channel.ts (line 36, imports from onboard), inference/ollama/proxy.ts (line 794, only env). This is source-of-truth drift risk.
  • Impact: Changes to non-interactive logic must be made in multiple places; inconsistency between implementations.
  • Recommended action: Create src/lib/core/non-interactive.ts exporting isNonInteractive(), update all 4 files to import from there. Low risk, small scope.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -r 'isNonInteractive' src/lib --include='*.ts' | grep -v test | grep -v '.d.ts' shows 4+ definitions/imports.
  • Missing regression test: No new test needed — refactoring. Verify existing tests pass.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -r 'isNonInteractive' src/lib --include='*.ts' | grep -v test | grep -v '.d.ts' shows 4+ definitions/imports.
  • Evidence: Found in onboard.ts, destroy.ts, policy-channel.ts, proxy.ts with different implementations.

PRA-10 Resolve/justify — Fake openshell is fragile — silently passes unhandled commands

  • Location: test/cli/destroy-gateway-unreachable.test.ts:1
  • Category: tests
  • Problem: The fake openshell script (GATEWAY_DOWN_OPENSHELL) silently passes all unhandled commands with 'exit 0'. It only handles 'sandbox list' and 'sandbox delete'. If the destroy flow adds new openshell calls (e.g., 'provider delete', 'gateway' commands), the fake will silently succeed, masking test gaps.
  • Impact: Test gaps go undetected when destroy flow evolves; false confidence in E2E coverage.
  • Recommended action: Make fake openshell stricter: fail on unexpected commands, or log them. At minimum, add a comment listing which openshell calls the destroy flow makes so future changes know to update the fake.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read test/cli/destroy-gateway-unreachable.test.ts lines 14-24 — the fake only handles sandbox list/delete.
  • Missing regression test: Add a test that verifies the fake fails on an unexpected command, or add a comment documenting all expected openshell calls.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read test/cli/destroy-gateway-unreachable.test.ts lines 14-24 — the fake only handles sandbox list/delete.
  • Evidence: GATEWAY_DOWN_OPENSHELL ends with 'exit 0' catching all unmatched commands.

PRA-11 Resolve/justify — Gateway teardown correctly skipped on forced cleanup — untested in E2E

  • Location: test/cli/destroy-gateway-unreachable.test.ts:1
  • Category: tests
  • Problem: Gateway teardown correctly skipped on forced cleanup is tested in the unit test (destroy-flow.test.ts: 'does not stop shared host services when --force cleans up the last sandbox with the gateway down' checks cleanupGatewaySpy.not.toHaveBeenCalled), but NOT tested in the E2E test. The E2E test only asserts registry removal and exit code.
  • Impact: E2E test doesn't verify the critical security property that forced cleanup doesn't tear down gateway for unconfirmed delete.
  • Recommended action: Add E2E test assertion that gateway teardown is not attempted when --force + gateway-down (e.g., verify no 'gateway remove' or 'gateway destroy' calls in fake openshell).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check test/cli/destroy-gateway-unreachable.test.ts — it only asserts registryHasAlpha and exit code.
  • Missing regression test: Add E2E test asserting cleanupGatewayAfterLastSandbox is NOT called when --force + gateway-down.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check test/cli/destroy-gateway-unreachable.test.ts — it only asserts registryHasAlpha and exit code.
  • Evidence: E2E test only checks r.code === 0, r.out contains 'Sandbox destroyed', registryHasAlpha === false.

PRA-12 Resolve/justify — --non-interactive flag inconsistency: destroy.ts only checks env var

  • Location: src/lib/actions/sandbox/destroy.ts:88
  • Category: correctness
  • Problem: isNonInteractive() in destroy.ts only checks process.env.NEMOCLAW_NON_INTERACTIVE === '1' but onboard.ts checks (NON_INTERACTIVE || process.env.NEMOCLAW_NON_INTERACTIVE === '1') where NON_INTERACTIVE is a module-level flag set by the --non-interactive CLI flag. This means --non-interactive flag works for onboard but NOT for destroy, creating inconsistent behavior.
  • Impact: Users expecting --non-interactive to work for destroy (as it does for onboard) will get interactive prompts, breaking CI automation.
  • Recommended action: Either lift to shared core (PRA-11) which would fix the inconsistency, or update destroy.ts to also check the module-level flag (requires importing from onboard or using a shared source).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Compare onboard.ts:668 (let NON_INTERACTIVE = false) and :675 (NON_INTERACTIVE || env) vs destroy.ts:88 (only env).
  • Missing regression test: Add test verifying --non-interactive flag (not just env var) works in destroy flow.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Compare onboard.ts:668 (let NON_INTERACTIVE = false) and :675 (NON_INTERACTIVE || env) vs destroy.ts:88 (only env).
  • Evidence: onboard.ts exports isNonInteractive but destroy.ts redefines it locally with subset logic.

PRA-13 Resolve/justify — Regex 'connection reset' specifically catches 'connection reset by peer' (gateway-up rejection)

  • Location: src/lib/domain/sandbox/destroy.ts:24
  • Category: security
  • Problem: The regex pattern 'connection reset' (without 'by peer') will match 'connection reset by peer' which is a gateway-up TCP-level rejection (the gateway accepted the connection but then reset it). This is distinct from 'connection refused' (gateway not listening). The current test does not cover this case.
  • Impact: A gateway that is UP but resetting connections (e.g., overloaded, crashing mid-request) will be misclassified as gateway-down, enabling --force bypass.
  • Recommended action: Either remove 'connection reset' from the regex, or make it more specific (e.g., 'connection refused|connection reset' with localhost context), or add explicit negative pattern for 'connection reset by peer'.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Run: expect(isGatewayUnreachableDeleteOutput('connection reset by peer')).toBe(false) — currently returns TRUE (bug).
  • Missing regression test: Add test case: expect(isGatewayUnreachableDeleteOutput('connection reset by peer')).toBe(false).
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Run: expect(isGatewayUnreachableDeleteOutput('connection reset by peer')).toBe(false) — currently returns TRUE (bug).
  • Evidence: Regex includes 'connection reset' as alternation; 'connection reset by peer' is a common gateway-up error distinct from 'connection refused'.

PRA-14 Improvement — Lift isNonInteractive() to src/lib/core/non-interactive.ts

  • Location: src/lib/actions/sandbox/destroy.ts:88
  • Category: correctness
  • Problem: isNonInteractive() should be lifted to src/lib/core/non-interactive.ts as a shared utility. This is an in-scope improvement since the duplication is in the changed file.
  • Impact: Eliminates duplication drift risk; ensures consistent non-interactive behavior across all callers.
  • Suggested action: Create src/lib/core/non-interactive.ts, move the canonical implementation there (checking both module flag and env var), update destroy.ts, policy-channel.ts, proxy.ts to import from it.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check if src/lib/core/ exists and create non-interactive.ts there.
  • Missing regression test: No new test — refactoring. Verify all callers still work.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: 4+ files duplicate or import this logic; core/ directory exists for shared utilities.

PRA-15 Improvement — Single-use PATH construction in E2E test — extract to constant

  • Location: test/cli/destroy-gateway-unreachable.test.ts:71
  • Category: architecture
  • Problem: Single-use PATH construction in E2E test: `${localBin}:${process.env.PATH || ''}` appears inline at lines 71 and 90. Should be extracted to a const at fixture scope for readability.
  • Impact: Minor readability/maintainability issue; duplicated string construction.
  • Suggested action: Extract to const testPath = `${localBin}:${process.env.PATH || ''}` at fixture scope.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read lines 71 and 90 in test/cli/destroy-gateway-unreachable.test.ts.
  • Missing regression test: No test needed — code style.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Two identical inline PATH constructions in the two test cases.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.ts`:
- Around line 434-439: The host-service shutdown decision is using
forcedLocalCleanup as if the sandbox delete was confirmed, which can stop shared
services for a sandbox that may still exist. Update destroy.ts so
shouldStopHostServicesAfterDestroy() receives only the real delete-confirmed
state from deleteSucceededOrAlreadyGone (or alreadyGone), and keep
forcedLocalCleanup limited to local artifact cleanup in the destroy flow. Verify
the call site around shouldStopHostServicesAfterDestroy and the destroy cleanup
path do not let forcedLocalCleanup bypass the public delete confirmation
boundary.
🪄 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: ce1277fe-b9c8-4ec1-a7ea-8e32bdc0c57d

📥 Commits

Reviewing files that changed from the base of the PR and between 09bef70 and dae074b.

📒 Files selected for processing (4)
  • src/lib/actions/sandbox/destroy.ts
  • src/lib/domain/sandbox/destroy.test.ts
  • src/lib/domain/sandbox/destroy.ts
  • test/cli/destroy-gateway-unreachable.test.ts

Comment thread src/lib/actions/sandbox/destroy.ts Outdated
…onsumers (#6046)

getSandboxDeleteOutcome now returns a third field (gatewayUnreachable); update the image-cleanup equality assertion and the snapshot test mock to match, fixing the cli-test-shards (5) failure.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Jason Ma <jama@nvidia.com>
@wscurran wscurran added area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression labels Jun 30, 2026
@wscurran

Copy link
Copy Markdown
Contributor

…eanup (#6046)

Forced local cleanup (gateway unreachable + --force) removes the local
sandbox record but cannot confirm the gateway-side delete — the sandbox
may still exist. The host-service teardown decision passed
`deleteSucceededOrAlreadyGone || forcedLocalCleanup` into
shouldStopHostServicesAfterDestroy(), so a forced cleanup of the last
registered sandbox would stop shared host services for a sandbox that was
never confirmed deleted (CodeRabbit review on #6050).

Gate host-service teardown on the confirmed delete state only, matching
the neighboring gateway-cleanup decision. Add a destroy-flow regression
test asserting stopAll() is not called when --force cleans up the last
sandbox with the gateway down.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Jason Ma <jama@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/lib/actions/sandbox/destroy.ts (1)

422-432: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Final success message contradicts the forced-cleanup warning.

Line 492 unconditionally prints ✓ Sandbox '${sandboxName}' destroyed even when forcedLocalCleanup is true. But the warnings just printed at lines 429-431 explicitly say the gateway-side delete was never confirmed and the sandbox "may still exist." Ending with an unqualified success checkmark contradicts that warning and could lead users to believe cleanup is fully done and stop following up (e.g. never running openshell sandbox delete once the gateway returns).

🪄 Proposed fix
-  console.log(`  ${G}✓${R} Sandbox '${sandboxName}' destroyed`);
+  if (forcedLocalCleanup) {
+    console.log(
+      `  ${G}✓${R} Local record for '${sandboxName}' removed; gateway-side delete unconfirmed.`,
+    );
+  } else {
+    console.log(`  ${G}✓${R} Sandbox '${sandboxName}' destroyed`);
+  }

As per path instructions, "Destructive lifecycle operations must validate before mutation, preserve state/backup invariants, and cover failure, recovery, rebuild, and resume behavior without bypassing the public action boundary."

Also applies to: 486-492

🤖 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/destroy.ts` around lines 422 - 432, The final success
message in destroy flow is misleading when forcedLocalCleanup is true, because
the sandbox was only removed locally and the gateway-side delete was not
confirmed. Update the success reporting in destroy.ts around the
forcedLocalCleanup branch and the final success print so that the checkmark
message is conditional on a confirmed remote destroy; otherwise emit a clearly
qualified message that reflects the local-only cleanup state. Use
forcedLocalCleanup, sandboxName, and the existing success/warning logging in
destroy() to keep the wording consistent.

Source: Path instructions

🧹 Nitpick comments (1)
src/lib/actions/sandbox/destroy-flow.test.ts (1)

186-205: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Good regression coverage for the host-service gating fix.

Asserts observable side effects (removeSandboxSpy called, stopAllSpy/cleanupGatewaySpy not called, no process.exit) via the public destroySandbox call rather than internal implementation details — solid behavioral test for the #6046 fix.

Consider also asserting the console output doesn't falsely claim full success, given the sibling destroy.ts comment about the misleading "✓ destroyed" message on the forced-cleanup path.

🤖 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/destroy-flow.test.ts` around lines 186 - 205, The
forced-cleanup regression test should also verify the user-facing output from
destroySandbox does not report a misleading full-success message on the
gateway-unconfirmed path. Update the existing destroy-flow test to assert the
relevant console/logger output from createDestroyHarness or the destroySandbox
call excludes the “✓ destroyed” success text when deleteStatus/deleteOutput
indicate the unconfirmed cleanup case. Keep the focus on observable behavior
alongside the existing stopAllSpy, cleanupGatewaySpy, and exitSpy assertions.
🤖 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.

Outside diff comments:
In `@src/lib/actions/sandbox/destroy.ts`:
- Around line 422-432: The final success message in destroy flow is misleading
when forcedLocalCleanup is true, because the sandbox was only removed locally
and the gateway-side delete was not confirmed. Update the success reporting in
destroy.ts around the forcedLocalCleanup branch and the final success print so
that the checkmark message is conditional on a confirmed remote destroy;
otherwise emit a clearly qualified message that reflects the local-only cleanup
state. Use forcedLocalCleanup, sandboxName, and the existing success/warning
logging in destroy() to keep the wording consistent.

---

Nitpick comments:
In `@src/lib/actions/sandbox/destroy-flow.test.ts`:
- Around line 186-205: The forced-cleanup regression test should also verify the
user-facing output from destroySandbox does not report a misleading full-success
message on the gateway-unconfirmed path. Update the existing destroy-flow test
to assert the relevant console/logger output from createDestroyHarness or the
destroySandbox call excludes the “✓ destroyed” success text when
deleteStatus/deleteOutput indicate the unconfirmed cleanup case. Keep the focus
on observable behavior alongside the existing stopAllSpy, cleanupGatewaySpy, and
exitSpy assertions.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 18e8868e-fef4-4999-82f7-51d67ce34943

📥 Commits

Reviewing files that changed from the base of the PR and between ca89987 and 4c79215.

📒 Files selected for processing (2)
  • src/lib/actions/sandbox/destroy-flow.test.ts
  • src/lib/actions/sandbox/destroy.ts

…6046)

Integrate origin/main into fix/6046-destroy-force-gateway-down.

main wrapped the destroy destructive path (wipe + active-timer shieldsUp
hardening + provider detach + delete) in withTimerBoundShieldsMutationLock,
returning a fail-closed {ok:false} on delete failure. This branch added the
--force gateway-unreachable local-cleanup fallback (#6046). Resolution folds
the forcedLocalCleanup decision inside the lock: an unreachable-gateway delete
under --force returns ok:true (proceed to local cleanup) instead of failing
closed, while every other delete failure still preserves the hardened locked
state. gatewayUnreachable is surfaced to the post-lock failure branch so the
no --force path keeps main's error plus the recovery hint. deleteSucceededOrAlreadyGone
stays false for forcedLocalCleanup, so host-service and gateway teardown remain
gated on a confirmed delete.

Tests: kept main's active-timer ordering/hardening-failure cases and this
branch's --force host-service-preservation case; unioned DestroyHarness
options (registeredSandboxCount + shieldsUpError) and the harness spies
(stopAllSpy + shieldsUpSpy + event-tracking killTimerSpy).

Verified locally: typecheck:cli clean, biome clean, destroy-flow /
destroy-gateway-unreachable / snapshot / domain destroy / image-cleanup
suites green.

Signed-off-by: Jason Ma <jama@nvidia.com>
@cv
cv merged commit 9e63c4a into main Jul 3, 2026
46 checks passed
@cv
cv deleted the fix/6046-destroy-force-gateway-down branch July 3, 2026 07:40
@ericksoa ericksoa mentioned this pull request Jul 4, 2026
21 tasks
ericksoa added a commit that referenced this pull request Jul 4, 2026
<!-- markdownlint-disable MD041 -->
## Summary
This PR prepares the user-facing documentation for v0.0.74 before the
release plan is frozen.
It expands the release notes across the 56-commit train and closes
durable documentation gaps found during the pre-tag commit scan.

## Changes
- Expand the `v0.0.74` release notes to cover OpenShell 0.0.72, managed
MCP, progressive tool disclosure, LangChain Deep Agents Code,
onboarding, local inference, messaging, recovery, and contributor
workflows.
- Correct the `destroy` contract for retained per-name volumes,
gateway-unreachable `--force` cleanup, managed MCP ownership, and
same-name recovery.
- Document separate remediation for an unreachable container DNS
resolver versus one that answers with `NXDOMAIN` or `REFUSED`.
- Document the Windows on Arm N1X automatic Ollama safeguard and its
remaining large-model limitations.
- State that messaging conflicts abort rebuild before backup or
deletion, leaving the original sandbox intact.
- Link the agent-runnable value benchmark from the contributor task
index.
- Synchronize generated agent command variants.
- Validate with `npm run docs:sync-agent-variants` and `npm run docs`;
Fern completed with 0 errors and 2 existing warnings.
- Source summary:
- [#6020](#6020) and
[#5876](#5876) ->
`docs/about/release-notes.mdx`: Consolidate the OpenShell 0.0.72 policy
boundary and managed MCP lifecycle.
- [#6251](#6251) and
[#5989](#5989) ->
`docs/about/release-notes.mdx`: Summarize progressive tool disclosure
and sandbox-first inference controls.
- [#6232](#6232),
[#6082](#6082),
[#6219](#6219),
[#6214](#6214),
[#6215](#6215),
[#6230](#6230), and
[#6260](#6260) ->
`docs/about/release-notes.mdx`: Summarize the experimental LangChain
Deep Agents Code status, secret, version, rebuild, snapshot, and MCP
boundaries.
- [#6166](#6166),
[#6254](#6254),
[#6265](#6265),
[#6164](#6164), and
[#6017](#6017) ->
`docs/about/release-notes.mdx`: Summarize BuildKit prebuild, validated
image reuse, bounded readiness, and preflight improvements.
- [#6150](#6150) ->
`docs/about/release-notes.mdx` and `docs/reference/troubleshooting.mdx`:
Separate unreachable-resolver remediation from reachable-but-rejected
DNS responses.
- [#6234](#6234) ->
`docs/about/release-notes.mdx`,
`docs/inference/use-local-inference.mdx`, and
`docs/get-started/windows-preparation.mdx`: Document N1X automatic 9B
selection and the remaining explicit-large-model boundary.
- [#6129](#6129),
[#5987](#5987),
[#5955](#5955), and
[#6220](#6220) ->
`docs/about/release-notes.mdx`,
`docs/manage-sandboxes/messaging-channels.mdx`,
`docs/reference/commands.mdx`, and
`docs/reference/commands-nemohermes.mdx`: Document messaging policy
persistence, status, and the pre-destructive conflict check.
- [#5963](#5963),
[#6050](#6050),
[#6094](#6094),
[#6238](#6238),
[#5988](#5988),
[#6235](#6235),
[#6181](#6181), and
[#5986](#5986) ->
`docs/about/release-notes.mdx`, `docs/reference/commands.mdx`, and
`docs/reference/commands-nemohermes.mdx`: Summarize day-two recovery and
clarify retained-volume and local-only destroy semantics.
- [#6200](#6200),
[#6248](#6248),
[#6168](#6168),
[#6270](#6270), and
[#5649](#5649) ->
`docs/about/release-notes.mdx` and `CONTRIBUTING.md`: Summarize
contributor setup and verification improvements and expose the advisory
value benchmark.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [x] Doc only (includes code sample changes)

## Quality Gates
<!-- Check exactly one tests line and one docs line. Check other lines
when applicable. Add every requested justification or approval
reference. -->
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: documentation-only release
preparation; generated-variant synchronization and the Fern docs build
validate the changed pages 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
<!-- Check each applicable item only when supported by the requested
evidence. Run targeted tests once per relevant change set and rerun
after later edits or hook autofixes that can affect the tested behavior.
Do not rerun hook-covered checks. -->
- [x] PR description includes the DCO sign-off declaration 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 — command/result or justification: tests
are not applicable to this documentation-only change; `npm run docs`
validates the source and generated routes.
- [ ] Applicable broad gate passed — `npm test` for broad
runtime/test-harness changes; `npm run check` for repo-wide
validation/coverage changes — command/result:
- [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)
- [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)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
* Expanded setup guidance for Windows on Arm devices with safer default
local model selection.
* Clarified local inference and sandbox messaging behavior, including
conflict checks before rebuilds and safer recovery steps.
* Updated destroy/rebuild/reference docs with more detailed warnings,
failure handling, and volume-retention guidance.
* Improved troubleshooting instructions for Docker DNS issues with
clearer paths for unreachable vs. blocked resolvers.

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

---------

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
… is down (NVIDIA#6046) (NVIDIA#6050)

## Summary

When the OpenShell gateway is not listening on `127.0.0.1:8080`, every
gateway call — including the final `sandbox delete` — returns a
connection-refused/transport error. `destroy` treated that as fatal with
no bypass (neither `--force` nor `--yes` helped, since both only skip
the confirmation prompt), so there was no supported way to remove a
sandbox while the gateway was down. This makes `--force` fall back to
local cleanup.

## Related Issue

Fixes NVIDIA#6046

## Changes

- `src/lib/domain/sandbox/destroy.ts`: add
`isGatewayUnreachableDeleteOutput()` and surface `gatewayUnreachable`
from `getSandboxDeleteOutcome()` — classifying gateway-transport
failures (connection refused / `os error 61|111` / tcp connect error /
…) separately from real delete rejections.
- `src/lib/actions/sandbox/destroy.ts`:
- With `--force` + gateway-unreachable: fall back to **local cleanup**
(remove the registry entry and local artifacts) with a clear warning
that the sandbox may still exist if the gateway returns. Gateway
teardown is intentionally skipped (the gateway-side delete was not
confirmed).
- Without `--force`: still fails, but now points at the recovery paths
(`<name> status` to start the gateway, or `--force`).
  - Real (non-transport) delete errors stay fatal, unchanged.
- Tests: domain classification unit tests; CLI E2E (`destroy --force`
removes the local record when the fake gateway delete returns
connection-refused; `destroy -y` without `--force` fails with the
recovery hint and preserves the registry entry).

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

## Quality Gates
- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [ ] Docs updated for user-facing behavior changes
- [x] Docs not applicable — justification: `--force` is already
documented for destroy; this extends its effect (gateway-down fallback)
without a new flag/command surface.
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [x] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification:
sandbox-destroy/data-lifecycle path. The fallback is gated behind
explicit `--force` AND a gateway-transport classification; it only
removes the *local* record (no gateway teardown), warns that the sandbox
may persist, and leaves real delete errors fatal. The pre-existing
`alreadyGone` / real-error paths are unchanged and still covered (37
destroy/rebuild tests pass).
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [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)
- [ ] Doc pages follow the style guide (doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

---
Signed-off-by: Jason Ma <jama@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
* Improved `sandbox delete` handling when the remote gateway is
unreachable by detecting connection/transport failures and surfacing
that outcome to callers.
* `alpha destroy --force` now safely performs local cleanup even when
gateway-side deletion can’t be reached, while avoiding shared host
teardown.
* Non-forced runs now provide clearer recovery messaging, including
guidance to retry with `--force` when the gateway is unavailable.
* Warning text now clarifies that local sandbox removal happened without
confirming gateway-side deletion.
* **Tests**
* Added CLI regression coverage for the gateway-unreachable `alpha
destroy` scenario.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Jason Ma <jama@nvidia.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
<!-- markdownlint-disable MD041 -->
## Summary
This PR prepares the user-facing documentation for v0.0.74 before the
release plan is frozen.
It expands the release notes across the 56-commit train and closes
durable documentation gaps found during the pre-tag commit scan.

## Changes
- Expand the `v0.0.74` release notes to cover OpenShell 0.0.72, managed
MCP, progressive tool disclosure, LangChain Deep Agents Code,
onboarding, local inference, messaging, recovery, and contributor
workflows.
- Correct the `destroy` contract for retained per-name volumes,
gateway-unreachable `--force` cleanup, managed MCP ownership, and
same-name recovery.
- Document separate remediation for an unreachable container DNS
resolver versus one that answers with `NXDOMAIN` or `REFUSED`.
- Document the Windows on Arm N1X automatic Ollama safeguard and its
remaining large-model limitations.
- State that messaging conflicts abort rebuild before backup or
deletion, leaving the original sandbox intact.
- Link the agent-runnable value benchmark from the contributor task
index.
- Synchronize generated agent command variants.
- Validate with `npm run docs:sync-agent-variants` and `npm run docs`;
Fern completed with 0 errors and 2 existing warnings.
- Source summary:
- [NVIDIA#6020](NVIDIA#6020) and
[NVIDIA#5876](NVIDIA#5876) ->
`docs/about/release-notes.mdx`: Consolidate the OpenShell 0.0.72 policy
boundary and managed MCP lifecycle.
- [NVIDIA#6251](NVIDIA#6251) and
[NVIDIA#5989](NVIDIA#5989) ->
`docs/about/release-notes.mdx`: Summarize progressive tool disclosure
and sandbox-first inference controls.
- [NVIDIA#6232](NVIDIA#6232),
[NVIDIA#6082](NVIDIA#6082),
[NVIDIA#6219](NVIDIA#6219),
[NVIDIA#6214](NVIDIA#6214),
[NVIDIA#6215](NVIDIA#6215),
[NVIDIA#6230](NVIDIA#6230), and
[NVIDIA#6260](NVIDIA#6260) ->
`docs/about/release-notes.mdx`: Summarize the experimental LangChain
Deep Agents Code status, secret, version, rebuild, snapshot, and MCP
boundaries.
- [NVIDIA#6166](NVIDIA#6166),
[NVIDIA#6254](NVIDIA#6254),
[NVIDIA#6265](NVIDIA#6265),
[NVIDIA#6164](NVIDIA#6164), and
[NVIDIA#6017](NVIDIA#6017) ->
`docs/about/release-notes.mdx`: Summarize BuildKit prebuild, validated
image reuse, bounded readiness, and preflight improvements.
- [NVIDIA#6150](NVIDIA#6150) ->
`docs/about/release-notes.mdx` and `docs/reference/troubleshooting.mdx`:
Separate unreachable-resolver remediation from reachable-but-rejected
DNS responses.
- [NVIDIA#6234](NVIDIA#6234) ->
`docs/about/release-notes.mdx`,
`docs/inference/use-local-inference.mdx`, and
`docs/get-started/windows-preparation.mdx`: Document N1X automatic 9B
selection and the remaining explicit-large-model boundary.
- [NVIDIA#6129](NVIDIA#6129),
[NVIDIA#5987](NVIDIA#5987),
[NVIDIA#5955](NVIDIA#5955), and
[NVIDIA#6220](NVIDIA#6220) ->
`docs/about/release-notes.mdx`,
`docs/manage-sandboxes/messaging-channels.mdx`,
`docs/reference/commands.mdx`, and
`docs/reference/commands-nemohermes.mdx`: Document messaging policy
persistence, status, and the pre-destructive conflict check.
- [NVIDIA#5963](NVIDIA#5963),
[NVIDIA#6050](NVIDIA#6050),
[NVIDIA#6094](NVIDIA#6094),
[NVIDIA#6238](NVIDIA#6238),
[NVIDIA#5988](NVIDIA#5988),
[NVIDIA#6235](NVIDIA#6235),
[NVIDIA#6181](NVIDIA#6181), and
[NVIDIA#5986](NVIDIA#5986) ->
`docs/about/release-notes.mdx`, `docs/reference/commands.mdx`, and
`docs/reference/commands-nemohermes.mdx`: Summarize day-two recovery and
clarify retained-volume and local-only destroy semantics.
- [NVIDIA#6200](NVIDIA#6200),
[NVIDIA#6248](NVIDIA#6248),
[NVIDIA#6168](NVIDIA#6168),
[NVIDIA#6270](NVIDIA#6270), and
[NVIDIA#5649](NVIDIA#5649) ->
`docs/about/release-notes.mdx` and `CONTRIBUTING.md`: Summarize
contributor setup and verification improvements and expose the advisory
value benchmark.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [x] Doc only (includes code sample changes)

## Quality Gates
<!-- Check exactly one tests line and one docs line. Check other lines
when applicable. Add every requested justification or approval
reference. -->
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: documentation-only release
preparation; generated-variant synchronization and the Fern docs build
validate the changed pages 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
<!-- Check each applicable item only when supported by the requested
evidence. Run targeted tests once per relevant change set and rerun
after later edits or hook autofixes that can affect the tested behavior.
Do not rerun hook-covered checks. -->
- [x] PR description includes the DCO sign-off declaration 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 — command/result or justification: tests
are not applicable to this documentation-only change; `npm run docs`
validates the source and generated routes.
- [ ] Applicable broad gate passed — `npm test` for broad
runtime/test-harness changes; `npm run check` for repo-wide
validation/coverage changes — command/result:
- [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)
- [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)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
* Expanded setup guidance for Windows on Arm devices with safer default
local model selection.
* Clarified local inference and sandbox messaging behavior, including
conflict checks before rebuilds and safer recovery steps.
* Updated destroy/rebuild/reference docs with more detailed warnings,
failure handling, and volume-retention guidance.
* Improved troubleshooting instructions for Docker DNS issues with
clearer paths for unreachable vs. blocked resolvers.

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

---------

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

Labels

area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[macOS][Sandbox] nemoclaw destroy aborts when the OpenShell gateway is down — no auto-recover, no --force bypass

3 participants