test(e2e): migrate sandbox rebuild smoke to Vitest

[cv]

Summary

Migrates the manual sandbox rebuild smoke from a standalone bash script into the live Vitest E2E project. The replacement preserves the real sandbox lifecycle proof while keeping the destructive rebuild path behind an explicit opt-in.

Related Issue

Part of #5098.

Changes

• Add test/e2e-scenario/live/sandbox-rebuild.test.ts with opt-in live coverage for onboard, marker preservation, stale registry simulation, rebuild, registry refresh, and backup credential scanning.
• Gate the destructive sandbox rebuild flow behind NEMOCLAW_E2E_SANDBOX_REBUILD=1 in addition to the live E2E project opt-in.
• Delete test/e2e/test-sandbox-rebuild.sh.

Type of Change

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

Verification

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

---

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

[copy-pr-bot]

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

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 9e22096a-db6c-4ed0-90b3-3f9bab25e360

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/e2e-migrate-sandbox-rebuild

---

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

[github-actions]

E2E Advisor Recommendation

Required E2E: None
Optional E2E: rebuild-openclaw-e2e, state-backup-restore-e2e, sandbox-operations-e2e, upgrade-stale-sandbox-e2e

Dispatch hint: rebuild-openclaw-e2e,state-backup-restore-e2e,sandbox-operations-e2e,upgrade-stale-sandbox-e2e

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/codex/e2e-simplify-migration-tracking
Head: HEAD
Confidence: high

Required E2E

• None. No merge-blocking E2E is required because this PR changes only E2E test files and does not modify runtime/user-flow code. Optional adjacent sandbox lifecycle jobs can be run for confidence, and the migrated test should be wired into an existing workflow before relying on it for CI coverage.

Optional E2E

• rebuild-openclaw-e2e (high): Closest existing rebuild lifecycle job in nightly-e2e; useful to compare the migrated sandbox rebuild coverage against established rebuild behavior, but not merge-blocking because only E2E test files changed.
• state-backup-restore-e2e (high): Adjacent coverage for state backup/restore behavior that the new sandbox rebuild test also validates via workspace marker preservation and backup credential checks.
• sandbox-operations-e2e (high): Broad sandbox lifecycle smoke coverage for create/status/exec/destroy operations used by the migrated rebuild scenario.
• upgrade-stale-sandbox-e2e (high): Adjacent stale-sandbox/version metadata coverage, relevant because the migrated test manually sets a stale registry agentVersion before rebuild.

New E2E recommendations

• sandbox rebuild Vitest migration (high): The new free-standing live Vitest test is not currently wired into .github/workflows/e2e-vitest-scenarios.yaml, whose free-standing section only runs openshell-version-pin.test.ts and whose matrix runs registry-scenarios.test.ts. Add a dedicated workflow_dispatch job or manifest entry for test/e2e-scenario/live/sandbox-rebuild.test.ts with NEMOCLAW_E2E_SANDBOX_REBUILD=1 so the migrated coverage remains runnable in CI after deleting the legacy script.
  • Suggested test: sandbox-rebuild-vitest workflow job running npx vitest run --project e2e-scenarios-live test/e2e-scenario/live/sandbox-rebuild.test.ts with NEMOCLAW_E2E_SANDBOX_REBUILD=1

Dispatch hint

• Workflow: .github/workflows/nightly-e2e.yaml
• jobs input: rebuild-openclaw-e2e,state-backup-restore-e2e,sandbox-operations-e2e,upgrade-stale-sandbox-e2e

[github-actions]

E2E Scenario Advisor Recommendation

Required scenario E2E: e2e-scenarios-all
Optional scenario E2E: None

Dispatch required scenario E2E:

• gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref>

Workflow run

Full scenario advisor summary

E2E Scenario Advisor

Base: origin/codex/e2e-simplify-migration-tracking
Head: HEAD
Confidence: medium

Required scenario E2E

• e2e-scenarios-all: A new live Vitest scenario test was added under test/e2e-scenario/live. It is not a trusted-main live-supported typed registry scenario ID, so use the scenario workflow fan-out rather than inventing a targeted ID.
  • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref>

Optional scenario E2E

• None.

Relevant changed files

• test/e2e-scenario/live/sandbox-rebuild.test.ts

[github-actions]

PR Review Advisor

Findings: 1 needs attention, 8 worth checking, 0 nice ideas
Since last review: 0 prior items resolved, 7 still apply, 1 new item found

Review findings

🛠️ Needs attention

• New sandbox rebuild test is not reachable from the existing live Vitest workflow (test/e2e-scenario/live/sandbox-rebuild.test.ts:16): The PR deletes the standalone bash runner and adds a gated Vitest test, but the current live workflow invokes only `test/e2e-scenario/live/registry-scenarios.test.ts` for the matrix plus the separate OpenShell version-pin test. I found no workflow or documented command setting `NEMOCLAW_E2E_SANDBOX_REBUILD=1`, so the replacement can be skipped after the legacy script is removed.
  • Recommendation: Add an explicit runnable entry point for this free-standing live test, such as a dedicated workflow/manual job or documented command that runs `test/e2e-scenario/live/sandbox-rebuild.test.ts` with both `NEMOCLAW_RUN_E2E_SCENARIOS=1` and `NEMOCLAW_E2E_SANDBOX_REBUILD=1`.
  • Evidence: `runSandboxRebuildTest` skips unless `NEMOCLAW_E2E_SANDBOX_REBUILD === "1"`; `.github/workflows/e2e-vitest-scenarios.yaml` runs `registry-scenarios.test.ts` and `openshell-version-pin.test.ts`, and repository grep found no other `NEMOCLAW_E2E_SANDBOX_REBUILD` reference.

🔎 Worth checking

• Source-of-truth review needed: Missing rebuild-backup directory tolerated by `listCredentialLeaks`: The advisor marked localized patch analysis as needs_followup.
  • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Evidence: `listCredentialLeaks` catches `ENOENT` and returns without recording failure; the final assertion treats the empty leak list as success.
• Source-of-truth review needed: Stale rebuild warning tolerated when absent: The advisor marked localized patch analysis as needs_followup.
  • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Evidence: When `/rebuild/i` is absent, the test writes `No rebuild warning was observed; legacy bash treated this as acceptable when the sandbox is not live.` and continues.
• Source-of-truth review needed: Cleanup destroy errors suppressed: The advisor marked localized patch analysis as needs_followup.
  • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Evidence: The cleanup callback catches all errors from `host.nemoclaw([sandboxName, "destroy", "--yes"], ...)` and ignores them.
• Live sandbox commands inherit the full runner environment (test/e2e-scenario/live/sandbox-rebuild.test.ts:132): The new test passes `inheritEnv: true` to host and sandbox commands throughout a destructive sandbox lifecycle. `ShellProbe` redacts captured output, but full environment inheritance bypasses the fixture layer's least-privilege child-env model and can expose unrelated runner secrets to NemoClaw, OpenShell, or sandbox child processes if they persist or forward environment values.
  • Recommendation: Pass only the environment variables required for each command, especially `NVIDIA_API_KEY`, `NEMOCLAW_NON_INTERACTIVE`, `NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE`, and `NEMOCLAW_RECREATE_SANDBOX` where needed. Avoid inheriting secrets into `sandbox.exec` calls that only read or write the marker file.
  • Evidence: Every command option in the new test uses `inheritEnv: true`; `test/e2e-scenario/fixtures/shell-probe.ts` defaults to explicit env only, and `fixtures/redaction.ts` documents child-env minimization as fixture-owned security hygiene.
• Stale-warning probe uses PATH `nemoclaw` and does not assert the warning (test/e2e-scenario/live/sandbox-rebuild.test.ts:195): Most lifecycle steps use `host.nemoclaw`, which respects `NEMOCLAW_CLI_BIN`, but the stale-warning probe shells out through `bash -lc` and invokes bare `nemoclaw` from PATH. If PATH resolves a different installed CLI, this step may not test the PR checkout. The check also only writes a note when no rebuild warning appears, so the migrated test does not prove the stale-warning behavior it claims to cover.
  • Recommendation: Use the repository CLI path from `host.commandPath` or `host.nemoclaw`, preferably without `bash -lc` string interpolation. If stale warnings are part of the preserved contract, assert them in a deterministic live state; otherwise narrow the PR claim to say this step records diagnostic evidence only.
  • Evidence: The test runs `host.command("bash", ["-lc", `timeout 10 nemoclaw ${shellQuote(sandboxName)} connect <<<"exit"`], ...)`; on missing `/rebuild/i`, it writes `connect-stale-warning-note.txt` and continues.
