Walkthrough

This PR adds Hermes gateway runtime cleanup logic to detect and safely remove stale gateway artifacts, and introduces configurability for OpenShell sandbox startup commands through environment-variable injection in Docker GPU patch creation and recreation flows.

Changes

Hermes gateway runtime cleanup

OpenShell sandbox command configurability

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• NVIDIA/NemoClaw#3797: Both PRs modify agents/hermes/start.sh startup flow by building around Hermes/Tirith marker handling (retry_tirith_marker_if_needed); the main PR runs its Hermes gateway stale-runtime cleanup immediately after that marker retry step.
• NVIDIA/NemoClaw#3515: Both PRs modify src/lib/onboard/docker-gpu-patch.ts GPU docker clone run args construction logic; one adds OPENSHELL_SANDBOX_COMMAND command/env injection while the other injects security/capability flags.
• NVIDIA/NemoClaw#3434: Both PRs modify the Docker GPU sandbox onboarding flow in src/lib/onboard.ts and GPU patch modules, threading extra GPU/sandbox-create options into sandbox recreation/polling logic.

Suggested labels

Integration: Hermes, Sandbox, OpenShell

Suggested reviewers

• cv
• jyaunches

Poem

🐰 A gateway once left in a stale, tangled mess,
Now cleaned with grace, removing distress!
And sandbox commands, once rigid and bound,
Now flow like clear streams—injection profound! 🌊✨

❌ Failed checks (1 warning)

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

• hermes-e2e (high): Covers the real Hermes install/onboard flow, Hermes sandbox gateway health, and live inference. Required because the PR changes agents/hermes/start.sh gateway startup cleanup and the onboarding path that launches Hermes sandboxes.
• gpu-e2e (high): Covers the real GPU onboarding path with local Ollama, Docker sandbox creation/recreation, and end-to-end inference through the sandbox. Required because the PR changes Docker GPU patch command propagation during sandbox create, which can decide whether the recreated OpenShell container runs nemoclaw-start instead of an idle command.

Optional E2E

• gpu-double-onboard-e2e (high): Useful adjacent coverage for repeated GPU onboarding/recreation and post-reonboard inference. It is not strictly merge-blocking if gpu-e2e passes, but it can catch state or command propagation regressions during a second onboard.
• rebuild-hermes-e2e (high): Optional confidence for Hermes lifecycle after sandbox recreation/rebuild, which is adjacent to stale gateway runtime lock cleanup in agents/hermes/start.sh.
• hermes-onboard-security-posture-e2e (high): Optional because Hermes start.sh changed process and file cleanup around runtime state. This job validates the full Hermes onboard path with security-posture assertions, but the specific stale lock/GPU recreation behavior is better covered by a targeted future test.

New E2E recommendations

• Hermes GPU Docker recreation (high): Existing GPU E2E covers OpenClaw/local Ollama and existing Hermes E2E covers CPU/cloud Hermes, but there is no combined E2E that recreates a Hermes OpenShell Docker sandbox with GPU access and verifies stale gateway.lock cleanup plus Hermes gateway/inference after recreation.
  • Suggested test: Add a Hermes GPU sandbox recreation E2E that onboards Hermes with a GPU-enabled Docker OpenShell sandbox, forces the Docker GPU patch/recreate path, verifies OPENSHELL_SANDBOX_COMMAND runs nemoclaw-start, confirms stale gateway.pid/gateway.lock cleanup, and performs Hermes health plus inference.

Dispatch hint

• Workflow: .github/workflows/nightly-e2e.yaml
• jobs input: hermes-e2e,gpu-e2e

PR Review Advisor

Base: origin/main
Head: HEAD
Analyzed SHA: 92d38d3a7817c3afb734d68f98656ec5ed0500c0
Recommendation: blocked
Confidence: high

The patch targets real active sandbox/Hermes GPU startup paths, but merge is blocked by pending CI/E2E, mergeStateStatus=BLOCKED, CodeRabbit still pending, large-file budget blockers, and a security concern around serializing sensitive startup env into OPENSHELL_SANDBOX_COMMAND.

