fix(cli): preserve shared routes during sandbox upgrades

[sandl99]

Summary

Prevent legacy multi-sandbox upgrades from destroying the first sandbox when a sibling on the same OpenShell gateway records the same inference route without its credential environment name. Rebuild now migrates only missing canonical credential identities under the registry lock and revalidates the shared route at the delete edge; explicit route conflicts still fail closed before deletion.

Related Issue

Fixes #7798

Changes

• Add a rebuild-scoped registry transaction that fills missing legacy credential identities from the selected provider's canonical configuration and validates the resulting shared route.
• Carry a route receipt through rebuild and re-read the target plus every registered peer immediately before sandbox deletion, blocking deletion on route or gateway drift.
• Keep the ordinary shared-route compatibility guard literal and fail-closed. A direct relaxation would hide real credential, model, endpoint, or inference API conflicts; the provider-matrix and delete-edge tests protect the scoped migration.
• Extend the stale-sandbox live E2E target to upgrade two legacy sandboxes sharing one gateway and verify both registry identities after the batch upgrade.
• Document legacy shared-route migration and conflict recovery behavior.

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:
• 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: The implementation fills only missing identities from canonical provider configuration while holding the registry lock; explicit identity, model, endpoint, and inference API conflicts remain blocked, and delete-edge drift is covered by focused tests.
• Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

• Documentation writer subagent reviewed the completed changes
• Result: docs-updated
• Evidence: docs/inference/use-shared-gateway-routes.mdx, docs/reference/commands.mdx; npm run docs completed with 0 errors.
• Agent: Codex Desktop

DGX Station Hardware Evidence

• Tested on DGX Station
• Tested commit: Not applicable; scripts/prepare-dgx-station-host.sh is unchanged.
• Station profile/scenario: Not applicable.
• Result: Not applicable.
• Supporting evidence: Not applicable.

Verification

• PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
• Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run check:diff passed when hooks were skipped or unavailable
• Targeted behavior tests pass for the current change set, or tests are marked not applicable above — npx vitest run --project cli src/lib/actions/sandbox/rebuild-route-preflight.test.ts src/lib/actions/sandbox/rebuild-destroy-phase.test.ts src/lib/actions/sandbox/rebuild-flow.test.ts src/lib/actions/sandbox/rebuild-resume-snapshot.test.ts (144 passed); npm run typecheck:cli; npm run test:titles:check; npm run test:e2e-phases:check; focused E2E-support workflow tests (133 passed); live target discovery passed.
• Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result: Live Docker/Brev upgrade execution is delegated to the added E2E CI scenario. npm run checks and the source-architecture budget passed locally.
• 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: San Dang sdang@nvidia.com
Signed-off-by: Prekshi Vyas prekshiv@nvidia.com

Summary by CodeRabbit

• Bug Fixes
  • Rebuild flow now performs a final “delete-edge” route revalidation to prevent deleting drifted shared-route sandboxes.
  • Legacy shared-route sandboxes can be reconciled by filling only missing provider credential environment-variable names while preserving explicit values; rebuild aborts on route/endpoint/credential/gateway conflicts.
• Documentation
  • Updated command reference with clearer agent selection, base-image resolution, and maintenance/recovery guidance; expanded legacy shared-route rebuild and snapshot/uninstall details.
• Tests / CI
  • Expanded rebuild and upgrade stale-sandbox coverage to handle sibling sandboxes.
  • Increased the end-to-end “upgrade-stale-sandbox” job timeout for longer runs.

[coderabbitai]

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: 07c9fc6c-5492-4206-b99e-e2594854c6d6

📥 Commits

Reviewing files that changed from the base of the PR and between 25a4199 and 91afb03.

📒 Files selected for processing (6)

• docs/inference/use-shared-gateway-routes.mdx
• src/lib/actions/sandbox/rebuild-destroy-phase.test.ts
• src/lib/actions/sandbox/rebuild-preflight-guards.ts
• src/lib/actions/sandbox/rebuild-preflight-target-phase.ts
• src/lib/actions/sandbox/rebuild-resume-snapshot.test.ts
• src/lib/actions/sandbox/rebuild-route-preflight.test.ts

🚧 Files skipped from review as they are similar to previous changes (3)

• docs/inference/use-shared-gateway-routes.mdx
• src/lib/actions/sandbox/rebuild-preflight-target-phase.ts
• src/lib/actions/sandbox/rebuild-preflight-guards.ts

---

📝 Walkthrough

Walkthrough

Changes

The rebuild flow now reconciles legacy shared-gateway routes, migrates missing credential metadata, revalidates routes before deletion, and aborts safely on drift. Unit, lifecycle, and two-sandbox E2E coverage were added alongside documentation and timeout updates.