• Credential-backup scan can pass without inspecting backup evidence (test/e2e-scenario/live/sandbox-rebuild.test.ts:79): The credential leak check treats a missing rebuild-backup directory as an empty leak list and only scans `.json` files for three token shapes. That preserves the legacy script's tolerance, but it weakens the claimed backup credential-stripping proof: a rebuild that produces no expected backup evidence, or leaks credentials in `.env` files or other canonical token shapes, can still pass.
  • Recommendation: Clarify the backup lifecycle contract. If this rebuild should create a backup, assert the backup directory exists and scan files using the same snapshot credential-scan policy and canonical token patterns used by the product/fixture security code. If absence is expected because backups are cleaned up, add source-level coverage proving credential stripping before cleanup.
  • Evidence: `listCredentialLeaks` returns on `ENOENT`, filters to `.json`, and searches only `/nvapi-|sk-|Bearer /`; the final assertion is `expect(leaks).toEqual([])`. Product code exposes broader snapshot scan helpers and token patterns.
• Environment-provided sandbox name is used before fixture validation (test/e2e-scenario/live/sandbox-rebuild.test.ts:111): `NEMOCLAW_SANDBOX_NAME` is read from the environment and used in host CLI lifecycle calls, artifact data, cleanup labels, and registry mutation before the `SandboxClient` validator is reached. The argv calls reduce shell-injection risk, and the later shell snippet uses quoting, but this still leaves an untrusted test-control value outside the established fixture client contract.
  • Recommendation: Validate `sandboxName` immediately after reading it, using the same `validateSandboxName` contract as `SandboxClient`, before any host lifecycle command, artifact labeling, cleanup registration, or registry mutation.
  • Evidence: `const sandboxName = process.env.NEMOCLAW_SANDBOX_NAME || "e2e-rebuild";` is followed by cleanup and `host.nemoclaw` calls before any explicit validation. `SandboxClient.exec/status` validate names with `/^[A-Za-z0-9][A-Za-z0-9_.-]*$/`.
• Cleanup destroy errors are silently suppressed (test/e2e-scenario/live/sandbox-rebuild.test.ts:135): The cleanup callback catches and ignores all destroy failures. That mirrors the legacy script, but the Vitest cleanup fixture normally records cleanup failures and asserts them after the test. Silently swallowing destroy errors can hide leaked sandboxes or cleanup regressions in exactly the lifecycle path this test is meant to validate.
  • Recommendation: Prefer reporting cleanup failure through the fixture cleanup result, or write an explicit artifact/warning that makes the leak visible. If destroy failures must be tolerated for this scenario, document the invalid state, source boundary, regression coverage, and removal condition.
  • Evidence: The cleanup callback wraps `host.nemoclaw([sandboxName, "destroy", "--yes"], ...)` in `try { ... } catch { /* Cleanup is best-effort; the legacy script also ignored destroy errors. */ }`, bypassing `CleanupRegistry` failure reporting.

🌱 Nice ideas

• None.

Consider writing more tests for

• **Runtime validation** — Dispatch the sandbox-rebuild Vitest entry point with both `NEMOCLAW_RUN_E2E_SCENARIOS=1` and `NEMOCLAW_E2E_SANDBOX_REBUILD=1`.. Although the diff is test-only, it deletes a legacy live runner and claims preserved destructive sandbox rebuild coverage. Confidence depends on proving the new gated Vitest path is actually runnable and exercises the repository CLI and backup/security contracts.
• **Runtime validation** — Verify the stale registry connect-warning probe invokes the repository CLI path from `NEMOCLAW_CLI_BIN`, not PATH `nemoclaw`.. Although the diff is test-only, it deletes a legacy live runner and claims preserved destructive sandbox rebuild coverage. Confidence depends on proving the new gated Vitest path is actually runnable and exercises the repository CLI and backup/security contracts.
• **Runtime validation** — Fail credential stripping verification when expected rebuild backup evidence is missing, if rebuild is expected to create or preserve a snapshot.. Although the diff is test-only, it deletes a legacy live runner and claims preserved destructive sandbox rebuild coverage. Confidence depends on proving the new gated Vitest path is actually runnable and exercises the repository CLI and backup/security contracts.
• **Runtime validation** — Scan rebuild backup files using the canonical snapshot credential scan policy, including `.env` files and supported token shapes.. Although the diff is test-only, it deletes a legacy live runner and claims preserved destructive sandbox rebuild coverage. Confidence depends on proving the new gated Vitest path is actually runnable and exercises the repository CLI and backup/security contracts.
• **Runtime validation** — Reject invalid `NEMOCLAW_SANDBOX_NAME` before any host lifecycle command or registry mutation runs.. Although the diff is test-only, it deletes a legacy live runner and claims preserved destructive sandbox rebuild coverage. Confidence depends on proving the new gated Vitest path is actually runnable and exercises the repository CLI and backup/security contracts.
• **Stale-warning probe uses PATH `nemoclaw` and does not assert the warning** — Use the repository CLI path from `host.commandPath` or `host.nemoclaw`, preferably without `bash -lc` string interpolation. If stale warnings are part of the preserved contract, assert them in a deterministic live state; otherwise narrow the PR claim to say this step records diagnostic evidence only.
• **Acceptance clause:** Migrates the manual sandbox rebuild smoke from a standalone bash script into the live Vitest E2E project. — add test evidence or identify existing coverage. The PR deletes `test/e2e/test-sandbox-rebuild.sh` and adds `test/e2e-scenario/live/sandbox-rebuild.test.ts`, which matches the live project glob when `NEMOCLAW_RUN_E2E_SCENARIOS=1`. However, the existing live workflow does not invoke this free-standing file or set `NEMOCLAW_E2E_SANDBOX_REBUILD=1`.
• **Acceptance clause:** The replacement preserves the real sandbox lifecycle proof while keeping the destructive rebuild path behind an explicit opt-in. — add test evidence or identify existing coverage. The new test is gated by `NEMOCLAW_E2E_SANDBOX_REBUILD=1` and performs onboard, marker write/read, registry mutation, rebuild, marker restore, registry refresh, and credential scan. The stale-warning proof is diagnostic-only, the backup scan can pass with no backup directory, and no runnable workflow/manual entry point was found.

Since last review details

Current findings:

• Source-of-truth review needed: Missing rebuild-backup directory tolerated by `listCredentialLeaks`: The advisor marked localized patch analysis as needs_followup.
  • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Evidence: `listCredentialLeaks` catches `ENOENT` and returns without recording failure; the final assertion treats the empty leak list as success.
• Source-of-truth review needed: Stale rebuild warning tolerated when absent: The advisor marked localized patch analysis as needs_followup.
  • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Evidence: When `/rebuild/i` is absent, the test writes `No rebuild warning was observed; legacy bash treated this as acceptable when the sandbox is not live.` and continues.
• Source-of-truth review needed: Cleanup destroy errors suppressed: The advisor marked localized patch analysis as needs_followup.
  • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Evidence: The cleanup callback catches all errors from `host.nemoclaw([sandboxName, "destroy", "--yes"], ...)` and ignores them.
• New sandbox rebuild test is not reachable from the existing live Vitest workflow (test/e2e-scenario/live/sandbox-rebuild.test.ts:16): The PR deletes the standalone bash runner and adds a gated Vitest test, but the current live workflow invokes only `test/e2e-scenario/live/registry-scenarios.test.ts` for the matrix plus the separate OpenShell version-pin test. I found no workflow or documented command setting `NEMOCLAW_E2E_SANDBOX_REBUILD=1`, so the replacement can be skipped after the legacy script is removed.
  • Recommendation: Add an explicit runnable entry point for this free-standing live test, such as a dedicated workflow/manual job or documented command that runs `test/e2e-scenario/live/sandbox-rebuild.test.ts` with both `NEMOCLAW_RUN_E2E_SCENARIOS=1` and `NEMOCLAW_E2E_SANDBOX_REBUILD=1`.
  • Evidence: `runSandboxRebuildTest` skips unless `NEMOCLAW_E2E_SANDBOX_REBUILD === "1"`; `.github/workflows/e2e-vitest-scenarios.yaml` runs `registry-scenarios.test.ts` and `openshell-version-pin.test.ts`, and repository grep found no other `NEMOCLAW_E2E_SANDBOX_REBUILD` reference.
• Live sandbox commands inherit the full runner environment (test/e2e-scenario/live/sandbox-rebuild.test.ts:132): The new test passes `inheritEnv: true` to host and sandbox commands throughout a destructive sandbox lifecycle. `ShellProbe` redacts captured output, but full environment inheritance bypasses the fixture layer's least-privilege child-env model and can expose unrelated runner secrets to NemoClaw, OpenShell, or sandbox child processes if they persist or forward environment values.
  • Recommendation: Pass only the environment variables required for each command, especially `NVIDIA_API_KEY`, `NEMOCLAW_NON_INTERACTIVE`, `NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE`, and `NEMOCLAW_RECREATE_SANDBOX` where needed. Avoid inheriting secrets into `sandbox.exec` calls that only read or write the marker file.
  • Evidence: Every command option in the new test uses `inheritEnv: true`; `test/e2e-scenario/fixtures/shell-probe.ts` defaults to explicit env only, and `fixtures/redaction.ts` documents child-env minimization as fixture-owned security hygiene.