Gate status

• CI: pending — Head SHA 92d38d3 has pending/in-progress/queued contexts including cli-parity, E2E recommendation, wsl-e2e, macos-e2e, PR review advisor, CodeQL javascript-typescript, CodeQL python, unit-vitest-linux, checks, ShellCheck SARIF, build-sandbox-images, build-sandbox-images-arm64, and CodeRabbit.
• Mergeability: fail — GitHub GraphQL reports mergeStateStatus=BLOCKED and reviewDecision=REVIEW_REQUIRED for PR fix(hermes): restore Spark GPU recreate startup #3963.
• Review threads: unknown — GraphQL reviewThreads.nodes is empty, but trusted context says no review thread state was available; CodeRabbit issue comment says review is in progress and CodeRabbit status is PENDING.
• Risky code tested: warning — Risky areas detected: onboarding/host glue. Unit tests were added for docker GPU patch command preservation and Hermes runtime cleanup, but runtime/sandbox/infrastructure behavior requires E2E validation for this head SHA.

🔴 Blockers

• Required CI and review automation have not completed for the head SHA: The latest head SHA has many in-progress or queued contexts, and CodeRabbit is still pending. Mergeability is also blocked.
  • Recommendation: Wait for the required checks and review automation to complete successfully for 92d38d3 before considering merge.
  • Evidence: GraphQL statusCheckRollup shows IN_PROGRESS/QUEUED contexts including E2E recommendation, wsl-e2e, macos-e2e, CodeQL, unit-vitest-linux, ShellCheck SARIF, build-sandbox-images, and CodeRabbit=PENDING; mergeStateStatus=BLOCKED.
• Runtime and sandbox lifecycle changes need E2E confirmation: The PR changes Hermes entrypoint startup, stale runtime cleanup, socat cleanup, Docker GPU container recreation, and OpenShell sandbox command propagation. Unit tests exercise extracted helpers, but cannot prove real OpenShell/Docker/Hermes supervisor reconnect behavior.
  • Recommendation: Require the E2E Advisor recommendation to finish and ensure its required E2E jobs pass for this exact head SHA, especially paths covering Docker GPU recreate and Hermes startup.
  • Evidence: Changed files include agents/hermes/start.sh, src/lib/onboard.ts, src/lib/onboard/docker-gpu-patch.ts, and src/lib/onboard/docker-gpu-sandbox-create.ts. E2E recommendation, wsl-e2e, and macos-e2e are IN_PROGRESS.
• Large-file hotspot grew past the monolith budget (src/lib/onboard/docker-gpu-patch.ts:251): The Docker GPU patch implementation is already a large-file hotspot and grew by 25 lines in this PR.
  • Recommendation: Extract the new OpenShell sandbox command serialization/recreate plumbing into a smaller helper module or offset the growth before merge, consistent with the repository's monolith budget policy.
  • Evidence: Trusted monolithDeltas reports src/lib/onboard/docker-gpu-patch.ts baseLines=1178, headLines=1203, delta=25, severity=blocker.
• Large test hotspot grew past the monolith budget (src/lib/onboard/docker-gpu-patch.test.ts:1): The Docker GPU patch test file is already a large-file hotspot and grew by 63 lines in this PR.
  • Recommendation: Move the new OpenShell sandbox command preservation tests into a focused test file or offset the growth before merge.
  • Evidence: Trusted monolithDeltas reports src/lib/onboard/docker-gpu-patch.test.ts baseLines=606, headLines=669, delta=63, severity=blocker.

🟡 Warnings

