fix(sandbox): give each Hermes sandbox its own OpenAI-compatible API port - #8577
Open
laitingsheng wants to merge 46 commits into
Open
fix(sandbox): give each Hermes sandbox its own OpenAI-compatible API port#8577laitingsheng wants to merge 46 commits into
laitingsheng wants to merge 46 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
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughHermes now supports per-sandbox API ports in the 8642–8652 range. The selected port flows through onboarding, registry state, startup, health checks, forwarding, recovery, cloning, installer restoration, tests, and documentation. ChangesHermes API port lifecycle
Estimated code review effort: 5 (Critical) | ~90 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-8577.docs.buildwithfern.com/nemoclaw |
laitingsheng
added
integration: hermes
![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: 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
Contributor
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence 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 for the commit under review. Recommended E2E: Manual-only E2E: 3 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
This was referenced Aug 7, 2026
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
cv
requested changes
Aug 9, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
agents/hermes/start.sh:3319 publishes the trusted Hermes API-port marker only on the root startup path. The supported managed non-root path exits through lines 3254-3284 without creating it. agents/hermes/mcp-config-transaction.py, agents/hermes/plugin/__init__.py, and scripts/managed-gateway-control.py then silently use port 8642. For a sandbox configured for port 8643, a sandbox user can bind 8642 and cause reload or lifecycle probes to accept the wrong listener. Publish a supervisor-owned, sandbox-user-unwritable binding before non-root helpers can run, or make those helpers fail closed when the binding is absent. Add an integration regression test that starts managed non-root Hermes on 8643, verifies the binding ownership and mode, verifies all helpers probe 8643, and proves a listener on 8642 cannot satisfy readiness.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/plugin/__init__.py`:
- Around line 1048-1051: Update the port-parsing logic around the raw marker
conversion to catch ValueError from int(raw) and return the existing 8642
fallback, including for non-ASCII digit strings accepted by isdigit(). Add a
negative-path test covering such input.
In `@src/lib/onboard/hermes-api-port.ts`:
- Around line 125-154: Update the missing-or-invalid registry-state path used by
restore_onboard_forward_after_post_checks so it reuses the persisted result from
resolveOnboardHermesApiPort instead of falling back to the fixed 8642 port. If
no valid resolved port is available, fail closed rather than starting a watcher
on the default port, and add coverage for a custom hermesApiPort followed by
invalid registry state.
🪄 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: e00f63d3-7c15-4071-a6d8-692b6001f10a
📒 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
🚧 Files skipped from review as they are similar to previous changes (35)
- src/commands/sandbox/agent.ts
- src/lib/core/ports.ts
- src/lib/state/registry.ts
- src/lib/actions/sandbox/forward-recovery-declared-ports.test.ts
- docs/deployment/set-up-mcp-bridge.mdx
- docs/deployment/deploy-to-headless-server.mdx
- ci/source-architecture-budget.json
- src/lib/agent/onboard.test.ts
- src/lib/onboard/managed-startup-profile.test.ts
- src/lib/onboard/sandbox-registration.ts
- src/lib/onboard/preflight-ports.ts
- docs/get-started/quickstart-hermes.mdx
- src/lib/actions/sandbox/snapshot.ts
- scripts/managed-gateway-control.py
- src/lib/state/gateway-registry.ts
- src/lib/state/registry/types.ts
- src/lib/actions/sandbox/agent/passthrough.test.ts
- src/lib/actions/sandbox/process-recovery.ts
- docs/reference/troubleshooting.mdx
- src/lib/actions/sandbox/agent/passthrough.ts
- src/lib/actions/sandbox/snapshot-restore-clone-ports.test.ts
- src/lib/onboard/managed-startup/profile.ts
- src/lib/onboard/hermes-dashboard.ts
- src/lib/onboard/hermes-api-port.test.ts
- scripts/install.sh
- src/lib/actions/sandbox/forward-recovery.ts
- src/lib/actions/sandbox/agent/passthrough-help.ts
- src/lib/onboard/agent-dashboard-forward.ts
- agents/hermes/mcp-config-transaction.py
- agents/hermes/start.sh
- docs/reference/commands.mdx
- test/hermes-mcp-reload-convergence.test.ts
- src/lib/onboard/dashboard-port.ts
- src/lib/agent/onboard.ts
- src/lib/onboard/sandbox-create-launch.ts
The profile module is bundled into the managed image and executed directly by node type stripping, so a relative value import broke both consumers. Declare the Hermes API port range locally and pin it to core/ports through the profile test. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
The non-root startup path returned before the root-only marker write, so the same-uid helpers that probe the public relay fell back to the default port. It now publishes the marker with the identity that runs the gateway and refuses to start when an allocated port cannot be published. Documentation states the per-sandbox port where a page still named the default. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Rerunning onboarding with NEMOCLAW_HERMES_API_PORT and without --recreate-sandbox moves the host forward while the relay keeps the recorded port, which is the opposite of what the reference stated. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
… into fix/hermes-api-port-per-sandbox
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 2915-2929: Update both current-user and root API-port marker
publication paths, including publish_hermes_api_port_marker_current_user and the
corresponding root flow, to write the value to a temporary file, apply final
permissions (and root ownership where applicable), then atomically rename it
into place. Remove the successful default-port fallback when cleanup or
replacement fails: if an old marker remains or publication cannot complete, fail
closed with a nonzero result. Add restart coverage for stale markers and
publication failures.
- Around line 3342-3344: Harden the marker publication around
HERMES_API_PORT_MARKER by validating that its parent directory is a real
root-owned, non-writable directory before writing. Replace the direct printf
redirection with a root-created temporary file in that directory, set its
contents and permissions, then atomically rename it to the marker so planted
symlinks cannot be followed; add a negative test covering a pre-existing marker
symlink.
In `@docs/security/credential-rotation.mdx`:
- Around line 219-221: Update the credential-rotation commands to use the
placeholder <api-port> instead of the literal 8642, including forwarding and API
probe commands. Explicitly instruct users to replace <api-port> with the port
shown for their selected <sandbox> by openshell forward list, while preserving
the existing first-sandbox context.
In `@src/lib/onboard/managed-startup-profile.test.ts`:
- Around line 978-986: Update the test around validateManagedStartupProfile to
iterate through every port from HERMES_API_PORT_RANGE_START through
HERMES_API_PORT_RANGE_END inclusively, asserting each reserved port throws.
Preserve or add adjacent-port assertions to confirm ports immediately outside
the declared range remain accepted.
In `@test/hermes-api-port-marker.test.ts`:
- Around line 15-33: Remove both conditionals from
runHermesApiPortMarkerPublication by extracting the runtime-parent and
stale-marker setup variants into named helpers, such as writable, blocked, and
read-only-stale-marker setups. Update the runner to accept and invoke the
selected setup helper so its body remains linear while preserving the existing
test scenarios.
🪄 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: 0f741197-7e5c-4cb9-8f95-03fa38a1feb2
📒 Files selected for processing (19)
agents/hermes/mcp-config-transaction.pyagents/hermes/plugin/__init__.pyagents/hermes/start.shci/source-architecture-budget.jsondocs/get-started/quickstart-hermes.mdxdocs/manage-sandboxes/run-sandboxes.mdxdocs/reference/commands.mdxdocs/reference/troubleshooting.mdxdocs/security/credential-rotation.mdxscripts/managed-gateway-control.pysrc/lib/actions/sandbox/agent/passthrough-help.tssrc/lib/actions/sandbox/forward-recovery-declared-ports.test.tssrc/lib/agent/onboard.test.tssrc/lib/agent/onboard.tssrc/lib/onboard/managed-startup-profile.test.tssrc/lib/onboard/managed-startup/profile.tstest/hermes-api-port-marker.test.tstest/mcp-tool-discovery-image-contract.test.tstools/mcp-tool-discovery-runtime/reviewed-runtime-bundle/managed-startup-image-runtime.bundle
🚧 Files skipped from review as they are similar to previous changes (12)
- docs/get-started/quickstart-hermes.mdx
- src/lib/actions/sandbox/agent/passthrough-help.ts
- scripts/managed-gateway-control.py
- src/lib/actions/sandbox/forward-recovery-declared-ports.test.ts
- docs/reference/troubleshooting.mdx
- agents/hermes/plugin/init.py
- src/lib/agent/onboard.ts
- src/lib/agent/onboard.test.ts
- src/lib/onboard/managed-startup/profile.ts
- ci/source-architecture-budget.json
- agents/hermes/mcp-config-transaction.py
- docs/reference/commands.mdx
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Collaborator
|
Addressed every current inline review thread in b0ff41f: added the non-root marker-owner rejection, made both probe fixtures independent of host PyYAML, and put all temporary setup under guaranteed cleanup (including the older TemporaryDirectory suggestion). Focused integration and installer suites passed 11/11, repository commit gates passed, and the exact pre-push CLI typecheck passed. The new head starts a clean all-job CI attempt, avoiding the prior attempt-1/attempt-2 managed-contract artifact mismatch. @cv please re-review after the fresh checks complete. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e/live/managed-image-activation-e2e-helpers.ts`:
- Around line 363-368: Update the container-log collection in the managed
activation failure helper around host.command to stop persisting raw docker
logs. Replace docker logs --tail 1000 with approved content-free metadata or a
bounded safe diagnostic, while preserving the existing artifact naming,
environment, timeout, and redaction configuration as applicable.
🪄 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: 9fc4c8bd-a6f4-46a9-ba7b-3cce9af8423c
📒 Files selected for processing (1)
test/e2e/live/managed-image-activation-e2e-helpers.ts
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
23 tasks
Collaborator
|
Addressed the remaining exact managed-runtime activation request at head
The pushed head has now triggered the requested exact all-agent managed runtime activation workflow; that remote privileged job is the authoritative E2E proof. |
# Conflicts: # test/mcp-tool-discovery-image-contract.test.ts # tools/mcp-tool-discovery-runtime/reviewed-runtime-bundle/managed-startup-image-runtime.bundle
23 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agents/hermes/plugin/__init__.py (1)
325-332: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winHonor
security.allow_private_urlsin the runtime URL guard.
_install_broker_url_safety_patch()resolves the flag, but_broker_safe_url()never reads it. Private URLs remain blocked even when the value istrue._load_hermes_config()also parses YAML without schema validation. Add end-to-end tests for missing, non-boolean,False, and explicitTruevalues.🤖 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/plugin/__init__.py` around lines 325 - 332, Update _broker_safe_url() to read browser_tool._allow_private_urls_resolved and permit private URLs only when it is explicitly true, while preserving the default blocking behavior for missing, non-boolean, or false values. Ensure _load_hermes_config() validates the parsed YAML schema before resolving security.allow_private_urls, and add end-to-end coverage for missing, invalid non-boolean, false, and explicit true configurations.Source: Path instructions
🧹 Nitpick comments (1)
agents/hermes/plugin/__init__.py (1)
332-332: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse direct assignment for the fixed attribute name.
Ruff B010 flags this
setattrcall because the attribute name is constant. Replace it with direct assignment. This preserves the resolved boolean value and removes the lint warning.Proposed fix
- setattr(browser_tool, "_allow_private_urls_resolved", allow_private_urls) + browser_tool._allow_private_urls_resolved = allow_private_urls🤖 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/plugin/__init__.py` at line 332, Replace the setattr call assigning _allow_private_urls_resolved with direct attribute assignment on browser_tool, preserving the existing allow_private_urls value.Source: Linters/SAST tools
🤖 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 `@test/e2e/live/managed-image-activation-e2e-helpers.ts`:
- Around line 410-416: Replace the direct spawnSync call in the docker logs flow
with the project’s audited, progress-aware non-persisting helper for bounded
raw-output capture. Preserve the existing 30-second timeout, SIGKILL behavior,
output limit, environment, and container log arguments, and ensure the result is
not routed through normal artifact capture.
---
Outside diff comments:
In `@agents/hermes/plugin/__init__.py`:
- Around line 325-332: Update _broker_safe_url() to read
browser_tool._allow_private_urls_resolved and permit private URLs only when it
is explicitly true, while preserving the default blocking behavior for missing,
non-boolean, or false values. Ensure _load_hermes_config() validates the parsed
YAML schema before resolving security.allow_private_urls, and add end-to-end
coverage for missing, invalid non-boolean, false, and explicit true
configurations.
---
Nitpick comments:
In `@agents/hermes/plugin/__init__.py`:
- Line 332: Replace the setattr call assigning _allow_private_urls_resolved with
direct attribute assignment on browser_tool, preserving the existing
allow_private_urls value.
🪄 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: 36bb9bf8-d173-4754-abc5-163728ac87bb
📒 Files selected for processing (12)
agents/hermes/plugin/__init__.pydocs/get-started/quickstart-hermes.mdxdocs/reference/commands.mdxdocs/reference/troubleshooting.mdxscripts/install.shsrc/commands/sandbox/agent.tssrc/lib/actions/sandbox/process-recovery.tssrc/lib/onboard/sandbox-registration.tssrc/lib/state/registry.tssrc/lib/state/registry/types.tstest/e2e/live/managed-image-activation-e2e-helpers.tstest/e2e/support/managed-image-activation-diagnostics.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/commands/sandbox/agent.ts
- docs/get-started/quickstart-hermes.mdx
- src/lib/actions/sandbox/process-recovery.ts
- src/lib/onboard/sandbox-registration.ts
- src/lib/state/registry/types.ts
- scripts/install.sh
- src/lib/state/registry.ts
- docs/reference/troubleshooting.mdx
- docs/reference/commands.mdx
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
1 task
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/reference/troubleshooting.mdx`:
- Around line 3312-3313: Update the manual migration guidance in the
recreate-sandbox documentation to state that NEMOCLAW_HERMES_API_PORT must be
set to an available port in the inclusive range 8642–8652, preserving the
existing explanation about retaining the recorded port when the variable is
omitted.
🪄 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: a5168fc6-9dca-4073-92ab-f8db9794240d
📒 Files selected for processing (8)
docs/deployment/deploy-to-headless-server.mdxdocs/get-started/quickstart-hermes.mdxdocs/manage-sandboxes/backup-restore.mdxdocs/manage-sandboxes/run-sandboxes.mdxdocs/reference/commands.mdxdocs/reference/troubleshooting.mdxdocs/security/credential-rotation.mdxsrc/lib/actions/sandbox/gateway-restart.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/manage-sandboxes/run-sandboxes.mdx
- docs/get-started/quickstart-hermes.mdx
- docs/security/credential-rotation.mdx
- docs/reference/commands.mdx
- docs/deployment/deploy-to-headless-server.mdx
The launch harnesses assembled a runnable script from the start script's own functions, but the launch path now delegates the background process to a separate helper. Without it the generated script reached the call and exited 127. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Publishing the API port marker asserted that the runtime directory was already root-owned with mode 0755 and refused startup otherwise, where the previous code reset both. The managed runtime presents that directory before the root-separated gateway starts, so the assertion crash-looped the container. Startup now restores the owner and mode from root and refuses only when the restore is not permitted, and the activation diagnostics name which Hermes refusal ended the container. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Container log capture ran through a direct synchronous spawn to keep raw output out of the artifact sink. The shell probe now accepts a non-persisting run, so the capture keeps the bounded timeout, kill signal, output limit, and redaction of every other probe without writing the logs. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e/fixtures/shell-probe.ts`:
- Around line 31-32: Update the persistArtifacts documentation near
ShellProbe.run to describe the actual behavior: output is redacted and
size-bounded before persistence, and setting it to false also prevents writing
the result JSON artifact; do not describe the retained data as raw output.
🪄 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: 36e3261f-57a0-4b54-9705-ac94697b500c
📒 Files selected for processing (3)
docs/reference/troubleshooting.mdxtest/e2e/fixtures/shell-probe.tstest/e2e/live/managed-image-activation-e2e-helpers.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/reference/troubleshooting.mdx
- test/e2e/live/managed-image-activation-e2e-helpers.ts
cv
dismissed
their stale review
August 12, 2026 10:07
The latest PR commit uses the selected Hermes API port, has passing managed-startup evidence, and retains the verified marker contract. Dismissing this resolved review before a full current-commit review.
Signed-off-by: Apurv Kumaria <akumaria@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
5 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
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. The port is published back toNEMOCLAW_HERMES_API_PORTso 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. A registered sandbox keeps its recorded port: an explicit environment value is accepted only at the create, recreate, and created-sandbox registration boundaries, and every other consumer rejects a conflicting value before it mutates a host forward.isValidHermesApiPortaccepts only the allocation range, so an out-of-range operator value is refused outright.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 owns a different one, and it targets the sandbox's own API port.agents/hermes/start.shreadsNEMOCLAW_HERMES_API_PORTand publishes the resolved value as a0444marker at/run/nemoclaw/hermes-api-port, on the root-separated path and on the same-uid path that OpenShell-managed and macOS VM startups take. 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 instead. Under privilege separation the marker is root-owned and the sandbox user cannot rewrite it; the transaction helper opens it withO_NOFOLLOWand checks ownership, mode, link count, and size before trusting it. In the same-uid topology the gateway already runs as the sandbox user, so the marker carries that same authority, and a sandbox that cannot publish an allocated port there refuses to start rather than leave its helpers probing a default port any process can bind.src/lib/onboard/managed-startup/profile.tsdeclares the reserved range locally instead of importingcore/ports. That module is bundled into the managed image and is also executed directly bynode --experimental-strip-types, and neither path resolves a relative value import;src/lib/onboard/managed-startup-profile.test.tspins the two declarations to the same values.tools/mcp-tool-discovery-runtime/reviewed-runtime-bundle/managed-startup-image-runtime.bundleis regenerated for that change, and its pinned digest intest/mcp-tool-discovery-image-contract.test.tsis updated.bundle:reviewed:checkreproduced the other three reviewed artefacts byte-for-byte, so only the managed-startup bundle moved.src/lib/agent/onboard.tspassessandboxNamethrough the API-kind dashboard branch, which otherwise announced the manifest default for a Hermes sandbox that owns another port.sandbox agentrejection message, the onboarding ready summary, andscripts/install.shresolve the sandbox's port instead of naming8642, and the installer restores a registered Hermes forward rather than the manifest default.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 edges tosnapshot.tsfan-out, thesrc/lib/onboardroot-file count, 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.8642instead of calling it the first sandbox's port, document the clone's own API port allocation and the installer's forward-restore failure, and replace the interactive port prompt in the headless procedure with the<api-port>placeholder the credential-rotation page already uses.agents/hermes/start.shrestores the runtime directory's owner and mode from root before it refuses startup. Publishing the API port marker replaced an unconditionalinstall -d -m 0755 -o root -g root /run/nemoclawwith an assertion that the directory was already root-owned with mode 0755, and the managed runtime presents that directory before the root-separated gateway starts, so the assertion crash-looped the container during managed activation. Startup still refuses when the restore is not permitted.test/e2e/live/managed-image-activation-e2e-helpers.tsadds three fixed startup signals for the Hermes refusals, so an activation failure names which refusal ended the container instead of leaving only a restart count. The container log capture moves from a direct synchronous spawn onto the audited shell probe:test/e2e/fixtures/shell-probe.tsaccepts a non-persisting run, which keeps the probe's bounded timeout, kill signal, output limit, and redaction while raw output stays out of the artifact sink.docs/reference/troubleshooting.mdxstates the range and the occupancy behaviour for a manualNEMOCLAW_HERMES_API_PORToverride.src/lib/actions/sandbox/gateway-restart.tsis reformatted, and three launch harnesses undertest/extractlaunch_openclaw_gateway_process. Both arrived frommain: the log-tail command landed in a shape the repository formatter rewrites, which failedstatic-checks, and the launch path now delegates the background process to a helper the harnesses did not assemble, which failed threecli-test-shardsjobs. Both reproduce on a pristineorigin/maincheckout.origin/mainto keep the branch current. The merges were clean.Large change note: this PR changes 60 files with 2,587 additions and 210 deletions. The scope spans port allocation, registry state, forward recovery, runtime hardening, tests, and documentation, so the large review surface remains explicitly flagged.
Type of Change
Quality Gates
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,docs/security/credential-rotation.mdx,docs/manage-sandboxes/run-sandboxes.mdx, anddocs/manage-sandboxes/backup-restore.mdx, plus the changed comments, CLI strings, error strings, and test titles across the diff. The subagent reviewed the writing rules and documentation style over eleven passes, eight of them on the merged tree. The early passes closed six blocking items, including asandbox agentrejection message and an onboarding ready summary that still emitted the manifest default, docs that pointed operators atstatus, which prints no port, a guard that applied the Hermes port to any agent, and unit tests that had begun reading the real sandbox registry. A later pass returneddocs-missingwith 14 findings, among them a Hermes credential-rotation procedure that still hardcoded8642, an agent-neutral port guard described as Hermes-only, an override whose failure result was unstated, and three reader docstrings that claimed a root-owned marker in a topology where the gateway itself owns it. All were applied. Two further passes corrected an override consequence that had been stated in the wrong direction and a guard that would have rejected the very--recreate-sandboxcommand its own error recommends; both corrections were traced throughsrc/lib/onboard/dashboard.ts,src/lib/onboard/agent-dashboard-forward.ts, andsrc/lib/onboard/sandbox-lifecycle.tsbefore they were accepted. The final pass covers the merged tree at this head and applied nine findings across seven pages: the allocation condition stated as first-sandbox ownership on four pages, the missing clone API-port allocation and its pre-delete abort, the missing range-exhaustion and installer forward-restore entries, an off-vocabulary registry term, a hedged port claim, and the headless procedure's interactive prompt andexit 1, which ends the reader's own SSH session when the typed port is rejected. Every finding was verified against source before it was applied, and two requests to add explanatory comments were declined and recorded for maintainer review.main, added fail-closed validation for the documented Hermes API port before token retrieval, and confirmed no other documentation changes were needed. The later test-only revision adds non-root marker-owner rejection, dependency stubbing, and fixture cleanup; no user-facing documentation changed. The diagnostic-only revision persists only content-free container state and fixed boolean startup signals when managed onboarding fails. Raw logs remain in memory and never enter the artifact sink; a focused regression proves arbitrary credential-like output is absent from the saved summary. The current head mergesorigin/mainand carries the documentation pass described above. The later commits repair the startup refusal that crash-looped managed activation, name that refusal in the activation diagnostics, and restore two inherited gates; none of them changes a user-facing surface, so no further documentation follows. The final documentation build completed with 0 errors and the 2 previously documented warnings. The internal E2E comment correction accurately describes persisted output as redacted and size-bounded and confirms that disabling persistence writes neither output nor result metadata; no user-facing documentation changed.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 run --project cli src/lib/onboard/hermes-api-port.test.ts— 22 passed.npx vitest run --project integration test/generate-managed-startup-profile-fixture.test.ts test/mcp-tool-discovery-image-contract.test.ts test/source-architecture.test.ts— 31 passed.npx vitest run --project cli src/lib/onboard/managed-startup-profile.test.ts— 105 passed. On this head:npm run typecheck:cli— exit 0.npm run checks:repository— exit 0.npx biome formatover the formatter hook's file set — no drift.npm run docs— 0 errors, 2 warnings.npx vitest run --project integration test/nemoclaw-start-gateway-health.test.ts test/nemoclaw-start-gateway-marker.test.ts test/openclaw-2026-7-startup-compat.test.ts— 51 passed, 1 skipped.npx vitest run --project integration test/hermes-api-port-startup.test.ts test/hermes-api-port-marker.test.ts— 10 passed.npx vitest run --project e2e-support— 2481 passed, 10 skipped. CI on the preceding head reported all eightcli-test-shardsgreen and all threePR build and direct managed startupjobs passing, and the PR Review Advisor reported 0 blockers. The later internal-comment correction was validated with 52 focused E2E-support tests and all applicable repository hooks.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: not run locally; left to CI.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
8642–8652.Bug Fixes
Documentation