• Stale-warning probe uses PATH `nemoclaw` and does not assert the warning (test/e2e-scenario/live/sandbox-rebuild.test.ts:195): Most lifecycle steps use `host.nemoclaw`, which respects `NEMOCLAW_CLI_BIN`, but the stale-warning probe shells out through `bash -lc` and invokes bare `nemoclaw` from PATH. If PATH resolves a different installed CLI, this step may not test the PR checkout. The check also only writes a note when no rebuild warning appears, so the migrated test does not prove the stale-warning behavior it claims to cover.
  • Recommendation: Use the repository CLI path from `host.commandPath` or `host.nemoclaw`, preferably without `bash -lc` string interpolation. If stale warnings are part of the preserved contract, assert them in a deterministic live state; otherwise narrow the PR claim to say this step records diagnostic evidence only.
  • Evidence: The test runs `host.command("bash", ["-lc", `timeout 10 nemoclaw ${shellQuote(sandboxName)} connect <<<"exit"`], ...)`; on missing `/rebuild/i`, it writes `connect-stale-warning-note.txt` and continues.
• Credential-backup scan can pass without inspecting backup evidence (test/e2e-scenario/live/sandbox-rebuild.test.ts:79): The credential leak check treats a missing rebuild-backup directory as an empty leak list and only scans `.json` files for three token shapes. That preserves the legacy script's tolerance, but it weakens the claimed backup credential-stripping proof: a rebuild that produces no expected backup evidence, or leaks credentials in `.env` files or other canonical token shapes, can still pass.
  • Recommendation: Clarify the backup lifecycle contract. If this rebuild should create a backup, assert the backup directory exists and scan files using the same snapshot credential-scan policy and canonical token patterns used by the product/fixture security code. If absence is expected because backups are cleaned up, add source-level coverage proving credential stripping before cleanup.
  • Evidence: `listCredentialLeaks` returns on `ENOENT`, filters to `.json`, and searches only `/nvapi-|sk-|Bearer /`; the final assertion is `expect(leaks).toEqual([])`. Product code exposes broader snapshot scan helpers and token patterns.
• Environment-provided sandbox name is used before fixture validation (test/e2e-scenario/live/sandbox-rebuild.test.ts:111): `NEMOCLAW_SANDBOX_NAME` is read from the environment and used in host CLI lifecycle calls, artifact data, cleanup labels, and registry mutation before the `SandboxClient` validator is reached. The argv calls reduce shell-injection risk, and the later shell snippet uses quoting, but this still leaves an untrusted test-control value outside the established fixture client contract.
  • Recommendation: Validate `sandboxName` immediately after reading it, using the same `validateSandboxName` contract as `SandboxClient`, before any host lifecycle command, artifact labeling, cleanup registration, or registry mutation.
  • Evidence: `const sandboxName = process.env.NEMOCLAW_SANDBOX_NAME || "e2e-rebuild";` is followed by cleanup and `host.nemoclaw` calls before any explicit validation. `SandboxClient.exec/status` validate names with `/^[A-Za-z0-9][A-Za-z0-9_.-]*$/`.
• Cleanup destroy errors are silently suppressed (test/e2e-scenario/live/sandbox-rebuild.test.ts:135): The cleanup callback catches and ignores all destroy failures. That mirrors the legacy script, but the Vitest cleanup fixture normally records cleanup failures and asserts them after the test. Silently swallowing destroy errors can hide leaked sandboxes or cleanup regressions in exactly the lifecycle path this test is meant to validate.
  • Recommendation: Prefer reporting cleanup failure through the fixture cleanup result, or write an explicit artifact/warning that makes the leak visible. If destroy failures must be tolerated for this scenario, document the invalid state, source boundary, regression coverage, and removal condition.
  • Evidence: The cleanup callback wraps `host.nemoclaw([sandboxName, "destroy", "--yes"], ...)` in `try { ... } catch { /* Cleanup is best-effort; the legacy script also ignored destroy errors. */ }`, bypassing `CleanupRegistry` failure reporting.

Workflow run details

This is an automated advisory review. A human maintainer must make the final merge decision.

[wscurran]

✨
Related open issues:

• #5098 [DRAFT] Epic: Migrate test/e2e bash suite to typed Vitest E2E scenarios

[jyaunches]

Closing this draft codex migration PR as superseded by the simplified #5098 per-script migration process. The branch carries broad/stale framework-era diff noise; if this test is still needed, we'll salvage the useful core in a fresh/simple one-script PR using nemoclaw-e2e-legacy-migrate.