Shared-route rebuild

Layer / File(s)	Summary
Route preflight commit and revalidation src/lib/actions/sandbox/rebuild-preflight-guards.ts, src/lib/actions/sandbox/rebuild-route-preflight.test.ts	Adds route normalization, compatibility checks, locked registry updates, credential migration, drift detection, and focused tests.
Preflight receipt and delete-edge guard src/lib/actions/sandbox/rebuild-preflight-*.ts, src/lib/actions/sandbox/rebuild-pipeline.ts, src/lib/actions/sandbox/rebuild-destroy-phase.*	Carries the route receipt through rebuild and prevents deletion when final validation fails.
Rebuild lifecycle test wiring test/helpers/rebuild-flow-*, src/lib/actions/sandbox/rebuild-resume-snapshot.test.ts	Mocks route preflight operations and verifies recovery, abort, skipped onboarding, and skipped deletion after route drift.
Two-sandbox shared-route E2E coverage test/e2e/live/upgrade-stale-sandbox*, .github/workflows/e2e.yaml, tools/e2e/workflow-boundary.mts	Expands stale-sandbox coverage to two shared-route sandboxes and raises the job timeout to 85 minutes.
Rebuild and command documentation docs/inference/use-shared-gateway-routes.mdx, docs/reference/commands.mdx	Documents shared-route reconciliation and updates command behavior references.

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

Sequence Diagram(s)

sequenceDiagram
  participant RebuildPipeline
  participant Registry
  participant RebuildDestroyPhase
  participant OpenShell
  RebuildPipeline->>Registry: Commit route preflight
  Registry-->>RebuildPipeline: Return route receipt
  RebuildPipeline->>RebuildDestroyPhase: Validate receipt before delete
  RebuildDestroyPhase->>Registry: Revalidate current route
  Registry-->>RebuildDestroyPhase: Return success or drift
  RebuildDestroyPhase->>OpenShell: Delete sandbox on success

Loading

Possibly related issues

• refactor(state): make registry and gateway lifecycle updates atomic #6492: Adds atomic registry updates and delete-edge drift validation for rebuild state transitions.
• fix(rebuild): journal same-name sandbox replacement #7734: Addresses the same rebuild pipeline through durable journaling for delete/recreate transactions.
• rebuild/upgrade: atomic build-verify-swap before deleting old sandbox (health-before-delete) #5801: Addresses rebuild deletion safety through route validation and replacement safeguards.

Possibly related PRs

• NVIDIA/NemoClaw#7369: Modifies the same rebuild deletion phase with sandbox-absence polling.
• NVIDIA/NemoClaw#7665: Overlaps on the upgrade-stale-sandbox E2E workflow jobs.
• NVIDIA/NemoClaw#7788: Modifies the rebuild delete boundary with recreate journaling and absence checks.

Suggested labels: area: sandbox, area: e2e

Suggested reviewers: cv, apurvvkumaria

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 15.38% 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 reflects the main change: preserving shared routes during sandbox upgrade/rebuild flows.
Linked Issues check	✅ Passed	The changes address #7798 by keeping sibling sandboxes isolated, migrating missing shared-route metadata, and blocking delete on route drift.
Out of Scope Changes check	✅ Passed	The workflow timeout, docs, tests, and rebuild logic all support the shared-route upgrade fix and do not appear unrelated.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/7798-shared-route-upgrade

---

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

[github-actions]

🌿 Preview your docs: https://nvidia-preview-pr-7840.docs.buildwithfern.com/nemoclaw

[github-code-quality]

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 91afb03 in the fix/7798-shared-rout... branch remains at 96%, unchanged from commit 25de83d in the main branch.

TypeScript / code-coverage/cli

The overall coverage in commit 91afb03 in the fix/7798-shared-rout... branch remains at 81%, unchanged from commit 25de83d in the main branch.

Show a code coverage summary of the most impacted files.

File	main 25de83d	fix/7798-shared-rout... 91afb03	+/-
src/lib/onboard...ndbox-create.ts	83%	33%	-50%
src/lib/onboard...box-prebuild.ts	92%	74%	-18%
src/lib/actions...ocker-health.ts	82%	65%	-17%
src/lib/actions...light-guards.ts	90%	74%	-16%
src/lib/actions...confirmation.ts	79%	69%	-10%
src/lib/onboard...ndbox-create.ts	91%	83%	-8%
src/lib/domain/.../connect-env.ts	97%	89%	-8%
src/lib/onboard/preflight.ts	80%	80%	0%
src/lib/actions...me-preflight.ts	85%	92%	+7%
src/lib/onboard/docker-cdi.ts	70%	80%	+10%

---

_{Updated July 30, 2026 18:18 UTC}

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[github-actions]

PR Review Advisor — Blocking findings reported

Advisor assessment: Blockers require maintainer review
Next action: Review the blockers below.
Findings: 1 blocker · 0 warnings · 0 suggestions

Model lanes

• GPT-5.6 Terra (primary): Completed · high confidence · 1 blocker · 0 warnings · 0 suggestions
• Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
• Model comparison: normalized findings differ; normalized E2E selections match; Nemotron reported 1 fewer blocker, the same number of warnings, the same number of suggestions.

Nemotron output stays in workflow artifacts and does not change the assessment above.

E2E guidance

Advisory only. E2E / PR Gate selects and runs jobs independently.

Recommended E2E: cloud-inference, cloud-onboard, security-posture, onboard-repair, onboard-resume, state-backup-restore, upgrade-stale-sandbox

Blockers

PRA-1 Blocker — Do not mutate sibling sandbox metadata during a single-sandbox rebuild

• Location: src/lib/actions/sandbox/rebuild-preflight-guards.ts:165
• Category: acceptance
• Problem: The rebuild preflight assigns the canonical credential environment-variable name to each compatible same-gateway peer before it deletes the requested sandbox. A rebuild of one sandbox therefore changes sibling registry records.
• Impact: A sibling sandbox can have its durable configuration changed even though it was not the rebuild target. This violates the linked issue's required isolation outcome and can alter later sibling recovery behavior.
• Fix: Keep legacy-route compatibility validation read-only for sibling entries during a single-sandbox rebuild. Update only the requested sandbox, or use a separately authorized migration that targets every affected sandbox.
• Verification: Inspect the registry before and after rebuilding one sandbox with a compatible same-gateway legacy peer; the peer credentialEnv must not change.
• Test coverage: Add a rebuild regression test with a target and compatible legacy sibling. Assert that a successful target rebuild leaves the sibling registry entry byte-for-byte unchanged.
• Evidence: src/lib/actions/sandbox/rebuild-preflight-guards.ts:143-169 mutates peer.credentialEnv for same-gateway peers. src/lib/actions/sandbox/rebuild-route-preflight.test.ts:105-137 expects the peer credentialEnv to be persisted. Linked issue [Ubuntu 24.04][Upgrade] recovering one sandbox interrupts sibling sandboxes on the same host #7798 states that rebuilding {primary-sandbox} must not stop, disconnect, or mutate {sibling-sandbox}.

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (4)

src/lib/actions/sandbox/rebuild-route-preflight.test.ts (1)

82-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Guard the derived provider matrix against becoming empty.

remoteProviders is filtered from REMOTE_PROVIDER_CONFIG; if that config shape changes (e.g., credentialEnv moves), it.each silently degenerates and the migration contract stops being exercised. A single expect(remoteProviders.length).toBeGreaterThan(0) in a sanity test keeps the matrix honest.

🤖 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/rebuild-route-preflight.test.ts` around lines 82 -
96, Add a sanity test near the remoteProviders-derived test matrix that asserts
remoteProviders.length is greater than zero. Keep the existing it.each migration
coverage unchanged, ensuring configuration changes that filter out every
provider fail explicitly instead of silently skipping the contract.

src/lib/actions/sandbox/rebuild-preflight-guards.ts (1)

187-192: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value

Delete-edge revalidation reads the registry without the registry lock.

commitRebuildRoutePreflight performs its read under withLock, but this function reads via a bare load(). A concurrent writer can be mid-write when the delete edge reads, so the guard could observe a torn or stale view right before the destructive delete. If the synchronous no-await edge intentionally avoids the lock, a short comment stating that tradeoff would make the asymmetry explicit.

🤖 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/rebuild-preflight-guards.ts` around lines 187 - 192,
Update revalidateRebuildRouteBeforeDelete to read the sandbox registry through
the same lock-protected mechanism used by commitRebuildRoutePreflight,
preserving synchronous behavior where required. If this delete-edge path must
intentionally use the bare load without locking, add a concise comment
documenting that tradeoff and the resulting consistency boundary.

test/e2e/live/upgrade-stale-sandbox.test.ts (1)

36-39: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Check the 75-minute cap against the doubled per-sandbox work.

Worst-case inner budgets now sum well past LIVE_TIMEOUT_MS: install (20 min) + old-base build (20 min) + two creates (15 min each) + readiness waits + the batch rebuild (45 min). A slow-but-healthy run would be killed by the test timeout rather than by any inner boundary.

