fix(installer): report orphaned sandboxes honestly across uninstall and reinstall#6539
Merged
Merged
Conversation
…nd reinstall `nemoclaw uninstall --yes` preserves sandboxes.json by design but removes the gateway registration, provider registrations, and Docker image its recorded sandboxes depend on. A later reinstall could not recover those records, yet reported "Installation complete" and "Existing sandboxes were recovered and upgraded" — false success that silently stranded the user's sandbox with no remediation path. The root cause is that no layer could tell a genuinely orphaned record apart from benign skips. Staleness could not carry the signal either: a same-version reinstall classifies the orphan "current" and never surfaces it at all, while keying on the generic "not observed on the selected gateway" skip line would misfire on sandboxes healthy on another live gateway or ones reconnecting mid-run. The CLI now derives a dedicated orphan signal from observation rather than version state: a recorded sandbox absent from the selected gateway in any phase, whose persisted binding resolves to that same gateway, has nowhere else to be running. Sandboxes bound to other gateways, exonerated by the confirming second listing, or restored by prepared-backup recovery are excluded. The marker prints with concrete remediation (`<name> destroy`, then `onboard`) and is exported as a shared constant so the installer harness drives the real grep against it — drift on either side fails the suite. install.sh mirrors recovery output through a trap-registered temp log (taking the CLI's own exit status via PIPESTATUS so a tee failure cannot fake the #5735 failure path), downgrades the banner to "completed with warnings", and replaces the false recovery claim with hedged wording and the same remediation. Uninstall now warns at preserve time that the kept registry is not self-sufficient, uses "already removed or unreachable" wording for provider/sandbox delete no-ops instead of the contradictory "Deleted X skipped", and stale listings label version regressions with a "(downgrade)" suffix instead of framing them as routine upgrades. Fixes #6520 Signed-off-by: Dongni Yang <dongniy@nvidia.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces orphaned sandbox detection for recorded sandboxes missing from their recorded gateway, then threads that state through upgrade-sandboxes, install recovery, uninstall warnings, stale-upgrade labeling, and related documentation/tests. ChangesOrphaned sandbox recovery and remediation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-6539.docs.buildwithfern.com/nemoclaw |
The codebase-growth guardrail requires changed test files to stay free of if statements; express the run stub's command dispatch as a conditional expression instead. Refs #6520 Signed-off-by: Dongni Yang <dongniy@nvidia.com>
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Contributor
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 3 items to resolve/justify, 0 in-scope improvements
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/reference/commands.mdx (1)
2415-2418: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the em dash.
Line 2418 uses an em dash ("— this typically means..."). As per path instructions, docs should "Avoid filler, hype, rhetorical questions, emoji, em-dashes, and unnecessary bold text."
✏️ Proposed fix
-A recorded sandbox that is not observed in any phase on its own recorded gateway is reported as not found there, with remediation guidance — this typically means its gateway registration or Docker image was removed (for example by `$$nemoclaw uninstall`, which preserves `sandboxes.json` but removes both). +A recorded sandbox that is not observed in any phase on its own recorded gateway is reported as not found there, with remediation guidance. This typically means its gateway registration or Docker image was removed (for example by `$$nemoclaw uninstall`, which preserves `sandboxes.json` but removes both).🤖 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 `@docs/reference/commands.mdx` around lines 2415 - 2418, Update the prose in the relevant docs entry under the commands reference to remove the em dash in the sentence describing a recorded sandbox not found on its recorded gateway; rewrite that clause as a plain sentence or use a period/semicolon instead. Keep the meaning and remediation guidance intact, and make sure the edited text stays consistent with the surrounding command documentation style in the same section.Source: Path instructions
🤖 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/uninstall/run-plan-preserved-registry.test.ts`:
- Around line 70-74: The test mock in run-plan-preserved-registry.test.ts is
using branching inside the run helper to dispatch behavior based on command and
args, which hides incorrect calls instead of asserting them. Refactor the mock
around the run function in the test setup to avoid if-based logic: use separate
scenario-specific deps/mocks or a simple command-to-result lookup so each case
is explicit and observable. Keep the behavior coverage the same, but remove the
conditional dispatch from the mock and assert directly on the expected command
invocation and returned value.
In `@src/lib/actions/upgrade-sandboxes.ts`:
- Around line 352-384: The orphan detection in upgrade-sandboxes is overlapping
with the version classification, so the same sandbox can be printed twice.
Update the logic around `checkAgentVersionForUpgrade`, `unknown`, and
`unobservedOwnGatewaySandboxes` so orphaned records are excluded from the
version-based lists or the orphan print path is skipped when they already appear
there. Keep the deduping focused on the `printOrphanedRegistrySandboxes` branch
and the `unknown` classification flow.
---
Nitpick comments:
In `@docs/reference/commands.mdx`:
- Around line 2415-2418: Update the prose in the relevant docs entry under the
commands reference to remove the em dash in the sentence describing a recorded
sandbox not found on its recorded gateway; rewrite that clause as a plain
sentence or use a period/semicolon instead. Keep the meaning and remediation
guidance intact, and make sure the edited text stays consistent with the
surrounding command documentation style in the same section.
🪄 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: e767ab8c-d790-422a-a9f8-18acb11c04c7
📒 Files selected for processing (11)
docs/get-started/quickstart.mdxdocs/manage-sandboxes/lifecycle.mdxdocs/reference/commands.mdxdocs/reference/host-files-and-state.mdxscripts/install.shsrc/lib/actions/uninstall/run-plan-preserved-registry.test.tssrc/lib/actions/uninstall/run-plan.tssrc/lib/actions/upgrade-sandboxes-display.test.tssrc/lib/actions/upgrade-sandboxes-recovery.test.tssrc/lib/actions/upgrade-sandboxes.tstest/install-orphaned-sandbox-recovery.test.ts
An orphaned record with no cached or probeable version landed in both the "Unknown version" bucket (start-and-rerun guidance) and the orphan block (destroy/onboard remediation) — conflicting advice for the same sandbox. Its version is unknown because the sandbox is gone, so only the orphan block reports it. Stale orphans keep their stale listing: version drift there is real information. Refs #6520 Signed-off-by: Dongni Yang <dongniy@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/actions/upgrade-sandboxes-recovery.test.ts (1)
372-394: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest correctly validates the unknown-version orphan de-duplication fix.
Title follows behavior-oriented +
(#1234)suffix convention, and the assertions target the public console-output boundary rather than internals.One optional gap: unlike the sibling orphan tests in this suite, this test doesn't assert
rebuildSpywas not called, even thoughNEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATEis set to"0". Adding that assertion would tighten confidence that no rebuild attempt is triggered for this orphan.✅ Optional strengthening
expect(console.log).toHaveBeenCalledWith( expect.stringContaining("were not found on their recorded gateway: my-assistant"), ); expect(console.log).not.toHaveBeenCalledWith(expect.stringContaining("Unknown version")); + expect(harness.rebuildSpy).not.toHaveBeenCalled(); });🤖 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/upgrade-sandboxes-recovery.test.ts` around lines 372 - 394, The new orphan de-duplication test in upgradeSandboxes should also verify that no rebuild is attempted when NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE is "0". Update the test around createRecoveryHarness and upgradeSandboxes to assert rebuildSpy was not called, matching the sibling orphan cases and strengthening the public-behavior coverage without changing the existing console-output 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.
Nitpick comments:
In `@src/lib/actions/upgrade-sandboxes-recovery.test.ts`:
- Around line 372-394: The new orphan de-duplication test in upgradeSandboxes
should also verify that no rebuild is attempted when
NEMOCLAW_RESTORE_LATEST_BACKUP_ON_RECREATE is "0". Update the test around
createRecoveryHarness and upgradeSandboxes to assert rebuildSpy was not called,
matching the sibling orphan cases and strengthening the public-behavior coverage
without changing the existing console-output assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 80d63cb7-c4a6-4f16-a916-909e2ad55b91
📒 Files selected for processing (2)
src/lib/actions/upgrade-sandboxes-recovery.test.tssrc/lib/actions/upgrade-sandboxes.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/actions/upgrade-sandboxes.ts
21 tasks
… into domain modules PR Review Advisor findings on #6539: upgrade-sandboxes.ts and run-plan.ts absorbed the #6520 logic as monolith growth. Move the pure pieces to the domain layer where they are independently unit-tested: - domain/maintenance/orphan-detection.ts: ORPHANED_SANDBOX_MARKER, the own-gateway orphan classification, and the summary/remediation wording; the action file keeps only a rendering wrapper. The install.sh harness now imports the marker from the domain module. - domain/maintenance/upgrade.ts: describeStaleUpgrade with the downgrade suffix, plus compareDottedVersions as its canonical home (onboard/docker-driver-gateway-compat re-exports it for existing consumers). Display tests move next to the domain module. - domain/uninstall/messaging.ts: preserved-registry warnings and the delete/no-op skip wording, shared by run-plan.ts. Also print the orphan diagnosis in `upgrade-sandboxes --check` so check mode and auto mode agree on the source-of-truth diagnosis (advisor PRA-2), covered by a new recovery test. Refs #6520 Signed-off-by: Dongni Yang <dongniy@nvidia.com>
Contributor
Author
|
PR Review Advisor disposition (54ffc02):
Signed-off-by: Dongni Yang dongniy@nvidia.com |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/upgrade-sandboxes.ts (1)
316-329: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExclude rejected recoveries from the orphan summary
src/lib/actions/upgrade-sandboxes.ts:318-329, 459-461
rejectedRecoveriescan still satisfyclassifyOrphanedRegistrySandboxes(...), so a sandbox can be printed once under “Backup recovery blocked:” and again in the orphan block at the end. Filter rejected names out beforeprintOrphanedRegistrySandboxes(...)and add coverage for the rejected-recovery + orphan overlap.🤖 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/upgrade-sandboxes.ts` around lines 316 - 329, Rejected recoveries are still being treated as orphans, which can cause the same sandbox to appear in both the recovery-blocked summary and the orphan summary. Update the logic around classifyOrphanedRegistrySandboxes in upgrade-sandboxes.ts to exclude rejectedRecovery names before calling printOrphanedRegistrySandboxes, and make sure the orphan filtering also respects the rejectedRecoveries set when building the final lists. Add a test that covers the overlap between rejected recoveries and orphaned sandboxes so the same sandbox is not printed twice.
🤖 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/domain/maintenance/orphan-detection.test.ts`:
- Around line 15-25: The helper in classify currently introduces a new if
statement inside resolveGatewayBinding, which violates the Codebase Growth
Guardrails for this test. Refactor the mock in classifyOrphanedRegistrySandboxes
to avoid any conditional statement, using a conditional expression or a small
throwing helper for the "corrupt" gatewayName case instead. Keep the existing
behavior for Entry inputs and the observedNames/reconnectedNames setup
unchanged.
---
Outside diff comments:
In `@src/lib/actions/upgrade-sandboxes.ts`:
- Around line 316-329: Rejected recoveries are still being treated as orphans,
which can cause the same sandbox to appear in both the recovery-blocked summary
and the orphan summary. Update the logic around
classifyOrphanedRegistrySandboxes in upgrade-sandboxes.ts to exclude
rejectedRecovery names before calling printOrphanedRegistrySandboxes, and make
sure the orphan filtering also respects the rejectedRecoveries set when building
the final lists. Add a test that covers the overlap between rejected recoveries
and orphaned sandboxes so the same sandbox is not printed twice.
🪄 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: 0bca91ba-0e8c-47b2-97cf-b040a0487b7f
📒 Files selected for processing (11)
src/lib/actions/uninstall/run-plan.tssrc/lib/actions/upgrade-sandboxes-recovery.test.tssrc/lib/actions/upgrade-sandboxes.tssrc/lib/domain/maintenance/orphan-detection.test.tssrc/lib/domain/maintenance/orphan-detection.tssrc/lib/domain/maintenance/upgrade.test.tssrc/lib/domain/maintenance/upgrade.tssrc/lib/domain/uninstall/messaging.test.tssrc/lib/domain/uninstall/messaging.tssrc/lib/onboard/docker-driver-gateway-compat.tstest/install-orphaned-sandbox-recovery.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/domain/uninstall/messaging.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/install-orphaned-sandbox-recovery.test.ts
- src/lib/actions/uninstall/run-plan.ts
…linear The compareDottedVersions move shifted DEFAULT_COMPAT_IMAGE from line 11 to 16 in docker-driver-gateway-compat.ts; update the platform-matrix citation and regenerate the platform docs so the cited line stays non-empty. Also restructure the orphan-detection resolver stub so changed test files stay free of if statements per the growth guardrail. Refs #6520 Signed-off-by: Dongni Yang <dongniy@nvidia.com>
Contributor
Author
|
Live verification on macOS (Mac Studio) + colima — full repro-before / verify-after cycle (answers the PR Review Advisor runtime-validation follow-ups T1–T3/T6):
Environment: macOS (Mac Studio), colima container runtime, installed via Signed-off-by: Dongni Yang dongniy@nvidia.com |
cv
approved these changes
Jul 9, 2026
21 tasks
cv
pushed a commit
that referenced
this pull request
Jul 9, 2026
<!-- markdownlint-disable MD041 --> ## Summary Adds the pre-tag v0.0.79 release notes entry to `docs/about/release-notes.mdx` so the release plan can be generated after docs merge. The entry summarizes the merged v0.0.79 release train across inference, diagnostics, runtime hardening, policies, onboarding recovery, and release validation. ## Changes - Added the v0.0.79 release notes section with linked follow-up documentation for OpenRouter onboarding, managed vLLM changes, completion and logging, Deep Agents runtime limits, policy updates, onboarding recovery, and release validation. - Source summary: - #6461 -> `docs/about/release-notes.mdx`: Documents OpenRouter onboarding support and links to inference/provider references. - #6271 and #6272 -> `docs/about/release-notes.mdx`: Documents shell completion and structured logging highlights. - #6465, #6539, #6570, and #6528 -> `docs/about/release-notes.mdx`: Documents status route-drift, orphaned sandbox, gateway cleanup, and DGX Spark express-install diagnostics. - #6523, #6551, #6484, #6488, #6324, and #6542 -> `docs/about/release-notes.mdx`: Documents managed vLLM, Qwen3.6 tool parser, compaction, and timeout/readiness improvements. - #6559, #6538, #6560, #6568, #6552, #6567, and #6587 -> `docs/about/release-notes.mdx`: Documents runtime, credential, proxy, PID namespace, TOML, and provider-state hardening. - #6541, #5415, #6246, #6496, and #6573 -> `docs/about/release-notes.mdx`: Documents GitHub policy, Gmail policy, MCP allowlist, WhatsApp, and messaging-variant updates. - #6253, #6572, #6444, #6536, and #5860 -> `docs/about/release-notes.mdx`: Documents onboarding resume and create-step recovery improvements. - #6508, #6527, #5506, #6588, #6446, #6447, #6582, #6296, #6367, #6397, and #6505 -> `docs/about/release-notes.mdx`: Documents docs, release-risk, and E2E validation updates. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] 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: Release-note prose only. - [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 not applicable, release-note prose only. - [ ] 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) Docs validation note: `npm run docs:check-agent-variants && npm run docs:check-routes && git diff --check` passed. Full `npm run docs` is currently blocked before Fern validation because the pinned `fern-api@5.65.2` package is unavailable from npm (`ETARGET No matching version found`). --- <!-- 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: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added release notes for v0.0.79 with a new summary of recent improvements, including onboarding and inference options, operator/CLI diagnostics, sandbox recovery hardening, runtime limits, network policy behavior, and release validation updates. * Added updated references and links for the latest release. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
…nd reinstall (NVIDIA#6539) <!-- markdownlint-disable MD041 --> ## Summary `nemoclaw uninstall --yes` preserves `sandboxes.json` but removes the gateway registration, provider registrations, and Docker image its recorded sandboxes depend on; a later reinstall then reported "Installation complete" and "Existing sandboxes were recovered and upgraded" while silently stranding the user's sandbox with no remediation path. This PR makes the whole cycle honest: the CLI derives a dedicated, observation-based orphan signal (independent of version classification, so the same-version reinstall repro is also caught), the installer consumes it to report "completed with warnings" with concrete remediation instead of false success, and uninstall warns at preserve time that the kept registry is not self-sufficient. ## Related Issue Fixes NVIDIA#6520 ## Changes - `src/lib/actions/upgrade-sandboxes.ts`: new orphan classifier — a recorded sandbox absent in any phase from the selected gateway, whose persisted binding resolves to that same gateway, is reported via the exported `ORPHANED_SANDBOX_MARKER` line with `<name> destroy` / `onboard` remediation. Sandboxes bound to another live gateway, exonerated by the confirming second listing, or restored by prepared-backup recovery are excluded, preserving the NVIDIA#6114 multi-gateway exit-0 contract. Stale listings now label version regressions with a `(downgrade)` suffix (`describeStaleUpgrade` exported for tests). - `scripts/install.sh`: `recover_preexisting_sandboxes_before_onboard` mirrors recovery output through a trap-registered temp log (`_cleanup_files`), takes the CLI's own exit status via `PIPESTATUS[0]` so a `tee` write failure cannot fake the NVIDIA#5735 failure path, and greps the orphan marker into `_PREEXISTING_SANDBOX_ORPHANED`; `print_done` downgrades the banner to `=== Installation completed with warnings ===` and replaces the false recovery claim with hedged wording plus remediation; the onboarding-skip line no longer claims recovery in the orphaned case. - `src/lib/actions/uninstall/run-plan.ts`: warns at preserve time that preserved `sandboxes.json` records cannot be recovered automatically on reinstall (non-interactive and interactive keep paths, suppressed under `--destroy-user-data`); provider/sandbox delete no-ops now print `already removed or unreachable` instead of the contradictory `Deleted … skipped`. - Tests: `test/install-orphaned-sandbox-recovery.test.ts` (bash harness driving the real `install.sh` classification and `print_done`; the stub output is built from the exported marker constant so wording drift on either side fails the suite), `src/lib/actions/upgrade-sandboxes-display.test.ts` (downgrade labeling), five orphan-classifier cases in `upgrade-sandboxes-recovery.test.ts` (own-gateway orphan for current- and stale-classified records; negatives for other-gateway, reconnect-race, and recovered sandboxes), `src/lib/actions/uninstall/run-plan-preserved-registry.test.ts` (preserve warning on non-interactive and interactive keep paths, no-op wording fix, purge suppression). - Docs: `reference/commands.mdx`, `manage-sandboxes/lifecycle.mdx`, `get-started/quickstart.mdx`, `reference/host-files-and-state.mdx` updated for the preserve-time warning, the warnings install outcome, the remediation path, and the `(downgrade)` label. ## Source-of-Truth Constraints (advisor PRA-1/6/8) - **Invalid state:** preserved `sandboxes.json` entries whose gateway registration, provider registrations, and Docker image were removed by a prior `nemoclaw uninstall`. - **Source boundary:** the uninstall command's preserve-by-default user-data contract. `--yes` is deliberately non-destructive; existing automation depends on it never purging user data. - **Why the root cause is not fixed here:** making uninstall/reinstall symmetric means either preserving the gateway/providers/image (defeats uninstall) or purging the registry by default (breaks the preserve contract and `--yes` automation). Both are behavior contracts owned by uninstall, out of scope for an honesty fix. The chosen resolution matches the issue's stated "Acceptable" bar: detect, diagnose with remediation, and never report false success. - **Regression tests:** `test/install-orphaned-sandbox-recovery.test.ts` (real install.sh classification + print_done, stub built from the domain marker constant), `src/lib/actions/upgrade-sandboxes-recovery.test.ts` (orphan classifier incl. multi-gateway/reconnect/recovered negatives and check-mode parity), `src/lib/domain/maintenance/orphan-detection.test.ts`, `src/lib/domain/uninstall/messaging.test.ts`, `src/lib/actions/uninstall/run-plan-preserved-registry.test.ts`. - **Removal condition:** when uninstall either preserves everything its recorded sandboxes depend on, purges the registry by default, or records gateway-removal evidence in the registry so reinstall can distinguish states — the observation-based classifier and installer marker grep can then be retired. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] 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. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] 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: `npx vitest run --project cli src/lib/actions/upgrade-sandboxes-display.test.ts src/lib/actions/upgrade-sandboxes-recovery.test.ts src/lib/actions/uninstall/` → 79 passed; `npx vitest run --project integration test/install-orphaned-sandbox-recovery.test.ts test/install-preexisting-sandbox-recovery.test.ts test/install-onboard-exit.test.ts` → 14 passed; `npm run typecheck:cli` clean; `shfmt -i 2 -ci -bn -d scripts/install.sh` clean - [ ] 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: Dongni Yang <dongniy@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added orphan-detection for preserved registry sandboxes to drive more accurate “recovered with warnings” vs “not recovered” messaging. * Enhanced stale-sandbox labeling (downgrade/unknown-build) and refined upgrade/recovery output to reduce ambiguity. * **Bug Fixes** * Uninstall and recovery messaging now better distinguishes unreachable/removal no-ops from successful deletions. * “Installation completed with warnings” is shown when recovery exits cleanly but sandboxes can’t be recovered. * **Documentation** * Updated quickstart, lifecycle, command, and state/uninstall docs with remediation guidance. * **Tests** * Added/expanded Vitest coverage for orphan detection, installer completion honesty, and uninstall messaging. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Dongni Yang <dongniy@nvidia.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary Adds the pre-tag v0.0.79 release notes entry to `docs/about/release-notes.mdx` so the release plan can be generated after docs merge. The entry summarizes the merged v0.0.79 release train across inference, diagnostics, runtime hardening, policies, onboarding recovery, and release validation. ## Changes - Added the v0.0.79 release notes section with linked follow-up documentation for OpenRouter onboarding, managed vLLM changes, completion and logging, Deep Agents runtime limits, policy updates, onboarding recovery, and release validation. - Source summary: - NVIDIA#6461 -> `docs/about/release-notes.mdx`: Documents OpenRouter onboarding support and links to inference/provider references. - NVIDIA#6271 and NVIDIA#6272 -> `docs/about/release-notes.mdx`: Documents shell completion and structured logging highlights. - NVIDIA#6465, NVIDIA#6539, NVIDIA#6570, and NVIDIA#6528 -> `docs/about/release-notes.mdx`: Documents status route-drift, orphaned sandbox, gateway cleanup, and DGX Spark express-install diagnostics. - NVIDIA#6523, NVIDIA#6551, NVIDIA#6484, NVIDIA#6488, NVIDIA#6324, and NVIDIA#6542 -> `docs/about/release-notes.mdx`: Documents managed vLLM, Qwen3.6 tool parser, compaction, and timeout/readiness improvements. - NVIDIA#6559, NVIDIA#6538, NVIDIA#6560, NVIDIA#6568, NVIDIA#6552, NVIDIA#6567, and NVIDIA#6587 -> `docs/about/release-notes.mdx`: Documents runtime, credential, proxy, PID namespace, TOML, and provider-state hardening. - NVIDIA#6541, NVIDIA#5415, NVIDIA#6246, NVIDIA#6496, and NVIDIA#6573 -> `docs/about/release-notes.mdx`: Documents GitHub policy, Gmail policy, MCP allowlist, WhatsApp, and messaging-variant updates. - NVIDIA#6253, NVIDIA#6572, NVIDIA#6444, NVIDIA#6536, and NVIDIA#5860 -> `docs/about/release-notes.mdx`: Documents onboarding resume and create-step recovery improvements. - NVIDIA#6508, NVIDIA#6527, NVIDIA#5506, NVIDIA#6588, NVIDIA#6446, NVIDIA#6447, NVIDIA#6582, NVIDIA#6296, NVIDIA#6367, NVIDIA#6397, and NVIDIA#6505 -> `docs/about/release-notes.mdx`: Documents docs, release-risk, and E2E validation updates. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] 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: Release-note prose only. - [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 not applicable, release-note prose only. - [ ] 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) Docs validation note: `npm run docs:check-agent-variants && npm run docs:check-routes && git diff --check` passed. Full `npm run docs` is currently blocked before Fern validation because the pinned `fern-api@5.65.2` package is unavailable from npm (`ETARGET No matching version found`). --- <!-- 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: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added release notes for v0.0.79 with a new summary of recent improvements, including onboarding and inference options, operator/CLI diagnostics, sandbox recovery hardening, runtime limits, network policy behavior, and release validation updates. * Added updated references and links for the latest release. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 task
jyaunches
added a commit
that referenced
this pull request
Jul 24, 2026
## Summary Reopen follow-up for #6520: `nemoclaw uninstall --yes` followed by a curl reinstall hard-aborts at the installer strict pre-upgrade backup. The uninstall removes the gateway registration and the sandbox container but preserves `sandboxes.json`, so the stranded record can only ever be skipped — and the strict gate (`NEMOCLAW_REQUIRE_ALL_SANDBOX_BACKUPS=1`, #6114) exits 1 before the installer recovery phase (`recover_preexisting_sandboxes_before_onboard`), the phase that knows how to surface orphans, ever runs. The #6539 orphan classifier is wired only into `upgrade-sandboxes`, which is downstream of the aborting backup. ## What changed - `backupAll()` now classifies stranded records with the existing `classifyOrphanedRegistrySandboxes` (unobserved on the selected gateway AND persisted binding resolving to that same gateway), gated on the new `isSandboxContainerDefinitivelyAbsent()`. Stranded records are tracked separately from `skipped`, so the strict gate keeps failing closed for every genuine skip, and the run ends with `orphanedRegistrySummary` + `orphanedRegistryRemediation` (same destroy/onboard guidance as the recovery phase). End-to-end, the installer now proceeds to its recovery phase, which reports the orphans and yields "completed with warnings" instead of a hard abort. - The exemption is two-phase (PRA-1): after the backup loop, a confirming second pinned listing re-checks every stranded candidate — the same #6114 confirmation idiom as `upgrade-sandboxes` — and any candidate the gateway observes again reverts to a genuine strict skip. Container absence is itself checked per candidate at skip time. - `isSandboxContainerDefinitivelyAbsent()` fails closed everywhere absence cannot be proven: non-docker or unknown driver (including a throwing registry read), and a failed or timed-out labeled listing. The listing is status-checked (`docker ps -a` with the OpenShell labels) rather than reusing `findLabeledSandboxContainers`, which swallows docker errors — a dead daemon yields `null`, never "absent" — and passes `ignoreError` so the probe cannot `process.exit` the run. - `backup-all` now pins its sandbox listing to the selected gateway (same #6114 rationale and idiom as `upgrade-sandboxes`): an unpinned list taken from a sibling gateway selection must not feed a fail-open stranded decision. - The exemption carries an in-code source-of-truth review block (PRA-2): source boundary (`nemoclaw uninstall` preserves `sandboxes.json` by design), source-fix constraint (backup-all must not reconcile the registry; record removal is owned by the recovery phase destroy/onboard flow), and removal condition (install/uninstall registry reconciliation, or running the recovery phase before the strict backup). ## Constraints preserved (pinned by tests) - Strict gate still aborts on any genuine skip (#6114 cases untouched and green). - A sandbox bound to a sibling gateway is never claimed stranded, even when its container is absent on this host. - An unobserved sandbox whose container still exists keeps the strict abort (reconnect race). - A stranded candidate the confirming listing observes again reverts to a strict skip (lifecycle race). - Real backup failures (EACCES, non-manifest paths) still re-throw/abort. - A registry row with a null/unknown driver keeps the strict abort — fail closed, since absence cannot be proven for it. - `ORPHANED_SANDBOX_MARKER` / summary / remediation wording unchanged; install.sh greps the marker only from the recovery-phase log, which this change does not touch. ## Review responses - **PR Review Advisor PRA-1** (revalidate absence before exempting the gate): addressed by the confirming second pinned listing after the backup loop plus per-candidate absence checks at skip time; new test pins the reappeared-candidate revert. - **PR Review Advisor PRA-2** (bound the workaround to its source fix): addressed with the in-code source-of-truth review block (source boundary / fix constraint / removal condition). - `reviewed-npm-audit` failure is upstream-wide (also fails on #7289) and is being remediated by #7286; this PR adds no dependencies. ## Verification - `vitest`: `maintenance.test.ts` (32), `stopped-sandbox-backup.test.ts` (24), `orphan-detection.test.ts`, `upgrade-sandboxes-recovery.test.ts`, `openshell-sandbox-list.test.ts` — 108 tests green; installer lane `test/install-orphaned-sandbox-recovery.test.ts` (8, drives strict backup through recovery and the real install.sh orphan grep) green; `typecheck:cli`, `lint`, and `biome check` clean. The new maintenance tests pin `GATEWAY_PORT` so they stay green under an exported `NEMOCLAW_GATEWAY_PORT`. - Live end-to-end on a workstation (real docker daemon, real registry file, openshell CLI shimmed to a healthy empty gateway, real built CLI): stranded record → warning + `0 skipped` + exit 0 under strict mode (through both listings); same record bound to `gatewayPort: 9999` → strict abort exit 1 with no orphan claim; same record with a labeled container present → strict abort exit 1 with no orphan claim. Refs #6520 Signed-off-by: Dongni Yang <dongniy@nvidia.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved backup-all orphan handling: potentially stranded sandboxes are skipped when their containers are definitively absent, then rechecked with the same selected gateway before remediation occurs. * Tightened strict-mode “fail closed” behavior for confirmed absent containers, including mismatched-gateway scenarios and reappearing candidates. * Reduced false “container absent” results by treating Docker/registry uncertainties as unknown rather than absent. * **Tests** * Expanded coverage for stranded-orphan detection and definitive container absence (including fail-closed, reappearance, and gateway-pinning scenarios) with improved Docker mock observability. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ## Takeover review response - The PR Review Advisor handoff warning is addressed by an installer boundary regression that drives strict `backup-all`, orphan recovery, and the completed-with-warnings result in order. ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: Reviewed the complete five-file diff at `71a02caebe9f`. No documentation paths changed; existing docs already cover strict backup and stranded-record recovery and remediation. - Agent: Codex Desktop - PR: #7290 <!-- docs-review-head-sha: 71a02ca --> <!-- docs-review-agents-blob-sha: 560ff38 --> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> --------- Signed-off-by: Dongni Yang <dongniy@nvidia.com> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> Co-authored-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: Julie Yaunches <jyaunches@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
nemoclaw uninstall --yespreservessandboxes.jsonbut removes the gateway registration, provider registrations, and Docker image its recorded sandboxes depend on; a later reinstall then reported "Installation complete" and "Existing sandboxes were recovered and upgraded" while silently stranding the user's sandbox with no remediation path. This PR makes the whole cycle honest: the CLI derives a dedicated, observation-based orphan signal (independent of version classification, so the same-version reinstall repro is also caught), the installer consumes it to report "completed with warnings" with concrete remediation instead of false success, and uninstall warns at preserve time that the kept registry is not self-sufficient.Related Issue
Fixes #6520
Changes
src/lib/actions/upgrade-sandboxes.ts: new orphan classifier — a recorded sandbox absent in any phase from the selected gateway, whose persisted binding resolves to that same gateway, is reported via the exportedORPHANED_SANDBOX_MARKERline with<name> destroy/onboardremediation. Sandboxes bound to another live gateway, exonerated by the confirming second listing, or restored by prepared-backup recovery are excluded, preserving the [All Platforms][Upgrade] v0.0.55 → v0.0.76 leaves pre-existing sandboxes stuck in Provisioning/Error — user data inaccessible until manual rebuild #6114 multi-gateway exit-0 contract. Stale listings now label version regressions with a(downgrade)suffix (describeStaleUpgradeexported for tests).scripts/install.sh:recover_preexisting_sandboxes_before_onboardmirrors recovery output through a trap-registered temp log (_cleanup_files), takes the CLI's own exit status viaPIPESTATUS[0]so ateewrite failure cannot fake the [Ubuntu][Upgrade] installer auto-rebuild destroys existing sandboxes then fails to recreate when ambient env/session does not match the original config #5735 failure path, and greps the orphan marker into_PREEXISTING_SANDBOX_ORPHANED;print_donedowngrades the banner to=== Installation completed with warnings ===and replaces the false recovery claim with hedged wording plus remediation; the onboarding-skip line no longer claims recovery in the orphaned case.src/lib/actions/uninstall/run-plan.ts: warns at preserve time that preservedsandboxes.jsonrecords cannot be recovered automatically on reinstall (non-interactive and interactive keep paths, suppressed under--destroy-user-data); provider/sandbox delete no-ops now printalready removed or unreachableinstead of the contradictoryDeleted … skipped.test/install-orphaned-sandbox-recovery.test.ts(bash harness driving the realinstall.shclassification andprint_done; the stub output is built from the exported marker constant so wording drift on either side fails the suite),src/lib/actions/upgrade-sandboxes-display.test.ts(downgrade labeling), five orphan-classifier cases inupgrade-sandboxes-recovery.test.ts(own-gateway orphan for current- and stale-classified records; negatives for other-gateway, reconnect-race, and recovered sandboxes),src/lib/actions/uninstall/run-plan-preserved-registry.test.ts(preserve warning on non-interactive and interactive keep paths, no-op wording fix, purge suppression).reference/commands.mdx,manage-sandboxes/lifecycle.mdx,get-started/quickstart.mdx,reference/host-files-and-state.mdxupdated for the preserve-time warning, the warnings install outcome, the remediation path, and the(downgrade)label.Source-of-Truth Constraints (advisor PRA-1/6/8)
sandboxes.jsonentries whose gateway registration, provider registrations, and Docker image were removed by a priornemoclaw uninstall.--yesis deliberately non-destructive; existing automation depends on it never purging user data.--yesautomation). Both are behavior contracts owned by uninstall, out of scope for an honesty fix. The chosen resolution matches the issue's stated "Acceptable" bar: detect, diagnose with remediation, and never report false success.test/install-orphaned-sandbox-recovery.test.ts(real install.sh classification + print_done, stub built from the domain marker constant),src/lib/actions/upgrade-sandboxes-recovery.test.ts(orphan classifier incl. multi-gateway/reconnect/recovered negatives and check-mode parity),src/lib/domain/maintenance/orphan-detection.test.ts,src/lib/domain/uninstall/messaging.test.ts,src/lib/actions/uninstall/run-plan-preserved-registry.test.ts.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project cli src/lib/actions/upgrade-sandboxes-display.test.ts src/lib/actions/upgrade-sandboxes-recovery.test.ts src/lib/actions/uninstall/→ 79 passed;npx vitest run --project integration test/install-orphaned-sandbox-recovery.test.ts test/install-preexisting-sandbox-recovery.test.ts test/install-onboard-exit.test.ts→ 14 passed;npm run typecheck:cliclean;shfmt -i 2 -ci -bn -d scripts/install.shcleannpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Dongni Yang dongniy@nvidia.com
Summary by CodeRabbit