fix(sandbox): give each Hermes sandbox its own OpenAI-compatible API port - #8577
fix(sandbox): give each Hermes sandbox its own OpenAI-compatible API port#8577laitingsheng wants to merge 4 commits into
Conversation
…port The API port was a fixed host-wide 8642, so a second Hermes sandbox on one host could never own an inference forward. A snapshot clone therefore received no inference forward and its gateway restart could not converge. Allocate the port per sandbox from 8642-8652, persist it on the registry row, and publish it to the sandbox through the create environment. The entrypoint re-publishes it as a root-owned read-only marker because a one-shot sandbox exec does not inherit the supervisor environment. Forward recovery now resolves manifest-declared ports against the sandbox's own ports instead of demanding the agent defaults a sibling sandbox already holds. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
The first pass left three host-side surfaces on the manifest default: the `sandbox agent` rejection message, the onboarding ready summary, and the docs. Each told an operator to forward a port their sandbox does not use. Relaunch also reached the allocator through the sandbox-create environment. A sandbox registered before the port became per-sandbox has no recorded value, so the allocator saw its own live forward holding the default and moved it, leaving the sandbox bound to one port while the host forwarded another. A registered sandbox without a port now keeps the default; only an unregistered sandbox takes a fresh allocation. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…er-sandbox Signed-off-by: Tinson Lai <tinsonl@nvidia.com> # Conflicts: # ci/source-architecture-budget.json
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughHermes now assigns API ports per sandbox from ports 8642–8652. The selected port is stored in registry state, passed to sandbox startup, published through a runtime marker, and used by forwarding, recovery, cloning, and health checks. ChangesHermes API port allocation and propagation
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-8577.docs.buildwithfern.com/nemoclaw |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/onboard/dashboard-port.ts (1)
318-344: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject explicit Hermes API/dashboard port overlaps before sandbox creation.
assertDashboardPortNotReservedrejects dashboard ports in8642–8652, butresolveOnboardHermesApiPortaccepts any validNEMOCLAW_HERMES_API_PORTwithout checking the resolved dashboard port. ThusNEMOCLAW_HERMES_API_PORT=18789can map both resources to one host port. Reject this equality and add regression tests for both cases.🤖 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/dashboard-port.ts` around lines 318 - 344, Update resolveOnboardHermesApiPort to reject configurations where the resolved Hermes API port equals the resolved dashboard port, including explicit NEMOCLAW_HERMES_API_PORT values and automatically selected ports. Ensure the rejection occurs before sandbox creation and add regression tests covering both explicit overlap and overlap produced by resolution.
🤖 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 `@agents/hermes/start.sh`:
- Around line 173-196: Use the shared Hermes API-port validity contract of
8642–8652 at all affected sites: update agents/hermes/start.sh lines 173-196,
agents/hermes/mcp-config-transaction.py lines 88-106,
agents/hermes/plugin/__init__.py lines 1035-1051,
scripts/managed-gateway-control.py lines 1125-1151, and scripts/install.sh lines
465-485 to reject values outside that inclusive range while preserving existing
defaults and error handling.
In `@docs/deployment/deploy-to-headless-server.mdx`:
- Line 292: Update all listed documentation examples to reflect dynamic Hermes
API-port allocation: in docs/deployment/deploy-to-headless-server.mdx lines
292-292, qualify 8642 as the first-sandbox default and revise adjacent curl
guidance; in docs/reference/commands.mdx lines 1216-1217, replace fixed-port
wording and update the Hermes agent example to use the port from openshell
forward list; in docs/reference/commands.mdx lines 1918-1919, use the selected
API port for the gateway-token example; and in
docs/reference/troubleshooting.mdx lines 3132-3133, qualify the 8642
troubleshooting path and update its health/client examples accordingly.
In `@docs/reference/troubleshooting.mdx`:
- Around line 3163-3167: Update the troubleshooting guidance around the final
destroy/re-onboard instruction to distinguish new second Hermes sandboxes from
existing pre-change sandboxes. State that new sandboxes receive the next free
API port automatically, while destroy and re-onboard is required only for an
existing sandbox with a registry row lacking hermesApiPort.
In `@src/lib/agent/onboard.ts`:
- Around line 559-577: The API dashboard branch that calls
printAdditionalForwardPorts must pass sandboxName so Hermes resolves its
registered sandbox-specific API port instead of the manifest default. Update
that kind === "api" call site, and add coverage for the Hermes dashboard.kind
=== "api" path verifying the sandbox-specific port is announced.
---
Outside diff comments:
In `@src/lib/onboard/dashboard-port.ts`:
- Around line 318-344: Update resolveOnboardHermesApiPort to reject
configurations where the resolved Hermes API port equals the resolved dashboard
port, including explicit NEMOCLAW_HERMES_API_PORT values and automatically
selected ports. Ensure the rejection occurs before sandbox creation and add
regression tests covering both explicit overlap and overlap produced by
resolution.
🪄 Autofix
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: dc739679-b1ab-436c-a70f-c20c3887833e
📒 Files selected for processing (37)
agents/hermes/mcp-config-transaction.pyagents/hermes/plugin/__init__.pyagents/hermes/start.shci/source-architecture-budget.jsondocs/deployment/deploy-to-headless-server.mdxdocs/deployment/set-up-mcp-bridge.mdxdocs/get-started/quickstart-hermes.mdxdocs/reference/commands.mdxdocs/reference/troubleshooting.mdxscripts/install.shscripts/managed-gateway-control.pysrc/commands/sandbox/agent.tssrc/lib/actions/sandbox/agent/passthrough-help.tssrc/lib/actions/sandbox/agent/passthrough.test.tssrc/lib/actions/sandbox/agent/passthrough.tssrc/lib/actions/sandbox/forward-recovery-declared-ports.test.tssrc/lib/actions/sandbox/forward-recovery.tssrc/lib/actions/sandbox/process-recovery.tssrc/lib/actions/sandbox/snapshot-restore-clone-ports.test.tssrc/lib/actions/sandbox/snapshot.tssrc/lib/agent/onboard.test.tssrc/lib/agent/onboard.tssrc/lib/core/ports.tssrc/lib/onboard/agent-dashboard-forward.tssrc/lib/onboard/dashboard-port.tssrc/lib/onboard/hermes-api-port.test.tssrc/lib/onboard/hermes-api-port.tssrc/lib/onboard/hermes-dashboard.tssrc/lib/onboard/managed-startup-profile.test.tssrc/lib/onboard/managed-startup/profile.tssrc/lib/onboard/preflight-ports.tssrc/lib/onboard/sandbox-create-launch.tssrc/lib/onboard/sandbox-registration.tssrc/lib/state/gateway-registry.tssrc/lib/state/registry.tssrc/lib/state/registry/types.tstest/hermes-mcp-reload-convergence.test.ts
| # The API port is a per-sandbox host resource: the host forwards the same | ||
| # number it is exposed on here, so two sandboxes on one host need two values. | ||
| # NemoClaw allocates the port and passes it in; the default keeps a sandbox | ||
| # whose create environment carries no value on the original port. | ||
| _api_port_raw="${NEMOCLAW_HERMES_API_PORT:-}" | ||
| if [ -z "$_api_port_raw" ]; then | ||
| PUBLIC_PORT=8642 | ||
| else | ||
| PUBLIC_PORT="$(printf '%s' "$_api_port_raw" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')" | ||
| _api_port_valid=1 | ||
| case "$PUBLIC_PORT" in | ||
| *[!0-9]* | '') _api_port_valid=0 ;; | ||
| esac | ||
| if [ "$_api_port_valid" -eq 1 ] && { [ "$PUBLIC_PORT" -lt 1024 ] || [ "$PUBLIC_PORT" -gt 65535 ]; }; then | ||
| _api_port_valid=0 | ||
| fi | ||
| if [ "$_api_port_valid" -ne 1 ]; then | ||
| echo "[SECURITY] Invalid NEMOCLAW_HERMES_API_PORT='${NEMOCLAW_HERMES_API_PORT}' - must be an integer between 1024 and 65535" >&2 | ||
| exit 1 | ||
| fi | ||
| fi | ||
|
|
||
| if [ "$_dashboard_port" -eq "$PUBLIC_PORT" ]; then | ||
| echo "[SECURITY] Invalid Hermes dashboard port ${PUBLIC_PORT} - reserved for the Hermes OpenAI-compatible API" >&2 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Use one Hermes API-port validity contract.
The PR defines Hermes API ports as 8642–8652. These consumers instead accept all unprivileged TCP ports. This permits unsupported values to bypass allocation and occupancy protection.
agents/hermes/start.sh#L173-L196: rejectNEMOCLAW_HERMES_API_PORTvalues outside8642–8652.agents/hermes/mcp-config-transaction.py#L88-L106: reject marker values outside8642–8652.agents/hermes/plugin/__init__.py#L1035-L1051: reject marker values outside8642–8652.scripts/managed-gateway-control.py#L1125-L1151: reject marker values outside8642–8652.scripts/install.sh#L465-L485: reject registry values outside8642–8652.
📍 Affects 5 files
agents/hermes/start.sh#L173-L196(this comment)agents/hermes/mcp-config-transaction.py#L88-L106agents/hermes/plugin/__init__.py#L1035-L1051scripts/managed-gateway-control.py#L1125-L1151scripts/install.sh#L465-L485
🤖 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 `@agents/hermes/start.sh` around lines 173 - 196, Use the shared Hermes
API-port validity contract of 8642–8652 at all affected sites: update
agents/hermes/start.sh lines 173-196, agents/hermes/mcp-config-transaction.py
lines 88-106, agents/hermes/plugin/__init__.py lines 1035-1051,
scripts/managed-gateway-control.py lines 1125-1151, and scripts/install.sh lines
465-485 to reject values outside that inclusive range while preserving existing
defaults and error handling.
| ``` | ||
|
|
||
| The Hermes OpenAI-compatible API uses the loopback forward on port `8642`. | ||
| The Hermes OpenAI-compatible API uses the loopback forward on the sandbox's API port, `8642` for the first Hermes sandbox on a host. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep public examples aligned with dynamic Hermes API-port allocation.
These passages introduce fallback allocation, but their surrounding examples still treat 8642 as fixed. Users can query or forward the wrong port. Use the assigned port from openshell forward list, or explicitly scope each example to an available first-sandbox default.
docs/deployment/deploy-to-headless-server.mdx#L292-L292: qualify8642as the default and update the adjacent curl guidance.docs/reference/commands.mdx#L1216-L1217: update the Hermes agent example and the preceding fixed-port wording.docs/reference/commands.mdx#L1918-L1919: update the gateway-token example to use the selected API port.docs/reference/troubleshooting.mdx#L3132-L3133: qualify the 8642 troubleshooting path and its health/client examples.
📍 Affects 3 files
docs/deployment/deploy-to-headless-server.mdx#L292-L292(this comment)docs/reference/commands.mdx#L1216-L1217docs/reference/commands.mdx#L1918-L1919docs/reference/troubleshooting.mdx#L3132-L3133
🤖 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/deployment/deploy-to-headless-server.mdx` at line 292, Update all listed
documentation examples to reflect dynamic Hermes API-port allocation: in
docs/deployment/deploy-to-headless-server.mdx lines 292-292, qualify 8642 as the
first-sandbox default and revise adjacent curl guidance; in
docs/reference/commands.mdx lines 1216-1217, replace fixed-port wording and
update the Hermes agent example to use the port from openshell forward list; in
docs/reference/commands.mdx lines 1918-1919, use the selected API port for the
gateway-token example; and in docs/reference/troubleshooting.mdx lines
3132-3133, qualify the 8642 troubleshooting path and update its health/client
examples accordingly.
| If `openshell forward list` does not show the sandbox's API port, run `nemohermes <name> connect --probe-only` (or `nemohermes <name> recover`) to ask the recovery path to re-establish every manifest-declared agent forward port that has gone missing. | ||
| Recovery targets each sandbox's own ports. | ||
| A second Hermes sandbox on the same host receives the next free API port, so check which sandbox owns each row before assuming a missing `8642` row belongs to the sandbox you are debugging. | ||
| A Hermes sandbox onboarded before the API port became per-sandbox carries no allocated port and keeps `8642`. | ||
| Run `nemohermes <name> destroy` and onboard the sandbox again when a second Hermes sandbox needs its own API forward, because NemoClaw allocates a port only for a sandbox that has no registry row. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify when destroy and re-onboard is required.
A newly created second Hermes sandbox has no registry row and should receive the next free API port. Destroy and re-onboard is needed only to migrate an existing pre-change sandbox that already has a registry row without hermesApiPort. As written, this guidance can cause an unnecessary destroy.
🤖 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/troubleshooting.mdx` around lines 3163 - 3167, Update the
troubleshooting guidance around the final destroy/re-onboard instruction to
distinguish new second Hermes sandboxes from existing pre-change sandboxes.
State that new sandboxes receive the next free API port automatically, while
destroy and re-onboard is required only for an existing sandbox with a registry
row lacking hermesApiPort.
| sandboxName?: string, | ||
| ): void { | ||
| const declared = Array.isArray(agent.forward_ports) ? agent.forward_ports : []; | ||
| if (declared.length === 0) return; | ||
| const apiPort = agent.healthProbe?.port; | ||
| for (const port of declared) { | ||
| if (!Number.isInteger(port) || port < 1024 || port > 65535) continue; | ||
| if (port === primaryPort || port === agent.forwardPort) continue; | ||
| const isApi = port === apiPort; | ||
| const declaredApiPort = agent.healthProbe?.port; | ||
| // The manifest names Hermes' default API port. This sandbox owns its own, so | ||
| // announce the port the operator actually has to forward. Only Hermes | ||
| // allocates a per-sandbox API port; every other agent keeps its declared one. | ||
| const sandboxApiPort = | ||
| agent.name === "hermes" | ||
| ? resolveSandboxHermesApiPort( | ||
| (sandboxName ? registry.getSandbox(sandboxName) : undefined) ?? {}, | ||
| ) | ||
| : 0; | ||
| for (const declaredPort of declared) { | ||
| if (!Number.isInteger(declaredPort) || declaredPort < 1024 || declaredPort > 65535) continue; | ||
| if (declaredPort === primaryPort || declaredPort === agent.forwardPort) continue; | ||
| const isApi = declaredPort === declaredApiPort; | ||
| const port = isApi && agent.name === "hermes" ? sandboxApiPort : declaredPort; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pass sandboxName from the API dashboard path.
The kind === "api" branch at Line 486 calls printAdditionalForwardPorts without sandboxName. For Hermes, this makes Line 570 resolve {} and announce manifest port 8642 instead of the sandbox-specific registered port.
Pass sandboxName in that branch. Add a Hermes dashboard.kind === "api" test.
Proposed fix
- printAdditionalForwardPorts(agent, info.port, deps.buildControlUiUrls);
+ printAdditionalForwardPorts(agent, info.port, deps.buildControlUiUrls, sandboxName);🤖 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/agent/onboard.ts` around lines 559 - 577, The API dashboard branch
that calls printAdditionalForwardPorts must pass sandboxName so Hermes resolves
its registered sandbox-specific API port instead of the manifest default. Update
that kind === "api" call site, and add coverage for the Hermes dashboard.kind
=== "api" path verifying the sandbox-specific port is announced.
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: Blockers
|
Summary
The Hermes OpenAI-compatible API port was a fixed host-wide
8642, so only one Hermes sandbox per host could own its host forward. A snapshot clone therefore received no inference forward, andgateway restarton the clone exited 1 with a forward recovery failure it could never repair. Each Hermes sandbox now allocates its own API port from8642through8652, and every consumer resolves that port instead of the manifest default.Related Issue
Fixes #8543
Changes
src/lib/core/ports.tsaddsHERMES_API_PORT_RANGE_START/ENDandisHermesApiPort.8642becomes the range start rather than a host-wide reservation.src/lib/onboard/hermes-api-port.tsis new: allocation,resolveOnboardHermesApiPort,resolveSandboxHermesApiPort, andretargetHermesApiPortInUrl. Resolution order is explicit environment value, then the registered value, then a fresh allocation, and the result is published back toNEMOCLAW_HERMES_API_PORT. That publication is the mechanism that lets the sandbox-create environment, the registry row, and the host forward agree on one value without threading an argument through the onboarding entrypoint, matching howensureAgentDashboardForwardalready propagates the dashboard port throughCHAT_UI_URL. Only a sandbox with no registry row takes a fresh allocation, because a registered sandbox without a port predates this change and is already bound to the default;src/lib/onboard/hermes-api-port.test.tsprotects both branches.src/lib/onboard/dashboard-port.tsextractsfindAvailablePortInRangeso the dashboard and API allocators share one forward-list, registry, and host-bind view, and addsgetRegistryOccupiedHermesApiPortsfor the cross-gateway view.hermesApiPort, sanitised insrc/lib/state/registry.tsand validated fail-closed insrc/lib/state/gateway-registry.tsso a corrupt value cannot drop occupancy and hand out a colliding port.src/lib/actions/sandbox/snapshot.tsallocates the clone's own port before any destructive step, so range exhaustion aborts beforedeleteSandboxForRestoreremoves a--forcedestination, and overrides the value the source-entry spread would otherwise inherit.src/lib/actions/sandbox/forward-recovery.tsresolves manifest-declared forward ports against the sandbox's own ports. This fixes both halves of the reported failure: recovery no longer demands the manifest dashboard port from a sandbox that was allocated another, and it targets the sandbox's own API port.agents/hermes/start.shreadsNEMOCLAW_HERMES_API_PORTand publishes it as a root-owned0444marker at/run/nemoclaw/hermes-api-port. A one-shotopenshell sandbox execdoes not inherit the supervisor environment, soagents/hermes/mcp-config-transaction.py,agents/hermes/plugin/__init__.py, andscripts/managed-gateway-control.pyread that marker. It is deliberately not writable by the sandbox user, because a writable source would let the agent redirect the relay health probes. The MCP reload phase identifiers drop their port literals, since the port now varies.sandbox agentrejection message, the onboarding ready summary, andscripts/install.shresolve the sandbox's port instead of naming8642.src/lib/onboard/preflight-ports.ts,src/lib/onboard/hermes-dashboard.ts, and the managed-startup profile validation cover the whole range rather than the single literal.ci/source-architecture-budget.jsonraises four limits. Introducing one module adds one edge each tosnapshot.tsfan-out,src/lib/onboardroot files, andcore/ports.ts/state/registry.tsfan-in. Two other increases were avoidable and were removed instead:getSandboxHealthProbeUrlmoved intoforward-recovery.ts, which already owned that dependency, andsnapshot.tsno longer importscore/ports.Type of Change
Quality Gates
0444marker at/run/nemoclaw/hermes-api-portthat three in-sandbox helpers read, and it widens the reserved-port set in the Shields-sealed managed-startup profile validation from one literal to a range. A maintainer should confirm the marker's trust boundary and the reservation change before merge.Documentation Writer Review
docs-updateddocs/get-started/quickstart-hermes.mdx,docs/reference/commands.mdx,docs/reference/troubleshooting.mdx,docs/deployment/deploy-to-headless-server.mdx,docs/deployment/set-up-mcp-bridge.mdx, plus the changed comments, CLI strings, and test titles across the diff. The subagent reviewed the writing rules and documentation style over three passes. Pass 1 raised six blocking findings, three of which were functional: thesandbox agentrejection message and the onboarding ready summary still emitted the manifest default, and the added docs pointed operators atstatus, which prints no port. Pass 2 raised five newly introduced items, including a guard that applied the Hermes port to any agent and unit tests that had begun reading the real sandbox registry. Pass 3 raised one blocking finding: the remedy for a pre-change sandbox namedrecreate, which preserves the registry row, where onlydestroyremoves it. All are applied. The final pass reported nothing blocking.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest runover the six affected suites — 193 passed.npm run checks:repository— exit 0.npm run typecheck:cli— exit 0.npm --prefix nemoclaw run typecheck— exit 0.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: not run locally; left to CI. An earlier serialnpx vitest run --project clion this branch reported 1155 files and 14423 tests passing, but that run predates the final two commits.npm run docsbuilds without warnings (doc changes only) —npm run docsexits 0 with 0 errors and 2 warnings. Both warnings are pre-existinglearn.microsoft.comlanguage-reference links indocs/reference/troubleshooting.mdxthat this change does not touch, so the box stays clear rather than claiming a warning-free build.Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
New Features
NEMOCLAW_HERMES_API_PORToverride support with validation and collision handling.Bug Fixes
Documentation