Skip to content

fix(sandbox): clear stale restore dirs and repair hermes venv perms#7436

Merged
prekshivyas merged 29 commits into
mainfrom
fix/hermes-venv-perms-and-restore-stale
Jul 24, 2026
Merged

fix(sandbox): clear stale restore dirs and repair hermes venv perms#7436
prekshivyas merged 29 commits into
mainfrom
fix/hermes-venv-perms-and-restore-stale

Conversation

@laitingsheng

@laitingsheng laitingsheng commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Two v0.0.91 regressions on hermes sandboxes. A snapshot restore left files created after the snapshot in state directories that were empty when the snapshot was taken, so a clean restore no longer matched the snapshot. The hermes venv was built with permissions the sandbox user could not read, so every python3 inside the sandbox crashed reading pyvenv.cfg before any user code ran. After this change a clean restore removes post-snapshot files only from directories declared by the snapshot manifest, preserves state introduced only by a newer target manifest, preserves each directory and its mode, and lets the sandbox user run python3.

Related Issue

Fixes #7428

Changes

  • Extend pre-extract restore cleanup to clear stale contents only for snapshot-manifest state directories that were successfully absent from the backup, preserving each directory and its owner and mode. Failed backups, legacy manifests without provenance, target-only state, runtime-auth directories, image-managed OpenClaw extensions, and symlinked directories remain untouched. Behavioral tests cover each safety boundary.
  • Make /opt/hermes/.venv readable and traversable for all users in the base image, and reassert it in the final image so a lagging base cannot regress it. Both steps run a build-time check that reproduces the reported failure by invoking python3 as the sandbox user, so a bad-umask build fails fast.
  • Recheck gateway and dashboard-forward health after finalization recovery and policy checks, immediately before deployment verification, and invoke the established recovery path only when needed so onboard hands off a reachable dashboard without restarting a healthy forward.

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)

Quality Gates

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: docs-updated
  • Evidence: docs/manage-sandboxes/backup-restore.mdx and docs/reference/commands.mdx accurately describe selected cleanup plus target-only and failed-backup preservation. Exact-head review at 7b37970 against base 0cb5a43 passed with no findings and confirmed the conditional dashboard recovery needs no additional user documentation. npm run docs passed with route checking OK, 0 errors, and 2 existing warnings. Targeted Biome, git diff --check, normal hooks, commitlint, and npm run check:diff passed. Verification includes 12 restore-related files and 124 tests, 4 focused onboarding files and 40 tests, CLI type-checking, and plugin and CLI builds.
  • Agent: Codex Desktop

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run check:diff passed when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — related snapshot/state suites → 12 files / 124 tests pass; focused onboarding suites → 4 files / 40 tests pass; npm run typecheck:cli and npm run check:diff pass
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result: not applicable; the change is scoped to snapshot restore cleanup and Hermes image permissions, with targeted suites and exact-diff hooks passing.
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • 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: Tinson Lai tinsonl@nvidia.com

Summary by CodeRabbit

  • Bug Fixes
    • Improved restore cleanup by removing stale state contents that aren’t present in backups, while preserving top-level directories and their permission settings.
    • Prevented accidental changes to symlinks and safeguarded managed extension content during restore.
    • Restore now respects a new “failed backup directories” record to expand cleanup scope correctly and to report accurate failed directories on precondition errors.
  • Tests
    • Added coverage for stale-directory restore cleanup and snapshot-based restore failure scenarios.
  • Chores
    • Strengthened container image build-time verification to ensure the Python environment is accessible under the sandbox user.

Two v0.0.91 regressions on hermes sandboxes:

- snapshot restore left files created after the snapshot in state dirs
  that were empty when the snapshot was taken. The pre-extract clean-up
  now also clears declared target state dirs missing from the backup,
  preserving the directory and its owner and mode, and never touches
  runtime auth dirs or image-managed extensions.

- the hermes venv was built with permissions the sandbox user could not
  read, so python3 crashed reading pyvenv.cfg during init_import_site.
  The venv is made readable in the base and final images, gated by a
  build-time check that runs python3 as the sandbox user.

Fixes #7428

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@github-code-quality

github-code-quality Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 7b37970 in the fix/hermes-venv-perm... branch remains at 96%, unchanged from commit ccf3dd4 in the main branch.

TypeScript / code-coverage/cli

The overall coverage in commit 7b37970 in the fix/hermes-venv-perm... branch remains at 80%, unchanged from commit ccf3dd4 in the main branch.

Show a code coverage summary of the most impacted files.
File main ccf3dd4 fix/hermes-venv-perm... 7b37970 +/-
src/lib/domain/.../connect-env.ts 97% 89% -8%
src/lib/shields/index.ts 72% 71% -1%
src/lib/state/sandbox.ts 76% 76% 0%
src/lib/state/o...d-extensions.ts 98% 98% 0%
src/lib/onboard...finalization.ts 100% 100% 0%
src/lib/sandbox...rce-identity.ts 87% 87% 0%
src/lib/tunnel/services.ts 73% 73% 0%
src/lib/onboard...ndbox-create.ts 83% 91% +8%
src/lib/onboard...-create-plan.ts 75% 88% +13%
src/lib/onboard...ndbox-create.ts 33% 83% +50%

Updated July 24, 2026 05:52 UTC

@laitingsheng laitingsheng added NV QA Bugs found by the NVIDIA QA Team integration: hermes Hermes integration behavior area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression labels Jul 23, 2026
@coderabbitai

coderabbitai Bot commented Jul 23, 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

Restore cleanup removes stale state contents while preserving protected directories and symlinks. Sandbox restore records failed backup directories and supplies eligible cleanup paths. Hermes image builds verify sandbox access to the Python virtual environment.

Changes

Hermes restore and runtime access

Layer / File(s) Summary
Stale restore content cleanup
src/lib/state/openclaw-managed-extensions.ts, src/lib/state/openclaw-managed-extensions.test.ts
Cleanup commands remove stale contents from eligible restore directories, preserve modes, and skip symlinks and protected directories; tests cover these behaviors.
Sandbox restore cleanup wiring and validation
src/lib/state/sandbox.ts, test/snapshot-stale-directory-restore.test.ts
Backup manifests record failed directories, restore derives stale cleanup paths, applies them before extraction, and integration tests verify stale directory handling and failure reporting.
Hermes virtual environment verification
agents/hermes/Dockerfile, agents/hermes/Dockerfile.base
Image builds update venv permissions, verify pyvenv.cfg, and test urllib.request imports as the sandbox user.

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

Sequence Diagram(s)

sequenceDiagram
  participant SnapshotBackup
  participant RebuildManifest
  participant SandboxRestore
  participant CleanupCommand
  participant TarExtraction
  SnapshotBackup->>RebuildManifest: persist failedBackupDirs
  RebuildManifest-->>SandboxRestore: provide restore manifest
  SandboxRestore->>CleanupCommand: build cleanup for local and stale directories
  CleanupCommand-->>SandboxRestore: return directory cleanup command
  SandboxRestore->>TarExtraction: extract restoreTar after cleanup
Loading

Suggested labels: v0.0.91

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes address both linked objectives: stale files are cleaned on restore and sandbox Python can read the Hermes venv.
Out of Scope Changes check ✅ Passed The added tests and Dockerfile permission checks are directly supporting the linked restore and venv fixes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly captures the two main changes: stale restore cleanup and Hermes venv permission fixes.
✨ 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-venv-perms-and-restore-stale

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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/lib/state/sandbox.ts (1)

1603-1641: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Stale-content cleanup is unreachable when no state dir was backed up.

The new staleContentDirs cleanup (1631-1639) is nested inside if (localDirs.length > 0). Since the bug being fixed is precisely about directories that were empty at snapshot time (hence absent from localDirs), if every declared state dir was empty when the snapshot was taken — e.g. a snapshot taken right after sandbox provisioning — localDirs.length stays 0 and this whole block (including the new cleanup) is skipped, leaving stale post-snapshot content in place for that restore. SSH setup already happens above this gate (lines 1542-1569), so cleanup could run independently of whether a tar extraction is needed.

