test(e2e): migrate upgrade stale sandbox to vitest#5542
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new live Vitest E2E test ( ChangesStale Sandbox Upgrade E2E Test
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over Test,Registry: Setup and stale state creation
end
participant Test as upgrade-stale-sandbox test
participant Docker as Docker
participant OpenShell as OpenShell
participant NemoClaw as nemoclaw CLI
participant Registry as NemoClaw Registry JSON
Test->>NemoClaw: install.sh --non-interactive (retry on transient failure)
Test->>Docker: docker build (OLD_OPENCLAW_VERSION base image)
Test->>OpenShell: create sandbox from fixture Dockerfile
OpenShell-->>Test: sandbox ready (readiness poll)
Test->>OpenShell: assert old OpenClaw version inside sandbox
Test->>Registry: writeStaleRegistryEntry (agentVersion = old version)
rect rgba(144, 238, 144, 0.5)
Note over Test,NemoClaw: Stale detection and rebuild
end
Test->>NemoClaw: upgrade-sandboxes --check
NemoClaw-->>Test: sandbox flagged as stale
Test->>NemoClaw: nemoclaw rebuild --yes
NemoClaw-->>Test: rebuild complete
Test->>OpenShell: poll for readiness
Test->>OpenShell: assert new OpenClaw version (old version absent)
Test->>NemoClaw: upgrade-sandboxes --check
NemoClaw-->>Test: sandbox reported as up to date
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the branch is 96%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the branch is 46%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 1 needs attention, 4 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
![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: 1
🧹 Nitpick comments (1)
test/e2e-scenario/live/upgrade-stale-sandbox.test.ts (1)
221-224: 💤 Low valueConsider validating the reason for exit code 1.
Accepting exit code 1 blindly could mask unexpected failures (e.g., permission errors, network issues). If exit code 1 specifically indicates "sandbox not found," consider asserting on the output to confirm.
- expect( - deleteInstalledSandbox.exitCode === 0 || deleteInstalledSandbox.exitCode === 1, - resultText(deleteInstalledSandbox), - ).toBe(true); + if (deleteInstalledSandbox.exitCode !== 0) { + // Exit code 1 is expected when sandbox doesn't exist + expect(deleteInstalledSandbox.exitCode, resultText(deleteInstalledSandbox)).toBe(1); + expect(resultText(deleteInstalledSandbox)).toMatch(/not found|does not exist/i); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e-scenario/live/upgrade-stale-sandbox.test.ts` around lines 221 - 224, The test for deleteInstalledSandbox accepts exit code 1 without validating the actual reason for failure, which could mask unexpected errors like permission issues or network problems. Modify the expectation to first check if exitCode is 0, or if it is 1, then validate the output or error message using resultText or similar output inspection to confirm the failure reason is specifically "sandbox not found" before allowing the test to pass. This ensures only expected failures are accepted while catching other types of errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e-vitest-scenarios.yaml:
- Around line 1698-1799: The free-standing workflow inventory file is missing
three required entries that will cause CI failures during the generate-matrix
validation step. Open tools/e2e-scenarios/free-standing-workflow-inventory.mts
and add upgrade-stale-sandbox-vitest to the allowed_jobs array,
upgrade-stale-sandbox to the free_standing_scenarios_csv string, and create the
mapping entry in free_standing_scenario_jobs_csv to connect the scenario to its
job. Additionally, on line 1781 in the upgrade-stale-sandbox-vitest job, the npx
vitest run command has excessive inline whitespace; refactor it to use backslash
line continuation for better readability and consistency with other multi-line
commands in the workflow.
---
Nitpick comments:
In `@test/e2e-scenario/live/upgrade-stale-sandbox.test.ts`:
- Around line 221-224: The test for deleteInstalledSandbox accepts exit code 1
without validating the actual reason for failure, which could mask unexpected
errors like permission issues or network problems. Modify the expectation to
first check if exitCode is 0, or if it is 1, then validate the output or error
message using resultText or similar output inspection to confirm the failure
reason is specifically "sandbox not found" before allowing the test to pass.
This ensures only expected failures are accepted while catching other types of
errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c8259484-c53b-4400-bf1d-f1bc59e1c496
📒 Files selected for processing (2)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/live/upgrade-stale-sandbox.test.ts
Comment on lines
+1698
to
+1799
| upgrade-stale-sandbox-vitest: | ||
| needs: generate-matrix | ||
| if: ${{ (inputs.jobs == '' && inputs.scenarios == '') || contains(format(',{0},', inputs.jobs), ',upgrade-stale-sandbox-vitest,') || contains(format(',{0},', inputs.scenarios), ',upgrade-stale-sandbox,') }} | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 55 | ||
| env: | ||
| FREE_STANDING_VITEST_JOB: "1" | ||
| FREE_STANDING_SCENARIO_ID: "upgrade-stale-sandbox" | ||
| E2E_ARTIFACT_DIR: ${{ github.workspace }}/e2e-artifacts/vitest/upgrade-stale-sandbox | ||
| NEMOCLAW_CLI_BIN: ${{ github.workspace }}/bin/nemoclaw.js | ||
| NEMOCLAW_RUN_E2E_SCENARIOS: "1" | ||
| NEMOCLAW_NON_INTERACTIVE: "1" | ||
| NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE: "1" | ||
| NEMOCLAW_SANDBOX_NAME: "e2e-upgrade-stale" | ||
| OPENSHELL_GATEWAY: "nemoclaw" | ||
| steps: | ||
| - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 | ||
| with: | ||
| persist-credentials: false | ||
|
|
||
| - name: Configure isolated Docker auth directory | ||
| run: echo "DOCKER_CONFIG=${RUNNER_TEMP}/docker-config-upgrade-stale-sandbox" >> "$GITHUB_ENV" | ||
|
|
||
| - name: Authenticate to Docker Hub | ||
| env: | ||
| DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }} | ||
| DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }} | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
| if [[ -z "${DOCKERHUB_USERNAME}" || -z "${DOCKERHUB_TOKEN}" ]]; then | ||
| echo "::notice::Docker Hub credentials not configured; continuing with anonymous pulls." | ||
| exit 0 | ||
| fi | ||
| mkdir -p "${DOCKER_CONFIG}" | ||
| chmod 700 "${DOCKER_CONFIG}" | ||
| login_succeeded=0 | ||
| for attempt in 1 2 3; do | ||
| if echo "${DOCKERHUB_TOKEN}" | timeout 30s docker login docker.io --username "${DOCKERHUB_USERNAME}" --password-stdin; then | ||
| login_succeeded=1 | ||
| break | ||
| fi | ||
| if [[ "$attempt" -lt 3 ]]; then | ||
| echo "::warning::Docker Hub login attempt ${attempt} failed; retrying." | ||
| sleep 5 | ||
| fi | ||
| done | ||
| if [[ "$login_succeeded" -ne 1 ]]; then | ||
| echo "::warning::Docker Hub login failed after 3 attempts; continuing with anonymous pulls." | ||
| fi | ||
|
|
||
| - name: Set up Node | ||
| uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.0.0 | ||
| with: | ||
| node-version: 22 | ||
| cache: npm | ||
|
|
||
| - name: Install root dependencies | ||
| run: npm ci --ignore-scripts | ||
|
|
||
| - name: Build CLI | ||
| run: npm run build:cli | ||
|
|
||
| - name: Install OpenShell CLI | ||
| run: bash scripts/install-openshell.sh | ||
|
|
||
| - name: Run upgrade stale sandbox live Vitest test | ||
| env: | ||
| NVIDIA_INFERENCE_API_KEY: ${{ secrets.NVIDIA_INFERENCE_API_KEY }} | ||
| run: | | ||
| set -euo pipefail | ||
| export PATH="$HOME/.local/bin:$HOME/.npm-global/bin:$PATH" | ||
| if command -v openshell >/dev/null 2>&1; then | ||
| OPENSHELL_BIN="$(command -v openshell)" | ||
| elif [ -x "$HOME/.local/bin/openshell" ]; then | ||
| OPENSHELL_BIN="$HOME/.local/bin/openshell" | ||
| else | ||
| echo "::error::OpenShell CLI not found after install" | ||
| ls -la /usr/local/bin/openshell "$HOME/.local/bin/openshell" 2>&1 || true | ||
| exit 1 | ||
| fi | ||
| export OPENSHELL_BIN | ||
| "$OPENSHELL_BIN" --version | ||
| npx vitest run --project e2e-scenarios-live test/e2e-scenario/live/upgrade-stale-sandbox.test.ts --silent=false --reporter=default | ||
|
|
||
| - name: Upload upgrade stale sandbox artifacts | ||
| if: always() | ||
| uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 | ||
| with: | ||
| name: e2e-vitest-scenarios-upgrade-stale-sandbox | ||
| path: e2e-artifacts/vitest/upgrade-stale-sandbox/ | ||
| include-hidden-files: false | ||
| if-no-files-found: ignore | ||
| retention-days: 14 | ||
|
|
||
| - name: Clean up Docker auth | ||
| if: always() | ||
| run: | | ||
| set -euo pipefail | ||
| docker logout docker.io || true | ||
| rm -rf "${DOCKER_CONFIG}" | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the free-standing inventory includes the new job and scenario
echo "=== Checking free-standing inventory for upgrade-stale-sandbox ==="
if fd -t f 'free-standing-workflow-inventory' tools/; then
inventory_file=$(fd -t f 'free-standing-workflow-inventory' tools/ | head -1)
echo "Found inventory file: $inventory_file"
echo ""
echo "--- Checking for job id ---"
rg -n 'upgrade-stale-sandbox-vitest' "$inventory_file" || echo "WARNING: Job not found in inventory"
echo ""
echo "--- Checking for scenario id ---"
rg -n 'upgrade-stale-sandbox[^-]' "$inventory_file" || echo "WARNING: Scenario not found in inventory"
else
echo "WARNING: Could not find free-standing-workflow-inventory file"
fiRepository: NVIDIA/NemoClaw
Length of output: 402
The new job is well-structured, but the free-standing inventory file is incomplete.
The upgrade-stale-sandbox-vitest job correctly implements isolated Docker auth, retry logic, and artifact handling. However, verification confirms that tools/e2e-scenarios/free-standing-workflow-inventory.mts is missing the required entries:
upgrade-stale-sandbox-vitestnot inallowed_jobsupgrade-stale-sandboxnot infree_standing_scenarios_csv- Mapping not in
free_standing_scenario_jobs_csv
Since the workflow's generate-matrix step validates inventory coverage, these missing entries will cause CI failures. Add all three required entries to the inventory file before merging.
Minor style note: Line 1781 has excessive inline whitespace in the vitest command. Consider using backslash line continuation for consistency.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e-vitest-scenarios.yaml around lines 1698 - 1799, The
free-standing workflow inventory file is missing three required entries that
will cause CI failures during the generate-matrix validation step. Open
tools/e2e-scenarios/free-standing-workflow-inventory.mts and add
upgrade-stale-sandbox-vitest to the allowed_jobs array, upgrade-stale-sandbox to
the free_standing_scenarios_csv string, and create the mapping entry in
free_standing_scenario_jobs_csv to connect the scenario to its job.
Additionally, on line 1781 in the upgrade-stale-sandbox-vitest job, the npx
vitest run command has excessive inline whitespace; refactor it to use backslash
line continuation for better readability and consistency with other multi-line
commands in the workflow.
…ade-stale-sandbox
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
…andbox' into e2e-migrate/test-upgrade-stale-sandbox
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
…andbox' into e2e-migrate/test-upgrade-stale-sandbox Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
79 tasks
This was referenced Jun 22, 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
2 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
Migrates
test/e2e/test-upgrade-stale-sandbox.shinto a typed live Vitest scenario. The new coverage preserves the #1904 stale sandbox upgrade contract through real install.sh, Docker image build, OpenShell sandbox creation,upgrade-sandboxes --check, and rebuild boundaries.Related Issue
Refs #5098
Changes
test/e2e-scenario/live/upgrade-stale-sandbox.test.tswith typed artifact capture, cleanup, and secret redaction.upgrade-stale-sandbox-vitestinto.github/workflows/e2e-vitest-scenarios.yamlas a free-standing dispatchable Vitest job.Type of Change
Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Targeted commands run:
npx biome check --write test/e2e-scenario/live/upgrade-stale-sandbox.test.tsNEMOCLAW_RUN_E2E_SCENARIOS=1 npx vitest run --project e2e-scenarios-live test/e2e-scenario/live/upgrade-stale-sandbox.test.ts -t __compile_only_nomatch__ --silent=false --reporter=default --passWithNoTestsnpx vitest run --project e2e-vitest-support test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tsnpx tsx scripts/check-test-file-size-budget.ts test/e2e-scenario/live/upgrade-stale-sandbox.test.tsSigned-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Tests
Chores