test(e2e): migrate VM driver privexec coverage#5189
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR replaces a shell-script-based privileged-exec routing regression test with a Vitest test. It adds the new Vitest test file, removes the old shell script from test allowlists and CI workflow wiring, and asserts the retirement through updated contract tests. ChangesTest migration and CI job removal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 |
E2E Advisor RecommendationRequired E2E: None Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: None Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Selective E2E Results — ❌ Some jobs failedRun: 27315928722
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/vm-driver-privileged-exec-routing.test.ts`:
- Around line 88-101: In loadHelperWithFakeHome, replace use of path.delimiter
when building process.env.PATH with the POSIX separator ':' so tests use a
consistent POSIX PATH (i.e., change the PATH assignment that currently uses
path.delimiter to use ':' and keep the fallback to process.env.PATH or empty
string); update the assignment in the function loadHelperWithFakeHome to use
`${fakeBin}:${process.env.PATH ?? ""}` to ensure CI Linux runners get the
expected PATH format.
🪄 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: e22b0408-4c81-4158-b8cd-5726a5e81a11
📒 Files selected for processing (4)
.github/workflows/nightly-e2e.yamltest/e2e-script-workflow.test.tstest/e2e/test-vm-driver-privileged-exec-routing.shtest/vm-driver-privileged-exec-routing.test.ts
💤 Files with no reviewable changes (1)
- test/e2e/test-vm-driver-privileged-exec-routing.sh
PR Review AdvisorFindings: 0 needs attention, 0 worth checking, 0 nice ideas Consider writing more tests for
This is an automated advisory review. A human maintainer must make the final merge decision. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/nightly-e2e.yaml:
- Line 143: The .coderabbit.yaml path_instructions still reference the removed
job vm-driver-privileged-exec-routing-e2e for paths "src/lib/shields/**",
"src/lib/sandbox/privileged-exec.ts", and "src/lib/sandbox/config.ts"; update
.coderabbit.yaml so these entries no longer mention
vm-driver-privileged-exec-routing-e2e and instead map those paths to the actual
jobs exported by the nightly-e2e workflow (e.g., shields-config-e2e,
rebuild-openclaw-e2e or other current job names), or remove the
path_instructions entirely for those files if no matching job exists. Ensure
references to vm-driver-privileged-exec-routing-e2e are removed so
.coderabbit.yaml matches the job set in the workflow.
🪄 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: e9d618db-8fa9-423c-a272-74b02d6ff503
📒 Files selected for processing (1)
.github/workflows/nightly-e2e.yaml
|
Addressed PR Review Advisor worth-checking item with commit |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e-script-workflow.test.ts (1)
222-227: ⚡ Quick winReuse the contract loaded on line 185 instead of re-calling
loadE2eWorkflowContract().
loadE2eWorkflowContract()is already called on line 185 to load the workflow contracts for the entire test suite. Now that it returnscliCoverageShardAction(per the helper changes), add it to the line 185 destructuring and remove the redundant call on line 223.♻️ Proposed refactor
Update line 185 to include
cliCoverageShardAction:- const { runnerWorkflow, nightlyWorkflow, action } = loadE2eWorkflowContract(); + const { runnerWorkflow, nightlyWorkflow, action, cliCoverageShardAction } = loadE2eWorkflowContract();Then remove the redundant call on line 223:
it("keeps the unwired VM driver privileged-exec lane covered by CLI Vitest", () => { - const { cliCoverageShardAction } = loadE2eWorkflowContract(); const runStepNames = cliCoverageShardAction.runs.steps.map((step) => step.name);🤖 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-script-workflow.test.ts` around lines 222 - 227, The test re-calls loadE2eWorkflowContract() to get cliCoverageShardAction even though that value is already returned by the earlier call; update the earlier destructuring where loadE2eWorkflowContract() is first invoked to include cliCoverageShardAction, then remove the redundant call and use the existing cliCoverageShardAction variable for computing runStepNames and cliShardRunStep (references: loadE2eWorkflowContract, cliCoverageShardAction, runStepNames, cliShardRunStep).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/e2e-script-workflow.test.ts`:
- Around line 222-227: The test re-calls loadE2eWorkflowContract() to get
cliCoverageShardAction even though that value is already returned by the earlier
call; update the earlier destructuring where loadE2eWorkflowContract() is first
invoked to include cliCoverageShardAction, then remove the redundant call and
use the existing cliCoverageShardAction variable for computing runStepNames and
cliShardRunStep (references: loadE2eWorkflowContract, cliCoverageShardAction,
runStepNames, cliShardRunStep).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b9715a50-d910-4b2d-b476-060900a87c25
📒 Files selected for processing (3)
.coderabbit.yamltest/e2e-script-workflow.test.tstest/helpers/e2e-workflow-contract.ts
Summary
Migrate the
test/e2e/test-vm-driver-privileged-exec-routing.shcontract into focused Vitest coverage while keeping the legacy shell file present for now.Seeded from Carlos's PR #5144, but rebuilt on current
mainwith only the one-script migration changes and no #5126-era framework diff.Related Issues
Refs #5098
Refs #4245
Contract mapping
openshell-gateway-nemoclaw.test/vm-driver-privileged-exec-routing.test.tsassertsalphaselectsopenshell-alpha-abc123while ignoringopenshell-alpha-childand gateway containers.dist/lib/sandbox/privileged-exec.jshelper plus fake registry and fakedocker pssubprocess boundary.alpha-childselectsopenshell-alpha-child.dockerboxandunknown-drivercases assert direct container argv.~/.nemoclaw/sandboxes.json.No running direct OpenShell sandbox container...driver: vmerror.docker pscontains only gateway/unrelated containers.Simplicity check
test/.test/e2e/and in the top-level frozen shell allowlist; replacement coverage runs in normal CLI Vitest.Verification
npm run build:clinpx vitest run --project cli test/vm-driver-privileged-exec-routing.test.ts --silent=false --reporter=defaultnpx vitest run --project cli test/e2e-script-workflow.test.ts --silent=false --reporter=defaultvm-driver-privileged-exec-routing-e2elane is absent from nightly dispatch/wiring, the legacy shell file remains present, the replacement test is in the CLI Vitest include path, and the CLI coverage shard rebuildsdistbefore Vitestnpm run test-size:check -- test/vm-driver-privileged-exec-routing.test.ts test/e2e-script-workflow.test.tsgit diff --checkCI follow-up
The first selective E2E auto-dispatch still used the trusted
mainnightly workflow, which attempted to run the shell-script lane. This PR now removesvm-driver-privileged-exec-routing-e2efrom the nightly dispatch input/list/job definitions so the unwired lane is not auto-dispatched. The legacy shell file has been restored per maintainer preference; this PR no longer deletes any file undertest/e2e/.Summary by CodeRabbit
Tests
Chores