• Sandbox startup command is serialized as a space-joined env value (src/lib/onboard/docker-gpu-patch.ts:251): openshellSandboxCommandEnvValue joins command parts with a plain space and writes the result into OPENSHELL_SANDBOX_COMMAND. The command can include env assignments derived from runtime credentials such as TOOL_GATEWAY_USER_TOKEN and BRAVE_API_KEY. Depending on how OpenShell later parses OPENSHELL_SANDBOX_COMMAND, this can expose secrets through Docker inspect/environment metadata and can break or become unsafe for values containing spaces or shell metacharacters.
  • Recommendation: Avoid embedding sensitive env assignments in OPENSHELL_SANDBOX_COMMAND, or encode the command with a structured/quoted representation that OpenShell parses without shell interpretation. Add negative tests for token/API-key values containing spaces, quotes, semicolons, and dollar signs, and assert sensitive values are not exposed in diagnostics or persisted environment when not strictly required.
  • Evidence: src/lib/onboard.ts line 5501 builds sandboxStartupCommand from envArgs; lines 5480-5488 can include TOOL_GATEWAY_USER_TOKEN and BRAVE_API_KEY. docker-gpu-patch.ts line 251 joins command parts with spaces, and lines 459-467 write OPENSHELL_SANDBOX_COMMAND= into docker run --env.
• Patch overlaps with many active PRs touching the same high-risk files: The changed files still exist and the patch is not obviously stale, but there is substantial active work on the same files, including security/sandbox and onboard refactor PRs. This increases drift and conflict risk.
  • Recommendation: Before merge, re-check against the latest main and coordinate with overlapping PRs, especially security-related Hermes startup and onboard extraction work.
  • Evidence: Open PR overlaps include fix(sandbox): harden startup log creation #3888 touching agents/hermes/start.sh, fix(onboard): scan default CDI dirs for NVIDIA specs (#3575) #3675 touching docker-gpu-patch.ts and docker-gpu-patch.test.ts, and numerous PRs touching src/lib/onboard.ts.

🔵 Suggestions

• Add edge-case tests for Hermes gateway process detection (agents/hermes/start.sh:159): The cleanup logic preserves stale runtime files if any cmdline contains a Hermes gateway run pattern. This is intentionally conservative, but false positives could preserve stale locks indefinitely.
  • Recommendation: Add tests for non-gateway processes whose arguments contain the text 'hermes gateway run', and for wrapped shell/step-down command lines used by the root path, to document the intended matching behavior.
  • Evidence: cmdline_is_hermes_gateway matches '"/hermes gateway run "' or '" hermes gateway run "' in agents/hermes/start.sh.

Acceptance coverage

• partial — preserve NemoClaw's env ... nemoclaw-start sandbox command when the Docker GPU patch recreates an OpenShell-managed container: src/lib/onboard.ts now builds sandboxStartupCommand=["env", ...envArgs, "nemoclaw-start"] and passes it to createDockerGpuSandboxCreatePatch. docker-gpu-patch.ts uses options.openshellSandboxCommand as both OPENSHELL_SANDBOX_COMMAND and final docker command args. Unit tests assert replacement of 'sleep infinity'. Remaining concern: serialization is a plain space join and may mishandle or expose sensitive env values.
• met — clear stale Hermes runtime/gateway.pid / runtime/gateway.lock state only when no Hermes gateway process is alive: agents/hermes/start.sh adds has_live_hermes_gateway and cleanup_stale_hermes_gateway_runtime, called before gateway launch in both non-root and root paths. test/hermes-start.test.ts covers stale pid/lock removal and preservation when a live gateway cmdline is present.
• met — remove orphaned Hermes socat forwarders before launching a fresh gateway: agents/hermes/start.sh adds cleanup_orphan_socat_forwarders and calls it from cleanup_stale_hermes_gateway_runtime before start_socat_forwarder. test/hermes-start.test.ts covers killing a fake orphan socat when no live Hermes gateway exists.
• unknown — npm run build:cli: The PR body claims this was run, but trusted CI for the head SHA is still pending and this review did not execute package-manager commands.
• unknown — bash -n agents/hermes/start.sh: The PR body claims this was run, but ShellCheck SARIF is still IN_PROGRESS and this review did not execute PR scripts.
• unknown — npx vitest run src/lib/onboard/docker-gpu-patch.test.ts test/hermes-start.test.ts: The PR body claims this was run and relevant tests were added, but unit-vitest-linux is QUEUED for the head SHA and this review did not run tests.

Security review

• warning — 1. Secrets and Credentials: No new hardcoded secret literals were found. However, the new OPENSHELL_SANDBOX_COMMAND value can be built from envArgs that include TOOL_GATEWAY_USER_TOKEN and BRAVE_API_KEY, causing runtime credentials to be persisted in Docker env/config metadata during GPU recreate.
• warning — 2. Input Validation and Data Sanitization: The sandbox command is serialized with parts.join(' ') without escaping or structured encoding. Inputs such as CHAT_UI_URL, proxy env, broker token, and Brave API key are not all constrained to shell-safe/no-space values at this serialization boundary.
• pass — 3. Authentication and Authorization: No new endpoints or authorization decisions are introduced. The change preserves existing OpenShell/Hermes startup and credential-rewrite boundaries, subject to the credential exposure warning above.
• pass — 4. Dependencies and Third-Party Libraries: No new package dependencies, installers, registries, or version pins are added in the shown diff.
• pass — 5. Error Handling and Logging: New logs report stale PID/lock cleanup and socat PIDs without directly printing secrets. Existing diagnostic sanitization tests remain present.
• pass — 6. Cryptography and Data Protection: Not applicable — no cryptographic operations or algorithms are introduced or modified.
• warning — 7. Configuration and Security Headers: The changed path recreates Docker sandbox containers and continues to add SYS_PTRACE and potentially apparmor=unconfined for GPU support. That behavior appears pre-existing and tested, but it remains high-risk sandbox configuration that needs E2E validation.
• warning — 8. Security Testing: Tests cover stale file cleanup, symlink preservation, orphan socat cleanup, and command replacement. Missing negative tests for sensitive startup env exposure and shell/space/metacharacter handling in OPENSHELL_SANDBOX_COMMAND. E2E runtime validation is pending.
• warning — 9. Holistic Security Posture: The PR addresses a real sandbox startup regression and avoids deleting locks when a live gateway is detected, but it changes high-risk sandbox lifecycle and credential-adjacent command propagation while CI/E2E and CodeRabbit remain pending.

Test / E2E status

• Test depth: e2e_required — Runtime/sandbox/infrastructure paths need real execution coverage: agents/hermes/start.sh, src/lib/onboard.ts, src/lib/onboard/docker-gpu-patch.ts, and src/lib/onboard/docker-gpu-sandbox-create.ts. Unit tests validate helper logic but not OpenShell supervisor reconnect, Docker inspect/env persistence, actual GPU recreate, or Hermes gateway startup.
• E2E Advisor: missing
• Required E2E jobs: E2E recommendation, wsl-e2e, macos-e2e
• Missing for analyzed SHA: E2E recommendation, wsl-e2e, macos-e2e

✅ What looks good

• The PR targets files that still exist and directly addresses the reported Spark GPU recreate startup path.
• The Docker GPU recreate path now has focused unit coverage for replacing the idle OpenShell sandbox command and adding the command env when absent.
• Hermes runtime cleanup tests cover stale pid/lock removal, orphan socat cleanup, and preserving state when a live gateway is present.
• The cleanup logic treats legacy Hermes PID symlinks carefully and avoids reading unsafe Tirith marker symlinks from existing coverage.

Review completeness

• Git diff in the prompt was truncated; repository reads were used for the main changed implementation sections, but not every unchanged surrounding line was re-reviewed.
• No PR scripts, package-manager commands, Docker commands, or tests were executed.
• Review thread state was unavailable beyond the provided GraphQL nodes; CodeRabbit was still pending.
• No linked issues were present in trusted context, so acceptance mapping used the PR body clauses and test claims as untrusted evidence.
• E2E Advisor comments were absent; only the in-progress E2E recommendation check run was available.
• Human maintainer review required: yes