🤖 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/e2e/live/upgrade-stale-sandbox.test.ts` around lines 36 - 39, Increase
LIVE_TIMEOUT_MS to cover the doubled per-sandbox setup and batch rebuild
budgets, including readiness waits, so the upgrade-sandboxes test can complete
during a slow-but-healthy run without being terminated by the outer timeout.

src/lib/actions/sandbox/rebuild-preflight-target-phase.ts (1)

296-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cross-sandbox registry mutation is only reported through the verbose rebuild log.

The migration writes credentialEnv onto other sandboxes' registry rows, but log() is the verbose channel, so a default-verbosity rebuild mutates peers with no user-visible notice. Consider surfacing this on the normal output path (as the other cross-sandbox warnings on this page do) so operators can correlate the change.

🤖 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/rebuild-preflight-target-phase.ts` around lines 296 -
301, The migratedSandboxNames notification in the rebuild preflight flow
currently uses verbose log(); switch this cross-sandbox mutation notice to the
normal user-visible output path used by nearby cross-sandbox warnings, while
preserving the existing message and conditional check in the routePreflight
receipt handling.

🤖 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/rebuild-destroy-phase.test.ts`:
- Around line 208-211: Update the assertion in the rebuild-destroy test to
reject sandbox deletion regardless of argument or options shape. Reuse the
existing expectNoSandboxDelete helper with mocks.runOpenshell, or inspect the
command and its arguments broadly enough to catch every sandbox delete
invocation.

---

Nitpick comments:
In `@src/lib/actions/sandbox/rebuild-preflight-guards.ts`:
- Around line 187-192: Update revalidateRebuildRouteBeforeDelete to read the
sandbox registry through the same lock-protected mechanism used by
commitRebuildRoutePreflight, preserving synchronous behavior where required. If
this delete-edge path must intentionally use the bare load without locking, add
a concise comment documenting that tradeoff and the resulting consistency
boundary.

In `@src/lib/actions/sandbox/rebuild-preflight-target-phase.ts`:
- Around line 296-301: The migratedSandboxNames notification in the rebuild
preflight flow currently uses verbose log(); switch this cross-sandbox mutation
notice to the normal user-visible output path used by nearby cross-sandbox
warnings, while preserving the existing message and conditional check in the
routePreflight receipt handling.

In `@src/lib/actions/sandbox/rebuild-route-preflight.test.ts`:
- Around line 82-96: Add a sanity test near the remoteProviders-derived test
matrix that asserts remoteProviders.length is greater than zero. Keep the
existing it.each migration coverage unchanged, ensuring configuration changes
that filter out every provider fail explicitly instead of silently skipping the
contract.

In `@test/e2e/live/upgrade-stale-sandbox.test.ts`:
- Around line 36-39: Increase LIVE_TIMEOUT_MS to cover the doubled per-sandbox
setup and batch rebuild budgets, including readiness waits, so the
upgrade-sandboxes test can complete during a slow-but-healthy run without being
terminated by the outer timeout.

🪄 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: bb5dee33-5049-4216-8b6d-abf7ad082b2b

📥 Commits

Reviewing files that changed from the base of the PR and between 1fba4c4 and d30e5d9.

📒 Files selected for processing (17)

• .github/workflows/e2e.yaml
• docs/inference/use-shared-gateway-routes.mdx
• docs/reference/commands.mdx
• src/lib/actions/sandbox/rebuild-destroy-phase.test.ts
• src/lib/actions/sandbox/rebuild-destroy-phase.ts
• src/lib/actions/sandbox/rebuild-pipeline.ts
• src/lib/actions/sandbox/rebuild-preflight-guards.ts
• src/lib/actions/sandbox/rebuild-preflight-phase.ts
• src/lib/actions/sandbox/rebuild-preflight-target-phase.ts
• src/lib/actions/sandbox/rebuild-route-preflight.test.ts
• test/e2e/live/upgrade-stale-sandbox-helpers.ts
• test/e2e/live/upgrade-stale-sandbox.test.ts
• test/helpers/rebuild-flow-harness.ts
• test/helpers/rebuild-flow-lifecycle-cases.ts
• test/helpers/rebuild-flow-test-harness.ts
• test/helpers/rebuild-flow-test-support.ts
• tools/e2e/workflow-boundary.mts

[prekshivyas]

Reviewed head 91afb03.

Security review: PASS across secrets/credentials, input validation, authorization, dependencies, logging, cryptography, configuration, security testing, and system security. The shared-route metadata migration is transactional and fails closed before sandbox deletion; targeted CLI tests (144) and type-checking pass.