fix(onboard): bind gateway name, state dir, and marker per gateway port (#4422) - #4645
Merged
Merged
Conversation
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 (11)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughThis PR makes OpenShell gateway identifiers, health checks, state dirs, Docker compat container names, and sandbox registry entries port-aware so multiple NemoClaw sandboxes can coexist on different gateway ports while keeping the default-port behavior unchanged. ChangesPer-port OpenShell gateway isolation
Sequence Diagram(s)sequenceDiagram
participant User
participant Onboard as Onboarding Flow
participant Resolver as Gateway Binding Resolvers
participant Classifier as Gateway State Classifiers
participant Docker as Docker Driver
participant Registry as Sandbox Registry
User->>Onboard: start onboard (GATEWAY_PORT)
Onboard->>Resolver: resolveGatewayName/StateDir/CompatContainer(GATEWAY_PORT)
Resolver-->>Onboard: GATEWAY_NAME, stateDirName, compatContainerName
Onboard->>Classifier: isGatewayHealthy/getGatewayReuseState(..., GATEWAY_NAME)
Classifier-->>Onboard: health/reuse/stale decision
Onboard->>Docker: build/run with compatContainerName, use stateDirName
Docker-->>Onboard: runtime identity and marker files
Onboard->>Registry: registerSandbox(..., gatewayName, gatewayPort)
Registry-->>Onboard: persisted sandbox entry
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
yimoj
force-pushed
the
fix/4422-support-per-sandbox-gateway-port
branch
from
June 2, 2026 05:47
59d6191 to
0125eae
Compare
![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: 3
🤖 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/onboard.ts`:
- Line 1237: Duplicate construction of the Docker-driver runtime identity (the
long call to dockerDriverGatewayLaunch.buildDockerDriverGatewayRuntimeIdentity
with gatewayBin, baseDesiredEnv/gatewayEnv, getDockerDriverGatewayStateDir(),
resolveOpenShellSandboxBinary(), and
resolveGatewayCompatContainerName(GATEWAY_PORT)) should be extracted into a
small helper function (e.g., buildDockerDriverRuntimeIdentity or
getRuntimeIdentity) that returns the same object; replace both call sites where
runtimeIdentity is assigned (the current inline call and the other duplicated
occurrence) with calls to that helper, passing in gatewayBin and any contextual
inputs (or capturing shared values like baseDesiredEnv and GATEWAY_PORT inside
the helper) to reduce duplication and complexity while keeping behavior
identical.
- Around line 3860-3861: Early-return reuse paths update dashboard metadata but
skip persisting gateway fields, leaving legacy registry shapes; ensure those
branches also write gatewayName and gatewayPort. Locate the reuse-return blocks
that call the dashboard update helper (the branches that return early after
updating metadata) and add the gateway fields (gatewayName: GATEWAY_NAME,
gatewayPort: GATEWAY_PORT) into the object passed to the registry
persistence/update function (or call the same save codepath used by fresh
registration) so the registry is backfilled on reuse. Make sure you use the
existing constants GATEWAY_NAME and GATEWAY_PORT and the same persistence helper
(the function that saves registry metadata) to avoid divergence.
In `@src/lib/onboard/docker-driver-gateway-launch.ts`:
- Around line 295-297: The code uses
env.NEMOCLAW_OPENSHELL_GATEWAY_COMPAT_CONTAINER_NAME before
options.compatContainerName when computing containerName, so the global env
always overrides per-sandbox names; change the selection to prefer
options.compatContainerName (trimmed) first, then fall back to the env var, then
DEFAULT_COMPAT_CONTAINER_NAME, and pass that chosen value into safeDockerName
(e.g., compute a chosenName = options.compatContainerName?.trim() ||
env.NEMOCLAW_OPENSHELL_GATEWAY_COMPAT_CONTAINER_NAME ||
DEFAULT_COMPAT_CONTAINER_NAME and use that when calling safeDockerName to set
containerName).
🪄 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: 449a53ca-bb44-4435-b729-5021091d3f24
📒 Files selected for processing (11)
src/lib/adapters/openshell/gateway-drift.tssrc/lib/core/ports.tssrc/lib/onboard.tssrc/lib/onboard/docker-driver-gateway-launch.tssrc/lib/onboard/gateway-binding.test.tssrc/lib/onboard/gateway-binding.tssrc/lib/onboard/gateway-reuse.tssrc/lib/state/gateway.tssrc/lib/state/registry.tstest/gateway-state.test.tstest/registry.test.ts
| runCaptureOpenshell(["--version"], { ignoreError: true }), | ||
| ); | ||
| const runtimeIdentity = gatewayBin ? dockerDriverGatewayLaunch.buildDockerDriverGatewayRuntimeIdentity({ gatewayBin, gatewayEnv: baseDesiredEnv, stateDir: getDockerDriverGatewayStateDir(), sandboxBin: resolveOpenShellSandboxBinary() }) : null; | ||
| const runtimeIdentity = gatewayBin ? dockerDriverGatewayLaunch.buildDockerDriverGatewayRuntimeIdentity({ gatewayBin, gatewayEnv: baseDesiredEnv, stateDir: getDockerDriverGatewayStateDir(), sandboxBin: resolveOpenShellSandboxBinary(), compatContainerName: resolveGatewayCompatContainerName(GATEWAY_PORT) }) : null; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Extract the Docker-driver runtime-identity builder.
This argument bundle is duplicated in both call sites. With the growth guardrail already failing for src/lib/onboard.ts, a small helper here would reduce drift risk and buy back a few lines.
♻️ Proposed refactor
+function buildDockerDriverRuntimeIdentity(
+ gatewayBin: string | null,
+ gatewayEnv: Record<string, string>,
+ stateDir: string = getDockerDriverGatewayStateDir(),
+) {
+ if (!gatewayBin) return null;
+ return dockerDriverGatewayLaunch.buildDockerDriverGatewayRuntimeIdentity({
+ gatewayBin,
+ gatewayEnv,
+ stateDir,
+ sandboxBin: resolveOpenShellSandboxBinary(),
+ compatContainerName: resolveGatewayCompatContainerName(GATEWAY_PORT),
+ });
+}
+
async function refreshDockerDriverGatewayReuseState(
gatewayReuseState: GatewayReuseState,
): Promise<GatewayReuseState> {
@@
- const runtimeIdentity = gatewayBin ? dockerDriverGatewayLaunch.buildDockerDriverGatewayRuntimeIdentity({ gatewayBin, gatewayEnv: baseDesiredEnv, stateDir: getDockerDriverGatewayStateDir(), sandboxBin: resolveOpenShellSandboxBinary(), compatContainerName: resolveGatewayCompatContainerName(GATEWAY_PORT) }) : null;
+ const runtimeIdentity = buildDockerDriverRuntimeIdentity(
+ gatewayBin,
+ baseDesiredEnv,
+ );
@@
- const runtimeIdentity = gatewayBin ? dockerDriverGatewayLaunch.buildDockerDriverGatewayRuntimeIdentity({ gatewayBin, gatewayEnv, stateDir, sandboxBin: resolveOpenShellSandboxBinary(), compatContainerName: resolveGatewayCompatContainerName(GATEWAY_PORT) }) : null;
+ const runtimeIdentity = buildDockerDriverRuntimeIdentity(
+ gatewayBin,
+ gatewayEnv,
+ stateDir,
+ );Also applies to: 2461-2461
🤖 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/onboard.ts` at line 1237, Duplicate construction of the Docker-driver
runtime identity (the long call to
dockerDriverGatewayLaunch.buildDockerDriverGatewayRuntimeIdentity with
gatewayBin, baseDesiredEnv/gatewayEnv, getDockerDriverGatewayStateDir(),
resolveOpenShellSandboxBinary(), and
resolveGatewayCompatContainerName(GATEWAY_PORT)) should be extracted into a
small helper function (e.g., buildDockerDriverRuntimeIdentity or
getRuntimeIdentity) that returns the same object; replace both call sites where
runtimeIdentity is assigned (the current inline call and the other duplicated
occurrence) with calls to that helper, passing in gatewayBin and any contextual
inputs (or capturing shared values like baseDesiredEnv and GATEWAY_PORT inside
the helper) to reduce duplication and complexity while keeping behavior
identical.
…rt (NVIDIA#4422) A second sandbox onboarded with NEMOCLAW_GATEWAY_PORT recreated the process-global `nemoclaw` gateway and reused the singleton Docker-driver state directory, so creating it tore down the first sandbox's gateway and overwrote its runtime marker. Derive a per-port gateway binding: the default port keeps the bare `nemoclaw` name, state dir, and compat container for backward compatibility, while a non-default port yields `nemoclaw-<port>`, `openshell-docker-gateway-<port>`, and `nemoclaw-openshell-gateway-<port>`. This isolates each sandbox's gateway registration, pid file, runtime marker, and compatibility container so a second onboard no longer recreates, kills, or overwrites the first sandbox's gateway. - Add gateway-binding resolver (default-port-preserving) and thread it through onboard gateway name, Docker-driver state dir, and compat container name. - Make gateway health/reuse classifiers gateway-name aware (defaults preserve singleton behavior); bind them to the resolved name in onboard, gateway-reuse, and the cluster-active drift probe. - Persist gatewayName/gatewayPort per sandbox in the registry. - Add regression coverage: distinct runtime markers for two gateway ports (second onboard does not overwrite/invalidate the first), per-port compat container naming, port-aware health classification, and registry persistence. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
yimoj
force-pushed
the
fix/4422-support-per-sandbox-gateway-port
branch
from
June 2, 2026 05:59
0125eae to
7e47e05
Compare
Collaborator
Author
|
Thanks for the review. Addressed the findings:
|
cv
approved these changes
Jun 2, 2026
12 tasks
cv
pushed a commit
that referenced
this pull request
Jun 3, 2026
## Summary - Add the missing `v0.0.57` release-notes section with links to the detailed docs pages for command, inference, onboarding, messaging, status, installer, and policy changes. - Remove public references to docs-skip terms from source docs and regenerate the NemoClaw user skills from the current Fern MDX docs. - Carry forward generated references for the per-agent documentation split, including Hermes-specific reference files. ## Source summary - #4615 and #4653 -> `docs/about/release-notes.mdx`, `docs/reference/commands.mdx`: Release notes now cover host-side `sessions` and `agents` commands plus `NEMOCLAW_EXTRA_AGENTS_JSON` secondary-agent baking. - #4163, #4204, #4611, #4619, and #4676 -> `docs/about/release-notes.mdx`, `docs/inference/use-local-inference.mdx`: Release notes now cover managed vLLM progress/readiness, DGX Spark model default changes, local Ollama streaming usage, and inference route divergence warnings. - #4267, #4601, #4609, #4642, #4645, and #4661 -> `docs/about/release-notes.mdx`, `docs/reference/commands.mdx`: Release notes now cover UFW auto-remediation, local-inference reachability gates, gateway reuse/binding, cancel rollback, and policy selection persistence. - #4577, #4582, #4607, and #4660 -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/messaging-channels.mdx`: Release notes now cover Slack validation, atomic `channels add`, WhatsApp QR diagnostics, and Slack placeholder normalization. - #4388, #4600, #4646, and #4647 -> `docs/about/release-notes.mdx`, `docs/reference/commands.mdx`: Release notes now cover status failure layers, paused-container hints, Docker-driver doctor behavior, and non-destructive stale-registry recovery. - #4569, #4579, and #4678 -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/lifecycle.mdx`, `docs/network-policy/integration-policy-examples.mdx`: Release notes now cover installer tag pinning, PyPI `uv` policy access, and observable Jira validation. - #4632 -> `.agents/skills/`: Regenerated user skills from the current per-agent docs source, including newly generated Hermes reference files. ## Verification - `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix nemoclaw-user --doc-platform fern-mdx` - `rg "permissive mode|shields down|shields up|shields status|config rotate-token|rotate-token" docs --glob "*.mdx"` - `rg "permissive mode|shields down|shields up|shields status|config rotate-token|rotate-token" .agents/skills --glob "*.md"` - `npm run docs` - `npm run build:cli` - Commit hooks: markdownlint, docs-to-skills verification, gitleaks, skills YAML, commitlint <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Restructured documentation to clearly distinguish OpenClaw and Hermes agent variants throughout user guides. * Enhanced security, credential storage, and deployment guidance with clearer setup flows. * Added Hermes plugin installation and ecosystem documentation. * Improved workspace, messaging, and policy management references with variant-specific command examples. * Refined troubleshooting and CLI reference sections for clarity. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
5 tasks
5 tasks
yimoj
added a commit
to yimoj/NemoClaw
that referenced
this pull request
Jun 10, 2026
…NVIDIA#4497) PR NVIDIA#4647 stopped `connect` from deleting the registry entry of a sandbox that is registered locally but absent from a healthy gateway, so the `rebuild --yes` hint `status` prints would have something to recover. But `rebuild` still dead-ended: its backup step aborted with "Sandbox '<name>' is not running. Cannot back up state." whenever the live sandbox was missing — exactly the stale state — so the recommended recovery path stayed broken and the issue reopened. Treat a registered-but-not-live sandbox as a recovery rebuild: there is no live workspace state to back up, so skip the backup/restore steps and recreate from the preserved registry + onboard-session metadata. Guard the no-backup path so it never destroys live state: - Confirm absence authoritatively against the NAMED nemoclaw gateway (getReconciledSandboxGatewayState runs `sandbox get`), and only treat `present` as live when nemoclaw is the active healthy gateway — a foreign active gateway (multi-gateway, NVIDIA#4645) or a list/get inconsistency must not trigger a destructive recreate. - Refuse the destructive path for a sandbox recorded on a non-default per-port gateway; point the operator at `openshell gateway select`. - On recreate failure, restore the captured registry entry verbatim (under the registry lock, target-only) so `rebuild`/`connect` stay retryable without clobbering concurrent changes; reclaim the default pointer. - Reset the gone sandbox's stale shields seal only after a successful recreate, and tell the operator to re-apply `shields up` if it was locked. Add a focused regression suite (rebuild-stale-recovery.test.ts) covering the recover, control, multi-gateway, per-port, and failed-recreate cases; update the NVIDIA#2276/NVIDIA#4497 reconcile Scenario 14 and the gateway-drift retry test to assert recovery instead of the dead-end; and strengthen the double-onboard E2E so the exact reporter workflow (status -> connect -> rebuild --yes) must recreate a live sandbox — the old probe only checked for "does not exist" and would have passed against this bug. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
cv
pushed a commit
that referenced
this pull request
Jun 10, 2026
…#4497) (#5034) ## Summary `rebuild --yes` no longer dead-ends on a stale sandbox. When a sandbox is registered locally but absent from a healthy OpenShell gateway (stuck/diverged provision, container reaped), rebuild now treats it as a recovery: it skips the impossible backup and recreates from the preserved registry + onboard-session metadata, instead of aborting with "Cannot back up state." ## Related Issue Fixes #4497 ## Changes - `rebuild`: when the sandbox is missing from the active gateway's `sandbox list`, confirm absence authoritatively against the **named** nemoclaw gateway (`getReconciledSandboxGatewayState` runs `sandbox get`). Only enter the destructive stale-recovery path once it is genuinely gone on a healthy named gateway. - Stale recovery skips backup/restore and recreates from registry/session metadata; built-in policy presets are re-applied from `sb.policies` (the same source the backup manifest uses). - **Multi-gateway data-loss guards:** never recreate-from-scratch when (a) a foreign gateway is active (a `present`/list result there is not trusted), or (b) the sandbox is recorded on a non-default per-port gateway (#4645) — the operator is pointed at `openshell gateway select` instead. - **Crash-safety:** on recreate failure, restore the captured registry entry verbatim (target-only, under the registry lock — no clobbering concurrent changes) and reclaim the default-sandbox pointer, so `rebuild`/`connect` stay retryable. - **Shields:** reset the gone sandbox's stale lock seal only *after* a successful recreate (a failed recreate keeps the lockdown record for retry), and prompt the operator to re-apply `shields up` if it had been locked. - New `registry.restoreSandboxEntry` and `shields.clearShieldsState` helpers. ## Type of Change - [x] Code change (feature, bug fix, or refactor) ## Verification - [x] `npm test` passes (only the pre-existing `test/ssrf-parity.test.ts` fails, identically on clean `upstream/main` — unrelated to this change) - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [x] `npx biome check` clean on changed files; `shfmt`/`shellcheck` clean on the E2E script; `tsc` type-check passes ### Local end-to-end proof (exact reporter workflow on a real OpenShell gateway) Drove `status -> connect -> rebuild --yes` against a real healthy gateway with a registered-but-not-live sandbox: ``` ⚠ Sandbox 'repro4497stale' is registered locally but absent from the live OpenShell gateway. No live workspace state to back up — recreating from the preserved registry metadata. Deleting old sandbox... ✓ Old sandbox deleted Creating new sandbox with current image... ``` No "Cannot back up state", no "Backing up sandbox state" — rebuild crossed the gate that previously dead-ended and proceeded into the recreate (`onboard --resume`). The user's other registry entries were untouched. ### Regression + pipeline coverage - `test/rebuild-stale-recovery.test.ts` — recover, live-control, foreign-gateway guard, non-default per-port guard, failed-recreate registry rollback (incl. `defaultSandbox`). - `test/gateway-state-reconcile-2276.test.ts` Scenario 14 and `rebuild-gateway-drift.test.ts` updated to assert recovery rather than the dead-end. - `test/e2e/test-double-onboard.sh` Phase 5 strengthened so the exact reporter workflow must recreate a **live** sandbox (the prior probe only checked for "does not exist" and would have passed against this bug). ### Known limitation (pre-existing, shared with normal rebuild) Custom `policy-add --from-file/--from-dir` egress rules (`customPolicies`) are not re-applied to the recreated sandbox — this is identical to a normal rebuild's recreate path (the registry entry is removed before `onboard --resume` runs) and is out of scope for this dead-end fix. --- Signed-off-by: Yimo Jiang <yimoj@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** * Enhanced sandbox recovery when a sandbox exists locally but is missing from the live gateway, preventing unnecessary destructive operations and enabling a “stale-sandbox” recovery path. * Improved rebuild behavior to preserve and restore local registry metadata so retrying recovery no longer fails. * **Improvements** * Updated post-recreate and error messaging to include clearer context and preserved backup paths; retry guidance is now configurable. * Shield handling adjusted to avoid unsafe unlock/relock behavior during stale recovery. * **New Features** * Added APIs to restore a removed registry entry and to clear persisted shields state. * **Tests** * Added and expanded end-to-end and unit tests covering stale-sandbox recovery scenarios and regressions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
This was referenced Jul 13, 2026
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
A second sandbox onboarded with
NEMOCLAW_GATEWAY_PORTrecreated the process-globalnemoclawgateway and reused the singleton Docker-driver state directory, so creating it tore down the first sandbox's gateway and overwrote its runtime marker. This binds the gateway name, Docker-driver state dir (pid file + runtime marker), and compatibility container per gateway port so the second onboard no longer disrupts the first.Related Issue
Fixes #4422.
Supersedes the runtime behavior left as a no-op in the groundwork PR #4598 (its resolver still returned the singleton name for every port).
Changes
src/lib/onboard/gateway-binding.ts— a per-port resolver. The default gateway port keeps the barenemoclawname,openshell-docker-gatewaystate dir, andnemoclaw-openshell-gatewaycompat container verbatim (backward compatible); a non-default port yieldsnemoclaw-<port>,openshell-docker-gateway-<port>, andnemoclaw-openshell-gateway-<port>.onboard.ts: resolveGATEWAY_NAME, the Docker-driver state dir, and the compat container name fromGATEWAY_PORT; bind the gateway health/reuse classifiers to the resolved name.state/gateway.ts:hasStaleGateway/isGatewayHealthy/getGatewayReuseState/shouldSelectNamedGatewayForReuseaccept an optional gateway name (default preserves the singleton).hasStaleGatewaynow matches the reported gateway name exactly instead of by substring.gateway-reuse.tsand thegateway-drift.tscluster-active probe forward the resolved gateway name to the classifiers.state/registry.ts: persistgatewayName/gatewayPortper sandbox.Follow-up (out of scope here)
Threading the persisted
gatewayNameinto every post-create read path (status/connect/destroy/doctor/snapshot) and anchoring the legacyopenshell-cluster-*volume-prefix cleanup remain for a follow-up; they affect the second sandbox's own later lifecycle under the legacy cluster driver, not the create-time teardown this PR fixes.Type of Change
Verification
npm testpasses (targeted gateway/registry/reuse/drift suites green; full-suite failures on this host are environmental — unbuilt plugin dist, real-process timeouts under load, and a pre-existing chmod-ownership test that also fails onmain)npm run typecheck:clipassesReproduced end-to-end with the worktree build: a second onboard on port 8081 overwrote the first sandbox's Docker-driver runtime marker (endpoint→8081, pid changed → first gateway drifts) before the fix; after the fix the two markers live in distinct per-port state dirs and the first sandbox's marker is preserved with no drift.
Signed-off-by: Yimo Jiang yimoj@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests