fix(upgrade-sandboxes): keep --check read-only and target the recorded gateway (#7279)

[yanyunl1991]

Summary

nemoclaw upgrade-sandboxes --check is documented as a read-only check, but it started a new OpenShell gateway and switched the active gateway, stranding sandboxes registered under a different (non-default-port) gateway — and a later nemoclaw uninstall --yes then targeted the wrong gateway and left the sandbox behind.

The trigger is a sandbox that is visible to the ambient-port registry while bound to a non-default gateway (recorded gatewayName/gatewayPort). Running the check with no NEMOCLAW_GATEWAY_PORT resolves the ambient default to nemoclaw, which does not match the sandbox's recorded nemoclaw-18080.

Root cause

upgradeSandboxes() pinned its target to resolveGatewayName(getGatewayPort()) (the ambient default) and ran the recovering preflight — captureSandboxListWithGatewayPreflightOrExit → recoverNamedGatewayRuntime, which gateway selects the target and starts it ([2/8] Starting OpenShell gateway) — before the checkOnly early-return. So --check mutated gateway state even though it never rebuilds anything.

Fix (scoped to --check; the auto/rebuild path is unchanged)

• openshell-sandbox-list.ts — add captureNamedGatewaySandboxListReadOnly: a sandbox list -g <name> that targets a named gateway without selecting it and never recovers/starts. State-RPC drift still blocks (shared detectors); a down/unreachable gateway is non-fatal (empty output → sandbox reported unobserved), mirroring status's probeGatewayHealth.
• upgrade-sandboxes.ts — in check mode, resolve the target from the registered sandboxes' resolveSandboxGatewayName binding (the single distinct recorded gateway) instead of the ambient port, and query it read-only. With several distinct recorded gateways (or none) it keeps the ambient default — the read-only query avoids any mutation either way.

Testing

• Unit (openshell-sandbox-list.test.ts, upgrade-sandboxes-preflight.test.ts, upgrade-sandboxes-recovery.test.ts): the read-only helper lists -g <name> and never calls recoverNamedGatewayRuntime, stays non-fatal when the gateway is down, still exits on state-RPC drift, and handles both the initial and absent-sandbox confirmation reads without the recovering preflight. --check targets the sandbox's recorded nemoclaw-18080 instead of the ambient nemoclaw. The paired guard confirms that the auto path is unchanged.
• Integration: the gateway-drift and installer prepared-recovery boundaries pass on the merged tree.
• Live (Linux, ollama, gateway port 18080): our fleet has no DGX Station (GB300) to run the reporter's Station Express onboard, but the report's registry state — a sandbox recorded in the ambient-port registry bound to nemoclaw-18080 — reproduces the defect on both v0.0.84 and main: --check (no env) prints [2/8] Starting OpenShell gateway + Active gateway set to 'nemoclaw' and flips the active gateway from nemoclaw-18080 to nemoclaw. With this change, --check leaves the active gateway on nemoclaw-18080, emits none of those side effects, targets nemoclaw-18080 read-only, and reports the sandbox correctly (as running, where the buggy path misreported it as stopped).

Closes #7279

Summary by CodeRabbit

• New Features
  • Enhanced upgrade-sandboxes --check to pin reads to the single recorded gateway from registry sandboxes.
  • Added read-only named-gateway sandbox listing used during check-mode and absent-sandbox confirmation.
• Bug Fixes
  • Ensures failure in read-only gateway listing stops further upgrade actions (no classification/backup/rebuild).
  • Improves gateway targeting for non-default, multi-gateway, and invalid recorded binding cases.
• Tests
  • Expanded coverage for read-only listing behavior, absent-sandbox confirmation flow, and updated recovery harness assertions.
• Documentation
  • Clarified that --check performs inspection only and how gateway selection is determined.

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: Codex Desktop reviewed PR SHA b6016a991 against all nine security categories. No findings. Gateway bindings remain namespace and port validated, and OpenShell receives the gateway name as a separate argument.
• 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: Exact-head review confirmed docs/manage-sandboxes/update-sandboxes.mdx accurately documents that upgrade-sandboxes --check does not start, recover, or select a gateway and queries the single recorded gateway when unambiguous. npm run docs completed with 0 errors and 2 repository warnings; git diff --check passed.
• Agent: Codex documentation writer subagent

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 — focused CLI tests: 95/95; gateway-drift, installer-recovery, and documentation integration tests: 9/9; affected E2E-support tests: 36/36; CLI typecheck, exact Vitest project membership, npm run docs, and npm run check:diff passed.
• Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result:
• 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: Yanyun Liao yanyunl@nvidia.com

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

upgrade-sandboxes --check now selects an unambiguous recorded gateway and uses read-only sandbox listing without recovery or gateway mutation. Non-check flows retain gateway preflight behavior, with tests covering gateway selection, failures, and absent-sandbox handling.

Changes

Sandbox check gateway flow

Layer / File(s)	Summary
Read-only named-gateway listing src/lib/openshell-sandbox-list.ts, src/lib/openshell-sandbox-list.test.ts	Adds and tests a named-gateway listing helper that uses sandbox list -g, ignores command errors, avoids recovery, and exits on detected drift.
Recorded gateway routing in upgrade checks src/lib/actions/upgrade-sandboxes.ts, src/lib/actions/upgrade-sandboxes-preflight.test.ts, src/lib/actions/upgrade-sandboxes-recovery.test.ts, docs/manage-sandboxes/update-sandboxes.mdx	Resolves recorded gateway names for check mode, routes check reads through the read-only helper, preserves preflight recovery for non-check mode, updates documentation, and tests gateway selection, failures, and absent-sandbox handling.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Operator
  participant upgradeSandboxes
  participant ReadOnlyGatewayList
  participant OpenShell
  Operator->>upgradeSandboxes: run upgrade-sandboxes --check
  upgradeSandboxes->>ReadOnlyGatewayList: resolve and query recorded gateway
  ReadOnlyGatewayList->>OpenShell: sandbox list -g gateway
  OpenShell-->>ReadOnlyGatewayList: sandbox state or drift result
  ReadOnlyGatewayList-->>upgradeSandboxes: read-only list
  upgradeSandboxes-->>Operator: report sandbox status

Loading

Possibly related PRs

• NVIDIA/NemoClaw#6305: Updates the gateway-scoped absent-recovery confirmation flow in the same upgrade path.

Suggested labels: refactor

Suggested reviewers: cv, apurvvkumaria

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% 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
Linked Issues check	✅ Passed	The changes satisfy #7279 by making --check read-only, targeting the recorded gateway, and avoiding gateway start/select/recovery side effects.
Out of Scope Changes check	✅ Passed	The diff stays focused on the check-mode gateway fix plus supporting tests and docs, with no clear unrelated changes.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly captures the main change: making upgrade-sandboxes --check read-only and routing it to the recorded gateway.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/7279-upgrade-check-readonly-gateway

---

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

[github-code-quality]

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 4a3b6ef in the fix/7279-upgrade-che... branch remains at 96%, unchanged from commit 70a8b6b in the main branch.

---

_{Updated July 24, 2026 03:21 UTC}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/lib/actions/upgrade-sandboxes.ts (1)

223-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider warning on invalid persisted gateway bindings, like the sibling helper does.

resolveCheckGatewayName silently discards resolveSandboxGatewayName throws via an empty catch, whereas isPreparedRecoveryCandidate (lines 171-178) warns via console.warn on the same failure mode. Since --check exists specifically to surface diagnostic problems, silently falling back to the ambient default when all/some recorded bindings are corrupt hides a signal an operator running --check would want to see.

♻️ Proposed fix to surface invalid bindings

   for (const sandbox of sandboxes) {
     try {
       recorded.add(resolveSandboxGatewayName(sandbox));
-    } catch {
-      // Invalid persisted binding — never drive a list against it.
+    } catch {
+      console.warn(
+        `  Warning: sandbox ${JSON.stringify(sandbox.name)} has an invalid persisted gateway binding; excluding it from check-mode gateway resolution.`,
+      );
     }
   }

🤖 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 223 - 236, Update
resolveCheckGatewayName so its catch block warns with console.warn when
resolveSandboxGatewayName fails, matching the warning behavior in
isPreparedRecoveryCandidate while still excluding invalid bindings and returning
the existing recorded-or-fallback gateway selection.

src/lib/actions/upgrade-sandboxes-preflight.test.ts (1)

131-152: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add coverage for the ambiguous/no-recorded-gateway fallback branch.

resolveCheckGatewayName's docstring (src/lib/actions/upgrade-sandboxes.ts, lines 213-222) explicitly documents that several distinct recorded gateways (or none) fall back to the ambient default, but no test in this suite exercises multiple sandboxes with differing gatewayPort/gatewayName bindings (or all-invalid bindings) to confirm that fallback actually fires.

🤖 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 131 - 152,
Add tests for the ambiguous gateway fallback in the upgrade-sandboxes preflight
suite, using multiple sandboxes with differing valid gatewayPort/gatewayName
bindings and a separate case where all bindings are invalid or absent. Verify
resolveCheckGatewayName falls back to the ambient default and the preflight uses
that fallback path, while preserving the existing recorded-gateway behavior.

🤖 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/upgrade-sandboxes-preflight.test.ts`:
- Line 116: Update the test title in the `it` block to append the local issue
reference `(`#7279`)` as the final suffix, matching the sibling tests while
preserving the existing behavior-oriented wording.

---

Nitpick comments:
In `@src/lib/actions/upgrade-sandboxes-preflight.test.ts`:
- Around line 131-152: Add tests for the ambiguous gateway fallback in the
upgrade-sandboxes preflight suite, using multiple sandboxes with differing valid
gatewayPort/gatewayName bindings and a separate case where all bindings are
invalid or absent. Verify resolveCheckGatewayName falls back to the ambient
default and the preflight uses that fallback path, while preserving the existing
recorded-gateway behavior.

In `@src/lib/actions/upgrade-sandboxes.ts`:
- Around line 223-236: Update resolveCheckGatewayName so its catch block warns
with console.warn when resolveSandboxGatewayName fails, matching the warning
behavior in isPreparedRecoveryCandidate while still excluding invalid bindings
and returning the existing recorded-or-fallback gateway selection.

🪄 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: 97a2b463-203a-4ee5-b608-db93bde3d85a

📥 Commits

Reviewing files that changed from the base of the PR and between d5fa194 and 9441634.

📒 Files selected for processing (4)

• src/lib/actions/upgrade-sandboxes-preflight.test.ts
• src/lib/actions/upgrade-sandboxes.ts
• src/lib/openshell-sandbox-list.test.ts
• src/lib/openshell-sandbox-list.ts

[github-actions]

PR Review Advisor — Informational

Advisor assessment: Informational / high confidence
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions
Status: No actionable findings remain in the canonical review ledger.

Model lanes

• GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
• Nemotron 3 Ultra (second opinion): Failed after a partial review · low confidence · 0 blockers · 2 warnings · 0 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: state-backup-restore, upgrade-stale-sandbox

1 optional E2E recommendation

• gateway-guard-recovery

Workflow run details

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

[yanyunl1991]

CI note: the red reviewed-npm-audit (and the checks umbrella that cascades from it) is not caused by this PR — it touches no package*.json/lockfile. It fails repo-wide (also red on unrelated open PRs) on newly-published advisories in the pinned OpenClaw runtime archive graph: tar GHSA-23hp-3jrh-7fpw (Critical) and axios GHSA-gcfj-64vw-6mp9 (High), both transitive deps of the OpenClaw/plugin packages pinned in ci/reviewed-npm-audit.json. That remediation is tracked in #7275 and will clear once the reviewed archive is bumped on main. E2E / PR Gate Coordination is likewise red on unrelated PRs (credential-less fork-PR coordination); the actual E2E lanes relevant here — test-e2e-gateway-isolation, test-e2e-port-overrides, test-e2e-sandbox — pass.

[github-actions]

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/lib/actions/upgrade-sandboxes-recovery.test.ts (1)

552-580: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Avoid coupling this test to private call shape.

The exact context objects and toHaveBeenNthCalledWith ordering can break on harmless orchestration changes without proving a user-visible outcome. Keep a narrow assertion for the recorded gateway if needed, but prefer asserting the resulting check behavior and that gateway mutation/recovery boundaries remain untouched.

🤖 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 552 - 580,
Update the test for upgradeSandboxes in check mode to avoid asserting exact
readOnlyListSpy context objects and call ordering. Assert the user-visible check
outcome, retain only a narrow gateway-recording assertion if needed, and
continue verifying that liveListSpy and rebuildSpy are not called.

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 `@docs/manage-sandboxes/update-sandboxes.mdx`:
- Around line 88-89: Rewrite the `upgrade-sandboxes --check` documentation in
direct, second-person wording, addressing the reader as “you” rather than
describing command behavior impersonally. Use the shared host CLI form
established by the surrounding MDX guidance, and preserve the behavior involving
recorded gateways and `NEMOCLAW_GATEWAY_PORT`.

---

Nitpick comments:
In `@src/lib/actions/upgrade-sandboxes-recovery.test.ts`:
- Around line 552-580: Update the test for upgradeSandboxes in check mode to
avoid asserting exact readOnlyListSpy context objects and call ordering. Assert
the user-visible check outcome, retain only a narrow gateway-recording assertion
if needed, and continue verifying that liveListSpy and rebuildSpy are not
called.

🪄 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: 8f97fc7b-c6fb-429d-8030-44c78c822385

📥 Commits

Reviewing files that changed from the base of the PR and between c5acbe6 and 15a6888.

📒 Files selected for processing (2)

• docs/manage-sandboxes/update-sandboxes.mdx
• src/lib/actions/upgrade-sandboxes-recovery.test.ts

[prekshivyas]

Reviewed exact head ced432b against base ac5579e. The read-only gateway fix matches #7279, focused and broad CI pass, both selected E2E scenarios pass, CodeRabbit and both PR advisors are clean, and the security review found no issues.