🛠️ Suggested restructuring direction
-      const staleContentDirs = targetAgent.stateDirs.filter(
-        (stateDir) => !targetRuntimeAuthDirs.has(stateDir),
-      );
-      const rmCmd = buildRestoreCleanupCommand(
-        dir,
-        localDirs,
-        pluginRestorePlan.preservedExtensionDirs,
-        new Set(pluginRestorePlan.requiredFreshExtensionDirs),
-        staleContentDirs,
-      );
-      _log(`Cleaning target dirs before restore: ${rmCmd}`);
-      const rmResult = spawnSync("ssh", [...sshArgs(configFile, sandboxName), rmCmd], {
-        stdio: ["ignore", "pipe", "pipe"],
-        timeout: 30000,
-      });
-      if (rmResult.status !== 0 || rmResult.error || rmResult.signal) {
-        ...
-      }
-
-      const extractCmd = ...
+      // (compute staleContentDirs and run rmCmd above the `if (localDirs.length > 0)`
+      // gate, or add `|| staleContentDirs.length > 0` to that gate, so cleanup still
+      // runs for dirs that were empty at snapshot time even when nothing was backed up.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/state/sandbox.ts` around lines 1603 - 1641, Move the stale-content
cleanup flow centered on staleContentDirs and buildRestoreCleanupCommand outside
the localDirs.length > 0 guard so it also runs when no state directories were
backed up. Keep tar creation/extraction conditional on localDirs being
non-empty, while preserving SSH setup and existing restore result handling.
🤖 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.

Outside diff comments:
In `@src/lib/state/sandbox.ts`:
- Around line 1603-1641: Move the stale-content cleanup flow centered on
staleContentDirs and buildRestoreCleanupCommand outside the localDirs.length > 0
guard so it also runs when no state directories were backed up. Keep tar
creation/extraction conditional on localDirs being non-empty, while preserving
SSH setup and existing restore result handling.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ba51b560-8e3d-4272-b928-ff5c8116aef7

📥 Commits

Reviewing files that changed from the base of the PR and between 21e60ae and a3f0e3f.

📒 Files selected for processing (5)
  • agents/hermes/Dockerfile
  • agents/hermes/Dockerfile.base
  • src/lib/state/openclaw-managed-extensions.test.ts
  • src/lib/state/openclaw-managed-extensions.ts
  • src/lib/state/sandbox.ts

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Informational

Advisor assessment: Informational / high confidence
Next action: Review the warnings below.
Findings: 0 blockers · 1 warning · 0 suggestions
Status: Canonical ledger: 0 blocker(s), 1 warning(s), 0 suggestion(s).

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 1 warning · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Completed · high confidence · 1 blocker · 5 warnings · 3 suggestions
  • Model comparison: normalized findings differ; normalized E2E selections differ; Nemotron reported 1 more blocker, 4 more warnings, 3 more suggestions.

Nemotron output stays in workflow artifacts and does not change the assessment above.

E2E guidance

Advisory only. E2E / PR Gate selects and runs jobs independently.

Recommended E2E: cloud-onboard, full-e2e, hermes-e2e, security-posture, onboard-repair, onboard-resume

2 optional E2E recommendations
  • state-backup-restore
  • rebuild-hermes
1 warning · 0 suggestions

Warnings

Warnings do not block.

PRA-1 Warning — Prove recovery repairs a post-warm-up stale forward

  • Location: src/lib/onboard/machine/handlers/finalization.ts:188
  • Category: correctness
  • Problem: The new second recovery call assumes scope warm-up can leave the gateway forward stale. The ordering test proves the call occurs before verification, but it does not simulate stale state or prove verification uses a repaired forward.
  • Impact: Onboarding can still report a deployment failure when scope warm-up invalidates a forward and the recovery implementation does not repair that state.
  • Recommendation: Add a focused handler test that makes the post-warm-up forward stale, verifies recovery repairs it, and verifies deployment validation uses the repaired forward. Document the source constraint and the condition that permits removal, or make warm-up converge at its source.
  • Verification: Read finalization.ts around the second checkAndRecoverSandboxProcesses call and finalization.test.ts to confirm no test models a stale forward after warmupScopeUpgrade.
  • Test coverage: A handler regression test where scope warm-up invalidates the dashboard forward and the second recovery restores it before buildVerifyChain and verifyDeployment run.
  • Evidence: src/lib/onboard/machine/handlers/finalization.ts calls checkAndRecoverSandboxProcesses again after warmupScopeUpgrade, autoPairScopeApproval, and optional verifyWebSearchInsideSandbox. src/lib/onboard/machine/handlers/finalization.test.ts asserts call ordering only; it does not model a stale forward or verify recovered-forward behavior. The code comment states that scope warm-up can outlive a healthy forward but does not identify a source-side constraint or removal condition.

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

@prekshivyas prekshivyas self-assigned this Jul 23, 2026
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>

@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: 2

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

Inline comments:
In `@src/lib/state/sandbox.ts`:
- Line 1560: Update the pre-cleanup failure return paths for SSH configuration,
plugin planning, and local-tar setup in the restore flow to report failed
directories from cleanupStateDirs instead of localDirs. Preserve the existing
behavior for successful cleanup and ensure stale-only restores return the
eligible stale directories when setup fails.

In `@test/snapshot-stale-directory-restore.test.ts`:
- Around line 14-20: Replace the HOME cleanup conditional in afterAll with
restoreEnv("HOME", ORIGINAL_HOME), and replace the direct PATH assignment at
test/snapshot-stale-directory-restore.test.ts lines 129-130 with
restoreEnv("PATH", oldPath) so both present and absent environment states are
restored deterministically.
🪄 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: ce967275-f8ed-4012-8633-2c53439507a9

📥 Commits

Reviewing files that changed from the base of the PR and between a3f0e3f and ea443a6.

📒 Files selected for processing (2)
  • src/lib/state/sandbox.ts
  • test/snapshot-stale-directory-restore.test.ts

Comment thread src/lib/state/sandbox.ts
Comment thread test/snapshot-stale-directory-restore.test.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>

@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.

🧹 Nitpick comments (1)
test/snapshot-stale-directory-restore.test.ts (1)

155-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the repository’s POSIX PATH separator.

Line 155 should use : instead of path.delimiter; these tests run on Linux CI and the repository’s established test convention is explicitly POSIX-only.

Suggested change
-    process.env.PATH = `${binDir}${path.delimiter}${oldPath || ""}`;
+    process.env.PATH = `${binDir}:${oldPath || ""}`;

Based on learnings, this repository’s Linux-only tests should construct PATH with : rather than path.delimiter.

🤖 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/snapshot-stale-directory-restore.test.ts` at line 155, Update the PATH
assignment in the test setup to join binDir and oldPath with the repository’s
POSIX separator “:” instead of path.delimiter, preserving the existing fallback
for an unset oldPath.

Source: Learnings

🤖 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.

Nitpick comments:
In `@test/snapshot-stale-directory-restore.test.ts`:
- Line 155: Update the PATH assignment in the test setup to join binDir and
oldPath with the repository’s POSIX separator “:” instead of path.delimiter,
preserving the existing fallback for an unset oldPath.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ca1fc551-9ef7-449f-a426-92a059d68f72

📥 Commits

Reviewing files that changed from the base of the PR and between ea443a6 and 050ff00.

📒 Files selected for processing (2)
  • src/lib/state/sandbox.ts
  • test/snapshot-stale-directory-restore.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/lib/state/sandbox.ts

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>

@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)
test/snapshot-stale-directory-restore.test.ts (1)

164-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the suite’s POSIX PATH separator.

Use : here rather than path.delimiter; this test lane runs on POSIX CI and the repository convention keeps these fixtures explicit.

Proposed change
-    process.env.PATH = `${binDir}${path.delimiter}${oldPath || ""}`;
+    process.env.PATH = `${binDir}:${oldPath || ""}`;

Based on learnings, prefer the established POSIX PATH separator : in these tests.

🤖 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/snapshot-stale-directory-restore.test.ts` at line 164, Update the PATH
assignment in the affected test to use the explicit POSIX separator ":" instead
of path.delimiter, preserving the existing binDir and oldPath composition.

Source: Learnings

🤖 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/snapshot-stale-directory-restore.test.ts`:
- Around line 155-157: Update the fake SSH command handling and related
assertion in the snapshot restore test to detect semantic destructive cleanup
targeting /sandbox/.openclaw/workspace rather than the exact d='...' shell
assignment shape. Ensure equivalent workspace-deletion command forms remove the
marker and are validated consistently.

---

Nitpick comments:
In `@test/snapshot-stale-directory-restore.test.ts`:
- Line 164: Update the PATH assignment in the affected test to use the explicit
POSIX separator ":" instead of path.delimiter, preserving the existing binDir
and oldPath composition.
🪄 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: 7156e0ab-2475-4f88-a70e-81631acc68db

📥 Commits

Reviewing files that changed from the base of the PR and between 050ff00 and 8ad43d3.

📒 Files selected for processing (1)
  • test/snapshot-stale-directory-restore.test.ts

Comment thread test/snapshot-stale-directory-restore.test.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

@prekshivyas prekshivyas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sensitive-path security review for exact head 5c5f4ef75588204a920330f61b51cd5cd1367f7d against base 0670d20352298dd4686d6efe6abc5118ab8f27d2: PASS.

  1. Secrets/credentials: no secret material or new credential flow.
  2. Authentication/authorization: no trust-boundary expansion; restored files retain intended sandbox use.
  3. Input validation/injection: archive paths remain manifest-scoped; symlink-safe handling is preserved.
  4. Network/SSRF: no new network destination or request construction.
  5. Filesystem/path traversal: cleanup is limited to snapshot-manifest entries and preserves failed or untracked backup directories; legacy manifests intentionally avoid absence cleanup.
  6. Privilege/isolation: the Hermes venv receives read/execute access only, not write access, for the sandbox user.
  7. Supply chain/dependencies: no dependency or provenance change.
  8. Cryptography: no cryptographic change.
  9. Logging/error handling/tests: failures remain surfaced; targeted snapshot, failed-backup, symlink, and Hermes venv tests cover the changed behavior.

I also checked the second-opinion advisor concerns. The alleged pre-extraction rollback gap predates this PR, failed discovered workspaces are omitted from the successful manifest and therefore preserved, legacy behavior is deliberately fail-safe, and the venv permission is a+rX rather than writable. No security blocker found; exact-head CI and credentialed E2E remain separate approval gates.

@prekshivyas

Copy link
Copy Markdown
Collaborator

Exact-head disposition of the Nemotron second-opinion warnings/suggestions on 5c5f4ef75588204a920330f61b51cd5cd1367f7d: no actionable code change.

  • Hermes declares workspace in agents/hermes/manifest.yaml under config root /sandbox/.hermes, and the integration test explicitly proves absent Hermes workspace cleanup. The issue's legacy /sandbox/.hermes-data layout is migrated into .hermes and removed by the image/startup migration.
  • The final Hermes Dockerfile applies and verifies a+rX even when the base image layer is cached, so a cached base cannot bypass the final-image permission repair. No write bit is granted.
  • Failed-backup preservation, legacy-manifest fail-safe behavior, stale-dir filtering, union/deduplication, symlink safety, and mode preservation are exercised by behavior/integration tests. Separate micro-tests of the same Set/filter expressions would duplicate coverage.
  • Cleanup-before-extract and retry semantics predate this change for restored directories; the PR does not create that transaction model.
  • Overlap was merge-order context; this exact head is refreshed onto current main and mergeable.

The canonical advisor ledger is already 0 blockers / 0 warnings. Required exact-head Hermes/security E2E remains the runtime gate.

@prekshivyas prekshivyas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verdict

PASS for 9272e0dc3c1a450077b4e797235a0ed6a3b7ea88. The PR does not weaken an existing security control. The finalization fix only changes when the existing loopback dashboard forward is refreshed; it adds no port, listener, policy, credential, or authorization surface. Snapshot cleanup remains restricted to manifest-declared directories, shell-quotes paths, preserves failed backups, and refuses symlink targets.

Findings

No security findings.

Detailed analysis

  1. Secrets and credentials — PASS: no secret material added or logged.
  2. Input validation and sanitization — PASS: existing port validation and shell quoting remain intact; cleanup rejects symlink targets.
  3. Authentication and authorization — PASS: no auth or scope checks are removed or bypassed.
  4. Dependencies — PASS: no dependency changes.
  5. Error handling and logging — PASS: no sensitive output or new exception path.
  6. Cryptography and data protection — PASS: no cryptographic behavior changes.
  7. Configuration and exposure — PASS: the refreshed forward remains bound to the existing 127.0.0.1 URL and declared ports.
  8. Security testing — PASS: symlink-preservation and failed-backup tests remain present; the new ordering test covers the lifecycle regression.
  9. System security — PASS: policy/recovery/scope checks still complete before the forward refresh and deployment verification.

Files reviewed

agents/hermes/Dockerfile, agents/hermes/Dockerfile.base, docs/manage-sandboxes/backup-restore.mdx, docs/reference/commands.mdx, src/lib/state/openclaw-managed-extensions.ts, src/lib/state/openclaw-managed-extensions.test.ts, src/lib/state/sandbox.ts, test/snapshot-stale-directory-restore.test.ts, src/lib/onboard/machine/handlers/finalization.ts, and src/lib/onboard/machine/handlers/finalization.test.ts.

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>

@prekshivyas prekshivyas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verdict

PASS for 7b3797056a1c76f6d36cc51314f4fc774817189b. The optimized finalization change preserves the existing loopback dashboard surface and conditionally invokes the established gateway/forward recovery path before verification. It does not add listeners, ports, privileges, credentials, policy changes, or authorization bypasses.

Findings

No security findings.

Detailed analysis

  1. Secrets and credentials — PASS: no secret material added or logged.
  2. Input validation and sanitization — PASS: existing validated ports and shell-quoted, symlink-guarded restore paths are unchanged.
  3. Authentication and authorization — PASS: policy recovery and scope approval still run before final verification; no auth check is bypassed.
  4. Dependencies — PASS: no dependency changes.
  5. Error handling and logging — PASS: the existing recovery path retains its fail-closed diagnostics and no sensitive output is introduced.
  6. Cryptography and data protection — PASS: no cryptographic behavior changes.
  7. Configuration and exposure — PASS: dashboard forwarding remains loopback-only and uses the existing declared ports.
  8. Security testing — PASS: existing symlink/failed-backup coverage remains; the ordering test requires a final recovery pass before verification.
  9. System security — PASS: the final pass rechecks gateway and forward health and restarts only when needed, avoiding a blind tunnel restart while preserving least exposure.

Files reviewed

agents/hermes/Dockerfile, agents/hermes/Dockerfile.base, docs/manage-sandboxes/backup-restore.mdx, docs/reference/commands.mdx, src/lib/state/openclaw-managed-extensions.ts, src/lib/state/openclaw-managed-extensions.test.ts, src/lib/state/sandbox.ts, test/snapshot-stale-directory-restore.test.ts, src/lib/onboard/machine/handlers/finalization.ts, and src/lib/onboard/machine/handlers/finalization.test.ts.

@prekshivyas

Copy link
Copy Markdown
Collaborator

Maintainer CI disposition for exact head 7b3797056a1c76f6d36cc51314f4fc774817189b: I accept the failed E2E / PR Gate check as a current-main cold-path performance-budget failure, not a regression introduced by this PR.

Evidence:

  • The PR run 30070659640 passed install, deployment verification, the expected first agent turn, onboard-resume, BuildKit prebuild, and no-fallback assertions. Its two full-e2e attempts failed only the absolute cold-path timing caps (plus output silence on the slower retry).
  • The unchanged PR base/current-main SHA 0cb5a4310929b6b2b67edb41b1b5446a38e19a22 independently reproduces the same failure in run 30066792304: functional flow and first turn passed, while root-start measured 361225 ms and sandbox measured 199813 ms against 228000 ms and 171000 ms.
  • The original dashboard-forward connection-refused defect no longer reproduces, and all selected functional E2E targets except this independently red performance assertion pass.

Follow-up: #6660 is reopened with the exact base/head evidence to restore a calibrated acceptance signal. Do not raise the checked-in budget in this unrelated bug-fix PR.

@prekshivyas prekshivyas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved after exact-head human review at 7b37970. Ordinary CI, CodeRabbit, both advisors, targeted restore/onboarding coverage, and PR-specific E2E evidence are clean with no unresolved threads. Maintainer waiver: the only non-passing gate is the known current-main OpenClaw cold-onboard performance-budget failure tracked by #6660; the functional flow completed and the PR body records the accepted non-success evidence.

@prekshivyas
prekshivyas merged commit 04e6dfa into main Jul 24, 2026
106 of 108 checks passed
@prekshivyas
prekshivyas deleted the fix/hermes-venv-perms-and-restore-stale branch July 24, 2026 06:57
@prekshivyas prekshivyas mentioned this pull request Jul 24, 2026
23 tasks
prekshivyas added a commit that referenced this pull request Jul 24, 2026
<!-- markdownlint-disable MD041 -->
## Summary

This PR adds the canonical dated release entry for NemoClaw v0.0.94
before the tag is cut.
The entry reconciles all 26 commits since v0.0.93 and links each
user-visible change to its owning documentation.

## Changes

- Add `docs/changelog/2026-07-24.mdx` with the exact `## v0.0.94`
heading, parser-safe SPDX comment, release summary, and detailed
bullets.
- Record sandbox restore and update behavior, onboarding and inference
changes, network policy behavior, security evidence, Hermes build
performance, DGX Station guidance, and E2E validation changes.
- Preserve `docs/` as the source of truth without changing the AI-agent
documentation routing skill.
- Use [E2E run
30075443016](https://github.com/NVIDIA/NemoClaw/actions/runs/30075443016)
for release QA evidence at exact tested SHA
`04e6dfa883071dda9df429c66e73168e1a995cba`.

### Source summary

- [#7461](#7461) ->
`docs/changelog/2026-07-24.mdx`: Record the ownership-preserving Hermes
image layer reduction and hosted timing comparison.
- [#7460](#7460) ->
`docs/changelog/2026-07-24.mdx`: Record removal of candidate Hermes swap
setup from E2E validation.
- [#7458](#7458) ->
`docs/security/fern-5.80.1-dependency-review.md`,
`docs/changelog/2026-07-24.mdx`: Record the reviewed Fern CLI update.
- [#7457](#7457) ->
`docs/changelog/2026-07-24.mdx`: Record periodic runner-pressure
telemetry.
- [#7455](#7455) ->
`docs/changelog/2026-07-24.mdx`: Record non-blocking absent Fern
previews.
- [#7450](#7450) ->
`docs/changelog/2026-07-24.mdx`: Record stable cancellation handling for
live-test child processes.
- [#7449](#7449) ->
`docs/changelog/2026-07-24.mdx`: Record parallel plugin EXDEV coverage.
- [#7448](#7448) ->
`docs/changelog/2026-07-24.mdx`: Record isolated long-running E2E lanes.
- [#7444](#7444) ->
`docs/changelog/2026-07-24.mdx`: Record exact-head Hermes swap
validation.
- [#7437](#7437) ->
`docs/manage-sandboxes/backup-restore.mdx`,
`docs/changelog/2026-07-24.mdx`: Record gateway pairing and
authenticated verification after cross-sandbox restore.
- [#7436](#7436) ->
`docs/manage-sandboxes/backup-restore.mdx`,
`docs/reference/commands.mdx`, `docs/changelog/2026-07-24.mdx`: Record
selected stale-state cleanup and Hermes virtual-environment access
repair.
- [#7385](#7385) ->
`docs/network-policy/customize-network-policy.mdx`,
`docs/changelog/2026-07-24.mdx`: Record the read-only agent-variant
route check.
- [#7371](#7371) ->
`docs/changelog/2026-07-24.mdx`: Record host-artifact verification for
session exports.
- [#7359](#7359) ->
`docs/changelog/2026-07-24.mdx`: Record platform validation for managed
vLLM model overrides.
- [#7356](#7356) ->
`docs/changelog/2026-07-24.mdx`: Record token-shaped value redaction for
`sandbox doctor --json`.
- [#7354](#7354) ->
`docs/security/advisory-early-warning.md`,
`docs/changelog/2026-07-24.mdx`: Record advisory correlation and
retained audit provenance.
- [#7352](#7352) ->
`docs/network-policy/customize-network-policy.mdx`,
`docs/network-policy/integration-policy-examples.mdx`,
`docs/reference/commands.mdx`, `docs/changelog/2026-07-24.mdx`: Record
preset reapplication and bounded `tls: skip` guidance.
- [#7345](#7345) ->
`docs/security/openclaw-2026.6.10-dependency-review.md`,
`docs/security/openclaw-2026.7.1-dependency-review.md`,
`docs/changelog/2026-07-24.mdx`: Record reviewed npm audit exception
enforcement.
- [#7340](#7340) ->
`docs/network-policy/customize-network-policy.mdx`,
`docs/changelog/2026-07-24.mdx`: Record the repaired CLI-reference
route.
- [#7334](#7334) ->
`docs/get-started/dgx-station-preparation.mdx`,
`docs/changelog/2026-07-24.mdx`: Record the qualified OTA metadata
fallback and narrowed override wording.
- [#7322](#7322) ->
`docs/changelog/2026-07-24.mdx`: Reconcile the gateway source tag added
to plugin registration banners.
- [#7284](#7284) ->
`docs/manage-sandboxes/update-sandboxes.mdx`,
`docs/changelog/2026-07-24.mdx`: Record read-only `upgrade-sandboxes
--check` behavior and recorded-gateway selection.
- [#7277](#7277) ->
`docs/changelog/2026-07-24.mdx`: Reconcile deterministic gateway TCP
refusal coverage.
- [#7234](#7234) ->
`docs/reference/troubleshooting.mdx`, `docs/changelog/2026-07-24.mdx`:
Record preserved DGX Spark managed vLLM Express intent on resume.
- [#7185](#7185) ->
`docs/reference/troubleshooting.mdx`, `docs/changelog/2026-07-24.mdx`:
Record IPv4 fallback DNS selection and exact resolver probing.
- [#6820](#6820) ->
`docs/reference/commands.mdx`, `docs/changelog/2026-07-24.mdx`: Record
the versioned, redacted `--events=jsonl` onboarding stream.

## Type of Change

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

## Quality Gates

- [ ] Tests added or updated for changed behavior
- [x] Existing tests cover changed behavior — justification: `npx vitest
run test/changelog-docs.test.ts` passed 6/6 tests.
- [ ] Tests not applicable — justification:
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [ ] 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:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Documentation Writer Review

- [x] Documentation writer subagent reviewed the completed changes
- Result: `docs-updated`
- Evidence: `docs/changelog/2026-07-24.mdx`; the writing rules,
documentation style, exact release range, skip terms, published routes,
and product scope were reviewed; the changelog test passed 6/6; `npm run
docs` passed with route checking OK, zero errors, and two existing
warnings.
- Agent: Codex Desktop
<!-- docs-review-head-sha: 65368f9 -->
<!-- docs-review-agents-blob-sha: 9c9b36d -->

## DGX Station Hardware Evidence

- [ ] Tested on DGX Station
- Tested commit: Not applicable
- Station profile/scenario: Not applicable
- Result: Not applicable
- Supporting evidence: Not applicable. This PR does not change
`scripts/prepare-dgx-station-host.sh`.

## Verification

- [x] PR description includes a `Signed-off-by:` line and every commit
appears as `Verified` in GitHub
- [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or
`npm run check:diff` passed when hooks were skipped or unavailable
- [x] Targeted behavior tests pass for the current change set, or tests
are marked not applicable above — `npx vitest run
test/changelog-docs.test.ts` passed 6/6 tests.
- [ ] Applicable broad gate passed — `npm test` for broad
runtime/test-harness changes; `npm run check` for repo-wide
validation/coverage changes — command/result: Not applicable to the
dated changelog entry.
- [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). The
build passed with zero errors and two existing Fern warnings.
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages
only). Native dated changelog entries use the required parser-safe MDX
SPDX comment and no frontmatter.

---
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>


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

## Summary by CodeRabbit

* **Documentation**
  * Added the v0.0.94 release changelog.
  * Documented improvements to sandbox snapshot and restore behavior.
* Added updates for gateway selection, policy comparisons, onboarding
event output, and DGX recovery workflows.
* Documented enhanced diagnostics redaction, npm audit provenance, image
assembly performance, and validation stability improvements.


<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
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 bug-fix PR fixes a bug or regression integration: hermes Hermes integration behavior NV QA Bugs found by the NVIDIA QA Team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Linux][Sandbox] hermes snapshot restore does not remove post-snapshot files and venv permissions block python3 inside sandbox

2 participants