fix(installer): ignore route-only sandbox reservations - #6764
Merged
Conversation
Signed-off-by: San Dang <sdang@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe registry now identifies route-only reservations and excludes them from sandbox backup, upgrade recovery, installer counts, and ambiguous-name detection. Tests cover reservation state transitions, empty or mixed registries, and installer onboarding and upgrade behavior. ChangesRoute reservation filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Installer
participant Registry
participant Backup
Installer->>Registry: Inspect sandbox registry
Registry-->>Installer: Return registered sandbox count
Installer->>Backup: Back up registered sandboxes
Backup-->>Installer: Return RESTORE and CONFIRMED_NAMES
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
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
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings This is an automated review. Required findings need action before merge. Warnings and optional suggestions do not require a response or follow-up. A human maintainer makes the final merge decision. |
Signed-off-by: San Dang <sdang@nvidia.com>
sandl99
added
bug-fix
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/actions/upgrade-sandboxes-preflight.test.ts (1)
40-44: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDo not duplicate the production reservation predicate in the mock.
This mock reimplements the classification logic, so the test can pass even if
registry.tschanges. Use a partial mock that retains the realisRouteOnlySandboxReservationexport, or move the pure predicate to a shared module and import it from both places.Suggested partial mock
-vi.mock("../state/registry", () => ({ - isRouteOnlySandboxReservation: (entry: { pendingRouteReservation?: true; createdAt?: string }) => - entry.pendingRouteReservation === true && entry.createdAt === undefined, - listSandboxes: mocks.listSandboxes, -})); +vi.mock("../state/registry", async (importOriginal) => { + const actual = await importOriginal<typeof import("../state/registry")>(); + return { + ...actual, + listSandboxes: mocks.listSandboxes, + }; +});As per path instructions, tests should verify behavior through the public boundary and must not copy production algorithms.
🤖 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-preflight.test.ts` around lines 40 - 44, Update the vi.mock for "../state/registry" in the upgrade sandbox preflight tests to preserve and reuse the real isRouteOnlySandboxReservation export instead of defining a duplicate predicate. Keep only the necessary mocked exports, such as listSandboxes, while retaining the production implementation for behavior verification through the public boundary.Source: Path instructions
scripts/install.sh (1)
1726-1728: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated route-only reservation predicate risks drift.
This inline Node snippet re-implements the exact same condition as
isRouteOnlySandboxReservationinsrc/lib/state/registry.ts(pendingRouteReservation === true && entry.createdAt === undefined). Sincescripts/install.shcan't import the TS module, consider adding a comment here pointing back to the canonical definition so future changes to the predicate are mirrored, or add a regression test that fails if the two definitions diverge.💡 Suggested comment anchor
+// Keep this predicate in sync with isRouteOnlySandboxReservation() +// in src/lib/state/registry.ts (pendingRouteReservation && !createdAt). const sandboxes = entries.filter( ([, entry]) => !(entry.pendingRouteReservation === true && entry.createdAt === undefined), );🤖 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 `@scripts/install.sh` around lines 1726 - 1728, Add a concise comment immediately above the `sandboxes` filter in the install script referencing `isRouteOnlySandboxReservation` in `src/lib/state/registry.ts` as the canonical predicate, and state that this inline condition must remain synchronized with it.
🤖 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/state/registry-route-reservation.test.ts`:
- Around line 47-50: Remove both added if-throw guards around the getSandbox
assertions in the registry reservation tests. Assert each reservation is defined
with the test framework before accessing its properties, then retain the
existing createdAt and route-only assertions for both retargeted cases.
---
Nitpick comments:
In `@scripts/install.sh`:
- Around line 1726-1728: Add a concise comment immediately above the `sandboxes`
filter in the install script referencing `isRouteOnlySandboxReservation` in
`src/lib/state/registry.ts` as the canonical predicate, and state that this
inline condition must remain synchronized with it.
In `@src/lib/actions/upgrade-sandboxes-preflight.test.ts`:
- Around line 40-44: Update the vi.mock for "../state/registry" in the upgrade
sandbox preflight tests to preserve and reuse the real
isRouteOnlySandboxReservation export instead of defining a duplicate predicate.
Keep only the necessary mocked exports, such as listSandboxes, while retaining
the production implementation for behavior verification through the public
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: dff905f6-e53c-4bea-b835-8f5672518733
📒 Files selected for processing (10)
scripts/install.shsrc/lib/actions/maintenance.test.tssrc/lib/actions/maintenance.tssrc/lib/actions/upgrade-sandboxes-preflight.test.tssrc/lib/actions/upgrade-sandboxes-recovery.test.tssrc/lib/actions/upgrade-sandboxes.tssrc/lib/state/registry-route-reservation.test.tssrc/lib/state/registry.tstest/install-openshell-upgrade-prompt.test.tstest/install-preexisting-sandbox-recovery.test.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/install-preexisting-sandbox-recovery.test.ts (1)
76-85: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear inherited
NEMOCLAW_SINGLE_SESSIONbefore applying the test option.Spreading
process.envleaves a runner-provided value active whensingleSessionis omitted or false, allowing tests to execute a different installer path nondeterministically. Remove that key from the copied environment first. As per coding guidelines, deterministic tests must clear environment/global stubs.Proposed fix
+ const childEnv = { ...process.env }; + delete childEnv.NEMOCLAW_SINGLE_SESSION; + const result = spawnSync("bash", ["-c", snippet], { encoding: "utf-8", env: { - ...process.env, + ...childEnv, BASH_ENV: "", ENV: "", HOME: tmp,🤖 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 `@test/install-preexisting-sandbox-recovery.test.ts` around lines 76 - 85, Update the spawnSync environment setup in the test to remove inherited NEMOCLAW_SINGLE_SESSION after spreading process.env and before applying options.singleSession. Preserve setting it to "1" only when singleSession is enabled, ensuring omitted or false options always run with the variable cleared.Source: Coding guidelines
🤖 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 `@test/install-preexisting-sandbox-recovery.test.ts`:
- Around line 76-85: Update the spawnSync environment setup in the test to
remove inherited NEMOCLAW_SINGLE_SESSION after spreading process.env and before
applying options.singleSession. Preserve setting it to "1" only when
singleSession is enabled, ensuring omitted or false options always run with the
variable cleared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d78fdafa-995c-48f7-9b70-11ac588f568e
📒 Files selected for processing (2)
src/lib/state/registry-route-reservation.test.tstest/install-preexisting-sandbox-recovery.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/state/registry-route-reservation.test.ts
Signed-off-by: San Dang <sdang@nvidia.com>
Contributor
E2E Target Results — ✅ All selected tests passedRun: 29261650292
|
prekshivyas
approved these changes
Jul 13, 2026
prekshivyas
left a comment
prekshivyas
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed current head. Required CI, both PR advisors, CodeRabbit, and all requested E2E targets, including state-backup-restore, are clean. LGTM.
21 tasks
cv
pushed a commit
that referenced
this pull request
Jul 14, 2026
<!-- markdownlint-disable MD041 --> ## Summary Release-prep documentation for v0.0.82 now summarizes user-facing changes merged since v0.0.81. It also closes stale wording in the stopped-sandbox backup, snapshot-clone, Ollama selection, and custom-policy authoring guidance. ## Changes - Add the `v0.0.82` section to `docs/about/release-notes.mdx` with links to the focused user guides. - Document that snapshot clones receive a destination-owned dashboard port before destructive replacement begins. - Align `backup-all` guidance with eligible stopped Docker-driver sandboxes that NemoClaw starts temporarily. - Describe the running and stopped Ollama menu states without claiming one fixed label. - Document runtime rejection of catch-all hosts in custom policy files. ### Source summary - [#6748](#6748) -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/lifecycle.mdx`, and `docs/reference/commands.mdx`: Summarize non-destructive sandbox `stop` and `start` commands. - [#6723](#6723) -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/backup-restore.mdx`, and `docs/reference/commands.mdx`: Record temporary startup and cleanup for eligible stopped-sandbox backups. - [#6749](#6749) -> `docs/about/release-notes.mdx` and `docs/manage-sandboxes/backup-restore.mdx`: Document destination-owned dashboard ports for snapshot clones. - [#6764](#6764) -> `docs/about/release-notes.mdx`: Summarize installer handling of route-only onboarding placeholders. - [#6771](#6771) -> `docs/about/release-notes.mdx`, `docs/inference/set-up-vllm.mdx`, `docs/inference/choose-inference-provider.mdx`, `docs/reference/commands.mdx`, and `docs/reference/platform-support.mdx`: Summarize managed-vLLM storage gates, immutable image digests, and the explicit override boundary. - [#6759](#6759) -> `docs/about/release-notes.mdx`: Record early, actionable OpenShell gateway-port conflict diagnostics. - [#6753](#6753) -> `docs/about/release-notes.mdx` and `docs/inference/set-up-ollama.mdx`: Document truthful running and stopped Ollama menu states. - [#6776](#6776) -> `docs/about/release-notes.mdx`: Summarize proxy-independent loopback readiness checks. - [#6769](#6769) -> `docs/about/release-notes.mdx`: Record compatible endpoint and agent guidance when Chat Completions is unavailable. - [#6730](#6730) -> `docs/about/release-notes.mdx`: Summarize bounded reuse of an eligible successful Chat Completions check. - [#6768](#6768) -> `docs/about/release-notes.mdx`: Record route-reservation repair during resumed onboarding. - [#6742](#6742) -> `docs/about/release-notes.mdx`: Summarize pre-mutation resolution of secret-free sandbox create intent. - [#6721](#6721) -> `docs/about/release-notes.mdx` and `docs/get-started/quickstart-langchain-deepagents-code.mdx`: Record bounded cleanup of completed managed Deep Agents headless sessions. - [#6731](#6731) -> `docs/about/release-notes.mdx` and `docs/network-policy/customize-network-policy.mdx`: Document runtime rejection of catch-all custom-policy destinations. - [#6729](#6729) -> `docs/about/release-notes.mdx` and `docs/get-started/prerequisites.mdx`: Record the Node.js 22.19 minimum. - [#6735](#6735) -> `docs/about/release-notes.mdx` and `docs/reference/platform-support.mdx`: Summarize the Ubuntu 26.04 userspace contract without claiming pending host or live validation. - [#6775](#6775) -> `docs/about/release-notes.mdx` and `docs/resources/community-contributions.mdx`: Route independent solutions outside canonical supported-product documentation. - [#6740](#6740) -> `docs/about/release-notes.mdx`: Summarize the semantic dependency-upgrade contributor workflow. - [#6777](#6777) -> `docs/about/release-notes.mdx` and `docs/CONTRIBUTING.md`: Summarize the route-safe documentation-refactor workflow. - [#6741](#6741) -> `docs/about/release-notes.mdx` and `docs/security/openclaw-2026.6.10-dependency-review.md`: Summarize reviewed npm archive verification and audit enforcement. - [#6739](#6739) -> `docs/about/release-notes.mdx` and `docs/security/openclaw-2026.6.10-dependency-review.md`: Record the locked offline dependency graph for the managed OpenClaw WeChat runtime. - [#6737](#6737) -> `docs/about/release-notes.mdx`: Record removal of the messaging build plan from final OpenClaw and Hermes image environments. - [#6733](#6733) -> `docs/about/release-notes.mdx`: Summarize cached plugin dependency layers for source and blueprint rebuilds. ### Skipped from docs-skip - None. No commit or changed path in `v0.0.81..origin/main` matched `openclaw-sandbox-permissive.yaml` or `config-show`, and the drafted content contains none of the configured skip terms. ## 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 - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: This is a documentation-only release-prep update; behavior is protected by the merged source PRs, and the documentation build validates the changed routes and agent variants. - [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 - [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 — tests are not applicable for this documentation-only change. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: not run for this documentation-only change. - [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) — 0 errors; two pre-existing Fern warnings remain. - [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) — no new pages. --- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated release notes with improvements to sandbox recovery, onboarding, session management, policy validation, storage checks, and system requirements. * Clarified Ollama setup instructions and status labels. * Documented safer snapshot restoration, including dedicated ports and protection against destructive failures. * Expanded `backup-all` coverage to include eligible stopped sandboxes. * Added guidance rejecting broad or catch-all network destinations in custom policies. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Charan Jagwani <cjagwani@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
4 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
Interrupted onboarding can leave a route-only registry reservation before sandbox registration. The installer previously counted that reservation as a real sandbox, requested legacy managed-image provenance, and failed strict backup even though there was no sandbox to back up. This completes the remaining #6500 registry case after #6723 while keeping real and legacy sandboxes fail-closed.
Related Issue
Fixes #6500
Changes
pendingRouteReservation: truewithoutcreatedAt.backup-all, automatic sandbox recovery, and the installer's existing-session guard.Type of Change
Quality Gates
nemoclaw onboard --resume.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/state/registry-route-reservation.test.ts src/lib/actions/maintenance.test.ts src/lib/actions/upgrade-sandboxes-preflight.test.ts src/lib/actions/upgrade-sandboxes-recovery.test.ts(67 passed);npx vitest run --project integration test/install-openshell-upgrade-prompt.test.ts test/install-preexisting-sandbox-recovery.test.ts(24 passed);npm run typecheck:cli;npm run test:projects:check;shellcheck scripts/install.sh.npm 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: San Dang sdang@nvidia.com
Summary by CodeRabbit