feat(sandbox): version staleness detection and rebuild command - #1870
Merged
Conversation
Detect when a sandbox is running an outdated agent version after a NemoClaw upgrade and warn users on connect/status. Add a rebuild command that safely backs up workspace state, destroys the old sandbox, recreates with the current image, and restores state. - Add expected_version and version_command to agent manifests - Track agentVersion in sandbox registry at creation time - Warn on nemoclaw <name> connect and status when sandbox is stale - Add nemoclaw <name> rebuild (--yes to skip prompt) - Manifest-driven backup/restore via SSH+tar for any agent type - Credential stripping on backups (shared with migration-state.ts) - Multi-agent swarm guard (deferred until swarm branch merges) - E2E test covering full rebuild lifecycle Closes: NVBug 6076156 Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
📝 WalkthroughWalkthroughAdds sandbox backup/restore and rebuild flows, agent version probing/staleness detection, credential-stripping for backups, CLI commands ( Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as NemoClaw CLI
participant Reg as Registry / AgentDefs
participant SS as SandboxState
participant SSH as Sandbox (ssh)
participant Onb as Onboard / OpenShell
User->>CLI: nemoclaw rebuild --yes <sandbox>
CLI->>Reg: resolve agent definition & expectedVersion
Reg-->>CLI: agentDef (expectedVersion)
CLI->>SS: backupSandboxState(sandbox)
SS->>SSH: obtain ssh-config (sandbox ssh-config)
SSH-->>SS: ssh-config
SS->>SSH: check state dirs & stream tar out
SSH-->>SS: tar stream (state archive)
SS->>SS: extract locally, sanitize configs, write rebuild-manifest.json
SS-->>CLI: backup result + backupPath
CLI->>Onb: stop/delete sandbox (openshell/docker)
Onb-->>CLI: sandbox deleted
CLI->>Onb: recreate sandbox (onboard resume/recreate)
Onb-->>CLI: sandbox ready
CLI->>SS: restoreSandboxState(sandbox, backupPath)
SS->>SSH: stream tar to remote extract
SSH-->>SS: restore results
SS->>SSH: optional chown inside sandbox
SS-->>CLI: restore complete
CLI->>Reg: update sandbox.agentVersion = agentDef.expectedVersion || null
CLI-->>User: rebuild complete (status + version)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
OpenClaw test builds a base image with older OpenClaw 2026.3.11, onboards, writes marker files, then rebuilds to current version and verifies state survives. Hermes test does the same with an older Hermes calver tag. Both jobs added to nightly-e2e.yaml with failure notifications. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…ns + rebuild jobs) Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
src/lib/credential-filter.test.ts (1)
80-119: Add tests for non-JSON credential sources used by Hermes.Current coverage is JSON-only. Please add cases for YAML config and env-token files so backup credential stripping is protected against regressions across agent types.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/credential-filter.test.ts` around lines 80 - 119, The test suite for sanitizeConfigFile only covers JSON files; add tests to verify it also strips credentials from Hermes' non-JSON credential sources: create one test that writes a YAML config (e.g., using file name like "openclaw.yaml") containing fields such as apiKey and gateway.authToken and assert after calling sanitizeConfigFile that secrets are replaced with "[STRIPPED_BY_MIGRATION]" and gateway is removed, and another test for env-token style files (e.g., a plain token file used by Hermes) that ensures sanitizeConfigFile removes or replaces the token content (and does not crash) when given that path; update credential-filter.test.ts to include these two cases alongside the existing JSON tests so sanitizeConfigFile is validated across formats.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agents/hermes/manifest.yaml`:
- Around line 18-19: Remove the unused manifest field version_constraint from
agents/hermes/manifest.yaml: the codebase only uses version_command ("hermes
--version") and expected_version ("2026.4.8") for version validation, so delete
the version_constraint entry to eliminate dead configuration (ensure
version_command and expected_version remain unchanged).
In `@src/lib/credential-filter.ts`:
- Around line 71-78: sanitizeConfigFile currently only parses JSON and returns
on parse failure, leaving Hermes YAML/.env files unsanitized; update
sanitizeConfigFile to handle .yaml/.yml (use a YAML parser to load and sanitize)
and .env files (parse with dotenv-style parsing) or detect filenames and
explicitly exclude known Hermes config names by removing/zeroing credential keys
(e.g., credentials, token, api_key, secret, private_key, HER?_* env names)
before rewriting; ensure the function still preserves non-credential content and
write back sanitized files for JSON/YAML/.env, and if the backup walker only
invokes sanitizeConfigFile for .json, update the caller to invoke this sanitizer
for .yaml/.yml/.env as well so Hermes config files are covered.
In `@src/lib/onboard.ts`:
- Around line 2804-2809: The sandbox registration is stamping custom builds
created with fromDockerfile using effectiveAgent.expectedVersion, causing false
stale-version warnings; change the agentVersion passed to
registry.registerSandbox to be null when fromDockerfile is true (i.e., set
agentVersion to fromDockerfile ? null : effectiveAgent.expectedVersion) so
custom --from images are not labeled with the manifest's expected version;
update the registry.registerSandbox call near where effectiveAgent and
fromDockerfile are referenced (and keep agent assignment logic unchanged).
In `@src/lib/sandbox-state.ts`:
- Around line 214-225: Current code uses spawnSync and reads the entire tar
archive into memory via result.stdout (see spawnSync call, sshArgs, tarCmd,
extractResult, backupPath), which causes large-memory buffering; replace the
synchronous buffered approach with streaming by using child_process.spawn for
the ssh command and piping its stdout directly into a spawned tar process
(spawn("tar", ["-xf","-","-C", backupPath])) so data flows stream-to-stream
without materializing result.stdout, forward errors/status from both child
processes and preserve timeouts/cleanup, and apply the same change to the second
occurrence around lines 288-302.
- Around line 179-181: The current early-return when stateDirs.length === 0
returns success: true after writing the manifest, which permits sandboxRebuild()
to destroy state; change this to fail-closed: when stateDirs is empty, still
write the manifest if needed but return success: false and populate failedDirs
(or include an explicit error message) so callers (e.g., sandboxRebuild) do not
proceed with destructive actions; update the return at the stateDirs.length ===
0 branch around writeManifest/backupPath/manifest to reflect success: false and
include identifying info in failedDirs instead of success: true.
- Around line 135-139: The credential stripping only calls sanitizeConfigFile
for files matching entry.name.endsWith(".json"), so YAML/.yml/.env (and other
non-JSON) configs are left unchanged; update the conditional in the loop that
currently uses isSensitiveFile and entry.name.endsWith(".json") to also match
YAML and env formats (e.g., entry.name.endsWith(".yaml") ||
entry.name.endsWith(".yml") || entry.name.endsWith(".env") or use a helper like
isConfigFile(entry.name)), and/or enhance sanitizeConfigFile to detect file type
and parse/sanitize YAML and dotenv formats in addition to JSON; ensure you still
skip actual sensitive files via isSensitiveFile and call
sanitizeConfigFile(fullPath) for those additional extensions so non-JSON agent
configs are sanitized before backup.
In `@src/lib/sandbox-version.test.ts`:
- Around line 50-52: The registry module computes REGISTRY_FILE at import time
so tests that change process.env.HOME after import still hit the real
~/.nemoclaw file; update the module to lazily compute the registry path (e.g.,
replace top-level REGISTRY_FILE with a function like getRegistryFilePath() used
by load() and save(), or compute REGISTRY_FILE inside load()/save() on first
use) so REGISTRY_FILE reflects the current HOME, and ensure functions load() and
save() call that lazy resolver; alternatively (if you prefer not to change
registry.js) update the tests to reset and re-require the registry module after
setting process.env.HOME in beforeEach (use jest.resetModules() and
require('./registry.js')) so imports see the updated HOME.
In `@src/nemoclaw.ts`:
- Around line 1572-1579: The restore flow currently prints a success banner and
returns 0 even when sandboxState.restoreSandboxState(sandboxName,
backup.manifest.backupPath) reports failure/partial restore; update the logic
that checks restore.success so that when restore.success is false you do NOT
print the success message (the console.log that uses G and R and restoredDirs)
and instead print failure details (as already done) and return a non-zero exit
code; make the same correction for the duplicated success block later in the
file (the second occurrence that prints success and returns 0) so both places
respect restore.success and fail the process on partial/failed restores.
- Around line 1529-1532: The code calls .includes() on liveNames but
parseLiveSandboxNames returns a Set<string>, causing a TypeError; change the
membership check to use Set.prototype.has() instead (update the condition that
currently uses liveNames.includes(sandboxName) to liveNames.has(sandboxName)),
keeping the surrounding logic in the block that uses captureOpenshell and the
variables liveNames and sandboxName intact.
In `@test/e2e/test-sandbox-rebuild.sh`:
- Around line 68-74: The test script never applies the documented
NEMOCLAW_E2E_TIMEOUT_SECONDS because calls still invoke `nemoclaw` directly and
a raw `timeout 10` is used; update the script to derive TIMEOUT from
NEMOCLAW_E2E_TIMEOUT_SECONDS (defaulting to a sensible value) and replace all
direct `timeout`/raw invocations with the wrapper `timeout_cmd()` (including
onboarding, rebuild, and the connect check), and change the connect check to use
`timeout_cmd` or a pure-shell retry loop so it no longer hard-depends on GNU
`timeout`; ensure all occurrences around the previously mentioned blocks (the
`timeout_cmd()` definition, the onboarding/rebuild execution sites, and the
connect check area) are updated to use the wrapper and the TIMEOUT variable.
---
Nitpick comments:
In `@src/lib/credential-filter.test.ts`:
- Around line 80-119: The test suite for sanitizeConfigFile only covers JSON
files; add tests to verify it also strips credentials from Hermes' non-JSON
credential sources: create one test that writes a YAML config (e.g., using file
name like "openclaw.yaml") containing fields such as apiKey and
gateway.authToken and assert after calling sanitizeConfigFile that secrets are
replaced with "[STRIPPED_BY_MIGRATION]" and gateway is removed, and another test
for env-token style files (e.g., a plain token file used by Hermes) that ensures
sanitizeConfigFile removes or replaces the token content (and does not crash)
when given that path; update credential-filter.test.ts to include these two
cases alongside the existing JSON tests so sanitizeConfigFile is validated
across formats.
🪄 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: Pro Plus
Run ID: 848628c4-521e-434d-b780-ee3e92ab996f
📒 Files selected for processing (14)
agents/hermes/manifest.yamlagents/openclaw/manifest.yamlnemoclaw/src/blueprint/state.tsnemoclaw/src/commands/slash.tssrc/lib/agent-defs.tssrc/lib/credential-filter.test.tssrc/lib/credential-filter.tssrc/lib/onboard.tssrc/lib/registry.tssrc/lib/sandbox-state.tssrc/lib/sandbox-version.test.tssrc/lib/sandbox-version.tssrc/nemoclaw.tstest/e2e/test-sandbox-rebuild.sh
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/test-rebuild-hermes.sh`:
- Around line 77-80: The current check uses OLD_VERSION_CHECK=$(docker run --rm
"${OLD_SANDBOX_TAG}" hermes --version ...) but unconditionally calls pass "Old
Hermes sandbox image built", so the OLD_HERMES_VERSION precondition isn't
enforced; update the script to compare OLD_VERSION_CHECK (trimmed) against the
expected OLD_HERMES_VERSION and exit non-zero or call fail if they differ, e.g.
after retrieving OLD_VERSION_CHECK add a conditional that tests string equality
(or uses grep) and only call pass "Old Hermes sandbox image built" when the
version matches, otherwise print an error including both OLD_VERSION_CHECK and
OLD_HERMES_VERSION and abort the test; reference the OLD_VERSION_CHECK variable,
OLD_SANDBOX_TAG usage, and the pass/fail flow in the surrounding block.
- Around line 143-145: The script only logs NEW_VERSION from openshell sandbox
exec "$SANDBOX_NAME" -- hermes --version but doesn't assert the rebuild
succeeded; add a validation step that compares NEW_VERSION to the expected
version (e.g., TARGET_HERMES_VERSION or the precomputed EXPECTED_VERSION) and
fail the script on mismatch: capture NEW_VERSION, then if [ "$NEW_VERSION" !=
"$EXPECTED_VERSION" ]; then log an error and exit 1; otherwise log success with
info. Ensure you reference the NEW_VERSION variable and the openshell command
(openshell sandbox exec "$SANDBOX_NAME" -- hermes --version) when adding the
assertion.
- Around line 155-159: The registry check currently only ensures
REGISTRY_VERSION isn't "null" or "error" but doesn't verify it changed; capture
the previous value (e.g., PREV_REGISTRY_VERSION) before the upgrade and replace
the conditional to assert REGISTRY_VERSION is not "null" or "error" and is
different from PREV_REGISTRY_VERSION, then call pass "Registry agentVersion
updated to ${REGISTRY_VERSION}" or fail "Registry agentVersion not updated:
${REGISTRY_VERSION}" using the same pass/fail functions.
- Line 166: The find invocation that sets CRED_LEAKS uses two -name tests
together (in the CRED_LEAKS assignment), which is currently an AND and therefore
matches no files; change the predicate to use OR logic (e.g., combine -name
"*.json" -o -name "*.yaml" with proper grouping/escaping or use -iname/-regex as
desired) so find searches for either JSON or YAML files before piping to grep
and preserving the existing 2>/dev/null || true behavior.
In `@test/e2e/test-rebuild-openclaw.sh`:
- Around line 155-160: The current check treats empty NEW_VERSION as an upgrade
because grep -qv succeeds on empty input; update the conditional for NEW_VERSION
to require non-empty output and a real difference from OLD_OPENCLAW_VERSION. For
example, after capturing NEW_VERSION from the openshell sandbox exec call ensure
you test both: [ -n "$NEW_VERSION" ] and that NEW_VERSION does not contain
OLD_OPENCLAW_VERSION (e.g., use: if [ -n "$NEW_VERSION" ] && ! echo
"$NEW_VERSION" | grep -qF "$OLD_OPENCLAW_VERSION"; then ...), or use shell
pattern comparison ([[ -n "$NEW_VERSION" && "$NEW_VERSION" !=
*"$OLD_OPENCLAW_VERSION"* ]]) so failed/empty version output is not treated as
success.
- Around line 109-113: The script currently logs a warning when SANDBOX_VERSION
doesn't match OLD_OPENCLAW_VERSION and continues; change this to fail fast so
the test stops if the sandbox isn't on the old version. Update the conditional
handling around the SANDBOX_VERSION check (the block using SANDBOX_VERSION,
OLD_OPENCLAW_VERSION, pass and info) to call the test failure path (e.g., use
fail or exit 1) with a clear message instead of calling info, so the E2E run
aborts immediately when the sandbox version is not the expected old version.
- Around line 171-175: The registry check currently treats the literal string
"error" as a valid update because the condition only checks for "null" and
OLD_OPENCLAW_VERSION; update the conditional that uses REGISTRY_VERSION and
OLD_OPENCLAW_VERSION so it also rejects parser/runtime error values (e.g.,
ensure REGISTRY_VERSION != "error" and not empty) before calling pass; change
the block referencing REGISTRY_VERSION and OLD_OPENCLAW_VERSION so failures from
the registry read ("error" or empty) go to fail instead of pass.
- Around line 181-188: The credential-leak check quietly skips when BACKUP_DIR
is missing; change the if [ -d "$BACKUP_DIR" ] branch to explicitly fail when
the directory does not exist by adding an else that calls fail (e.g., fail
"Backup directory $BACKUP_DIR not found") so the test fails instead of reporting
success; keep the existing CRED_LEAKS logic for the present branch (using the
CRED_LEAKS variable and pass/fail calls) and only treat the missing BACKUP_DIR
as an error condition.
🪄 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: Pro Plus
Run ID: c7f6e761-9dcd-4ca1-925b-139c32d39ecd
📒 Files selected for processing (4)
.github/workflows/nightly-e2e.yamlsrc/lib/sandbox-version.test.tstest/e2e/test-rebuild-hermes.shtest/e2e/test-rebuild-openclaw.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/sandbox-version.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
test/e2e/test-rebuild-hermes.sh (4)
143-145:⚠️ Potential issue | 🟠 MajorAssert the post-rebuild Hermes version, don’t only log it.
Line 144 logs
NEW_VERSIONbut does not validate upgrade success.Suggested fix
NEW_VERSION=$(openshell sandbox exec "$SANDBOX_NAME" -- hermes --version 2>/dev/null || true) -info "New Hermes version: ${NEW_VERSION}" +if [ -n "${NEW_VERSION}" ] && ! echo "${NEW_VERSION}" | grep -q "${OLD_HERMES_VERSION}"; then + pass "Hermes version upgraded (now: ${NEW_VERSION})" +else + fail "Hermes version still old after rebuild: ${NEW_VERSION}" +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-hermes.sh` around lines 143 - 145, The test currently only logs NEW_VERSION after rebuilding Hermes; change it to assert expected result: capture NEW_VERSION from the openshell call (the variable NEW_VERSION) and add an assertion that it matches the expected version or is non-empty (e.g., compare against EXPECTED_HERMES_VERSION or fail if NEW_VERSION is empty), using the test's existing assertion helper or exit with non-zero/ci-failure if the check fails; update the info call to remain but ensure the script fails when the version check (NEW_VERSION vs EXPECTED_HERMES_VERSION) does not pass.
77-80:⚠️ Potential issue | 🟠 MajorEnforce the old-version precondition before proceeding.
The script records
OLD_VERSION_CHECKbut still unconditionally passes. This allows the test to proceed without proving the sandbox image is actually old.Suggested fix
OLD_VERSION_CHECK=$(docker run --rm "${OLD_SANDBOX_TAG}" hermes --version 2>/dev/null || true) info "Old Hermes image version: ${OLD_VERSION_CHECK}" - -pass "Old Hermes sandbox image built" +if echo "${OLD_VERSION_CHECK}" | grep -q "${OLD_HERMES_VERSION}"; then + pass "Old Hermes sandbox image built (${OLD_HERMES_VERSION})" +else + fail "Expected Hermes ${OLD_HERMES_VERSION} in old image, got: ${OLD_VERSION_CHECK}" +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-hermes.sh` around lines 77 - 80, Ensure the script verifies that OLD_VERSION_CHECK actually contains a version before marking the old sandbox build as passed: after assigning OLD_VERSION_CHECK (the variable) run a conditional that checks it is non-empty (and/or does not match the current expected version) and call a failing helper (or exit non-zero) if the check fails; only call pass "Old Hermes sandbox image built" when OLD_VERSION_CHECK is valid, and keep the info "Old Hermes image version: ${OLD_VERSION_CHECK}" for context.
166-166:⚠️ Potential issue | 🟠 MajorFix
findpredicate logic; current query can never match.Using
-name "*.json" -name "*.yaml"applies AND logic, so no file is returned and credential scanning is effectively disabled.Suggested fix
- CRED_LEAKS=$(find "$BACKUP_DIR" -name "*.json" -name "*.yaml" -exec grep -l "nvapi-\|sk-\|Bearer " {} \; 2>/dev/null || true) + CRED_LEAKS=$(find "$BACKUP_DIR" \( -name "*.json" -o -name "*.yaml" \) -exec grep -l "nvapi-\|sk-\|Bearer " {} \; 2>/dev/null || true)#!/bin/bash set -euo pipefail tmp="$(mktemp -d)" trap 'rm -rf "$tmp"' EXIT touch "$tmp/a.json" "$tmp/b.yaml" "$tmp/c.txt" echo "AND query count (expected 0):" find "$tmp" -name "*.json" -name "*.yaml" | wc -l echo "OR query count (expected 2):" find "$tmp" \( -name "*.json" -o -name "*.yaml" \) | wc -l🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-hermes.sh` at line 166, The find command in the assignment to CRED_LEAKS uses two -name tests (AND) so it never matches; update the predicate to use OR grouping: replace the dual -name tests with a grouped expression like \( -name "*.json" -o -name "*.yaml" \) so the find starting from $BACKUP_DIR (used in the CRED_LEAKS assignment) returns JSON or YAML files before piping to grep -l; ensure you escape or quote the parentheses for the shell and keep the existing -exec grep -l "nvapi-\|sk-\|Bearer " {} \; 2>/dev/null || true behavior intact.
155-159:⚠️ Potential issue | 🟠 MajorRegistry check should assert version changed from stale value.
Current validation passes as long as
agentVersionis present, even if it is still the old version.Suggested fix
-if [ "$REGISTRY_VERSION" != "null" ] && [ "$REGISTRY_VERSION" != "error" ]; then +if [ "$REGISTRY_VERSION" != "null" ] \ + && [ "$REGISTRY_VERSION" != "error" ] \ + && [ "$REGISTRY_VERSION" != "${OLD_HERMES_VERSION}" ]; then pass "Registry agentVersion updated to ${REGISTRY_VERSION}" else fail "Registry agentVersion not updated: ${REGISTRY_VERSION}" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-hermes.sh` around lines 155 - 159, The current check only ensures REGISTRY_VERSION is set, not that it changed; capture the prior/stale value before the update (e.g., save it to PREV_REGISTRY_VERSION or STALE_REGISTRY_VERSION), then change the condition to assert REGISTRY_VERSION is neither "null" nor "error" and is different from that saved prior value (use the variables REGISTRY_VERSION and PREV_REGISTRY_VERSION in the test), and update the pass/fail messages to reflect whether the version actually changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/nemoclaw-user-workspace/SKILL.md:
- Line 3: Update the frontmatter "description" value in SKILL.md to correct
typos and grammar: replace the current string with a clear, user-facing sentence
such as "How to back up and restore OpenClaw workspace files before destructive
operations; what workspace personality and configuration files are, where they
live, and how they persist across sandbox restarts." Ensure you edit the
description key in the file (the frontmatter 'description' field) so the
corrected text is saved exactly as the new sentence.
In `@test/e2e/test-rebuild-hermes.sh`:
- Around line 164-173: The backup-dir credential check currently skips if
BACKUP_DIR is missing and uses an incorrect AND find test; change the logic so
that if BACKUP_DIR does not exist you call fail (e.g., fail "Missing backup dir:
$BACKUP_DIR"), and fix the find invocation that assigns CRED_LEAKS to use OR for
json/yaml (use grouping like \( -name "*.json" -o -name "*.yaml" \)) so files of
either type are scanned, then preserve the grep -l check and the existing
pass/fail on CRED_LEAKS; update only the block around BACKUP_DIR/CRED_LEAKS and
the find command and add the else branch that fails when the directory is
absent.
---
Duplicate comments:
In `@test/e2e/test-rebuild-hermes.sh`:
- Around line 143-145: The test currently only logs NEW_VERSION after rebuilding
Hermes; change it to assert expected result: capture NEW_VERSION from the
openshell call (the variable NEW_VERSION) and add an assertion that it matches
the expected version or is non-empty (e.g., compare against
EXPECTED_HERMES_VERSION or fail if NEW_VERSION is empty), using the test's
existing assertion helper or exit with non-zero/ci-failure if the check fails;
update the info call to remain but ensure the script fails when the version
check (NEW_VERSION vs EXPECTED_HERMES_VERSION) does not pass.
- Around line 77-80: Ensure the script verifies that OLD_VERSION_CHECK actually
contains a version before marking the old sandbox build as passed: after
assigning OLD_VERSION_CHECK (the variable) run a conditional that checks it is
non-empty (and/or does not match the current expected version) and call a
failing helper (or exit non-zero) if the check fails; only call pass "Old Hermes
sandbox image built" when OLD_VERSION_CHECK is valid, and keep the info "Old
Hermes image version: ${OLD_VERSION_CHECK}" for context.
- Line 166: The find command in the assignment to CRED_LEAKS uses two -name
tests (AND) so it never matches; update the predicate to use OR grouping:
replace the dual -name tests with a grouped expression like \( -name "*.json" -o
-name "*.yaml" \) so the find starting from $BACKUP_DIR (used in the CRED_LEAKS
assignment) returns JSON or YAML files before piping to grep -l; ensure you
escape or quote the parentheses for the shell and keep the existing -exec grep
-l "nvapi-\|sk-\|Bearer " {} \; 2>/dev/null || true behavior intact.
- Around line 155-159: The current check only ensures REGISTRY_VERSION is set,
not that it changed; capture the prior/stale value before the update (e.g., save
it to PREV_REGISTRY_VERSION or STALE_REGISTRY_VERSION), then change the
condition to assert REGISTRY_VERSION is neither "null" nor "error" and is
different from that saved prior value (use the variables REGISTRY_VERSION and
PREV_REGISTRY_VERSION in the test), and update the pass/fail messages to reflect
whether the version actually changed.
🪄 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: Pro Plus
Run ID: 44e16821-21e1-44cc-9f1a-abf7930465d9
📒 Files selected for processing (5)
.agents/skills/nemoclaw-user-reference/references/commands.md.agents/skills/nemoclaw-user-workspace/SKILL.md.agents/skills/nemoclaw-user-workspace/references/workspace-files.mdtest/e2e/test-rebuild-hermes.shtest/e2e/test-rebuild-openclaw.sh
✅ Files skipped from review due to trivial changes (1)
- .agents/skills/nemoclaw-user-reference/references/commands.md
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/test-rebuild-openclaw.sh
- Fix .includes() → .has() for Set returned by parseLiveSandboxNames - Set NEMOCLAW_SANDBOX_NAME env + update session before onboard resume so rebuild recreates with the same sandbox name - Rewrite E2E tests: install via install.sh, build old base images with patched blueprint min version, create old sandboxes via openshell directly, then rebuild and verify marker files survive - Verified locally: full OpenClaw 2026.3.11→2026.4.2 rebuild lifecycle Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (9)
test/e2e/test-rebuild-hermes.sh (4)
219-228:⚠️ Potential issue | 🟠 MajorCredential check silently skips when backup directory is missing.
Line 222's
findcommand uses correct OR logic for file extensions. However, ifBACKUP_DIRdoesn't exist, the check is silently skipped. The rebuild should always create a backup, so a missing directory is an unexpected failure.Proposed fix
# No credentials in backup BACKUP_DIR="$HOME/.nemoclaw/rebuild-backups/${SANDBOX_NAME}" if [ -d "$BACKUP_DIR" ]; then CRED_LEAKS=$(find "$BACKUP_DIR" \( -name "*.json" -o -name "*.yaml" \) -exec grep -l "nvapi-\|sk-\|Bearer " {} \; 2>/dev/null || true) if [ -z "$CRED_LEAKS" ]; then pass "No credentials in backup" else fail "Credentials found: $CRED_LEAKS" fi +else + fail "Expected backup directory not found: $BACKUP_DIR" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-hermes.sh` around lines 219 - 228, The script currently skips the credential check when BACKUP_DIR (constructed from SANDBOX_NAME) doesn't exist; treat that as a failure instead of silently skipping by adding an explicit check after computing BACKUP_DIR: if the directory is missing, call fail with a clear message (e.g., "Backup directory missing: $BACKUP_DIR") so the test fails fast; keep the existing credential grep logic (CRED_LEAKS) and pass/fail branches for the content check in place.
205-217:⚠️ Potential issue | 🟡 MinorRegistry check missing error handling.
Line 213 doesn't check for
"error"from the Python script. If the registry read fails,"error"passes the condition and reports success incorrectly.Proposed fix
REGISTRY_VERSION=$(python3 -c " import json with open('${REGISTRY_FILE}') as f: data = json.load(f) sb = data.get('sandboxes', {}).get('${SANDBOX_NAME}', {}) print(sb.get('agentVersion', 'null')) " 2>/dev/null || echo "error") -if [ "$REGISTRY_VERSION" != "null" ] && [ "$REGISTRY_VERSION" != "2026.3.12" ]; then +if [ "$REGISTRY_VERSION" != "null" ] \ + && [ "$REGISTRY_VERSION" != "error" ] \ + && [ "$REGISTRY_VERSION" != "2026.3.12" ]; then pass "Registry agentVersion updated to ${REGISTRY_VERSION}" else fail "Registry agentVersion not updated: ${REGISTRY_VERSION}" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-hermes.sh` around lines 205 - 217, The registry check currently assigns REGISTRY_VERSION from a subshell python3 invocation and then only compares against "null" and "2026.3.12", so a failed python invocation that returned "error" will be treated as success; update the check that reads REGISTRY_VERSION (derived from the python3 block using REGISTRY_FILE and SANDBOX_NAME) to explicitly detect the "error" sentinel and treat it as a failure (call fail with a clear message) before the existing comparisons, or incorporate "error" into the conditional that triggers fail; ensure the process distinguishes "error" from valid registry values and logs the REGISTRY_VERSION when failing.
93-100:⚠️ Potential issue | 🟠 MajorOld version precondition is not verified.
The docker build may succeed, but there's no verification that the resulting image actually contains
OLD_HERMES_VERSION. Add a check after the build to confirm the version.Proposed fix
docker build \ --build-arg "HERMES_VERSION=${OLD_HERMES_VERSION}" \ -f "${REPO_ROOT}/agents/hermes/Dockerfile.base" \ -t "${OLD_BASE_TAG}" \ "${REPO_ROOT}" \ || fail "Failed to build old Hermes base image" -pass "Old Hermes base image built (${OLD_HERMES_VERSION})" +# Verify the image actually has the old version +OLD_VERSION_CHECK=$(docker run --rm "${OLD_BASE_TAG}" hermes --version 2>/dev/null || true) +if echo "${OLD_VERSION_CHECK}" | grep -q "${OLD_HERMES_VERSION}"; then + pass "Old Hermes base image built (${OLD_HERMES_VERSION})" +else + fail "Expected Hermes ${OLD_HERMES_VERSION} in old image, got: ${OLD_VERSION_CHECK}" +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-hermes.sh` around lines 93 - 100, After building the old Hermes base image, add a verification step that runs the image and checks its reported version: use docker run --rm "${OLD_BASE_TAG}" <version-cmd> (e.g., hermes --version or the binary/entrypoint that prints version) and test the output contains "${OLD_HERMES_VERSION}"; if the check fails, call fail with an explanatory message. Reference the variables OLD_BASE_TAG and OLD_HERMES_VERSION and the existing fail helper so the script aborts when the image does not actually contain the expected version.
194-217:⚠️ Potential issue | 🟠 MajorMissing post-rebuild Hermes version verification.
The test verifies the marker file survived and the registry was updated, but doesn't verify that
hermes --versioninside the sandbox actually shows a different (newer) version after rebuild. This is the core upgrade assertion.Proposed fix - add version check after marker verification
# Marker file survived RESTORED=$(openshell sandbox exec --name "${SANDBOX_NAME}" -- cat "${MARKER_FILE}" 2>/dev/null || true) if [ "$RESTORED" = "${MARKER_CONTENT}" ]; then pass "Marker file survived rebuild" else fail "Marker file lost: got '${RESTORED}', expected '${MARKER_CONTENT}'" fi +# Version upgraded +NEW_VERSION=$(openshell sandbox exec --name "${SANDBOX_NAME}" -- hermes --version 2>&1 || true) +if [ -n "${NEW_VERSION}" ] && ! echo "${NEW_VERSION}" | grep -q "${OLD_HERMES_VERSION}"; then + pass "Hermes version upgraded: ${NEW_VERSION}" +else + fail "Hermes version still old after rebuild: ${NEW_VERSION}" +fi + # Registry updated🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-hermes.sh` around lines 194 - 217, Add a post-rebuild check that runs hermes --version inside the sandbox and asserts it changed to the rebuilt version: after the existing marker and registry checks, run openshell sandbox exec --name "${SANDBOX_NAME}" -- hermes --version (capture output to, e.g., NEW_HERMES_VERSION), compare it against the pre-rebuild/registry value (or ensure it differs from a known old value) and call pass "Hermes binary upgraded to ${NEW_HERMES_VERSION}" or fail "Hermes version not updated: ${NEW_HERMES_VERSION}" accordingly; reference the existing variables SANDBOX_NAME, REGISTRY_FILE/REGISTRY_VERSION, MARKER_FILE/MARKER_CONTENT to determine expected vs actual.test/e2e/test-rebuild-openclaw.sh (4)
137-141:⚠️ Potential issue | 🟡 MinorVersion precondition check does not fail on mismatch.
Line 139 only logs an info message if the sandbox version doesn't match
OLD_OPENCLAW_VERSION. This allows the test to pass without actually validating the stale-version scenario the test is designed to reproduce. Consider failing if the version doesn't match.Proposed fix
# Verify old version SANDBOX_VERSION=$(openshell sandbox exec --name "${SANDBOX_NAME}" -- openclaw --version 2>&1 || true) -echo "${SANDBOX_VERSION}" | grep -q "${OLD_OPENCLAW_VERSION}" || info "Version: ${SANDBOX_VERSION}" +if echo "${SANDBOX_VERSION}" | grep -q "${OLD_OPENCLAW_VERSION}"; then + pass "Sandbox confirmed running OpenClaw ${OLD_OPENCLAW_VERSION}" +else + fail "Expected OpenClaw ${OLD_OPENCLAW_VERSION}, got: ${SANDBOX_VERSION}" +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-openclaw.sh` around lines 137 - 141, The version precondition currently only logs info when SANDBOX_VERSION doesn't match OLD_OPENCLAW_VERSION, so change it to fail the test on mismatch: after computing SANDBOX_VERSION, replace the soft-check that calls info on mismatch with a hard failure that prints the actual and expected versions and exits non‑zero (or calls the test framework's fail helper) when the grep against OLD_OPENCLAW_VERSION fails; update the block that references SANDBOX_VERSION, OLD_OPENCLAW_VERSION and the info/pass flow so a version mismatch stops the script instead of merely logging.
224-236:⚠️ Potential issue | 🟠 MajorRegistry check treats parser errors as success.
Line 232 checks for
"null"andOLD_OPENCLAW_VERSIONbut not"error". If the Python script fails (line 231 echoes"error"), the condition passes and incorrectly reports the registry was updated.Proposed fix
REGISTRY_VERSION=$(python3 -c " import json with open('${REGISTRY_FILE}') as f: data = json.load(f) sb = data.get('sandboxes', {}).get('${SANDBOX_NAME}', {}) print(sb.get('agentVersion', 'null')) " 2>/dev/null || echo "error") -if [ "$REGISTRY_VERSION" != "null" ] && [ "$REGISTRY_VERSION" != "${OLD_OPENCLAW_VERSION}" ]; then +if [ "$REGISTRY_VERSION" != "null" ] \ + && [ "$REGISTRY_VERSION" != "error" ] \ + && [ "$REGISTRY_VERSION" != "${OLD_OPENCLAW_VERSION}" ]; then pass "Registry agentVersion updated to ${REGISTRY_VERSION}" else fail "Registry agentVersion not updated: ${REGISTRY_VERSION}" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-openclaw.sh` around lines 224 - 236, The registry check currently treats the python fallback string "error" as a valid update; update the conditional that evaluates REGISTRY_VERSION so it also rejects the "error" sentinel (i.e., ensure [ "$REGISTRY_VERSION" != "error" ] is included alongside the existing checks) so that failures from the python3 block are treated as test failures; modify the if that compares REGISTRY_VERSION and OLD_OPENCLAW_VERSION to explicitly exclude "error" (and keep the existing "null" check) so pass is only called for a real version value.
238-247:⚠️ Potential issue | 🟠 MajorCredential check silently skips when backup directory is missing.
If
BACKUP_DIRdoesn't exist, the test continues without failing. Since the rebuild should always create a backup, a missing directory indicates an unexpected failure that should fail the test.Proposed fix
# No credentials in backup BACKUP_DIR="$HOME/.nemoclaw/rebuild-backups/${SANDBOX_NAME}" if [ -d "$BACKUP_DIR" ]; then CRED_LEAKS=$(find "$BACKUP_DIR" -name "*.json" -exec grep -l "nvapi-\|sk-\|Bearer " {} \; 2>/dev/null || true) if [ -z "$CRED_LEAKS" ]; then pass "No credentials in backup" else fail "Credentials found: $CRED_LEAKS" fi +else + fail "Expected backup directory not found: $BACKUP_DIR" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-openclaw.sh` around lines 238 - 247, The test silently skips credential checks when BACKUP_DIR (constructed from SANDBOX_NAME) is missing; update the test-rebuild-openclaw.sh logic to treat a missing BACKUP_DIR as a test failure by explicitly checking if [ -d "$BACKUP_DIR" ] is false and calling fail with a clear message (e.g., "Backup directory missing: $BACKUP_DIR") before the current credential-grep block that sets CRED_LEAKS, so the absence of the expected rebuild backup fails the test instead of being ignored.
216-222:⚠️ Potential issue | 🟠 MajorVersion upgrade check passes on empty output.
Line 218 uses
grep -qvwhich succeeds on empty input because there's nothing matchingOLD_OPENCLAW_VERSION. Ifopenclaw --versionfails or returns empty, the test incorrectly reports success.Proposed fix
# Version upgraded NEW_VERSION=$(openshell sandbox exec --name "${SANDBOX_NAME}" -- openclaw --version 2>&1 || true) -if echo "${NEW_VERSION}" | grep -qv "${OLD_OPENCLAW_VERSION}"; then +if [ -n "${NEW_VERSION}" ] && ! echo "${NEW_VERSION}" | grep -q "${OLD_OPENCLAW_VERSION}"; then pass "OpenClaw version upgraded: ${NEW_VERSION}" else fail "Version still old: ${NEW_VERSION}" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-openclaw.sh` around lines 216 - 222, The test currently treats empty NEW_VERSION as success because grep -qv on empty input matches; update the check around NEW_VERSION (the variable set by the openshell exec of openclaw --version) to first verify the command produced non-empty output (and/or succeeded) and then verify it differs from OLD_OPENCLAW_VERSION; in practice change the if condition that uses NEW_VERSION and grep to require [ -n "$NEW_VERSION" ] (or check the command exit status) AND that echo "$NEW_VERSION" does not contain OLD_OPENCLAW_VERSION (using grep -q or a direct string comparison) before calling pass, otherwise call fail. Ensure you reference the NEW_VERSION assignment and the if block that currently contains grep -qv to implement these checks.src/nemoclaw.ts (1)
1581-1599:⚠️ Potential issue | 🟠 MajorSuccess banner prints even after partial restore failure.
When
restore.successis false (lines 1581-1587), the code logs errors but then unconditionally prints the success banner at lines 1595-1599. This misleads users and scripts into thinking the rebuild fully succeeded when workspace state may be partially lost.Proposed fix
const restore = sandboxState.restoreSandboxState(sandboxName, backup.manifest.backupPath); if (!restore.success) { console.error(` Partial restore: ${restore.restoredDirs.join(", ") || "none"}`); console.error(` Failed: ${restore.failedDirs.join(", ")}`); console.error(` Manual restore available from: ${backup.manifest.backupPath}`); + console.error(""); + console.error(` Sandbox '${sandboxName}' rebuilt but restore incomplete.`); + process.exit(1); } else { console.log(` ${G}\u2713${R} State restored (${restore.restoredDirs.length} directories)`); - } - - // Step 6: Update registry with new version - const agentDef = agent ? require("./lib/agent-defs").loadAgent(agent.name) : require("./lib/agent-defs").loadAgent("openclaw"); - registry.updateSandbox(sandboxName, { - agentVersion: agentDef.expectedVersion || null, - }); - console.log(""); - console.log(` ${G}\u2713${R} Sandbox '${sandboxName}' rebuilt successfully`); - if (versionCheck.expectedVersion) { - console.log(` Now running: ${agentName} v${versionCheck.expectedVersion}`); + // Step 6: Update registry with new version + const agentDef = agent ? require("./lib/agent-defs").loadAgent(agent.name) : require("./lib/agent-defs").loadAgent("openclaw"); + registry.updateSandbox(sandboxName, { + agentVersion: agentDef.expectedVersion || null, + }); + + console.log(""); + console.log(` ${G}\u2713${R} Sandbox '${sandboxName}' rebuilt successfully`); + if (versionCheck.expectedVersion) { + console.log(` Now running: ${agentName} v${versionCheck.expectedVersion}`); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 1581 - 1599, The success banner and version message are printed unconditionally even when restore.success is false; move the console.log of the success banner (`console.log(` ${G}\u2713${R} Sandbox '${sandboxName}' rebuilt successfully`)`) and the conditional version message (`if (versionCheck.expectedVersion) { console.log(... agentName ...) }`) so they only run when `restore.success` is true (i.e., inside the else branch where you already log the positive restore message). Ensure you do not change the error logging for the partial restore (`restore.restoredDirs`, `restore.failedDirs`, `backup.manifest.backupPath`) and keep `registry.updateSandbox(sandboxName, { agentVersion: agentDef.expectedVersion || null, })` placement as intended unless you explicitly decide to gate registry updates on `restore.success`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/nemoclaw.ts`:
- Around line 1581-1599: The success banner and version message are printed
unconditionally even when restore.success is false; move the console.log of the
success banner (`console.log(` ${G}\u2713${R} Sandbox '${sandboxName}' rebuilt
successfully`)`) and the conditional version message (`if
(versionCheck.expectedVersion) { console.log(... agentName ...) }`) so they only
run when `restore.success` is true (i.e., inside the else branch where you
already log the positive restore message). Ensure you do not change the error
logging for the partial restore (`restore.restoredDirs`, `restore.failedDirs`,
`backup.manifest.backupPath`) and keep `registry.updateSandbox(sandboxName, {
agentVersion: agentDef.expectedVersion || null, })` placement as intended unless
you explicitly decide to gate registry updates on `restore.success`.
In `@test/e2e/test-rebuild-hermes.sh`:
- Around line 219-228: The script currently skips the credential check when
BACKUP_DIR (constructed from SANDBOX_NAME) doesn't exist; treat that as a
failure instead of silently skipping by adding an explicit check after computing
BACKUP_DIR: if the directory is missing, call fail with a clear message (e.g.,
"Backup directory missing: $BACKUP_DIR") so the test fails fast; keep the
existing credential grep logic (CRED_LEAKS) and pass/fail branches for the
content check in place.
- Around line 205-217: The registry check currently assigns REGISTRY_VERSION
from a subshell python3 invocation and then only compares against "null" and
"2026.3.12", so a failed python invocation that returned "error" will be treated
as success; update the check that reads REGISTRY_VERSION (derived from the
python3 block using REGISTRY_FILE and SANDBOX_NAME) to explicitly detect the
"error" sentinel and treat it as a failure (call fail with a clear message)
before the existing comparisons, or incorporate "error" into the conditional
that triggers fail; ensure the process distinguishes "error" from valid registry
values and logs the REGISTRY_VERSION when failing.
- Around line 93-100: After building the old Hermes base image, add a
verification step that runs the image and checks its reported version: use
docker run --rm "${OLD_BASE_TAG}" <version-cmd> (e.g., hermes --version or the
binary/entrypoint that prints version) and test the output contains
"${OLD_HERMES_VERSION}"; if the check fails, call fail with an explanatory
message. Reference the variables OLD_BASE_TAG and OLD_HERMES_VERSION and the
existing fail helper so the script aborts when the image does not actually
contain the expected version.
- Around line 194-217: Add a post-rebuild check that runs hermes --version
inside the sandbox and asserts it changed to the rebuilt version: after the
existing marker and registry checks, run openshell sandbox exec --name
"${SANDBOX_NAME}" -- hermes --version (capture output to, e.g.,
NEW_HERMES_VERSION), compare it against the pre-rebuild/registry value (or
ensure it differs from a known old value) and call pass "Hermes binary upgraded
to ${NEW_HERMES_VERSION}" or fail "Hermes version not updated:
${NEW_HERMES_VERSION}" accordingly; reference the existing variables
SANDBOX_NAME, REGISTRY_FILE/REGISTRY_VERSION, MARKER_FILE/MARKER_CONTENT to
determine expected vs actual.
In `@test/e2e/test-rebuild-openclaw.sh`:
- Around line 137-141: The version precondition currently only logs info when
SANDBOX_VERSION doesn't match OLD_OPENCLAW_VERSION, so change it to fail the
test on mismatch: after computing SANDBOX_VERSION, replace the soft-check that
calls info on mismatch with a hard failure that prints the actual and expected
versions and exits non‑zero (or calls the test framework's fail helper) when the
grep against OLD_OPENCLAW_VERSION fails; update the block that references
SANDBOX_VERSION, OLD_OPENCLAW_VERSION and the info/pass flow so a version
mismatch stops the script instead of merely logging.
- Around line 224-236: The registry check currently treats the python fallback
string "error" as a valid update; update the conditional that evaluates
REGISTRY_VERSION so it also rejects the "error" sentinel (i.e., ensure [
"$REGISTRY_VERSION" != "error" ] is included alongside the existing checks) so
that failures from the python3 block are treated as test failures; modify the if
that compares REGISTRY_VERSION and OLD_OPENCLAW_VERSION to explicitly exclude
"error" (and keep the existing "null" check) so pass is only called for a real
version value.
- Around line 238-247: The test silently skips credential checks when BACKUP_DIR
(constructed from SANDBOX_NAME) is missing; update the test-rebuild-openclaw.sh
logic to treat a missing BACKUP_DIR as a test failure by explicitly checking if
[ -d "$BACKUP_DIR" ] is false and calling fail with a clear message (e.g.,
"Backup directory missing: $BACKUP_DIR") before the current credential-grep
block that sets CRED_LEAKS, so the absence of the expected rebuild backup fails
the test instead of being ignored.
- Around line 216-222: The test currently treats empty NEW_VERSION as success
because grep -qv on empty input matches; update the check around NEW_VERSION
(the variable set by the openshell exec of openclaw --version) to first verify
the command produced non-empty output (and/or succeeded) and then verify it
differs from OLD_OPENCLAW_VERSION; in practice change the if condition that uses
NEW_VERSION and grep to require [ -n "$NEW_VERSION" ] (or check the command exit
status) AND that echo "$NEW_VERSION" does not contain OLD_OPENCLAW_VERSION
(using grep -q or a direct string comparison) before calling pass, otherwise
call fail. Ensure you reference the NEW_VERSION assignment and the if block that
currently contains grep -qv to implement these checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 60984664-81d9-451f-8545-4d151c2714c6
📒 Files selected for processing (3)
src/nemoclaw.tstest/e2e/test-rebuild-hermes.shtest/e2e/test-rebuild-openclaw.sh
…interactive shell hang Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…m/NVIDIA/NemoClaw into fix/sandbox-rebuild-stale-version
sandboxDestroy() cleans up the gateway when destroying the last sandbox and nulls session.sandboxName — both break the immediate onboard --resume that follows. Replace with inline sandbox deletion that preserves gateway and session state. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…d E2E - Use openshell sandbox delete (not nemoclaw destroy) in E2E Phase 1 to preserve the nemoclaw gateway for the rebuild step - Pass --gateway nemoclaw to openshell sandbox create so the old sandbox uses the same gateway onboard --resume expects - Add NEMOCLAW_REBUILD_VERBOSE=1 mode: logs SSH commands, exit codes, session state, registry mutations, and backup/restore details - Add [DIAG] dumps in E2E tests: registry, session, live sandboxes, gateway status at each phase boundary - Add failure diagnostics dump on E2E test failure Verified locally from clean slate (no gateway, no sandboxes): OpenClaw 2026.3.11 → 2026.4.2, marker file survived, credentials stripped, registry updated. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
… chain Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…ion structure repair Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…ll.sh - Add `nemoclaw backup-all` command that backs up all registered live sandboxes to ~/.nemoclaw/rebuild-backups/ - Hook into install.sh: before onboarding (which may upgrade OpenShell), check registry for existing sandboxes and back them up - Protects against sandbox data loss if an OpenShell upgrade destroys container contents (the OpenShell team cannot guarantee persistence across major version changes) - Registry check uses python3 on the JSON file directly to avoid calling nemoclaw stubs in test environments Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…m/NVIDIA/NemoClaw into fix/sandbox-rebuild-stale-version
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (13)
src/lib/onboard.ts (1)
2828-2833:⚠️ Potential issue | 🟠 MajorDon't cache
expectedVersionfor custom--frombuilds.
fromDockerfileimages can carry any agent version. Recording the manifest'sexpectedVersionhere will generate false stale-version warnings and can push users toward an unnecessary rebuild.Suggested fix
const effectiveAgent = agent || agentDefs.loadAgent("openclaw"); + const cachedAgentVersion = fromDockerfile + ? null + : (effectiveAgent.expectedVersion || null); registry.registerSandbox({ name: sandboxName, gpuEnabled: !!gpu, agent: agent ? agent.name : null, - agentVersion: effectiveAgent.expectedVersion || null, + agentVersion: cachedAgentVersion, dangerouslySkipPermissions: dangerouslySkipPermissions || undefined, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 2828 - 2833, The code currently records effectiveAgent.expectedVersion into registry.registerSandbox even for custom `--from` builds; change this so agentVersion is null when a custom agent was supplied (i.e., when the local `agent` parameter is present). Concretely, in the block that calls registry.registerSandbox (around effectiveAgent, agent, agentDefs.loadAgent), set agentVersion to effectiveAgent.expectedVersion only when agent is falsy (the loaded agent), and otherwise pass null to avoid caching expectedVersion for `--from`/fromDockerfile images.src/lib/sandbox-state.ts (3)
187-189:⚠️ Potential issue | 🟠 MajorDon't collapse "no state dirs" into a normal backup success.
success: truehere is indistinguishable from a real backup. Callers cannot tell whether the agent is intentionally stateless or whether the manifest accidentally dropped its state paths before a destructive rebuild.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sandbox-state.ts` around lines 187 - 189, When stateDirs.length === 0, do not return success: true; instead still call writeManifest(backupPath, manifest) but return a non-success result so callers can differentiate an intentional stateless case from a successful backup. Update the return from the block that currently returns { success: true, manifest, backedUpDirs, failedDirs } to return { success: false, manifest, backedUpDirs, failedDirs, noStateDirs: true } or include an explicit error/reason field; adjust callers of this function to handle the new flag/false success as needed. Ensure you modify the code where stateDirs, writeManifest, backupPath, manifest, backedUpDirs, and failedDirs are referenced.
126-139:⚠️ Potential issue | 🔴 CriticalCredential stripping still misses non-JSON configs.
This walk only sanitizes
*.json. YAML,.env, and other config formats copied out ofstateDirswill survive into the rebuild backup unless their basename happens to be on the delete list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sandbox-state.ts` around lines 126 - 139, sanitizeBackupDirectory currently only calls sanitizeConfigFile for files ending with ".json", so non-JSON configs (YAML, .env, TOML, INI, etc.) are left untouched; update sanitizeBackupDirectory to treat common config extensions (.yaml, .yml, .env, .toml, .ini, and .json) the same way it treats ".json" and call sanitizeConfigFile(fullPath) for those files, while still deleting sensitive basenames via isSensitiveFile(entry.name); ensure sanitizeConfigFile can accept and safely process these additional extensions (or perform a generic credential-stripping pass) so all copied configs are sanitized before backing up.
227-242:⚠️ Potential issue | 🟠 MajorStream the tar transfer instead of buffering it in
stdout.Both backup and restore materialize the full archive in memory before piping it onward. Larger sandboxes will hit the 256 MB cap or spike memory even though SSH and
tarcan be connected stream-to-stream withspawn().Also applies to: 311-325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sandbox-state.ts` around lines 227 - 242, The code currently uses spawnSync and buffers the entire tar stream into memory (see tarCmd, sshArgs, backupPath and spawnSync usages); change to stream-to-stream processing by using spawn(...) for the SSH process and piping its stdout into a spawned local tar extraction process (spawn("tar", ["-xf","-","-C", backupPath])) instead of reading result.stdout, attach listeners to capture exit codes and stderr data for logging, propagate errors and timeouts via timers or child.kill(), and remove maxBuffer usage; apply the same streaming replacement for the corresponding restore path (the similar spawnSync block referenced in the comment).test/e2e/test-rebuild-hermes.sh (4)
251-260:⚠️ Potential issue | 🟡 MinorTreat a missing backup directory as a test failure.
Skipping the leak scan when
${BACKUP_DIR}doesn't exist lets this E2E pass without checking the credential-sanitization artifact at all.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-hermes.sh` around lines 251 - 260, The test currently skips credential scanning when BACKUP_DIR (constructed from SANDBOX_NAME) is missing; change the logic so a missing backup directory is treated as a test failure: detect if BACKUP_DIR does not exist and call fail with a clear message (e.g., "Backup directory missing: $BACKUP_DIR"), otherwise proceed to run the credential leak grep and use pass/fail based on CRED_LEAKS; update the conditional around BACKUP_DIR and ensure the pass and fail calls remain for the leak check in the existing block.
237-249:⚠️ Potential issue | 🟠 MajorReject empty/error registry reads here too.
REGISTRY_VERSION="error"or""still satisfies the current condition, so a failed JSON read can look like a successful version update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-hermes.sh` around lines 237 - 249, The check around REGISTRY_VERSION (set by the python3 snippet reading REGISTRY_FILE and SANDBOX_NAME) currently treats "error" or empty string as valid; update the if condition that compares REGISTRY_VERSION to also reject "error" and empty values (and still reject "null") before accepting the expected version "2026.3.12". In practice modify the if in the if [ "$REGISTRY_VERSION" != "null" ] && [ "$REGISTRY_VERSION" != "2026.3.12" ] block to explicitly check that REGISTRY_VERSION is not "error" and not "" (e.g. add checks [ "$REGISTRY_VERSION" != "error" ] && [ -n "$REGISTRY_VERSION" ] or validate with a semantic-version regex) so failed JSON reads are treated as failures.
133-142:⚠️ Potential issue | 🟠 MajorAssert the sandbox is actually running the old Hermes version before rebuild.
Phase 3 only waits for
Readyand then passes. Without ahermes --versioncheck here, the test can miss the stale-version scenario entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-hermes.sh` around lines 133 - 142, After the sandbox becomes Ready, verify it is running the old Hermes binary by executing hermes --version inside the sandbox and asserting the output matches the expected old-version string; use openshell sandbox exec "${SANDBOX_NAME}" -- hermes --version (or equivalent) and grep -q against an expected variable like OLD_HERMES_VERSION (or HERMES_OLD_VERSION), and call fail "Sandbox is not running old Hermes" if the version check does not pass—place this check immediately after the Ready assertion that uses openshell sandbox list and before passing "Old Hermes sandbox created".
212-235:⚠️ Potential issue | 🟠 MajorVerify the sandbox version changed after rebuild, not just the registry metadata.
This phase never checks
hermes --versioninside the rebuilt sandbox. The E2E can pass even if onlyagentVersioninsandboxes.jsonchanged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-hermes.sh` around lines 212 - 235, The verify phase only inspects registry metadata and not the actual binary version inside the rebuilt sandbox; capture the sandbox's runtime hermes version before rebuild and then after rebuild by running hermes --version via openshell sandbox exec --name "${SANDBOX_NAME}" and assert the two outputs differ (fail if unchanged) to ensure the rebuilt sandbox binary actually changed rather than only agentVersion in sandboxes.json; add this check alongside the existing marker and inference verifications in Phase 7.src/nemoclaw.ts (1)
1627-1666:⚠️ Potential issue | 🟠 MajorStop the rebuild when restore is partial or failed.
When
restore.successis false, this flow still runs the post-restore migration, updatesagentVersion, prints a success banner, and exits 0. That turns a data-loss path into a reported success for both users and scripts.Proposed fix
const restore = sandboxState.restoreSandboxState(sandboxName, backup.manifest.backupPath); log(`Restore result: success=${restore.success}, restored=${restore.restoredDirs.join(",")}, failed=${restore.failedDirs.join(",")}`); if (!restore.success) { console.error(` Partial restore: ${restore.restoredDirs.join(", ") || "none"}`); console.error(` Failed: ${restore.failedDirs.join(", ")}`); console.error(` Manual restore available from: ${backup.manifest.backupPath}`); - } else { - console.log(` ${G}\u2713${R} State restored (${restore.restoredDirs.length} directories)`); + process.exit(1); } + console.log(` ${G}\u2713${R} State restored (${restore.restoredDirs.length} directories)`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 1627 - 1666, The flow continues past a failed or partial restore (restore.success === false) and proceeds to post-restore migration (executeSandboxCommand/doctor), registry.updateSandbox, and prints a success banner; change this by making the function abort immediately when restore.success is false — after logging the existing error messages for restore.restoredDirs/failedDirs/backup.manifest.backupPath, return or call process.exit(1) (or throw) to stop further steps such as calling require(...).loadAgent, executeSandboxCommand(sandboxName,...), registry.updateSandbox(...), and printing the success banner/agent version (versionCheck/agentName).test/e2e/test-rebuild-openclaw.sh (4)
152-156:⚠️ Potential issue | 🟠 MajorFail fast if the sandbox didn't start on the pinned old OpenClaw version.
This branch only logs the mismatch and still reports success, so the rebuild can pass without ever exercising the stale-version upgrade path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-openclaw.sh` around lines 152 - 156, The script currently logs a version mismatch but still reports success; change the check around SANDBOX_VERSION (obtained via openshell sandbox exec --name "${SANDBOX_NAME}" -- openclaw --version") so that if grep -q does not find OLD_OPENCLAW_VERSION it exits non‑zero instead of calling info; replace the "echo ... | grep -q ... || info ..." behavior with an immediate failure path (e.g., log an error with context including SANDBOX_NAME and SANDBOX_VERSION and exit 1) so the test fails fast and does not reach pass "Old sandbox created (OpenClaw ${OLD_OPENCLAW_VERSION})".
247-259:⚠️ Potential issue | 🟠 MajorReject parser/runtime failures in the registry assertion.
REGISTRY_VERSION=""or"error"currently passes this check, so a failed JSON read can be reported as a successful upgrade.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-openclaw.sh` around lines 247 - 259, The registry assertion is too lax: REGISTRY_VERSION can be empty or "error" and still pass; update the shell check around REGISTRY_VERSION (the variable set by the python snippet that reads REGISTRY_FILE for SANDBOX_NAME) to explicitly reject empty string, the literal "error", and "null" before comparing to OLD_OPENCLAW_VERSION, and only call pass when REGISTRY_VERSION is non-empty/non-error/non-null and differs from OLD_OPENCLAW_VERSION; adjust the if condition that currently inspects REGISTRY_VERSION to include these negative checks so failed JSON reads are treated as failures.
239-245:⚠️ Potential issue | 🟠 MajorRequire a real post-rebuild version before marking upgrade success.
NEW_VERSIONcan be empty or just error output and still satisfy this condition. Require non-empty version output and a real difference fromOLD_OPENCLAW_VERSIONbefore passing the test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-openclaw.sh` around lines 239 - 245, The current test treats any NEW_VERSION (including empty or error text) as success; update the post-rebuild check so NEW_VERSION is non-empty and contains a real version string different from OLD_OPENCLAW_VERSION before calling pass. Specifically, after capturing NEW_VERSION from the openshell sandbox exec invocation, ensure you test that NEW_VERSION is non-empty (e.g., test -n), does not contain common error markers (e.g., "error" or "not found"), and that NEW_VERSION differs from OLD_OPENCLAW_VERSION (use a string-inequality check rather than a simple grep inversion) before pass; otherwise call fail with the captured output for debugging.
275-284:⚠️ Potential issue | 🟡 MinorFail the credential scan when the backup directory is missing.
If rebuild never wrote
${BACKUP_DIR}, this block is skipped and the script still reports overall success for the leak-check objective.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-openclaw.sh` around lines 275 - 284, The script currently skips the credential check if BACKUP_DIR (variable BACKUP_DIR using SANDBOX_NAME) does not exist, silently passing; change the logic so missing backups cause the test to fail: replace the current if [ -d "$BACKUP_DIR" ] conditional with an explicit check that fails when the directory is absent (call the existing fail function with a clear message like "Backup directory missing: $BACKUP_DIR"), and keep the existing credential-search logic in the branch that runs when the directory exists (preserve use of CRED_LEAKS and pass/fail semantics). Ensure the failure path triggers the same test-failure behavior as when credentials are found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nemoclaw.ts`:
- Around line 1821-1823: The "backup-all" switch arm in the command dispatcher
(case "backup-all": backupAll(); break;) is never reached because
GLOBAL_COMMANDS does not include "backup-all"; add "backup-all" to the
GLOBAL_COMMANDS set/array so the CLI recognizes the command, and verify any
help/usage listing includes it; ensure the entry in GLOBAL_COMMANDS uses the
exact string "backup-all" to match the case and run the existing backupAll()
function.
In `@test/e2e/test-rebuild-hermes.sh`:
- Around line 73-74: The script currently masks failures by running bash
"${REPO_ROOT}/install.sh" --non-interactive >"$INSTALL_LOG" 2>&1 || true; change
this so install failures are detected and cause the test to stop: run the
installer without "|| true", capture its exit code ($?), and if non-zero, print
or tail "$INSTALL_LOG" and exit with that installer exit code (or use "set -e"
at top to fail fast). Ensure you still redirect output to INSTALL_LOG but remove
the unconditional ignore so failures of install.sh (referenced via REPO_ROOT and
INSTALL_LOG) break the E2E run.
In `@test/e2e/test-rebuild-openclaw.sh`:
- Around line 75-76: The test currently ignores failures from the installer
because the command bash "${REPO_ROOT}/install.sh" --non-interactive
>"$INSTALL_LOG" 2>&1 || true swallows errors; change this so the test fails when
install.sh fails by removing the "|| true" and/or explicitly checking the
installer exit code (use the INSTALL_LOG to capture output and if the installer
exit status is non-zero, print the log and exit non-zero). Update references to
INSTALL_LOG and the bash invocation to implement this: run bash
"${REPO_ROOT}/install.sh" --non-interactive >"$INSTALL_LOG" 2>&1, capture its
exit code, and fail the test when that code is non-zero.
- Around line 110-123: Wrap the blueprint backup/restore in an EXIT trap so
restoration always runs even if docker build fails: after creating BLUEPRINT_BAK
and before running docker build, install a trap 'trap "mv \"${BLUEPRINT_BAK}\"
\"${BLUEPRINT}\" || true; rm -f \"${BLUEPRINT}.tmp\" || true" EXIT' to restore
the original BLUEPRINT and remove any temp file on exit; then run the sed->tmp,
docker build, and at successful completion remove the trap (trap - EXIT) and
cleanup BLUEPRINT_BAK if desired. Ensure you reference the existing variables
BLUEPRINT, BLUEPRINT_BAK, BLUEPRINT.tmp, OLD_OPENCLAW_VERSION and BUILD_RC so
the mv and cleanup happen unconditionally even when set -euo pipefail aborts the
script.
---
Duplicate comments:
In `@src/lib/onboard.ts`:
- Around line 2828-2833: The code currently records
effectiveAgent.expectedVersion into registry.registerSandbox even for custom
`--from` builds; change this so agentVersion is null when a custom agent was
supplied (i.e., when the local `agent` parameter is present). Concretely, in the
block that calls registry.registerSandbox (around effectiveAgent, agent,
agentDefs.loadAgent), set agentVersion to effectiveAgent.expectedVersion only
when agent is falsy (the loaded agent), and otherwise pass null to avoid caching
expectedVersion for `--from`/fromDockerfile images.
In `@src/lib/sandbox-state.ts`:
- Around line 187-189: When stateDirs.length === 0, do not return success: true;
instead still call writeManifest(backupPath, manifest) but return a non-success
result so callers can differentiate an intentional stateless case from a
successful backup. Update the return from the block that currently returns {
success: true, manifest, backedUpDirs, failedDirs } to return { success: false,
manifest, backedUpDirs, failedDirs, noStateDirs: true } or include an explicit
error/reason field; adjust callers of this function to handle the new flag/false
success as needed. Ensure you modify the code where stateDirs, writeManifest,
backupPath, manifest, backedUpDirs, and failedDirs are referenced.
- Around line 126-139: sanitizeBackupDirectory currently only calls
sanitizeConfigFile for files ending with ".json", so non-JSON configs (YAML,
.env, TOML, INI, etc.) are left untouched; update sanitizeBackupDirectory to
treat common config extensions (.yaml, .yml, .env, .toml, .ini, and .json) the
same way it treats ".json" and call sanitizeConfigFile(fullPath) for those
files, while still deleting sensitive basenames via isSensitiveFile(entry.name);
ensure sanitizeConfigFile can accept and safely process these additional
extensions (or perform a generic credential-stripping pass) so all copied
configs are sanitized before backing up.
- Around line 227-242: The code currently uses spawnSync and buffers the entire
tar stream into memory (see tarCmd, sshArgs, backupPath and spawnSync usages);
change to stream-to-stream processing by using spawn(...) for the SSH process
and piping its stdout into a spawned local tar extraction process (spawn("tar",
["-xf","-","-C", backupPath])) instead of reading result.stdout, attach
listeners to capture exit codes and stderr data for logging, propagate errors
and timeouts via timers or child.kill(), and remove maxBuffer usage; apply the
same streaming replacement for the corresponding restore path (the similar
spawnSync block referenced in the comment).
In `@src/nemoclaw.ts`:
- Around line 1627-1666: The flow continues past a failed or partial restore
(restore.success === false) and proceeds to post-restore migration
(executeSandboxCommand/doctor), registry.updateSandbox, and prints a success
banner; change this by making the function abort immediately when
restore.success is false — after logging the existing error messages for
restore.restoredDirs/failedDirs/backup.manifest.backupPath, return or call
process.exit(1) (or throw) to stop further steps such as calling
require(...).loadAgent, executeSandboxCommand(sandboxName,...),
registry.updateSandbox(...), and printing the success banner/agent version
(versionCheck/agentName).
In `@test/e2e/test-rebuild-hermes.sh`:
- Around line 251-260: The test currently skips credential scanning when
BACKUP_DIR (constructed from SANDBOX_NAME) is missing; change the logic so a
missing backup directory is treated as a test failure: detect if BACKUP_DIR does
not exist and call fail with a clear message (e.g., "Backup directory missing:
$BACKUP_DIR"), otherwise proceed to run the credential leak grep and use
pass/fail based on CRED_LEAKS; update the conditional around BACKUP_DIR and
ensure the pass and fail calls remain for the leak check in the existing block.
- Around line 237-249: The check around REGISTRY_VERSION (set by the python3
snippet reading REGISTRY_FILE and SANDBOX_NAME) currently treats "error" or
empty string as valid; update the if condition that compares REGISTRY_VERSION to
also reject "error" and empty values (and still reject "null") before accepting
the expected version "2026.3.12". In practice modify the if in the if [
"$REGISTRY_VERSION" != "null" ] && [ "$REGISTRY_VERSION" != "2026.3.12" ] block
to explicitly check that REGISTRY_VERSION is not "error" and not "" (e.g. add
checks [ "$REGISTRY_VERSION" != "error" ] && [ -n "$REGISTRY_VERSION" ] or
validate with a semantic-version regex) so failed JSON reads are treated as
failures.
- Around line 133-142: After the sandbox becomes Ready, verify it is running the
old Hermes binary by executing hermes --version inside the sandbox and asserting
the output matches the expected old-version string; use openshell sandbox exec
"${SANDBOX_NAME}" -- hermes --version (or equivalent) and grep -q against an
expected variable like OLD_HERMES_VERSION (or HERMES_OLD_VERSION), and call fail
"Sandbox is not running old Hermes" if the version check does not pass—place
this check immediately after the Ready assertion that uses openshell sandbox
list and before passing "Old Hermes sandbox created".
- Around line 212-235: The verify phase only inspects registry metadata and not
the actual binary version inside the rebuilt sandbox; capture the sandbox's
runtime hermes version before rebuild and then after rebuild by running hermes
--version via openshell sandbox exec --name "${SANDBOX_NAME}" and assert the two
outputs differ (fail if unchanged) to ensure the rebuilt sandbox binary actually
changed rather than only agentVersion in sandboxes.json; add this check
alongside the existing marker and inference verifications in Phase 7.
In `@test/e2e/test-rebuild-openclaw.sh`:
- Around line 152-156: The script currently logs a version mismatch but still
reports success; change the check around SANDBOX_VERSION (obtained via openshell
sandbox exec --name "${SANDBOX_NAME}" -- openclaw --version") so that if grep -q
does not find OLD_OPENCLAW_VERSION it exits non‑zero instead of calling info;
replace the "echo ... | grep -q ... || info ..." behavior with an immediate
failure path (e.g., log an error with context including SANDBOX_NAME and
SANDBOX_VERSION and exit 1) so the test fails fast and does not reach pass "Old
sandbox created (OpenClaw ${OLD_OPENCLAW_VERSION})".
- Around line 247-259: The registry assertion is too lax: REGISTRY_VERSION can
be empty or "error" and still pass; update the shell check around
REGISTRY_VERSION (the variable set by the python snippet that reads
REGISTRY_FILE for SANDBOX_NAME) to explicitly reject empty string, the literal
"error", and "null" before comparing to OLD_OPENCLAW_VERSION, and only call pass
when REGISTRY_VERSION is non-empty/non-error/non-null and differs from
OLD_OPENCLAW_VERSION; adjust the if condition that currently inspects
REGISTRY_VERSION to include these negative checks so failed JSON reads are
treated as failures.
- Around line 239-245: The current test treats any NEW_VERSION (including empty
or error text) as success; update the post-rebuild check so NEW_VERSION is
non-empty and contains a real version string different from OLD_OPENCLAW_VERSION
before calling pass. Specifically, after capturing NEW_VERSION from the
openshell sandbox exec invocation, ensure you test that NEW_VERSION is non-empty
(e.g., test -n), does not contain common error markers (e.g., "error" or "not
found"), and that NEW_VERSION differs from OLD_OPENCLAW_VERSION (use a
string-inequality check rather than a simple grep inversion) before pass;
otherwise call fail with the captured output for debugging.
- Around line 275-284: The script currently skips the credential check if
BACKUP_DIR (variable BACKUP_DIR using SANDBOX_NAME) does not exist, silently
passing; change the logic so missing backups cause the test to fail: replace the
current if [ -d "$BACKUP_DIR" ] conditional with an explicit check that fails
when the directory is absent (call the existing fail function with a clear
message like "Backup directory missing: $BACKUP_DIR"), and keep the existing
credential-search logic in the branch that runs when the directory exists
(preserve use of CRED_LEAKS and pass/fail semantics). Ensure the failure path
triggers the same test-failure behavior as when credentials are found.
🪄 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: Pro Plus
Run ID: 522c40aa-1872-4bf5-aa11-6008784f4a62
📒 Files selected for processing (6)
scripts/install.shsrc/lib/onboard.tssrc/lib/sandbox-state.tssrc/nemoclaw.tstest/e2e/test-rebuild-hermes.shtest/e2e/test-rebuild-openclaw.sh
…tch, banner - Add backup-all to GLOBAL_COMMANDS so nemoclaw backup-all actually works - Strip .env file credentials in backup (Hermes stores API keys there) - Don't print success banner after failed restore - Don't stamp agentVersion when --from custom Dockerfile is used - Log warning when agent manifest has empty state_dirs Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
src/lib/sandbox-state.ts (3)
205-208:⚠️ Potential issue | 🔴 CriticalFail closed when
state_dirsis empty.Lines 205-208 currently return
success: trueafter writing only the manifest. That lets rebuild continue into sandbox deletion with no state archived at all.Suggested fix
if (stateDirs.length === 0) { _log("WARNING: Agent manifest declares no state_dirs — nothing to back up"); writeManifest(backupPath, manifest); - return { success: true, manifest, backedUpDirs, failedDirs }; + return { + success: false, + manifest, + backedUpDirs, + failedDirs: ["state_dirs"], + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sandbox-state.ts` around lines 205 - 208, The code currently treats an empty stateDirs array as a success; change the early-return in the stateDirs.length === 0 branch to fail closed: call writeManifest(backupPath, manifest) as before but log an error (use _log or existing logger), set the returned object to success: false, and populate failedDirs to indicate no directories were backed up (e.g., leave backedUpDirs empty and add a sentinel or message to failedDirs). Update the return to return { success: false, manifest, backedUpDirs, failedDirs } so callers (rebuild/sandbox deletion) will stop when there are no state_dirs.
249-261:⚠️ Potential issue | 🟠 MajorThese tar paths still buffer the entire archive in memory.
Lines 249-261 and Lines 330-344 materialize the tar stream in
spawnSyncbuffers and cap it at 256 MB. Largeworkspaceor similar state dirs will make rebuild fail or spike RAM even though SSH+tar can be streamed end-to-end.Also applies to: 330-344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sandbox-state.ts` around lines 249 - 261, The current use of spawnSync for the SSH+tar download (variables result and extractResult) buffers the entire tar in memory (capped by maxBuffer) causing OOM on large backups; replace the synchronous approach with stream-based child_process.spawn: spawn the "ssh" command (using sshArgs and tarCmd) and pipe its stdout directly into a spawned "tar -xf - -C backupPath" stdin (use child.stdout.pipe(tarChild.stdin)), forward stderr logging from both processes, handle exit codes and errors via 'close'/'exit' and 'error' events, and implement a proper timeout by killing both processes if exceeded; ensure no intermediate Buffer aggregation is used so the tar stream is streamed end-to-end without materializing in memory.
135-156:⚠️ Potential issue | 🔴 CriticalYAML-backed configs still bypass credential stripping.
Lines 137-156 only sanitize
.jsonand.env. The sharedsanitizeConfigFile()still no-ops on non-JSON input, so any*.yaml/*.ymlconfig copied into the backup remains verbatim even though Hermes already uses YAML config format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sandbox-state.ts` around lines 135 - 156, The current branch in the file removes credentials only for .json and .env, leaving .yaml/.yml files untouched; update the logic so YAML-backed configs are sanitized too by either extending the file-check branch that calls sanitizeConfigFile() to include filenames ending with ".yaml" or ".yml", or by enhancing sanitizeConfigFile() (referenced by sanitizeConfigFile and isSensitiveFile) to detect and strip secrets from YAML content as well; ensure the chosen change still preserves the existing try/catch "best effort" behavior and that chmodSync and writeFileSync are applied after sanitization where applicable.src/nemoclaw.ts (1)
1628-1672:⚠️ Potential issue | 🟠 MajorReturn a non-zero status when restore is incomplete.
This still warns on partial restore but falls through with exit code 0, so callers will treat a data-loss path as a successful rebuild.
Suggested change
console.log(""); if (restore.success) { console.log(` ${G}\u2713${R} Sandbox '${sandboxName}' rebuilt successfully`); if (versionCheck.expectedVersion) { console.log(` Now running: ${agentName} v${versionCheck.expectedVersion}`); } } else { console.log(` ${YW}\u26a0${R} Sandbox '${sandboxName}' rebuilt but state restore was incomplete`); console.log(` Backup available at: ${backup.manifest.backupPath}`); + process.exit(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 1628 - 1672, The restore path currently logs partial restores (using restore.success, restore.restoredDirs and backup.manifest.backupPath) but returns exit code 0; change the flow so when restore.success is false the process exits non‑zero (e.g., call process.exit(1) or throw an error) immediately after the existing error logs about partial restore/failedDirs/backup.manifest.backupPath (before the final success messages and registry.updateSandbox), so callers receive a non‑zero status for incomplete restores of sandboxName.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/sandbox-state.ts`:
- Around line 350-356: The chown invocation in restoreSandboxState ignores
spawnSync's result so ownership failures are treated as successful restores;
capture the spawnSync return (e.g., const result = spawnSync(...)) when
resolveOpenshell() returns a path and check result.status and result.error, and
if the chown failed propagate a failure from restoreSandboxState (throw or
return a non-success) and avoid marking directories as restored; keep the same
args (openshellBinary, "sandbox", "exec", sandboxName, "--", "chown", "-R",
"sandbox:sandbox", writableDir) and preserve timeout and stdio options while
handling errors consistently.
- Around line 228-243: The SSH probe result from spawnSync (existResult) is not
checked so a timeout/failure with empty stdout is treated as “no dirs” and the
backup is silently marked successful; update the logic around
spawnSync/sshArgs/existCheckCmd to check existResult.status (and
existResult.error/ stderr) before using stdout—if status is non-zero or error
exists, log the error, mark the backup as failed (or return { success: false,
manifest, backedUpDirs, failedDirs } with failedDirs populated) instead of
calling writeManifest(backupPath, manifest) and returning success; ensure any
timeout/error path surfaces to caller so we don’t treat an SSH failure as an
empty directory list.
In `@src/nemoclaw.ts`:
- Around line 992-1000: The synchronous call to
sandboxVersion.checkAgentVersion(sandboxName) blocks connect on SSH cache-miss;
change this to a non-blocking fast-path check by either invoking a fast-only API
(e.g. sandboxVersion.checkAgentVersionFast or sandboxVersion.checkAgentVersion
with a {fastOnly:true} option) or by kicking off
sandboxVersion.checkAgentVersion(sandboxName) asynchronously without awaiting it
and only logging its results when it completes; ensure you still use
sandboxVersion.formatStalenessWarning(sandboxName, versionCheck) to format
warnings but only after a fast-path return or from the detached async result,
and do not perform any SSH probe on the connect critical path.
- Around line 1689-1714: The current loop treats a failed openshell sandbox list
as “no sandboxes running” and ignores individual backup failures; update the
logic around captureOpenshell/parseLiveSandboxNames so that if
captureOpenshell(...) indicates failure (check liveList.success or equivalent)
you log the underlying error and abort/fail the command immediately instead of
proceeding, and also track per-sandbox backup failures from
sandboxState.backupSandboxState (the result object) so that any failedDirs or
result.success === false sets a non-zero exit/failure status for the whole
command (return non-zero or throw) rather than just printing an error;
references: captureOpenshell, liveList, parseLiveSandboxNames,
sandboxState.backupSandboxState, and the result variable.
---
Duplicate comments:
In `@src/lib/sandbox-state.ts`:
- Around line 205-208: The code currently treats an empty stateDirs array as a
success; change the early-return in the stateDirs.length === 0 branch to fail
closed: call writeManifest(backupPath, manifest) as before but log an error (use
_log or existing logger), set the returned object to success: false, and
populate failedDirs to indicate no directories were backed up (e.g., leave
backedUpDirs empty and add a sentinel or message to failedDirs). Update the
return to return { success: false, manifest, backedUpDirs, failedDirs } so
callers (rebuild/sandbox deletion) will stop when there are no state_dirs.
- Around line 249-261: The current use of spawnSync for the SSH+tar download
(variables result and extractResult) buffers the entire tar in memory (capped by
maxBuffer) causing OOM on large backups; replace the synchronous approach with
stream-based child_process.spawn: spawn the "ssh" command (using sshArgs and
tarCmd) and pipe its stdout directly into a spawned "tar -xf - -C backupPath"
stdin (use child.stdout.pipe(tarChild.stdin)), forward stderr logging from both
processes, handle exit codes and errors via 'close'/'exit' and 'error' events,
and implement a proper timeout by killing both processes if exceeded; ensure no
intermediate Buffer aggregation is used so the tar stream is streamed end-to-end
without materializing in memory.
- Around line 135-156: The current branch in the file removes credentials only
for .json and .env, leaving .yaml/.yml files untouched; update the logic so
YAML-backed configs are sanitized too by either extending the file-check branch
that calls sanitizeConfigFile() to include filenames ending with ".yaml" or
".yml", or by enhancing sanitizeConfigFile() (referenced by sanitizeConfigFile
and isSensitiveFile) to detect and strip secrets from YAML content as well;
ensure the chosen change still preserves the existing try/catch "best effort"
behavior and that chmodSync and writeFileSync are applied after sanitization
where applicable.
In `@src/nemoclaw.ts`:
- Around line 1628-1672: The restore path currently logs partial restores (using
restore.success, restore.restoredDirs and backup.manifest.backupPath) but
returns exit code 0; change the flow so when restore.success is false the
process exits non‑zero (e.g., call process.exit(1) or throw an error)
immediately after the existing error logs about partial
restore/failedDirs/backup.manifest.backupPath (before the final success messages
and registry.updateSandbox), so callers receive a non‑zero status for incomplete
restores of sandboxName.
🪄 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: Pro Plus
Run ID: f6862aa0-269c-401b-8d0c-8ed16084a268
📒 Files selected for processing (3)
src/lib/onboard.tssrc/lib/sandbox-state.tssrc/nemoclaw.ts
- Check SSH probe exit code before treating backup as empty - Log chown failure as warning (wrong ownership = agent can't read state) - backup-all exits non-zero on failure for install.sh upgrade hook - E2E: don't silently ignore install.sh failures - E2E: reject "error" and empty output in version/registry assertions - E2E: fail when backup directory is missing - E2E: include .env files in credential leak check Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…m/NVIDIA/NemoClaw into fix/sandbox-rebuild-stale-version
cv
approved these changes
Apr 14, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (11)
src/lib/sandbox-state.ts (3)
352-366:⚠️ Potential issue | 🟠 MajorDon't mark restore success before ownership repair succeeds.
restoredDirsare populated beforechown -Rruns. If ownership repair fails, the agent can be left unable to read its restored files whilerestoreSandboxState()still reports success.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sandbox-state.ts` around lines 352 - 366, restoredDirs is being updated before the ownership fix runs, so if the chown (spawnSync via resolveOpenshell and openshellBinary, producing chownResult) fails restoreSandboxState() can report success incorrectly; move the restoredDirs.push(...localDirs) so it only runs after the chownResult.status check succeeds (or, if openshellBinary is absent, after any no-op path that still guarantees correct ownership), and on chownResult.status !== 0 ensure you do not mark the restore as successful and propagate/return an error from restoreSandboxState() instead of pushing entries to restoredDirs.
205-208:⚠️ Potential issue | 🔴 CriticalFail closed when an agent declares no
stateDirs.Returning
success: truehere letssandboxRebuild()destroy the sandbox after copying nothing. A missing or regressed manifest entry becomes silent data loss instead of a blocked rebuild.🛠 Proposed fix
if (stateDirs.length === 0) { _log("WARNING: Agent manifest declares no state_dirs — nothing to back up"); writeManifest(backupPath, manifest); - return { success: true, manifest, backedUpDirs, failedDirs }; + return { + success: false, + manifest, + backedUpDirs, + failedDirs: ["<missing-state_dirs>"], + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sandbox-state.ts` around lines 205 - 208, The code currently treats an empty stateDirs as success which allows sandboxRebuild() to proceed and potentially destroy data; change the behavior so that when stateDirs.length === 0 you treat this as a failure: log an explicit error, call writeManifest(backupPath, manifest) if needed for diagnostics, and return { success: false, manifest, backedUpDirs, failedDirs } (or throw an error) instead of success: true; update the block that checks stateDirs to reference stateDirs, backedUpDirs and failedDirs and ensure callers (e.g., sandboxRebuild) will abort on success === false.
135-139:⚠️ Potential issue | 🔴 CriticalYAML configs still bypass credential scrubbing.
sanitizeBackupDirectory()only sanitizes.json, and the helper insrc/lib/credential-filter.tsreturns immediately on non-JSON input. Any.yaml/.ymlconfig copied out ofstateDirsis therefore backed up verbatim, which still leaves a credential leak path for Hermes and future agents.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sandbox-state.ts` around lines 135 - 139, sanitizeBackupDirectory currently only sanitizes ".json" via sanitizeConfigFile, leaving ".yaml"/".yml" files unsanitized; update sanitizeBackupDirectory to treat files ending with ".yaml" or ".yml" the same as ".json" (call sanitizeConfigFile or a new sanitizeYamlConfig), and update the credential-filter helper in credential-filter.ts (the function that currently returns early on non-JSON input) so it can handle YAML input—either by parsing YAML (e.g., with a YAML parser) and running the same credential-scrubbing logic, or by falling back to a generic text-scrub path—ensuring all config file types (.json, .yaml, .yml) are passed through the credential scrubber rather than being skipped.test/e2e/test-rebuild-openclaw.sh (3)
154-158:⚠️ Potential issue | 🟠 MajorMake the old-version precondition fatal.
Logging and continuing here lets the E2E pass without ever reproducing the stale-version scenario. Fail immediately when
openclaw --versiondoes not containOLD_OPENCLAW_VERSION.🛠 Proposed fix
# Verify old version SANDBOX_VERSION=$(openshell sandbox exec --name "${SANDBOX_NAME}" -- openclaw --version 2>&1 || true) -echo "${SANDBOX_VERSION}" | grep -q "${OLD_OPENCLAW_VERSION}" || info "Version: ${SANDBOX_VERSION}" +echo "${SANDBOX_VERSION}" | grep -q "${OLD_OPENCLAW_VERSION}" \ + || fail "Expected OpenClaw ${OLD_OPENCLAW_VERSION} before rebuild, got: ${SANDBOX_VERSION}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-openclaw.sh` around lines 154 - 158, The check that validates the sandbox OpenClaw version uses a non-fatal info path and allows the script to continue; make this precondition fatal by replacing the current grep-or-info pattern so that when SANDBOX_VERSION (from the openshell sandbox exec call) does not contain OLD_OPENCLAW_VERSION the script immediately fails (exit non-zero) and logs a clear error mentioning both SANDBOX_VERSION and OLD_OPENCLAW_VERSION; update the block that defines SANDBOX_VERSION and the subsequent grep check accordingly so the E2E run cannot continue on a version mismatch.
76-78:⚠️ Potential issue | 🟠 MajorDon't continue after a failed installer run.
If
install.shfails but an oldernemoclaworopenshellis already onPATH, the latercommand -vchecks still pass and this E2E can false-pass against stale binaries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-openclaw.sh` around lines 76 - 78, The install step currently logs a non-zero exit but continues, which allows stale binaries to make later `command -v` checks pass; update the `if ! bash "${REPO_ROOT}/install.sh" --non-interactive >"$INSTALL_LOG" 2>&1; then` block to treat a failed installer as a hard failure: include the install error in the log (referencing INSTALL_LOG and info/error helpers) and immediately exit non-zero (e.g., call exit 1 or an error helper) so the script stops instead of proceeding to `command -v nemoclaw`/`openshell` checks.
112-125:⚠️ Potential issue | 🟠 MajorGuarantee blueprint restoration on build failure.
BLUEPRINTis restored only afterdocker buildreturns. Withset -euo pipefail, a failing build exits before that cleanup runs, leavingnemoclaw-blueprint/blueprint.yamlpinned to the old minimum version for later steps and tests.#!/bin/bash # Show that blueprint restoration currently happens only after docker build # and no EXIT trap protects it. sed -n '110,125p' test/e2e/test-rebuild-openclaw.sh echo '---' rg -n 'trap .*BLUEPRINT|cleanup_blueprint' test/e2e/test-rebuild-openclaw.sh || true echo '---' bash -lc 'set -e; false; echo unreachable' && echo "unexpected success" || echo "set -e aborts before later cleanup commands"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-openclaw.sh` around lines 112 - 125, The blueprint backup (BLUEPRINT_BAK) is only restored after docker build completes, so a failing build (set -euo pipefail) leaves BLUEPRINT modified; add a cleanup function (e.g., cleanup_blueprint) that checks for BLUEPRINT_BAK and moves it back to BLUEPRINT, then register it with trap 'EXIT' (or 'ERR' and 'EXIT') at top of the script so restoration always runs; replace the lone mv "${BLUEPRINT_BAK}" "${BLUEPRINT}" with a call to that cleanup function and ensure the trap is set before running docker build and before creating the temp blueprint file.test/e2e/test-rebuild-hermes.sh (3)
214-251:⚠️ Potential issue | 🟠 MajorVerify the rebuilt Hermes runtime version, not just registry metadata.
The post-rebuild phase only checks marker persistence and
sandboxes.json. A metadata-only update can still pass this test even if the Hermes binary inside the sandbox never changed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-hermes.sh` around lines 214 - 251, The test currently only verifies registry metadata (REGISTRY_VERSION) and marker file; add a concrete check that queries the Hermes runtime binary inside the sandbox to ensure the actual runtime was rebuilt. After the existing registry check, exec into the sandbox (using openshell sandbox exec --name "${SANDBOX_NAME}" --) and run the Hermes binary's version command (e.g., hermes --version or the actual runtime binary path used in the sandbox) to capture RUNTIME_VERSION, then assert RUNTIME_VERSION is neither "null" nor "error" nor the old release "2026.3.12" and fail the test if it matches the old version; reference SANDBOX_NAME, REGISTRY_FILE/REGISTRY_VERSION and the runtime binary name (hermes or the exact agent binary used) so the new check is located and uses the same expected-version logic as the registry assertion.
74-76:⚠️ Potential issue | 🟠 MajorDon't continue after a failed installer run.
If
install.shfails but an oldernemoclaworopenshellis already onPATH, the latercommand -vchecks still pass and this E2E can false-pass against stale binaries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-hermes.sh` around lines 74 - 76, The script currently ignores a failed installer run and continues, which can let stale binaries (nemoclaw/openshell) cause false-passes; update the install block that runs bash "${REPO_ROOT}/install.sh" --non-interactive >"$INSTALL_LOG" 2>&1 so that on failure it logs the INSTALL_LOG contents and exits non-zero (e.g., call error/err function or exit 1) instead of the current info message; ensure you stop further execution before any subsequent command -v checks for nemoclaw or openshell.
132-145:⚠️ Potential issue | 🟠 MajorAssert the sandbox actually starts on
OLD_HERMES_VERSION.The test never checks
hermes --versioninside the created sandbox before rebuild. If the old-image build arg is ignored or a cached layer slips in, the stale-version scenario is never exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-rebuild-hermes.sh` around lines 132 - 145, After the sandbox is reported Ready, run hermes --version inside that sandbox and assert it matches the expected OLD_HERMES_VERSION before proceeding to rebuild: use the existing sandbox name variable (SANDBOX_NAME) with openshell to exec into the sandbox (e.g., openshell sandbox exec --name "${SANDBOX_NAME}" -- hermes --version), capture the output and compare to "${OLD_HERMES_VERSION}", and call fail with a clear message if it does not match so the stale-version scenario is actually exercised.src/nemoclaw.ts (2)
1628-1672:⚠️ Potential issue | 🟠 MajorReturn non-zero on partial restore.
This block logs the failure but then falls off the end of
sandboxRebuild(), so the process exits 0 after an incomplete restore. Scripts and CI will treat a data-loss path as success.🛠 Proposed fix
} else { console.log(` ${YW}\u26a0${R} Sandbox '${sandboxName}' rebuilt but state restore was incomplete`); console.log(` Backup available at: ${backup.manifest.backupPath}`); + process.exit(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 1628 - 1672, The function sandboxRebuild currently logs an incomplete restore but continues and exits 0; change the control flow so a partial restore returns non-zero: after the existing logs in the if (!restore.success) block (the lines referencing restore.restoredDirs, restore.failedDirs and backup.manifest.backupPath), immediately abort by throwing an Error or calling process.exit(1) (do not let execution fall through to registry.updateSandbox or the success messages). This ensures sandboxRebuild (and symbols like registry.updateSandbox and agentDef usage) are not executed on partial restores and the process exits non-zero.
1689-1720:⚠️ Potential issue | 🔴 CriticalAbort
backup-allwhen live sandbox enumeration fails.If
openshell sandbox listreturns non-zero here,parseLiveSandboxNames()sees empty output, every sandbox is counted as skipped, and the pre-upgrade hook can continue without any backup.🛠 Proposed fix
// Check which sandboxes are actually live const liveList = captureOpenshell(["sandbox", "list"], { ignoreError: true }); + if (liveList.status !== 0) { + console.error(" Failed to enumerate live sandboxes. Aborting pre-upgrade backup."); + process.exit(liveList.status || 1); + } const liveNames = parseLiveSandboxNames(liveList.output || "");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 1689 - 1720, The code assumes captureOpenshell(["sandbox", "list"]) succeeded; if it failed parseLiveSandboxNames sees empty output and everything is skipped. After calling captureOpenshell (symbol: captureOpenshell) check its success/exit status (e.g., liveList.success or non-zero exitCode) before calling parseLiveSandboxNames; if the call failed, log an error including liveList.output or error details and exit non-zero (process.exit(1)) so the backup-all operation aborts instead of silently skipping all sandboxes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/sandbox-state.ts`:
- Around line 352-366: restoredDirs is being updated before the ownership fix
runs, so if the chown (spawnSync via resolveOpenshell and openshellBinary,
producing chownResult) fails restoreSandboxState() can report success
incorrectly; move the restoredDirs.push(...localDirs) so it only runs after the
chownResult.status check succeeds (or, if openshellBinary is absent, after any
no-op path that still guarantees correct ownership), and on chownResult.status
!== 0 ensure you do not mark the restore as successful and propagate/return an
error from restoreSandboxState() instead of pushing entries to restoredDirs.
- Around line 205-208: The code currently treats an empty stateDirs as success
which allows sandboxRebuild() to proceed and potentially destroy data; change
the behavior so that when stateDirs.length === 0 you treat this as a failure:
log an explicit error, call writeManifest(backupPath, manifest) if needed for
diagnostics, and return { success: false, manifest, backedUpDirs, failedDirs }
(or throw an error) instead of success: true; update the block that checks
stateDirs to reference stateDirs, backedUpDirs and failedDirs and ensure callers
(e.g., sandboxRebuild) will abort on success === false.
- Around line 135-139: sanitizeBackupDirectory currently only sanitizes ".json"
via sanitizeConfigFile, leaving ".yaml"/".yml" files unsanitized; update
sanitizeBackupDirectory to treat files ending with ".yaml" or ".yml" the same as
".json" (call sanitizeConfigFile or a new sanitizeYamlConfig), and update the
credential-filter helper in credential-filter.ts (the function that currently
returns early on non-JSON input) so it can handle YAML input—either by parsing
YAML (e.g., with a YAML parser) and running the same credential-scrubbing logic,
or by falling back to a generic text-scrub path—ensuring all config file types
(.json, .yaml, .yml) are passed through the credential scrubber rather than
being skipped.
In `@src/nemoclaw.ts`:
- Around line 1628-1672: The function sandboxRebuild currently logs an
incomplete restore but continues and exits 0; change the control flow so a
partial restore returns non-zero: after the existing logs in the if
(!restore.success) block (the lines referencing restore.restoredDirs,
restore.failedDirs and backup.manifest.backupPath), immediately abort by
throwing an Error or calling process.exit(1) (do not let execution fall through
to registry.updateSandbox or the success messages). This ensures sandboxRebuild
(and symbols like registry.updateSandbox and agentDef usage) are not executed on
partial restores and the process exits non-zero.
- Around line 1689-1720: The code assumes captureOpenshell(["sandbox", "list"])
succeeded; if it failed parseLiveSandboxNames sees empty output and everything
is skipped. After calling captureOpenshell (symbol: captureOpenshell) check its
success/exit status (e.g., liveList.success or non-zero exitCode) before calling
parseLiveSandboxNames; if the call failed, log an error including
liveList.output or error details and exit non-zero (process.exit(1)) so the
backup-all operation aborts instead of silently skipping all sandboxes.
In `@test/e2e/test-rebuild-hermes.sh`:
- Around line 214-251: The test currently only verifies registry metadata
(REGISTRY_VERSION) and marker file; add a concrete check that queries the Hermes
runtime binary inside the sandbox to ensure the actual runtime was rebuilt.
After the existing registry check, exec into the sandbox (using openshell
sandbox exec --name "${SANDBOX_NAME}" --) and run the Hermes binary's version
command (e.g., hermes --version or the actual runtime binary path used in the
sandbox) to capture RUNTIME_VERSION, then assert RUNTIME_VERSION is neither
"null" nor "error" nor the old release "2026.3.12" and fail the test if it
matches the old version; reference SANDBOX_NAME, REGISTRY_FILE/REGISTRY_VERSION
and the runtime binary name (hermes or the exact agent binary used) so the new
check is located and uses the same expected-version logic as the registry
assertion.
- Around line 74-76: The script currently ignores a failed installer run and
continues, which can let stale binaries (nemoclaw/openshell) cause false-passes;
update the install block that runs bash "${REPO_ROOT}/install.sh"
--non-interactive >"$INSTALL_LOG" 2>&1 so that on failure it logs the
INSTALL_LOG contents and exits non-zero (e.g., call error/err function or exit
1) instead of the current info message; ensure you stop further execution before
any subsequent command -v checks for nemoclaw or openshell.
- Around line 132-145: After the sandbox is reported Ready, run hermes --version
inside that sandbox and assert it matches the expected OLD_HERMES_VERSION before
proceeding to rebuild: use the existing sandbox name variable (SANDBOX_NAME)
with openshell to exec into the sandbox (e.g., openshell sandbox exec --name
"${SANDBOX_NAME}" -- hermes --version), capture the output and compare to
"${OLD_HERMES_VERSION}", and call fail with a clear message if it does not match
so the stale-version scenario is actually exercised.
In `@test/e2e/test-rebuild-openclaw.sh`:
- Around line 154-158: The check that validates the sandbox OpenClaw version
uses a non-fatal info path and allows the script to continue; make this
precondition fatal by replacing the current grep-or-info pattern so that when
SANDBOX_VERSION (from the openshell sandbox exec call) does not contain
OLD_OPENCLAW_VERSION the script immediately fails (exit non-zero) and logs a
clear error mentioning both SANDBOX_VERSION and OLD_OPENCLAW_VERSION; update the
block that defines SANDBOX_VERSION and the subsequent grep check accordingly so
the E2E run cannot continue on a version mismatch.
- Around line 76-78: The install step currently logs a non-zero exit but
continues, which allows stale binaries to make later `command -v` checks pass;
update the `if ! bash "${REPO_ROOT}/install.sh" --non-interactive
>"$INSTALL_LOG" 2>&1; then` block to treat a failed installer as a hard failure:
include the install error in the log (referencing INSTALL_LOG and info/error
helpers) and immediately exit non-zero (e.g., call exit 1 or an error helper) so
the script stops instead of proceeding to `command -v nemoclaw`/`openshell`
checks.
- Around line 112-125: The blueprint backup (BLUEPRINT_BAK) is only restored
after docker build completes, so a failing build (set -euo pipefail) leaves
BLUEPRINT modified; add a cleanup function (e.g., cleanup_blueprint) that checks
for BLUEPRINT_BAK and moves it back to BLUEPRINT, then register it with trap
'EXIT' (or 'ERR' and 'EXIT') at top of the script so restoration always runs;
replace the lone mv "${BLUEPRINT_BAK}" "${BLUEPRINT}" with a call to that
cleanup function and ensure the trap is set before running docker build and
before creating the temp blueprint file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e5b9abc4-bc4a-4b30-b952-0c4e0ff9e02b
📒 Files selected for processing (4)
src/lib/sandbox-state.tssrc/nemoclaw.tstest/e2e/test-rebuild-hermes.shtest/e2e/test-rebuild-openclaw.sh
4 tasks
ericksoa
added a commit
that referenced
this pull request
Apr 15, 2026
## Summary Exposes the backup/restore plumbing from `sandbox-state.ts` (added in #1870) as user-facing CLI commands, per feedback from @bpelfrey: > nemoclaw backup-all — How do you 'restore' this as a user? It almost feels like a place for 'snapshot create' 'snapshot restore' or something. ## New commands ``` nemoclaw <name> snapshot create Create a timestamped snapshot of sandbox state nemoclaw <name> snapshot list List available snapshots with timestamps and versions nemoclaw <name> snapshot restore [ts] Restore state from a snapshot (latest if no timestamp given) ``` ### Example workflow ```bash # Before a risky operation $ nemoclaw my-sandbox snapshot create ✓ Snapshot created (12 directories) ~/.nemoclaw/rebuild-backups/my-sandbox/2026-04-14T... # Something goes wrong... # Restore from the snapshot $ nemoclaw my-sandbox snapshot restore Using latest snapshot: 2026-04-14T... ✓ Restored 12 directories ``` ## Implementation Thin wrappers around existing functions in `src/lib/sandbox-state.ts`: - `backupSandboxState()` → `snapshot create` - `restoreSandboxState()` → `snapshot restore` - `listBackups()` / `getLatestBackup()` → `snapshot list` No new modules or dependencies — just CLI dispatch wiring. ## Test plan - [x] All pre-commit and pre-push hooks pass - [ ] Manual: `nemoclaw <name> snapshot create` on a live sandbox - [ ] Manual: `nemoclaw <name> snapshot list` shows the snapshot - [ ] Manual: `nemoclaw <name> snapshot restore` restores state Closes #1891 Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added sandbox snapshot commands: create, list, and restore; list shows timestamp, optional item counts and backup path; restore accepts a timestamp (prefix matches allowed) or defaults to latest and provides clear guidance on ambiguous/missing selections. * **Tests** * New end-to-end test validates snapshot create/list/restore flows, targeted restores, and scans backups for sensitive data. * **Chores** * CI updated to run snapshot E2E nightly and report failures with artifacts. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
3 tasks
cv
pushed a commit
that referenced
this pull request
Apr 15, 2026
## Summary - Document `nemoclaw <name> snapshot create/list/restore` commands (from #1892) - Document `nemoclaw <name> policy-remove` command (from #1822) - Document `nemoclaw <name> rebuild` command with version staleness detection (from #1870) - Document `nemoclaw backup-all` command (from #1870) - Update `status` to mention live enforced policy display (from #1896) - Update `connect` and `status` to mention version staleness warnings (from #1870) - Update `destroy` to reference snapshots and rebuild as alternatives - Add snapshot commands section to backup-restore page - Add `policy-remove` to customize-network-policy page - Add "Sandbox is running an outdated agent version" troubleshooting entry - Bump doc version switcher through 0.0.16 - Regenerate agent skills from updated docs ## Test plan - [x] `make docs` builds without warnings - [x] All pre-commit hooks pass - [ ] Verify rendered pages in docs build output 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added documentation for policy preset removal command with selection flow * Documented new rebuild command for sandbox upgrades while preserving workspace state * Introduced snapshot commands for creating, listing, and restoring workspace backups * Added bulk backup capability for multiple sandboxes * Enhanced agent version warnings in connect and status commands with remediation guidance * Expanded troubleshooting guide with outdated agent version resolution steps <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
8 tasks
wscurran
added
area: cli
This was referenced Jul 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
connectandstatusnemoclaw <name> rebuildcommand that backs up workspace state, destroys the old sandbox, recreates with current image, and restores statenemoclaw backup-allcommand and automatic pre-upgrade backup ininstall.shopenclaw doctor --fixfor cross-version structure repair; Hermes auto-migrates via SessionDBNew CLI commands
nemoclaw <name> rebuild [--yes] [--verbose]Upgrades a sandbox to the current agent version while preserving workspace state.
Flags:
--yes/--force— skip confirmation prompt--verbose— log SSH commands, exit codes, session state, registry mutations (also enabled byNEMOCLAW_REBUILD_VERBOSE=1)What it does internally:
state_dirsfrom agent manifest via SSH+tar (single roundtrip)nemoclaw destroy)onboard --resumeto recreate sandbox with current imageopenclaw doctor --fix(OpenClaw only) for cross-version structure repairagentVersionHermes note: No explicit post-restore step needed. Hermes's
SessionDB._init_schema()auto-migratesstate.db(SQLite) on gateway startup via sequential idempotent ALTER TABLE migrations.nemoclaw backup-allBacks up all registered live sandboxes to
~/.nemoclaw/rebuild-backups/.Automatic pre-upgrade hook:
install.shcallsnemoclaw backup-allbefore onboarding (Step 3), which may upgrade OpenShell. If the OpenShell upgrade destroys sandbox contents, the backups let the user recover vianemoclaw <name> rebuild.Version staleness warnings (existing commands, new behavior)
nemoclaw <name> status— now shows agent version and upgrade hint:nemoclaw <name> connect— prints a non-blocking warning before dropping into the shell:E2E test coverage (nightly)
test-rebuild-openclaw.sh— OpenClaw 2026.3.11 → 2026.4.2install.shfrom scratch — Node, OpenShell, NemoClaw, onboardDockerfile.basewithOPENCLAW_VERSION=2026.3.11(patches blueprint min version)openshell sandbox create --gateway nemoclawagentVersion: "2026.3.11"Dockerfile.basewith current OpenClaw 2026.4.2nemoclaw <name> rebuild --yes --verbosetest-rebuild-hermes.sh— Hermes v2026.3.12 → currentSame structure for Hermes agent: old Hermes base image, markers in
/sandbox/.hermes-data/memories/, verifies Hermes state dirs survive rebuild.Verification checklist
rebuild-openclaw-e2epassed on CIrebuild-hermes-e2epassed on CISigned-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit
New Features
nemoclaw <sandbox> rebuildto upgrade agents while preserving/restoring workspace state.nemoclaw backup-allto back up all running sandboxes.Tests
Chores