Skip to content

fix(hermes): install safety-net + ciao NODE_OPTIONS preloads so recover can relaunch - #5416

Closed
hizrianraz wants to merge 4 commits into
NVIDIA:mainfrom
hizrianraz:fix/hermes-recover-node-guards-2478
Closed

fix(hermes): install safety-net + ciao NODE_OPTIONS preloads so recover can relaunch#5416
hizrianraz wants to merge 4 commits into
NVIDIA:mainfrom
hizrianraz:fix/hermes-recover-node-guards-2478

Conversation

@hizrianraz

@hizrianraz hizrianraz commented Jun 14, 2026

Copy link
Copy Markdown

Summary

The Hermes entrypoint (agents/hermes/start.sh) never installed the sandbox-safety-net and ciao-network-guard NODE_OPTIONS preloads that the OpenClaw entrypoint (scripts/nemoclaw-start.sh) installs. As a result the gateway-recovery path refuses to relaunch a stopped Hermes gateway ("proxy-env present but NODE_OPTIONS missing safety-net preload or ciao preload — refusing unguarded gateway relaunch"), and the @homebridge/ciao networkInterfaces() crash (#2478) can hit Hermes sandboxes. This ports the guard install to the Hermes entrypoint, hardened so it can never abort the entrypoint.

Related Issue

Relates to #2478 — the ciao-network-guard preload this ports to the Hermes entrypoint.

Changes

  • agents/hermes/start.sh: add install_nemoclaw_node_guards() — copies sandbox-safety-net.js + ciao-network-guard.js to /tmp/nemoclaw-*.js and adds them to NODE_OPTIONS (mirrors scripts/nemoclaw-start.sh, sourcing from the Hermes image's /opt/nemoclaw-blueprint/scripts).
  • Re-export both guards into the sourced proxy-env (/tmp/nemoclaw-proxy-env.sh) so connect/recovery sessions see them — this is what satisfies the recovery guard-check.
  • Hardened for the set -euo pipefail entrypoint: invoked … || true (errexit suppressed through the whole function), every fallible step inside an if, all vars ${x:-}-safe, and a guard added to NODE_OPTIONS only after its /tmp copy exists and is non-empty ([ -s ]). Worst case it logs a warning and the gateway still starts.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Verification

  • npx prek run --all-files passes
  • npm test passes
  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed
  • Docs updated for user-facing behavior changes
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

The prek/npm boxes are left for CI — this environment lacked the npm/prek toolchain. Verified manually: bash -n clean; an offline harness extracted the real install_nemoclaw_node_guards and ran it under set -euo pipefail across happy / emit-fails / missing-dir / empty-write — the entrypoint continued in every mode (an unhardened version aborted on emit-fail), and the happy path put both guard substrings in NODE_OPTIONS; a throwaway container from the built Hermes sandbox image confirmed the real emit_sandbox_sourced_file writes both /tmp guards. In production on a DGX Spark, rebuilding a Hermes sandbox with the patched entrypoint came up healthy and recover then relaunched cleanly (no #2478 refusal). Happy to add a regression test in your preferred harness — the change is in the shell entrypoint, which doesn't fit the vitest / Brev-e2e suites cleanly.


Signed-off-by: Hizrian Raz hizrian@ainfera.ai

Summary by CodeRabbit

  • Chores

    • Added a best-effort “Node preload guards” setup for Hermes connection and recovery sessions to improve startup safety.
    • Updated the generated runtime environment to conditionally re-export NODE_OPTIONS preload directives for both connect and recovery flows when guards are available.
    • Ensured Hermes startup continues gracefully when guard scripts are missing or cannot be staged.
  • Tests

    • Added automated coverage for guard installation, failure handling, and verification of the recovery preload asset contract.

@copy-pr-bot

copy-pr-bot Bot commented Jun 14, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds Hermes Node guard staging in start.sh, re-exports staged NODE_OPTIONS entries into the proxy env, updates the image preload layout, and adds tests for success, failure, missing-directory handling, and the recovery preload contract.

Changes

Hermes Node Preload Guard Installation and Runtime Env Wiring

Layer / File(s) Summary
Recovery preload image contract
agents/hermes/Dockerfile
Copies JavaScript preload files into /usr/local/lib/nemoclaw/preloads/ and applies separate permissions for those files versus the executable startup and validator scripts.
Guard staging and proxy env emission
agents/hermes/start.sh
install_nemoclaw_node_guards stages sandbox-safety-net.js and ciao-network-guard.js from the preload directory into /tmp, conditionally appends --require entries to NODE_OPTIONS, and continues on missing or failed installation. write_runtime_shell_env conditionally writes matching NODE_OPTIONS exports for connect and recovery sessions.
Installer and image contract tests
test/hermes-node-guard-install.test.ts
Adds Vitest coverage for successful staging, failure handling, missing preload directories, and the Dockerfile/start-script recovery preload contract.

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

Poem

🐇 I hop through /tmp with guards in tow,
--require twinkles in a steady glow.
A || true breeze keeps startup bright,
And proxy env hums through the night.
Two tiny guards keep the path all right.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: installing the safety-net and ciao NODE_OPTIONS preloads to enable recovery relaunch.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch fix/hermes-recover-node-guards-2478

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@agents/hermes/start.sh`:
- Around line 767-770: Remove the sandbox path
`/sandbox/.nemoclaw/blueprints/*/scripts` from the default search list in the
NEMOCLAW_GUARD_DIRS parameter expansion. The for loop iterating over _gd should
only default to image-owned preload directories
(`/opt/nemoclaw-blueprint/scripts` and `/usr/local/lib/nemoclaw/preloads`) when
NEMOCLAW_GUARD_DIRS is not explicitly set, allowing users to add sandbox paths
only through explicit environment variable overrides for test/dev purposes
rather than by default.
🪄 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: fdeffcd8-d465-4534-8380-0d54923f18db

📥 Commits

Reviewing files that changed from the base of the PR and between 1f75d03 and 0ccc925.

📒 Files selected for processing (1)
  • agents/hermes/start.sh

Comment thread agents/hermes/start.sh Outdated
…er can relaunch

The OpenClaw entrypoint (scripts/nemoclaw-start.sh) installs the
sandbox-safety-net and ciao-network-guard NODE_OPTIONS preloads, but the
Hermes entrypoint (agents/hermes/start.sh) did not. The gateway-recovery path
refuses to relaunch unless NODE_OPTIONS advertises both guards ("proxy-env
present but NODE_OPTIONS missing safety-net preload or ciao preload - refusing
unguarded gateway relaunch"), so a stopped Hermes gateway can never be brought
back with `recover`. The ciao guard is also what prevents the @homebridge/ciao
networkInterfaces() crash (#2478) on Hermes sandboxes.

Port the guard install to the Hermes entrypoint. Hardened so a guard-install
failure can never abort the `set -euo pipefail` entrypoint (an unguarded version
left the gateway dead): the install runs as `... || true` (errexit suppressed
through the whole function), every fallible command sits inside an `if`, all
vars are `${x:-}`-safe, and a guard is only added to NODE_OPTIONS once its /tmp
copy exists and is non-empty. Worst case it warns and the gateway still starts.
The guards are also re-exported into the sourced proxy-env so connect/recovery
sessions see them.

Signed-off-by: Hizrian Raz <hizrian@ainfera.ai>
@hizrianraz
hizrianraz force-pushed the fix/hermes-recover-node-guards-2478 branch from 0ccc925 to 8c9a3f7 Compare June 14, 2026 16:09
@hizrianraz

Copy link
Copy Markdown
Author

Thanks @coderabbitai — good catch. Removed the sandbox-writable /sandbox/.nemoclaw/blueprints/*/scripts from the default search list; the default is now image-owned dirs only (/opt/nemoclaw-blueprint/scripts, /usr/local/lib/nemoclaw/preloads), with NEMOCLAW_GUARD_DIRS reserved for explicit test/dev overrides. Force-pushed.

@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@hizrianraz: Perfect — restricting the defaults to image-owned, immutable directories (/opt/nemoclaw-blueprint/scripts, /usr/local/lib/nemoclaw/preloads) and making NEMOCLAW_GUARD_DIRS the only path to include sandbox-writable locations is exactly the right hardening posture. That way the trust boundary is explicit and opt-in rather than on by default. Thanks for the quick turnaround!

(ᵔᴥᵔ)🐇

@wscurran wscurran added bug-fix PR fixes a bug or regression integration: hermes Hermes integration behavior labels Jun 23, 2026
@wscurran

Copy link
Copy Markdown
Contributor

✨ Thanks for porting the sandbox-safety-net and ciao-network-guard preloads to the Hermes entrypoint to address the gateway recovery refusal. This proposes a way to install the Node.js guard scripts via install_nemoclaw_node_guards() in agents/hermes/start.sh so the recovery path can relaunch stopped Hermes gateways without aborting the entrypoint.


Related open issues:

@harjothkhara

Copy link
Copy Markdown
Contributor

Thanks for this. Putting the guard install in the Hermes entrypoint is the right call, and it lines up with what we landed on in #5525. The #2478 side (loading the ciao guard at startup) looks good.

I don't think recover works on Hermes yet though, even with this change.

I built the Hermes image layers locally and there's no /usr/local/lib/nemoclaw/preloads/ directory. Only the OpenClaw image copies it (Dockerfile line 540). On Hermes the guard scripts only exist under /opt/nemoclaw-blueprint/scripts/.

That's the catch. The recover code from #5321 only reads guards from /usr/local/lib/nemoclaw/preloads/. On Hermes that path is missing, so it bails and refuses the relaunch before it ever reads the requires this PR writes into proxy-env.sh.

Adding the same COPY the OpenClaw image already has would fix it:

COPY nemoclaw-blueprint/scripts/*.js /usr/local/lib/nemoclaw/preloads/

in agents/hermes/Dockerfile. With that, recover can self-heal on Hermes too, and it would also close #5253 and #2426 ("Impossible to restart hermes gateway"), not just #2478.

Easy to confirm on a built image:

docker run --rm <hermes-image> ls /usr/local/lib/nemoclaw/preloads/

It would also be good to add a regression test. The advisor flagged the same gap on #5265, and the gateway-guard-recovery scenario already exists for it.

jyaunches pushed a commit that referenced this pull request Jun 30, 2026
<!-- markdownlint-disable MD041 -->
## Summary
Adds a supported host-mediated `gateway restart` command for
NemoClaw-managed OpenClaw and Hermes gateways and routes automatic
recovery through the same topology-specific authenticated controller or
supervisor path. The lifecycle acts only on proven process and listener
identity, applies configuration and state changes transactionally, and
fails closed when privileged execution or built-in health probes are
ambiguous.

## Related Issue
Fixes #2426
Fixes #5253
Supersedes #5416

## Changes
- Added `sandbox:gateway:restart` plus public `nemoclaw <name> gateway
restart` and `nemohermes <name> gateway restart` routing while keeping
healthy `recover` idempotent.
- Added managed gateway control for both supported topologies: a root
PID 1 request channel for direct-root entrypoints and an authenticated
root-owned controller paired with the nonroot supervisor in
OpenShell-managed sandboxes.
- Enforced exact PID and process-start identity checks, listener
ownership validation, post-stop absence proof, exact reaping or respawn
proof, health waits, and port-forward recovery.
- Added descriptor-based, no-follow, atomic configuration and state
guards, including bounded Hermes secret-boundary and configuration-hash
validation.
- Serialized shields mutations and recovery timers, and ordered
snapshot, destroy, and inference transitions so stale callbacks cannot
reapply state.
- Made built-in OpenClaw and Hermes probes fail closed after trusted
execution failure or timeout; unsupported privileged-exec drivers and
ambiguous container selection are rejected while explicit custom gateway
agents retain their compatibility path.
- Wired runtime helpers into optimized build contexts with root-owned,
non-writable permissions and preserved root PID 1 access to mutable
sandbox-group state when hardened runtimes drop `CAP_DAC_OVERRIDE`.
- Published a Trusted Computing Base page in both guide variants
covering the direct-root and OpenShell-managed boundaries, shared-UID
and mutable-config limits, JSON5 read compatibility, root group
membership, and compatibility-removal conditions.
- Restricted managed-controller procfs and filesystem overrides to
source execution with the explicit test flag, and verified the pinned
Node/JSON5 installation is root-owned and non-writable.
- Added final-image proof for both built-in images covering root-only
helper modes, required supplementary groups, root probe access, and
sandbox-user refusal; made sandbox-operations assertions topology-aware
for direct-root and OpenShell-managed runs.
- Excluded authenticated, exact-identity Hermes controller replacements
from crash quarantine with a root-only `0600` lifecycle lock and
root-owned `0444` authorization bound to the live root controller; all
orphaned, mismatched, unexpected, and failed replacement exits still
count.
- Tightened the OpenClaw image health fallback to read the tracked PID
start identity before and after an exact installed gateway command line,
while retaining known rewritten process-title compatibility.
- Preserved the injected managed supervisor through the post-restart
settle probe and locked the built-in completion protocol to
`GATEWAY_PID=` rather than accepting the legacy custom-script
`ALREADY_RUNNING` marker.
- Migrated the branch's live recovery coverage onto the typed E2E
workflow and removed the legacy shell lanes deleted on `main`.
- Updated lifecycle, runtime-control, command, troubleshooting,
security, and Hermes documentation.
- Review scope: the existing OpenShell bridge-host allowance is
preserved while adjacent private or internal endpoint shapes are
DNS-validated and rejected. Broad module splitting and consumer-wide
HTTPS binding remain separate architecture work rather than widening
this security-sensitive recovery fix.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [x] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates
- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification: Independent security
review found no blocking findings across privilege, process identity,
filesystem, network, fail-closed, and regression-coverage boundaries;
human security acceptance remains pending and required before merge.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [ ] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only)
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [x] New doc pages include SPDX header and frontmatter (new pages only)

Exact-head evidence for `8dbe420d3d835546683cbfa67fec0537791ad7c1`:

- Signed, DCO-compliant commit
`d86c60c325a3dbc74fc42f74447bc00f89be4400` addresses the current
CodeRabbit correctness and documentation findings. Signed merge commit
`c9061c68fd82b1b73044383bc4e8962808644744` incorporates `main` at
`e10462ff3e2e0727350a2532fc7bb7edc64116b2` without rebasing or rewriting
the verified history. Signed cleanup commit
`8dbe420d3d835546683cbfa67fec0537791ad7c1` addresses the resulting
CodeQL/code-quality diagnostics.
- Post-merge controller, trust-contract, provisioning, managed-exit
authorization, OpenClaw/Hermes config-guard, state-dir guard,
endpoint-security, restart/boundary, and workflow-boundary suites pass:
15 files, 258 tests.
- `npm run build:cli`, `npm run typecheck:cli`, Biome, ShellCheck,
Python compilation, branch diff checks, test-size/project/title guards,
and `npm run docs:check-agent-variants` pass.
- `npm run docs` completes with 0 errors and two existing Fern warnings;
the warning-free checkbox remains unchecked.
- All 80 PR commits appear as GitHub `Verified`; all three new commits
contain the DCO sign-off declaration.
- The review-fix commit's normal formatting, lint, repository,
secret-scan, commitlint, source-shape, and test-size hooks pass. The
normal pre-push CLI typecheck and version-sync hooks pass. The full
local `test-cli` hook and full-`npm test` boxes remain unchecked;
exact-head CI is required for those broader platform lanes.
- Standard CI, automated current-head review, human security acceptance,
and exact-head E2E evidence are pending on this head and must settle
before merge.

---
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Added a `gateway restart` command for sandboxes with forced reload and
post-restart health/forward verification.

* **Bug Fixes**
* Improved recovery so host forwards and supervised gateway processes
are repaired more reliably.
* Strengthened failure handling to be fail-closed when required Hermes
secret-boundary validation or supervisor control is unavailable.
* Corrected forward-health classification so stopped/occupied forwards
are handled consistently.

* **Documentation**
* Expanded lifecycle, runtime-controls, command reference, and
troubleshooting guidance for `recover`, `gateway restart`, shields
windows, and Hermes recovery constraints.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
@cv cv added the v0.0.77 label Jul 8, 2026
@ericksoa ericksoa added v0.0.78 and removed v0.0.77 labels Jul 8, 2026
Signed-off-by: cjagwani <cjagwani@nvidia.com>
Signed-off-by: cjagwani <cjagwani@nvidia.com>
@cjagwani

cjagwani commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Maintainer salvage pushed at exact head e8ede911 to close the outstanding recovery contract noted above:

  • copy the image-owned JS preloads into /usr/local/lib/nemoclaw/preloads/, the path host recovery actually restores from
  • restrict production guard discovery to that immutable path; the test seam is now an explicit function argument rather than NEMOCLAW_GUARD_DIRS inherited from the environment
  • add focused coverage for successful staging/NODE_OPTIONS export, missing sources, staging failure, and the Dockerfile recovery-path contract

Local verification on Node 22:

  • test/hermes-start.test.ts + test/hermes-node-guard-install.test.ts — 38/38 passed
  • bash -n agents/hermes/start.sh — passed
  • pinned shfmt 3.12.0 — clean
  • Biome format/lint — clean
  • test-file size budget — passed

Security disposition: no hardcoded secrets or new dependencies; sources are image-owned and no longer environment-selectable in production; staging remains atomic through emit_sandbox_sourced_file; missing/failed staging remains best-effort and cannot abort gateway startup. Fresh exact-head CI is now authoritative.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
agents/hermes/start.sh (1)

762-796: 🩺 Stability & Availability | 🔵 Trivial

Run the Hermes recovery E2E lanes for this change.

This touches Hermes gateway recovery relaunch behavior. As per path instructions, changes here should exercise hermes-e2e, hermes-inference-switch-e2e, and rebuild-hermes-e2e (and related lanes) before merge:

gh workflow run nightly-e2e.yaml --ref <branch> -f jobs=hermes-e2e,hermes-inference-switch-e2e,hermes-discord-e2e,hermes-slack-e2e,hermes-onboard-security-posture-e2e,rebuild-hermes-e2e,rebuild-hermes-stale-base-e2e
🤖 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 762 - 796, This change affects Hermes
gateway recovery relaunch behavior, so validate it by running the
recovery-focused E2E lanes before merging. Exercise the relevant workflows that
cover install_nemoclaw_node_guards and related relaunch paths, including
hermes-e2e, hermes-inference-switch-e2e, hermes-discord-e2e, hermes-slack-e2e,
hermes-onboard-security-posture-e2e, rebuild-hermes-e2e, and
rebuild-hermes-stale-base-e2e.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/hermes-node-guard-install.test.ts`:
- Line 51: Update the affected test titles in the `hermes-node-guard-install`
suite to follow the `**/*.test.ts` guideline by appending the local issue
reference suffix `(`#2478`)`. Specifically, adjust the `it(...)` descriptions in
the test cases covering the guard-preload and recovery-refusal behavior so they
remain behavior-oriented but end with the required `(`#2478`)` tag. Use the
existing test title strings in `hermes-node-guard-install.test.ts` as the unique
anchors for the change.

---

Nitpick comments:
In `@agents/hermes/start.sh`:
- Around line 762-796: This change affects Hermes gateway recovery relaunch
behavior, so validate it by running the recovery-focused E2E lanes before
merging. Exercise the relevant workflows that cover install_nemoclaw_node_guards
and related relaunch paths, including hermes-e2e, hermes-inference-switch-e2e,
hermes-discord-e2e, hermes-slack-e2e, hermes-onboard-security-posture-e2e,
rebuild-hermes-e2e, and rebuild-hermes-stale-base-e2e.
🪄 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: 014e17da-7305-4e33-98a8-be7497e61eaf

📥 Commits

Reviewing files that changed from the base of the PR and between 1646a35 and e8ede91.

📒 Files selected for processing (3)
  • agents/hermes/Dockerfile
  • agents/hermes/start.sh
  • test/hermes-node-guard-install.test.ts

}

describe("Hermes Node guard installation", () => {
it("stages both image-owned guards and exports them through NODE_OPTIONS", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add (#2478) suffix to test titles per guideline.

These tests directly cover the recovery-refusal / guard-preload behavior for issue #2478 (as referenced in the start.sh warning strings), but their titles lack the local issue-reference suffix required for **/*.test.ts files.

📝 Proposed fix
-  it("stages both image-owned guards and exports them through NODE_OPTIONS", () => {
+  it("stages both image-owned guards and exports them through NODE_OPTIONS (`#2478`)", () => {
...
-  it("keeps startup alive when guard staging fails", () => {
+  it("keeps startup alive when guard staging fails (`#2478`)", () => {
...
-  it("keeps startup alive when the image guard directory is missing", () => {
+  it("keeps startup alive when the image guard directory is missing (`#2478`)", () => {

As per coding guidelines, "**/*.test.ts: Write behavior-oriented test titles, and put local issue references in a final (#1234) suffix."

Also applies to: 68-68, 84-84

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/hermes-node-guard-install.test.ts` at line 51, Update the affected test
titles in the `hermes-node-guard-install` suite to follow the `**/*.test.ts`
guideline by appending the local issue reference suffix `(`#2478`)`. Specifically,
adjust the `it(...)` descriptions in the test cases covering the guard-preload
and recovery-refusal behavior so they remain behavior-oriented but end with the
required `(`#2478`)` tag. Use the existing test title strings in
`hermes-node-guard-install.test.ts` as the unique anchors for the change.

Source: Coding guidelines

@cjagwani cjagwani closed this Jul 8, 2026
@cjagwani

cjagwani commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Closed as superseded by #5874, which explicitly names this PR in its merged scope and landed the complete host-mediated OpenClaw/Hermes restart design.

Current main already:

  • copies the guard sources into /usr/local/lib/nemoclaw/preloads/
  • pins them root-owned and read-only (0444), stricter than this branch's 0644
  • routes Hermes recovery through the authenticated controller/supervisor path
  • carries live Hermes E2E coverage proving both normal startup and recovery succeed with no OpenClaw Node safety-net/ciao preloads in the Python gateway environment

Rebasing this branch would therefore retain only the start.sh preload installation that now contradicts the intentional #5253 contract. The original recovery gap is covered on main; no unique safe change remains to merge here. Thanks for the original diagnosis and implementation work.

Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
<!-- markdownlint-disable MD041 -->
## Summary
Adds a supported host-mediated `gateway restart` command for
NemoClaw-managed OpenClaw and Hermes gateways and routes automatic
recovery through the same topology-specific authenticated controller or
supervisor path. The lifecycle acts only on proven process and listener
identity, applies configuration and state changes transactionally, and
fails closed when privileged execution or built-in health probes are
ambiguous.

## Related Issue
Fixes NVIDIA#2426
Fixes NVIDIA#5253
Supersedes NVIDIA#5416

## Changes
- Added `sandbox:gateway:restart` plus public `nemoclaw <name> gateway
restart` and `nemohermes <name> gateway restart` routing while keeping
healthy `recover` idempotent.
- Added managed gateway control for both supported topologies: a root
PID 1 request channel for direct-root entrypoints and an authenticated
root-owned controller paired with the nonroot supervisor in
OpenShell-managed sandboxes.
- Enforced exact PID and process-start identity checks, listener
ownership validation, post-stop absence proof, exact reaping or respawn
proof, health waits, and port-forward recovery.
- Added descriptor-based, no-follow, atomic configuration and state
guards, including bounded Hermes secret-boundary and configuration-hash
validation.
- Serialized shields mutations and recovery timers, and ordered
snapshot, destroy, and inference transitions so stale callbacks cannot
reapply state.
- Made built-in OpenClaw and Hermes probes fail closed after trusted
execution failure or timeout; unsupported privileged-exec drivers and
ambiguous container selection are rejected while explicit custom gateway
agents retain their compatibility path.
- Wired runtime helpers into optimized build contexts with root-owned,
non-writable permissions and preserved root PID 1 access to mutable
sandbox-group state when hardened runtimes drop `CAP_DAC_OVERRIDE`.
- Published a Trusted Computing Base page in both guide variants
covering the direct-root and OpenShell-managed boundaries, shared-UID
and mutable-config limits, JSON5 read compatibility, root group
membership, and compatibility-removal conditions.
- Restricted managed-controller procfs and filesystem overrides to
source execution with the explicit test flag, and verified the pinned
Node/JSON5 installation is root-owned and non-writable.
- Added final-image proof for both built-in images covering root-only
helper modes, required supplementary groups, root probe access, and
sandbox-user refusal; made sandbox-operations assertions topology-aware
for direct-root and OpenShell-managed runs.
- Excluded authenticated, exact-identity Hermes controller replacements
from crash quarantine with a root-only `0600` lifecycle lock and
root-owned `0444` authorization bound to the live root controller; all
orphaned, mismatched, unexpected, and failed replacement exits still
count.
- Tightened the OpenClaw image health fallback to read the tracked PID
start identity before and after an exact installed gateway command line,
while retaining known rewritten process-title compatibility.
- Preserved the injected managed supervisor through the post-restart
settle probe and locked the built-in completion protocol to
`GATEWAY_PID=` rather than accepting the legacy custom-script
`ALREADY_RUNNING` marker.
- Migrated the branch's live recovery coverage onto the typed E2E
workflow and removed the legacy shell lanes deleted on `main`.
- Updated lifecycle, runtime-control, command, troubleshooting,
security, and Hermes documentation.
- Review scope: the existing OpenShell bridge-host allowance is
preserved while adjacent private or internal endpoint shapes are
DNS-validated and rejected. Broad module splitting and consumer-wide
HTTPS binding remain separate architecture work rather than widening
this security-sensitive recovery fix.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [x] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates
- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification: Independent security
review found no blocking findings across privilege, process identity,
filesystem, network, fail-closed, and regression-coverage boundaries;
human security acceptance remains pending and required before merge.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [ ] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only)
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [x] New doc pages include SPDX header and frontmatter (new pages only)

Exact-head evidence for `8dbe420d3d835546683cbfa67fec0537791ad7c1`:

- Signed, DCO-compliant commit
`d86c60c325a3dbc74fc42f74447bc00f89be4400` addresses the current
CodeRabbit correctness and documentation findings. Signed merge commit
`c9061c68fd82b1b73044383bc4e8962808644744` incorporates `main` at
`e10462ff3e2e0727350a2532fc7bb7edc64116b2` without rebasing or rewriting
the verified history. Signed cleanup commit
`8dbe420d3d835546683cbfa67fec0537791ad7c1` addresses the resulting
CodeQL/code-quality diagnostics.
- Post-merge controller, trust-contract, provisioning, managed-exit
authorization, OpenClaw/Hermes config-guard, state-dir guard,
endpoint-security, restart/boundary, and workflow-boundary suites pass:
15 files, 258 tests.
- `npm run build:cli`, `npm run typecheck:cli`, Biome, ShellCheck,
Python compilation, branch diff checks, test-size/project/title guards,
and `npm run docs:check-agent-variants` pass.
- `npm run docs` completes with 0 errors and two existing Fern warnings;
the warning-free checkbox remains unchecked.
- All 80 PR commits appear as GitHub `Verified`; all three new commits
contain the DCO sign-off declaration.
- The review-fix commit's normal formatting, lint, repository,
secret-scan, commitlint, source-shape, and test-size hooks pass. The
normal pre-push CLI typecheck and version-sync hooks pass. The full
local `test-cli` hook and full-`npm test` boxes remain unchecked;
exact-head CI is required for those broader platform lanes.
- Standard CI, automated current-head review, human security acceptance,
and exact-head E2E evidence are pending on this head and must settle
before merge.

---
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Added a `gateway restart` command for sandboxes with forced reload and
post-restart health/forward verification.

* **Bug Fixes**
* Improved recovery so host forwards and supervised gateway processes
are repaired more reliably.
* Strengthened failure handling to be fail-closed when required Hermes
secret-boundary validation or supervisor control is unavailable.
* Corrected forward-health classification so stopped/occupied forwards
are handled consistently.

* **Documentation**
* Expanded lifecycle, runtime-controls, command reference, and
troubleshooting guidance for `recover`, `gateway restart`, shields
windows, and Hermes recovery constraints.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug-fix PR fixes a bug or regression integration: hermes Hermes integration behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants