Skip to content

fix(sandbox/recover): parse framed exec output for Hermes boundary - #5590

Merged
cv merged 7 commits into
mainfrom
fix/hermes-recover-root-exec-framing
Jun 22, 2026
Merged

fix(sandbox/recover): parse framed exec output for Hermes boundary#5590
cv merged 7 commits into
mainfrom
fix/hermes-recover-root-exec-framing

Conversation

@cv

@cv cv commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes the framed-output cause of San's Hermes recover/connect regression from #5530 without reverting the running-gateway secret-boundary enforcement. This intentionally retains the #5525 security contract: a running Hermes gateway still refuses when the validator reports raw secret-shaped values; the accepted healthy path is framed SECRET_BOUNDARY_OK, not ignoring the env-file boundary. The sandbox exec path now tolerates OpenShell stdout framing such as stdout: ..., avoids multiline payloads for the Hermes validator probe, and falls back to local Docker root exec when the OpenShell exec transport does not produce the startup marker, so healthy sandboxes can read SECRET_BOUNDARY_OK instead of failing closed as inconclusive.

Related Issue

Related to #5589
Addresses #5525

Changes

  • Normalize stdout-framed openshell sandbox exec output after the NemoClaw startup marker in process-recovery.ts.
  • Reuse the robust marker extraction for both synchronous recovery checks and async status probes.
  • Encode Hermes validator payloads before passing them through sh -c so OpenShell does not receive multiline exec payloads.
  • Fall back to local Docker root exec for Docker-backed sandboxes when OpenShell exec output does not include the startup marker.
  • Add regression coverage for framed root-exec output, newline-free Hermes validator payloads, Docker root-exec fallback, and the Hermes running-gateway boundary success/refusal paths with framed markers.

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

  • 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
  • Targeted tests pass for changed behavior
  • Full npm test passes (broad runtime changes only)
  • 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)

Signed-off-by: Carlos Villela cvillela@nvidia.com

Summary by CodeRabbit

  • Bug Fixes
    • Improved sandbox stdout handling to correctly ignore optional line framing (for example, stdout:/[stdout]) and extract output only after the startup marker.
    • Updated sandbox exec status handling to return null when the marker is missing or extraction isn’t reliable.
  • Tests
    • Added unit tests covering correct stdout extraction and the failure scenario when the marker appears outside the expected format.
    • Updated recovery test mocks to match the new framed marker output and added coverage for the “falls through” recovery path.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv cv added the security label Jun 22, 2026
@cv cv self-assigned this Jun 22, 2026
@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds internal helpers parseSandboxExecStdoutFrame, extractSandboxExecCommandStdout, and buildSandboxExecMarkedCommand to handle stdout frame prefixes, locate the startup marker as a standalone line, and base64-encode sandbox exec commands for transport safety. Both executeSandboxExecCommand and executeSandboxExecCommandForStatus are updated to use the new extraction logic. Test infrastructure adds helpers to decode base64 payloads; unit tests verify framed output parsing; regression tests are refactored to work with base64 encoding; Hermes secret-boundary scenarios are updated and extended to verify recovery with stdout-framed markers.

Changes

Sandbox exec stdout frame extraction and base64 encoding

Layer / File(s) Summary
Marker extraction and command encoding helpers
src/lib/actions/sandbox/process-recovery.ts
Adds Buffer import and introduces parseSandboxExecStdoutFrame to strip optional [stdout] or stdout: prefixes from line beginnings, extractSandboxExecCommandStdout to locate SANDBOX_EXEC_STARTED_MARKER as its own trimmed line and return post-marker output or null, and buildSandboxExecMarkedCommand to base64-encode a command and wrap it with printf/base64 decode execution inside the sandbox.
Command function integration with extraction helpers
src/lib/actions/sandbox/process-recovery.ts
Updates executeSandboxExecCommand to replace manual trim/split/slice logic with extractSandboxExecCommandStdout on result.stdout, returning null on extraction failure; similarly updates executeSandboxExecCommandForStatus on result.output; replaces inline printf-based command construction with buildSandboxExecMarkedCommand call.
Test infrastructure and unit tests for extraction
test/process-recovery.test.ts
Adds Buffer import, imports executeSandboxExecCommand for unit testing, introduces decodeSandboxExecShellPayload and getSandboxExecShellCommand helpers to decode base64 payloads and extract the invoked shell command from spawnSync raw arguments, and adds comprehensive unit tests verifying stdout post-marker extraction with [stdout] and stdout: framing, rejection when markers appear outside a postamble, and verification of newline-free base64 payload construction.
Regression test mock refactoring for base64 payloads
test/process-recovery.test.ts
Updates all spawnSync mocks across checkAndRecoverSandboxProcesses test scenarios to derive shellCommand via getSandboxExecShellCommand(rawArgs) instead of manual stringification, enabling reliable command routing in mocks that work with the new base64-encoded command format.
Hermes secret-boundary test scenarios with stdout framing
test/process-recovery.test.ts
Adjusts the Hermes "refuses recovery … raw secret-shaped values" test fixtures to format gateway probe and secret-boundary validator stdout as stdout-framed root exec markers; adds a new "falls through when the Hermes secret-boundary check parses stdout-framed root exec markers" test that verifies recovery proceeds with both gateway probe and validator returning stdout-framed markers, asserts no secretBoundaryRefused fields are set, and confirms the boundary check is invoked exactly once.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related issues

Possibly related PRs

  • NVIDIA/NemoClaw#5182: Passes executeSandboxExecCommand into the wedge-diagnostics recovery flow, which now benefits from the hardened stdout frame extraction introduced here.
  • NVIDIA/NemoClaw#5530: Also modifies process-recovery's marker extraction path for Hermes SECRET_BOUNDARY_* markers, sharing the same stdout-parsing code paths updated in this PR.

Suggested labels

bug-fix, integration: hermes, area: sandbox, v0.0.66

Suggested reviewers

  • ericksoa

Poem

🐇 A sentinel was lost in a frame,
The stdout prefix hid all acclaim,
Now helpers strip "[stdout]" away,
And base64 encodes the day,
Recovery flows without shame! 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% 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
Title check ✅ Passed The title 'fix(sandbox/recover): parse framed exec output for Hermes boundary' clearly and concisely summarizes the main change: fixing the Hermes boundary check by parsing OpenShell stdout framing in the recovery module, which matches the core problem described in the PR objectives.
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/hermes-recover-root-exec-framing

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

@github-code-quality

github-code-quality Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the fix/hermes-recover-r... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/hermes-recover-r... 7cfa0db +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/li...bprocess-env.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/bl...print/runner.ts 95%
nemoclaw/src/co...ration-state.ts 94%
nemoclaw/src/bl...ate-networks.ts 94%
nemoclaw/src/index.ts 94%

TypeScript / code-coverage/cli

The overall coverage in the fix/hermes-recover-r... branch is 46%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/hermes-recover-r... 7cfa0db +/-
src/lib/state/o...oard-session.ts 91%
src/lib/inference/local.ts 76%
src/lib/sandbox/config.ts 72%
src/lib/actions...dbox/rebuild.ts 67%
src/lib/onboard/preflight.ts 64%
src/lib/actions...licy-channel.ts 56%
src/lib/state/sandbox.ts 55%
src/lib/policy/index.ts 49%
src/lib/onboard...er-gpu-patch.ts 44%
src/lib/onboard.ts 18%

Updated June 22, 2026 17:51 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: sandbox-operations-e2e, hermes-secret-boundary-e2e, issue-2478-crash-loop-recovery-e2e
Optional E2E: sandbox-survival-e2e, gateway-health-honest-vitest

Dispatch hint: sandbox-operations-e2e,hermes-secret-boundary-e2e,issue-2478-crash-loop-recovery-e2e

Auto-dispatched E2E: hermes-secret-boundary-e2e, issue-2478-crash-loop-recovery-e2e via nightly-e2e.yaml at 7cfa0dbf8295a4ad0414ce0b174d25be853f2e40nightly run

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • sandbox-operations-e2e (medium-high): Covers real sandbox lifecycle/status behavior, TC-SBX-08 process recovery after killing the OpenClaw gateway process, forward handling, and destructive gateway recovery. This is the primary live regression suite for changes in process-recovery.ts.
  • hermes-secret-boundary-e2e (medium-high): Validates the real Hermes sandbox secret-boundary flow that this PR changes by base64-wrapping validator commands and parsing framed root-exec output. This is merge-blocking because the touched code enforces a security boundary.
  • issue-2478-crash-loop-recovery-e2e (medium): Exercises real gateway crash-loop/recovery behavior and guard-chain preservation. The changed sandbox exec parser and Docker fallback can affect whether recovery probes and recovery scripts observe the expected in-sandbox state.

Optional E2E

  • sandbox-survival-e2e (medium): Useful broader install/onboard/sandbox survival confidence because status and in-sandbox health probes use the modified exec-output path, but it is less targeted than the required recovery/security suites.
  • gateway-health-honest-vitest (low-medium): Adjacent live Vitest coverage for real OpenShell/Docker gateway health behavior; useful if maintainers want extra confidence around health reporting, but not as directly tied as sandbox recovery and Hermes boundary tests.

New E2E recommendations

  • OpenShell sandbox exec transport compatibility (medium): Existing live E2E will cover the OpenShell version present in CI, but there is no targeted E2E that deliberately verifies both framed stdout and no-marker Docker fallback behavior against real sandbox recovery/status flows.
    • Suggested test: Add a targeted live regression that runs nemoclaw <sandbox> status/recovery through a controlled OpenShell exec framing mode and separately forces the Docker root-exec fallback path.

Dispatch hint

  • Workflow: nightly-e2e.yaml
  • jobs input: sandbox-operations-e2e,hermes-secret-boundary-e2e,issue-2478-crash-loop-recovery-e2e

@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: issue-2478-crash-loop-recovery-vitest
Optional Vitest E2E scenarios: ubuntu-repo-docker-post-reboot-recovery

Dispatch required Vitest E2E scenarios:

  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=issue-2478-crash-loop-recovery-vitest

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

  • issue-2478-crash-loop-recovery-vitest: The PR changes sandbox process recovery and openshell sandbox exec stdout parsing/fallback behavior. The issue-2478 crash-loop recovery live Vitest job exercises the production OpenClaw recovery path through real Docker/OpenShell/NemoClaw boundaries after gateway disruption.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=issue-2478-crash-loop-recovery-vitest

Optional Vitest E2E scenarios

  • ubuntu-repo-docker-post-reboot-recovery: Adjacent typed registry coverage for host-side sandbox/container preservation and nemoclaw <sandbox> status recovery after a simulated post-reboot container stop. Useful because the PR changes process-recovery internals, but the free-standing crash-loop job is the more direct required OpenClaw recovery probe.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-docker-post-reboot-recovery

Relevant changed files

  • src/lib/actions/sandbox/process-recovery.ts

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Blocked

Merge posture: Do not merge until addressed
Primary next action: Fix PRA-4: Automatic Docker root-exec fallback bypasses the OpenShell sandbox exec boundary; then add or justify PRA-T1.
Open items: 1 required · 6 warnings · 0 suggestions · 8 test follow-ups
Since last review: 0 prior items resolved · 4 still apply · 1 new item found

Action checklist

  • PRA-4 Fix: Automatic Docker root-exec fallback bypasses the OpenShell sandbox exec boundary in src/lib/actions/sandbox/process-recovery.ts:233
  • PRA-1 Resolve or justify: Source-of-truth review needed: OpenShell stdout-frame compatibility parser in `extractSandboxExecCommandStdout()`
  • PRA-2 Resolve or justify: Source-of-truth review needed: Local Docker root-exec fallback from `executeSandboxExecCommand()`
  • PRA-3 Resolve or justify: Source-of-truth review needed: Hermes validator base64 payload special case in `buildSandboxExecMarkedCommand()`
  • PRA-5 Resolve or justify: Preserve raw child stdout that begins with OpenShell frame-like prefixes in src/lib/actions/sandbox/process-recovery.ts:101
  • PRA-6 Resolve or justify: Add direct coverage for framed async/status exec output in src/lib/actions/sandbox/process-recovery.ts:274
  • PRA-7 Resolve or justify: Bound the Hermes base64 payload workaround at its source in src/lib/actions/sandbox/process-recovery.ts:68
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Runtime validation
  • PRA-T6 Add or justify test follow-up: Add direct coverage for framed async/status exec output
  • PRA-T7 Add or justify test follow-up: Acceptance clause
  • PRA-T8 Add or justify test follow-up: Acceptance clause

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-3 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-4 Required security src/lib/actions/sandbox/process-recovery.ts:233 Remove the automatic Docker fallback from the shared `executeSandboxExecCommand()` path, or replace it with a separately invoked, explicitly authorized recovery path that cannot be triggered by parse failure/error output and that preserves the OpenShell policy/audit boundary. Missing marker or OpenShell errors should fail closed for the Hermes boundary path.
PRA-5 Resolve/justify security src/lib/actions/sandbox/process-recovery.ts:101 Track the mode of the line that contains `__NEMOCLAW_SANDBOX_EXEC_STARTED__`. If the marker is found as a raw line, return later raw lines unchanged. Only strip recognized stdout frame prefixes from later lines when the marker itself was found through a recognized framed stdout line.
PRA-6 Resolve/justify tests src/lib/actions/sandbox/process-recovery.ts:274 Add focused tests around `isSandboxGatewayRunningForStatus()` with `captureOpenshellForStatus()` mocked to return framed output and boundary-invalid output. Keep the tests direct and local to `test/process-recovery.test.ts`.
PRA-7 Resolve/justify architecture src/lib/actions/sandbox/process-recovery.ts:68 Add a concise code comment or refactor naming the invalid OpenShell payload state, the source boundary, why the source cannot be fixed in this PR, and the removal condition. If the issue is general multiline shell payload handling, make the helper's contract explicit rather than matching a validator filename.

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

PRA-4 Required — Automatic Docker root-exec fallback bypasses the OpenShell sandbox exec boundary

  • Location: src/lib/actions/sandbox/process-recovery.ts:233
  • Category: security
  • Problem: `executeSandboxExecCommand()` now falls back to `docker exec -u root <container> sh -c <markedCommand>` whenever the OpenShell result cannot be parsed or the OpenShell call throws. A missing startup marker can mean human framing drift, but it can also mean an OpenShell transport error, policy denial, authorization/audit boundary, or other failure. The changed code treats all of those as permission to locate a local container named `openshell-${sandboxName}` and execute the same shell command as root.
  • Impact: Sandbox recovery, status probes, and Hermes secret-boundary enforcement can bypass OpenShell's trusted execution path and run privileged commands directly in Docker. That weakens the sandbox lifecycle policy boundary and can hide explicit OpenShell failures behind a second root execution path.
  • Required action: Remove the automatic Docker fallback from the shared `executeSandboxExecCommand()` path, or replace it with a separately invoked, explicitly authorized recovery path that cannot be triggered by parse failure/error output and that preserves the OpenShell policy/audit boundary. Missing marker or OpenShell errors should fail closed for the Hermes boundary path.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read `executeSandboxExecCommand()` and confirm that `parseSandboxCommandResult(result) ?? executeLocalDockerSandboxCommand(...)` is no longer used for generic OpenShell parse failures, and that the `catch` path does not silently run Docker root exec.
  • Missing regression test: Add `executeSandboxExecCommand does not run Docker root exec when OpenShell returns an error or denial without the startup marker`, mocking `spawnSync` to return an explicit OpenShell failure and asserting `dockerSpawnSync` is not called and the result is `null`.
  • Done when: The required change is committed and verification passes: Read `executeSandboxExecCommand()` and confirm that `parseSandboxCommandResult(result) ?? executeLocalDockerSandboxCommand(...)` is no longer used for generic OpenShell parse failures, and that the `catch` path does not silently run Docker root exec.
  • Evidence: `executeSandboxExecCommand()` calls `executeLocalDockerSandboxCommand(sandboxName, markedCommand, effectiveTimeout)` when `parseSandboxCommandResult(result)` returns `null`, and `executeLocalDockerSandboxCommand()` invokes `dockerSpawnSync(["exec", "-u", "root", containerId, "sh", "-c", markedCommand], ...)`.
Review findings by urgency: 1 required fix, 6 items to resolve/justify, 0 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: OpenShell stdout-frame compatibility parser in `extractSandboxExecCommandStdout()`

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Partial: sync framed marker parsing and preamble rejection are tested. Missing raw-mode frame-like child stdout preservation and direct async/status parser tests.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: The comment above `extractSandboxExecCommandStdout()` names OpenShell framing and a removal condition, but the implementation strips prefixes from all lines via `.map(parseSandboxExecStdoutFrame)` before determining marker mode.

PRA-2 Resolve/justify — Source-of-truth review needed: Local Docker root-exec fallback from `executeSandboxExecCommand()`

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Only a happy-path Docker fallback test exists; there is no negative test proving Docker fallback is not used after explicit OpenShell errors or denials.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `executeSandboxExecCommand()` retries through `executeLocalDockerSandboxCommand()` whenever `parseSandboxCommandResult()` returns `null` or the OpenShell call throws.

PRA-3 Resolve/justify — Source-of-truth review needed: Hermes validator base64 payload special case in `buildSandboxExecMarkedCommand()`

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Partial: `passes a newline-free Hermes validator payload to OpenShell` verifies the encoded payload has no newline characters.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `buildSandboxExecMarkedCommand()` checks for `validate-hermes-env-secret-boundary.py`, base64-encodes the command, and pipes it to `sh` without a source-boundary/removal comment.

PRA-5 Resolve/justify — Preserve raw child stdout that begins with OpenShell frame-like prefixes

  • Location: src/lib/actions/sandbox/process-recovery.ts:101
  • Category: security
  • Problem: `extractSandboxExecCommandStdout()` maps every output line through `parseSandboxExecStdoutFrame()` before it knows whether the stream is actually framed. If the marker line was raw, child output after it should remain raw; today a legitimate child line such as `stdout: literal` or `[stdout] literal` is returned as `literal`.
  • Impact: This changes the shared sandbox exec contract for recovery, status, and Hermes boundary callers. Mutating raw child stdout can corrupt command output or make downstream lifecycle/security parsers interpret ordinary text as trusted markers.
  • Recommended action: Track the mode of the line that contains `__NEMOCLAW_SANDBOX_EXEC_STARTED__`. If the marker is found as a raw line, return later raw lines unchanged. Only strip recognized stdout frame prefixes from later lines when the marker itself was found through a recognized framed stdout line.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect `extractSandboxExecCommandStdout()` and confirm it no longer does `stdout.split(...).map(parseSandboxExecStdoutFrame)` before determining marker mode, and that raw-mode post-marker lines are returned without prefix stripping.
  • Missing regression test: Add `executeSandboxExecCommand preserves raw child stdout beginning with stdout colon after a raw startup marker`, using mocked stdout `__NEMOCLAW_SANDBOX_EXEC_STARTED__\nstdout: literal\n` and expecting `stdout: literal`. Table-drive the same behavior for `[stdout] literal`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect `extractSandboxExecCommandStdout()` and confirm it no longer does `stdout.split(...).map(parseSandboxExecStdoutFrame)` before determining marker mode, and that raw-mode post-marker lines are returned without prefix stripping.
  • Evidence: Current code builds `const lines = stdout.split(/\r?\n/).map(parseSandboxExecStdoutFrame);` and later returns `.slice(exactMarkerIndex + 1).map((line) => line.text)`, so post-marker prefixes are stripped regardless of whether the marker line was raw or framed.

PRA-6 Resolve/justify — Add direct coverage for framed async/status exec output

  • Location: src/lib/actions/sandbox/process-recovery.ts:274
  • Category: tests
  • Problem: `executeSandboxExecCommandForStatus()` now uses the same tolerant marker extractor as the synchronous recovery path, but the changed test inventory exercises `executeSandboxExecCommand()` and synchronous `checkAndRecoverSandboxProcesses()` flows, not the exported async status probe surface.
  • Impact: The async status path can regress independently while synchronous Hermes recovery tests remain green. Status probes influence sandbox lifecycle reporting and now share the trust-boundary parser changed by this PR.
  • Recommended action: Add focused tests around `isSandboxGatewayRunningForStatus()` with `captureOpenshellForStatus()` mocked to return framed output and boundary-invalid output. Keep the tests direct and local to `test/process-recovery.test.ts`.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search `test/process-recovery.test.ts` for `isSandboxGatewayRunningForStatus` or `captureOpenshellForStatus`; then inspect whether both framed `RUNNING` and invalid preamble cases are asserted.
  • Missing regression test: Add `isSandboxGatewayRunningForStatus parses stdout-framed captureOpenshellForStatus output as RUNNING`, with mocked output `stdout: __NEMOCLAW_SANDBOX_EXEC_STARTED__\nstdout: RUNNING\n`, expecting `true`. Add `isSandboxGatewayRunningForStatus returns null when status output only mentions the startup marker in an unframed preamble`, expecting `null`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search `test/process-recovery.test.ts` for `isSandboxGatewayRunningForStatus` or `captureOpenshellForStatus`; then inspect whether both framed `RUNNING` and invalid preamble cases are asserted.
  • Evidence: `executeSandboxExecCommandForStatus()` calls `extractSandboxExecCommandStdout(result.output || "")`; the changed test import list includes `executeSandboxExecCommand` but not `isSandboxGatewayRunningForStatus`, and the static test inventory lists only synchronous exec/recovery additions.

PRA-7 Resolve/justify — Bound the Hermes base64 payload workaround at its source

  • Location: src/lib/actions/sandbox/process-recovery.ts:68
  • Category: architecture
  • Problem: `buildSandboxExecMarkedCommand()` special-cases commands containing `validate-hermes-env-secret-boundary.py` by base64-encoding the entire shell payload and piping it to `sh`. The test proves the payload is newline-free, but the source-of-truth details for this localized workaround are not in the code: what exact OpenShell invalid state creates the need, why only this validator path is affected, and when the workaround can be removed.
  • Impact: A string-match special case in a shared shell execution helper can become sticky compatibility debt or be accidentally broadened without understanding the OpenShell transport constraint it is compensating for.
  • Recommended action: Add a concise code comment or refactor naming the invalid OpenShell payload state, the source boundary, why the source cannot be fixed in this PR, and the removal condition. If the issue is general multiline shell payload handling, make the helper's contract explicit rather than matching a validator filename.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `buildSandboxExecMarkedCommand()` and confirm the special case documents the invalid state/source/removal condition or is replaced by a clearer helper contract.
  • Missing regression test: Existing coverage partially proves the behavior via `passes a newline-free Hermes validator payload to OpenShell`. Add `executeSandboxExecCommand leaves non-Hermes single-line payloads unencoded` if the filename-specific branch remains.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `buildSandboxExecMarkedCommand()` and confirm the special case documents the invalid state/source/removal condition or is replaced by a clearer helper contract.
  • Evidence: `buildSandboxExecMarkedCommand()` checks `command.includes("validate-hermes-env-secret-boundary.py")`, base64-encodes that command, and pipes it through `base64 -d | sh`; no nearby comment explains the source boundary or removal condition for this compatibility behavior.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

  • None.
Simplification opportunities: 1 possible cut, net -45 lines possible

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

  • PRA-4 yagni (src/lib/actions/sandbox/process-recovery.ts:233): The automatic `findLocalDockerSandboxContainer()` / `executeLocalDockerSandboxCommand()` fallback and the `?? executeLocalDockerSandboxCommand(...)`/`catch` fallback from the shared OpenShell exec helper.
    • Replacement: Keep the framed-output marker parser and return `null` on missing marker or OpenShell error so existing fail-closed recovery/SSH fallback behavior can handle inconclusive execution.
    • Net: -45 lines
    • Safety boundary: Do not remove the Hermes secret-boundary validator, marker exact-line requirement, preamble rejection, or credential-redaction/security diagnostics.
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — executeSandboxExecCommand preserves raw child stdout beginning with stdout colon after a raw startup marker. The PR changes sandbox recovery and root execution behavior. Unit tests cover several synchronous paths, but runtime/sandbox validation and more negative-path unit tests are needed because the affected code crosses OpenShell, Docker, shell, and Hermes boundary enforcement surfaces.
  • PRA-T2 Runtime validation — executeSandboxExecCommand preserves raw child stdout beginning with bracketed stdout after a raw startup marker. The PR changes sandbox recovery and root execution behavior. Unit tests cover several synchronous paths, but runtime/sandbox validation and more negative-path unit tests are needed because the affected code crosses OpenShell, Docker, shell, and Hermes boundary enforcement surfaces.
  • PRA-T3 Runtime validation — isSandboxGatewayRunningForStatus parses stdout-framed captureOpenshellForStatus output as RUNNING. The PR changes sandbox recovery and root execution behavior. Unit tests cover several synchronous paths, but runtime/sandbox validation and more negative-path unit tests are needed because the affected code crosses OpenShell, Docker, shell, and Hermes boundary enforcement surfaces.
  • PRA-T4 Runtime validation — isSandboxGatewayRunningForStatus returns null when the startup marker appears only in an unframed preamble. The PR changes sandbox recovery and root execution behavior. Unit tests cover several synchronous paths, but runtime/sandbox validation and more negative-path unit tests are needed because the affected code crosses OpenShell, Docker, shell, and Hermes boundary enforcement surfaces.
  • PRA-T5 Runtime validation — executeSandboxExecCommand does not run Docker root exec when OpenShell returns an explicit error or denial without the startup marker. The PR changes sandbox recovery and root execution behavior. Unit tests cover several synchronous paths, but runtime/sandbox validation and more negative-path unit tests are needed because the affected code crosses OpenShell, Docker, shell, and Hermes boundary enforcement surfaces.
  • PRA-T6 Add direct coverage for framed async/status exec output — Add focused tests around `isSandboxGatewayRunningForStatus()` with `captureOpenshellForStatus()` mocked to return framed output and boundary-invalid output. Keep the tests direct and local to `test/process-recovery.test.ts`.
  • PRA-T7 Acceptance clause — Related to [Hermes][CLI&UX] nemohermes sandbox connect and recover fail after #5530 lands probe path secret boundary check #5589 — add test evidence or identify existing coverage. The deterministic validation context did not include linked issue bodies or comments, so no literal [Hermes][CLI&UX] nemohermes sandbox connect and recover fail after #5530 lands probe path secret boundary check #5589 clauses were available to verify against the diff.
  • PRA-T8 Acceptance clause — Addresses [All Platforms][Security] NemoHermes still starts and serves after raw secret-shaped TELEGRAM_BOT_TOKEN in .env, no [SECURITY] refusal #5525 — add test evidence or identify existing coverage. The deterministic validation context did not include linked issue bodies or comments, so no literal [All Platforms][Security] NemoHermes still starts and serves after raw secret-shaped TELEGRAM_BOT_TOKEN in .env, no [SECURITY] refusal #5525 clauses were available to verify against the diff.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: OpenShell stdout-frame compatibility parser in `extractSandboxExecCommandStdout()`

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Partial: sync framed marker parsing and preamble rejection are tested. Missing raw-mode frame-like child stdout preservation and direct async/status parser tests.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: The comment above `extractSandboxExecCommandStdout()` names OpenShell framing and a removal condition, but the implementation strips prefixes from all lines via `.map(parseSandboxExecStdoutFrame)` before determining marker mode.

PRA-2 Resolve/justify — Source-of-truth review needed: Local Docker root-exec fallback from `executeSandboxExecCommand()`

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Only a happy-path Docker fallback test exists; there is no negative test proving Docker fallback is not used after explicit OpenShell errors or denials.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `executeSandboxExecCommand()` retries through `executeLocalDockerSandboxCommand()` whenever `parseSandboxCommandResult()` returns `null` or the OpenShell call throws.

PRA-3 Resolve/justify — Source-of-truth review needed: Hermes validator base64 payload special case in `buildSandboxExecMarkedCommand()`

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Partial: `passes a newline-free Hermes validator payload to OpenShell` verifies the encoded payload has no newline characters.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `buildSandboxExecMarkedCommand()` checks for `validate-hermes-env-secret-boundary.py`, base64-encodes the command, and pipes it to `sh` without a source-boundary/removal comment.

PRA-4 Required — Automatic Docker root-exec fallback bypasses the OpenShell sandbox exec boundary

  • Location: src/lib/actions/sandbox/process-recovery.ts:233
  • Category: security
  • Problem: `executeSandboxExecCommand()` now falls back to `docker exec -u root <container> sh -c <markedCommand>` whenever the OpenShell result cannot be parsed or the OpenShell call throws. A missing startup marker can mean human framing drift, but it can also mean an OpenShell transport error, policy denial, authorization/audit boundary, or other failure. The changed code treats all of those as permission to locate a local container named `openshell-${sandboxName}` and execute the same shell command as root.
  • Impact: Sandbox recovery, status probes, and Hermes secret-boundary enforcement can bypass OpenShell's trusted execution path and run privileged commands directly in Docker. That weakens the sandbox lifecycle policy boundary and can hide explicit OpenShell failures behind a second root execution path.
  • Required action: Remove the automatic Docker fallback from the shared `executeSandboxExecCommand()` path, or replace it with a separately invoked, explicitly authorized recovery path that cannot be triggered by parse failure/error output and that preserves the OpenShell policy/audit boundary. Missing marker or OpenShell errors should fail closed for the Hermes boundary path.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read `executeSandboxExecCommand()` and confirm that `parseSandboxCommandResult(result) ?? executeLocalDockerSandboxCommand(...)` is no longer used for generic OpenShell parse failures, and that the `catch` path does not silently run Docker root exec.
  • Missing regression test: Add `executeSandboxExecCommand does not run Docker root exec when OpenShell returns an error or denial without the startup marker`, mocking `spawnSync` to return an explicit OpenShell failure and asserting `dockerSpawnSync` is not called and the result is `null`.
  • Done when: The required change is committed and verification passes: Read `executeSandboxExecCommand()` and confirm that `parseSandboxCommandResult(result) ?? executeLocalDockerSandboxCommand(...)` is no longer used for generic OpenShell parse failures, and that the `catch` path does not silently run Docker root exec.
  • Evidence: `executeSandboxExecCommand()` calls `executeLocalDockerSandboxCommand(sandboxName, markedCommand, effectiveTimeout)` when `parseSandboxCommandResult(result)` returns `null`, and `executeLocalDockerSandboxCommand()` invokes `dockerSpawnSync(["exec", "-u", "root", containerId, "sh", "-c", markedCommand], ...)`.

PRA-5 Resolve/justify — Preserve raw child stdout that begins with OpenShell frame-like prefixes

  • Location: src/lib/actions/sandbox/process-recovery.ts:101
  • Category: security
  • Problem: `extractSandboxExecCommandStdout()` maps every output line through `parseSandboxExecStdoutFrame()` before it knows whether the stream is actually framed. If the marker line was raw, child output after it should remain raw; today a legitimate child line such as `stdout: literal` or `[stdout] literal` is returned as `literal`.
  • Impact: This changes the shared sandbox exec contract for recovery, status, and Hermes boundary callers. Mutating raw child stdout can corrupt command output or make downstream lifecycle/security parsers interpret ordinary text as trusted markers.
  • Recommended action: Track the mode of the line that contains `__NEMOCLAW_SANDBOX_EXEC_STARTED__`. If the marker is found as a raw line, return later raw lines unchanged. Only strip recognized stdout frame prefixes from later lines when the marker itself was found through a recognized framed stdout line.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect `extractSandboxExecCommandStdout()` and confirm it no longer does `stdout.split(...).map(parseSandboxExecStdoutFrame)` before determining marker mode, and that raw-mode post-marker lines are returned without prefix stripping.
  • Missing regression test: Add `executeSandboxExecCommand preserves raw child stdout beginning with stdout colon after a raw startup marker`, using mocked stdout `__NEMOCLAW_SANDBOX_EXEC_STARTED__\nstdout: literal\n` and expecting `stdout: literal`. Table-drive the same behavior for `[stdout] literal`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect `extractSandboxExecCommandStdout()` and confirm it no longer does `stdout.split(...).map(parseSandboxExecStdoutFrame)` before determining marker mode, and that raw-mode post-marker lines are returned without prefix stripping.
  • Evidence: Current code builds `const lines = stdout.split(/\r?\n/).map(parseSandboxExecStdoutFrame);` and later returns `.slice(exactMarkerIndex + 1).map((line) => line.text)`, so post-marker prefixes are stripped regardless of whether the marker line was raw or framed.

PRA-6 Resolve/justify — Add direct coverage for framed async/status exec output

  • Location: src/lib/actions/sandbox/process-recovery.ts:274
  • Category: tests
  • Problem: `executeSandboxExecCommandForStatus()` now uses the same tolerant marker extractor as the synchronous recovery path, but the changed test inventory exercises `executeSandboxExecCommand()` and synchronous `checkAndRecoverSandboxProcesses()` flows, not the exported async status probe surface.
  • Impact: The async status path can regress independently while synchronous Hermes recovery tests remain green. Status probes influence sandbox lifecycle reporting and now share the trust-boundary parser changed by this PR.
  • Recommended action: Add focused tests around `isSandboxGatewayRunningForStatus()` with `captureOpenshellForStatus()` mocked to return framed output and boundary-invalid output. Keep the tests direct and local to `test/process-recovery.test.ts`.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search `test/process-recovery.test.ts` for `isSandboxGatewayRunningForStatus` or `captureOpenshellForStatus`; then inspect whether both framed `RUNNING` and invalid preamble cases are asserted.
  • Missing regression test: Add `isSandboxGatewayRunningForStatus parses stdout-framed captureOpenshellForStatus output as RUNNING`, with mocked output `stdout: __NEMOCLAW_SANDBOX_EXEC_STARTED__\nstdout: RUNNING\n`, expecting `true`. Add `isSandboxGatewayRunningForStatus returns null when status output only mentions the startup marker in an unframed preamble`, expecting `null`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search `test/process-recovery.test.ts` for `isSandboxGatewayRunningForStatus` or `captureOpenshellForStatus`; then inspect whether both framed `RUNNING` and invalid preamble cases are asserted.
  • Evidence: `executeSandboxExecCommandForStatus()` calls `extractSandboxExecCommandStdout(result.output || "")`; the changed test import list includes `executeSandboxExecCommand` but not `isSandboxGatewayRunningForStatus`, and the static test inventory lists only synchronous exec/recovery additions.

PRA-7 Resolve/justify — Bound the Hermes base64 payload workaround at its source

  • Location: src/lib/actions/sandbox/process-recovery.ts:68
  • Category: architecture
  • Problem: `buildSandboxExecMarkedCommand()` special-cases commands containing `validate-hermes-env-secret-boundary.py` by base64-encoding the entire shell payload and piping it to `sh`. The test proves the payload is newline-free, but the source-of-truth details for this localized workaround are not in the code: what exact OpenShell invalid state creates the need, why only this validator path is affected, and when the workaround can be removed.
  • Impact: A string-match special case in a shared shell execution helper can become sticky compatibility debt or be accidentally broadened without understanding the OpenShell transport constraint it is compensating for.
  • Recommended action: Add a concise code comment or refactor naming the invalid OpenShell payload state, the source boundary, why the source cannot be fixed in this PR, and the removal condition. If the issue is general multiline shell payload handling, make the helper's contract explicit rather than matching a validator filename.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `buildSandboxExecMarkedCommand()` and confirm the special case documents the invalid state/source/removal condition or is replaced by a clearer helper contract.
  • Missing regression test: Existing coverage partially proves the behavior via `passes a newline-free Hermes validator payload to OpenShell`. Add `executeSandboxExecCommand leaves non-Hermes single-line payloads unencoded` if the filename-specific branch remains.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `buildSandboxExecMarkedCommand()` and confirm the special case documents the invalid state/source/removal condition or is replaced by a clearer helper contract.
  • Evidence: `buildSandboxExecMarkedCommand()` checks `command.includes("validate-hermes-env-secret-boundary.py")`, base64-encodes that command, and pipes it through `base64 -d | sh`; no nearby comment explains the source boundary or removal condition for this compatibility behavior.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 27968703210
Target ref: 2de59705881a84023a09c9ff73bd128eab6478bf
Workflow ref: main
Requested jobs: issue-2478-crash-loop-recovery-e2e,hermes-secret-boundary-e2e
Summary: 2 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
hermes-secret-boundary-e2e ✅ success
issue-2478-crash-loop-recovery-e2e ✅ success

@cv

cv commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator Author

Source-of-truth / risk rationale for the localized parser fix:

  • Invalid state: openshell sandbox exec can frame command stdout lines (for example stdout: __NEMOCLAW_SANDBOX_EXEC_STARTED__), while executeSandboxExecCommand() only accepted an exact unframed marker line. The Hermes running-gateway boundary then treated the root exec channel as unavailable and failed closed with Secret-boundary check could not run... even for a healthy sandbox.
  • Source boundary: the framing is produced at the OpenShell sandbox-exec transport boundary. NemoClaw owns the host-side parser in process-recovery.ts, and all downstream recovery/probe callers depend on its normalized command stdout.
  • Source-fix constraint: changing OpenShell's CLI output contract is broader than this regression and would not help existing OpenShell versions. The safer localized fix is to normalize recognized stdout frame prefixes at the parser boundary while keeping marker extraction strict.
  • Security constraint: the parser now accepts same-line markers only from recognized stdout frames, and the existing fail-closed handling remains in place for missing/unknown markers. Framed SECRET_BOUNDARY_REFUSED remains covered by the raw-secret regression test, and framed SECRET_BOUNDARY_OK is covered by the healthy Hermes regression test.
  • Removal condition: if OpenShell exposes a stable machine-readable exec output mode that preserves stdout/stderr without human framing, switch executeSandboxExecCommand() to that mode and remove the frame-normalization fallback.

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ⚠️ Run cancelled — no signal

Run: 27971664607
Target ref: 2829b1d04c5baddde7dcec2628663966d41fa5fc
Workflow ref: main
Requested jobs: issue-2478-crash-loop-recovery-e2e
Summary: 0 passed, 0 failed, 1 cancelled, 0 skipped

Job Result
issue-2478-crash-loop-recovery-e2e ⚠️ cancelled

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv

cv commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator Author

Additional source-of-truth update after San's manual retest:

  • The parser-only patch was incomplete. The likely remaining invalid state is that OpenShell can fail to surface the NemoClaw startup marker for the multiline Hermes standalone validator payload, not just frame stdout lines.
  • This PR now avoids passing that multiline validator script directly through openshell sandbox exec ... sh -c by base64-encoding only the Hermes validator payload before decoding/running it inside the sandbox. Other recovery commands keep their existing plain shell shape to avoid changing the broader recovery contract.
  • If OpenShell exec still does not produce the startup marker, Docker-backed local sandboxes fall back to docker exec -u root against the exact openshell-<sandbox> container name. This preserves the root authority needed by the [All Platforms][Security] NemoHermes still starts and serves after raw secret-shaped TELEGRAM_BOT_TOKEN in .env, no [SECURITY] refusal #5525/fix(sandbox/recover): enforce Hermes env-file secret boundary on probe path #5530 boundary for local Docker sandboxes without weakening the refusal path.
  • I could not run San's exact live repro locally because my OpenShell gateway is currently unavailable (openshell sandbox list fails with transport connection refused), so I asked San to retest the latest branch and included a direct openshell sandbox exec marker probe for the next datapoint if it still fails.

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ⚠️ Run cancelled — no signal

Run: 27972358572
Target ref: 7d2f991c3de7add110325b3935ab7e6e57a81ddf
Workflow ref: main
Requested jobs: issue-2478-crash-loop-recovery-e2e
Summary: 0 passed, 0 failed, 1 cancelled, 0 skipped

Job Result
issue-2478-crash-loop-recovery-e2e ⚠️ cancelled

@cv

cv commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator Author

Maintainer decision on PRA-4 / Docker root-exec fallback: keep the fallback in this hotfix and treat the advisor finding as an accepted, bounded risk rather than removing it before merge.

Rationale:

  • The parser-only fix was manually tested by @sandl99 and still failed with Secret-boundary check could not run... on both Ubuntu and macOS.
  • The latest branch including the base64 Hermes payload plus local Docker root-exec fallback was manually validated by @sandl99 on the original repro sandbox: both nemohermes hm connect and nemohermes hm recover now work. See fix(sandbox/recover): revert Hermes probe boundary enforcement #5547 (comment).
  • Removing or significantly narrowing the fallback now is more likely to invalidate San's successful validation than to reduce practical risk.

Risk bounds / why this is acceptable for the hotfix:

Removal condition: remove the Docker fallback once OpenShell provides a stable machine-readable root exec mode or otherwise reliably preserves the startup marker for this Hermes boundary command across the supported local Docker runtimes.

Given that #5547's broad revert has been closed and this branch is the only validated forward fix, I recommend merging #5590 with this rationale rather than changing the behavior again pre-merge.

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 27972872415
Target ref: 7cfa0dbf8295a4ad0414ce0b174d25be853f2e40
Workflow ref: main
Requested jobs: hermes-secret-boundary-e2e,issue-2478-crash-loop-recovery-e2e
Summary: 2 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
hermes-secret-boundary-e2e ✅ success
issue-2478-crash-loop-recovery-e2e ✅ success

@cv
cv merged commit 61c43ad into main Jun 22, 2026
44 of 45 checks passed
@cv
cv deleted the fix/hermes-recover-root-exec-framing branch June 22, 2026 18:09
@cv cv added the v0.0.66 label Jun 22, 2026
@wscurran wscurran added area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery area: security Security controls, permissions, secrets, or hardening bug-fix PR fixes a bug or regression labels Aug 1, 2026
@wscurran wscurran added the integration: hermes Hermes integration behavior label Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery area: security Security controls, permissions, secrets, or hardening 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.

3 participants