refactor(e2e): route live-Vitest invocations through a validated helper - #6996
Conversation
The repeated live-Vitest command shape in e2e.yaml now runs through tools/e2e/live-vitest-invocation.mts, which fixes the e2e-live project, reporters, and the silence flag and rejects unsafe test paths and selectors before spawning without a shell. Adopted across the 69 jobs sharing the exact contract; the variable-project, templated-file, and reordered-flag jobs, Docker Hub authentication, and expect host installs stay explicit. Workflow-boundary validators now assert the helper contract. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
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:
📝 WalkthroughWalkthroughLive E2E workflows now use a validated ChangesLive Vitest invocation standardization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant LiveVitestInvocation
participant Vitest
Workflow->>LiveVitestInvocation: invoke with test path and selector
LiveVitestInvocation->>LiveVitestInvocation: validate invocation inputs
LiveVitestInvocation->>Vitest: spawn npx vitest with reporters
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
laitingsheng
added
the
refactor
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@tools/e2e/live-vitest-invocation.mts`:
- Around line 97-98: Update the exit handling after spawnSync in the live Vitest
invocation to first surface result.error for launch failures, then preserve
result.signal by re-emitting the received signal, and only use result.status
when available; retain a fallback exit code for unexpected missing status.
🪄 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: e5d154c9-dda0-4f57-9de7-a8698f36b01e
📒 Files selected for processing (14)
.github/workflows/e2e.yamltest/e2e/support/e2e-operations-workflow-boundary.test.tstest/e2e/support/live-vitest-invocation.test.tstest/e2e/support/mcp-workflow-boundary.test.tstools/e2e/hermes-dashboard-workflow-boundary.mtstools/e2e/hermes-gpu-startup-workflow-boundary.mtstools/e2e/live-vitest-invocation.mtstools/e2e/mcp-workflow-boundary.mtstools/e2e/openclaw-plugin-runtime-exdev-workflow-boundary.mtstools/e2e/openshell-gateway-auth-contract-workflow-boundary.mtstools/e2e/operations-workflow-boundary.mtstools/e2e/sandbox-operations-workflow-boundary.mtstools/e2e/upload-e2e-artifacts-workflow-boundary.mtstools/e2e/workflow-boundary.mts
Missing npx and signal-terminated runs both exited 1, hiding launch failures and interrupts behind identical diagnostics. Throw the spawn error and reuse the shared exit-code mapping so each case reports distinctly. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
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 `@tools/e2e/live-vitest-invocation.mts`:
- Around line 103-109: Update parseArgs and the runLiveVitestCli argument flow
to reject unknown CLI options and throw or otherwise fail when a recognized
value-taking option such as --selector is provided without a value, while
preserving the distinction between an omitted selector and a supplied selector
value. Add regression tests covering an unknown option typo and a bare
--selector, ensuring neither silently runs the full test file.
🪄 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: fc779e81-b959-490b-854d-fca8c378a0a8
📒 Files selected for processing (14)
.github/workflows/e2e.yamltest/e2e/support/e2e-operations-workflow-boundary.test.tstest/e2e/support/live-vitest-invocation.test.tstest/e2e/support/mcp-workflow-boundary.test.tstools/e2e/hermes-dashboard-workflow-boundary.mtstools/e2e/hermes-gpu-startup-workflow-boundary.mtstools/e2e/live-vitest-invocation.mtstools/e2e/mcp-workflow-boundary.mtstools/e2e/openclaw-plugin-runtime-exdev-workflow-boundary.mtstools/e2e/openshell-gateway-auth-contract-workflow-boundary.mtstools/e2e/operations-workflow-boundary.mtstools/e2e/sandbox-operations-workflow-boundary.mtstools/e2e/upload-e2e-artifacts-workflow-boundary.mtstools/e2e/workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (10)
- tools/e2e/sandbox-operations-workflow-boundary.mts
- tools/e2e/openshell-gateway-auth-contract-workflow-boundary.mts
- tools/e2e/upload-e2e-artifacts-workflow-boundary.mts
- tools/e2e/hermes-gpu-startup-workflow-boundary.mts
- tools/e2e/operations-workflow-boundary.mts
- tools/e2e/hermes-dashboard-workflow-boundary.mts
- test/e2e/support/mcp-workflow-boundary.test.ts
- tools/e2e/mcp-workflow-boundary.mts
- tools/e2e/workflow-boundary.mts
- .github/workflows/e2e.yaml
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Approved exact head 30059b5 against base b528371. All 54 current checks are green. Deterministic E2E plan f6e940c030389233afdbe4647e95cc6683fdc1a74165f1f5b7698844c669eb34 passed cloud-onboard, credential-sanitization, and both security-posture shards in controller 29488323832 / child 29488351352. Full e2e-support passed 121 files / 1096 tests; focused security-posture validation passed; CLI typecheck and check:diff passed. Canonical exact-head Advisor has 0 blockers, warnings, or suggestions; CodeRabbit has no unresolved threads. Nine-category security review passed: strict argv validation and no shell execution, no broader secret or authorization scope, and preserved reporter, environment, artifact, timeout, exit, and cleanup behavior. DCO is present and all 9 commits are GitHub Verified. Comparator selects this PR over #6984; #6984 remains changes-requested with incomplete compatible-job migration and unsafe direct-entry/process handling.
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Approved at exact head 30059b5 on base b528371 after substantive security-posture helper migration; all 54 current checks green, credential sanitization/cloud onboard/OpenClaw+Hermes security posture E2E passed, focused 62 and full 1,096 E2E-support tests passed, DCO and 9/9 commits Verified, and no unresolved major security/advisor finding.
<!-- markdownlint-disable MD041 --> ## Summary Restore live E2E execution through the workflow's `npx tsx` command. The shared helper now loads the root CommonJS exit utility correctly instead of failing every converted live job during module initialization. ## Changes - Normalize the CommonJS and ESM representations of `src/lib/core/process-exit.ts` in `tools/e2e/live-vitest-invocation.mts`. The current consumer is the live E2E workflow introduced by #6996; `tsx` and Vitest expose the root module differently, so the existing repository interop pattern is required to keep one shared exit-code implementation. - Run the helper's subprocess regression coverage through the local `tsx` executable that `npx tsx` resolves, protecting the production loader boundary that the previous native Node test did not exercise. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This is an internal CI loader fix; the workflow command and all user-facing behavior remain unchanged. - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run --project e2e-support test/e2e/support/live-vitest-invocation.test.ts` passed 24 tests; `npm run typecheck:cli` and Biome also passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — not applicable to this focused CI helper fix. The full E2E-support attempt reached five unrelated failures in an unchanged platform-parity shell script under macOS Bash 3.2; the other 41 tests in the diagnostic subset passed. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved handling of exit codes when running live Vitest commands across different module export formats. * Updated end-to-end workflow checks to invoke the CLI consistently, preserving expected failure reporting. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
2 participants
Summary
The live-Vitest command shape repeated across
.github/workflows/e2e.yamlis now built by a validated first-party helper,tools/e2e/live-vitest-invocation.mts, so a job supplies only a test path and optional selector while thee2e-liveproject, reporters, and silence flag are fixed and matrix-derived inputs are checked before Vitest runs without a shell. The helper is adopted in the 70 jobs that share the exact invocation contract; the remaining jobs and the privileged setup stay explicit.Related Issue
Refs #6961
Changes
tools/e2e/live-vitest-invocation.mts: fixes thee2e-liveproject,--reporter=default,--reporter=test/e2e/risk-signal-reporter.ts, and--silent=false; validates the test path (undertest/e2e/live/, a.test.tsfile, no..traversal, no absolute path, no shell metacharacters) and the optional-tselector (no shell metacharacters); builds an argv array and spawns without a shell, fail-closed. Current consumer: the 70 adoptede2e.yamljobs. A direct inline change is insufficient because the shape is duplicated across ~70 jobs and the matrix-derived path and selector cross a trust boundary that must be validated once in a directly tested unit. Protected bytest/e2e/support/live-vitest-invocation.test.ts..github/workflows/e2e.yaml, including the templated security-posture matrix; per-job PATH/OpenShell resolution prefixes andenv -ucredential-scrubbing wrappers are unchanged.tools/e2e/workflow-boundary.mtsand the per-job*-workflow-boundary.mts, plus the operations, MCP, and upload-artifact validators) to accept the helper invocation as the live-Vitest contract and as the risk-signal-reporter attachment.expecthost installs (privileged setup).Type of Change
Quality Gates
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project e2e-support→ 1082 passed; the single unrelated failureworkflow-plan › reports CLI failures as workflow annotationsis a local tsxmodule.register()deprecation-warning-on-stderr flake that reproduces on an unmodified base checkout and passes on CI's pinned Node.npx vitest run --project integration test/regression-e2e-workflow.test.ts test/e2e-recommendations.test.tspassed.npm run typecheck:cli,npm run test:projects:check, and Biome all clean.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
New Features
Tests