Skip to content

ci(e2e): filter Vitest result comments - #5593

Merged
jyaunches merged 1 commit into
mainfrom
fix/vitest-result-comment-filter
Jun 22, 2026
Merged

ci(e2e): filter Vitest result comments#5593
jyaunches merged 1 commit into
mainfrom
fix/vitest-result-comment-filter

Conversation

@jyaunches

@jyaunches jyaunches commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Filter Vitest E2E result comments to selected/requested jobs for selective dispatches
  • Add cancelled and missing-requested-job handling to match nightly E2E comments
  • Extend workflow boundary checks so the compact reporting behavior does not regress

Testing

  • npm run format:check
  • npm test -- --run test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts

Summary by CodeRabbit

  • Chores
    • Improved PR comment reporting for e2e test runs with enhanced selective dispatch handling, cancelled job tracking, and missing requested job notifications.

@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The report-to-pr GitHub Script in the e2e Vitest workflow is extended to support selective workflow-dispatch runs: it now parses inputs.jobs into a requested-job set, filters reported entries, computes missing requested jobs, tracks cancelled counts, and adjusts PR comment status logic. Matching validations and test assertions are added in the workflow boundary tool and its test file.

Changes

report-to-pr selective dispatch & cancelled handling

Layer / File(s) Summary
PR comment script: selective dispatch, cancelled, and missing jobs
.github/workflows/e2e-vitest-scenarios.yaml
Parses inputs.jobs into a requestedSet, derives selectiveDispatch, filters needs entries into reportedEntries, expands status logic to cover cancelled/pass/skipped/no-selected-jobs combinations, adds cancelled count to the summary line, and appends a "Missing requested jobs" block when applicable.
Workflow boundary validation and test assertions
tools/e2e-scenarios/workflow-boundary.mts, test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts
Adds three new reportScript content checks in validateE2eVitestScenariosWorkflowBoundary for reportedEntries, missingRequested, and cancelled; extends the existing test case with three matching expected error assertions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • NVIDIA/NemoClaw#5243: Introduced selector-based reporting in report-to-pr and the associated workflow-boundary/test logic that this PR extends with reportedEntries/missingRequested/cancelled markers.
  • NVIDIA/NemoClaw#5329: Earlier rewrote report-to-pr and validateE2eVitestScenariosWorkflowBoundary to use inputs.jobs only, which this PR builds upon.
  • NVIDIA/NemoClaw#5330: Modified report-to-pr logic and workflow-boundary.mts validations to gate PR-comment table reporting based on selector-validation outcomes, directly adjacent to this PR's changes.

Suggested labels

area: ci, area: e2e, chore, v0.0.66

Suggested reviewers

  • cv

Poem

🐇 A rabbit hopped through workflow lanes,
Counting cancelled jobs with care,
Missing requests? Now flagged with flair!
Selective dispatch, tidy and bright—
The PR table finally right. ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'ci(e2e): filter Vitest result comments' directly and accurately summarizes the main change: filtering Vitest result comments in the CI/CD E2E workflow for selective dispatches.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/vitest-result-comment-filter

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

@github-code-quality

github-code-quality Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the fix/vitest-result-co... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/vitest-result-co... 7758820 +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/li...bprocess-env.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/bl...print/runner.ts 95%
nemoclaw/src/co...ration-state.ts 94%
nemoclaw/src/bl...ate-networks.ts 94%
nemoclaw/src/index.ts 94%

TypeScript / code-coverage/cli

The overall coverage in the fix/vitest-result-co... branch is 46%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/vitest-result-co... 7758820 +/-
src/lib/state/o...oard-session.ts 91%
src/lib/inference/local.ts 76%
src/lib/sandbox/config.ts 72%
src/lib/actions...dbox/rebuild.ts 67%
src/lib/onboard/preflight.ts 64%
src/lib/actions...licy-channel.ts 56%
src/lib/state/sandbox.ts 55%
src/lib/policy/index.ts 49%
src/lib/onboard...er-gpu-patch.ts 44%
src/lib/onboard.ts 18%

Updated June 22, 2026 17:50 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: None
Optional E2E: None

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • None. No product E2E is required. The changed files affect GitHub Actions reporting and static workflow-boundary validation only; they do not change installer/onboarding, sandbox lifecycle, credentials handling in runtime code, security boundaries, network policy enforcement, inference routing, deployment behavior, or real assistant user flows.

Optional E2E

  • None.

New E2E recommendations

  • ci-workflow-reporting (medium): Existing boundary coverage appears to validate the workflow report script mostly by static string checks. A dry-run integration test with synthetic needs data would provide stronger coverage for selective dispatches, missing requested jobs, and cancelled states without running live E2E scenarios.
    • Suggested test: Add a report-to-pr rendering dry-run test for selective workflow_dispatch reporting

@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Recommendation

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

Dispatch required Vitest E2E scenarios:

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

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

  • e2e-scenarios-all: The PR changes the shared Vitest scenario workflow machinery and its workflow-boundary validation/reporting behavior. Per policy, workflow machinery changes require the full Vitest scenario fan-out.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref>

Optional Vitest E2E scenarios

  • None.

Relevant changed files

  • .github/workflows/e2e-vitest-scenarios.yaml
  • test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts
  • tools/e2e-scenarios/workflow-boundary.mts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/e2e-vitest-scenarios.yaml:
- Around line 4509-4513: The fallback logic for reportedEntries in the selective
dispatch case is incorrectly including all non-skipped entries from allEntries,
which can reintroduce the generate-matrix entry that was explicitly excluded
earlier. When selectiveDispatch is true and selectedEntries is empty, the filter
should exclude generate-matrix along with skipped entries to prevent a no-signal
run from being marked as passing. Modify the selective dispatch fallback
condition to also filter out generate-matrix entries in addition to filtering
out skipped results, matching the explicit exclusion logic that appears on line
4506.
🪄 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: 9d3fdcea-a520-4796-ac8e-5e96a6a71cf2

📥 Commits

Reviewing files that changed from the base of the PR and between 32d5008 and 7758820.

📒 Files selected for processing (3)
  • .github/workflows/e2e-vitest-scenarios.yaml
  • test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts
  • tools/e2e-scenarios/workflow-boundary.mts

Comment on lines +4509 to +4513
const reportedEntries = selectedEntries.length > 0
? selectedEntries
: selectiveDispatch
? allEntries.filter(([, { result }]) => result !== 'skipped')
: allEntries;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

reportedEntries fallback can incorrectly mark a no-signal run as passing.

On Line 4511, the selective-dispatch fallback repopulates from all non-skipped needs, which can pull generate-matrix back in as a passing entry after Line 4506 explicitly excluded it. That makes status resolve to a pass even when no selected jobs actually ran.

💡 Suggested fix
-            const reportedEntries = selectedEntries.length > 0
-              ? selectedEntries
-              : selectiveDispatch
-                ? allEntries.filter(([, { result }]) => result !== 'skipped')
-                : allEntries;
+            const reportedEntries = selectedEntries.length > 0
+              ? selectedEntries
+              : selectiveDispatch
+                ? allEntries.filter(([name]) => name !== 'generate-matrix')
+                : allEntries;

Also applies to: 4521-4540

🤖 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 4509 - 4513, The
fallback logic for reportedEntries in the selective dispatch case is incorrectly
including all non-skipped entries from allEntries, which can reintroduce the
generate-matrix entry that was explicitly excluded earlier. When
selectiveDispatch is true and selectedEntries is empty, the filter should
exclude generate-matrix along with skipped entries to prevent a no-signal run
from being marked as passing. Modify the selective dispatch fallback condition
to also filter out generate-matrix entries in addition to filtering out skipped
results, matching the explicit exclusion logic that appears on line 4506.

@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Source-of-truth review needed: Selective Vitest result-comment filtering and fallback reporting.
Open items: 0 required · 2 warnings · 0 suggestions · 8 test follow-ups
Top item: Add behavioral coverage for selective result-comment rendering

Action checklist

  • PRA-1 Resolve or justify: Source-of-truth review needed: Selective Vitest result-comment filtering and fallback reporting
  • PRA-2 Resolve or justify: Add behavioral coverage for selective result-comment rendering in test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts:956
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Runtime validation
  • PRA-T6 Add or justify test follow-up: Add behavioral coverage for selective result-comment rendering
  • PRA-T7 Add or justify test follow-up: Acceptance clause
  • PRA-T8 Add or justify test follow-up: Selective Vitest result-comment filtering and fallback reporting

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify tests test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts:956 Add focused behavior-level coverage for the changed reporter logic in this PR. Keep it local and small: either execute the `github-script` body with stubbed `github`, `core`, `context`, and synthetic `needs`, or factor just the report-entry/status calculation into a tiny testable helper used by the script. Avoid introducing a new runner or generalized workflow-testing framework.
Review findings by urgency: 0 required fixes, 2 items to resolve/justify, 0 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: Selective Vitest result-comment filtering and fallback reporting

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Current regression coverage only checks string presence in the embedded script. Behavior-level tests should prove row selection, missing-job rows, cancelled status buckets, and rejected-selector redaction.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `.github/workflows/e2e-vitest-scenarios.yaml` adds `selectedEntries`/`reportedEntries` fallback logic and missing/cancelled handling; `tools/e2e-scenarios/workflow-boundary.mts` validates only that key tokens appear in the script.

PRA-2 Resolve/justify — Add behavioral coverage for selective result-comment rendering

  • Location: test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts:956
  • Category: tests
  • Problem: The PR changes the actual PR-comment signal for selective dispatches, missing requested jobs, and cancelled jobs, but the regression coverage added here only checks that the workflow script contains variable names such as `reportedEntries`, `missingRequested`, and `cancelled`. That would still pass if the variables were present but the table/status logic posted unrelated skipped jobs, omitted missing requested rows, or reported cancelled-only runs as passing.
  • Impact: A future workflow edit could silently regress the compact result comments and give maintainers a misleading pass/fail signal for E2E scenario dispatches, especially because this central workflow is under active same-file churn from other Vitest migration PRs.
  • Recommended action: Add focused behavior-level coverage for the changed reporter logic in this PR. Keep it local and small: either execute the `github-script` body with stubbed `github`, `core`, `context`, and synthetic `needs`, or factor just the report-entry/status calculation into a tiny testable helper used by the script. Avoid introducing a new runner or generalized workflow-testing framework.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Shortest read-only check: inspect `tools/e2e-scenarios/workflow-boundary.mts` around the `reportScript.includes(...)` checks and `test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts` around the added expected errors; confirm they assert token presence rather than rendered rows/statuses.
  • Missing regression test: Add tests named for the behaviors: `selective jobs report includes only requested job rows and omits skipped unrelated jobs`, `missing requested job renders a not reported row and non-pass status`, `cancelled-only selected jobs render Run cancelled — no signal`, `mixed success and cancelled selected jobs render Some jobs cancelled — partial pass`, and `rejected selector comments omit raw requested selectors and surface generate-matrix failure`.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Shortest read-only check: inspect `tools/e2e-scenarios/workflow-boundary.mts` around the `reportScript.includes(...)` checks and `test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts` around the added expected errors; confirm they assert token presence rather than rendered rows/statuses.
  • Evidence: The workflow logic is in `.github/workflows/e2e-vitest-scenarios.yaml` lines 4499-4570, while the validator additions in `tools/e2e-scenarios/workflow-boundary.mts` only check `reportScript.includes("reportedEntries")`, `reportScript.includes("missingRequested")`, and `reportScript.includes("cancelled")`; the changed test only expects those validator errors.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

  • None.
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — selective jobs report includes only requested job rows and omits skipped unrelated jobs. This PR changes workflow infrastructure and PR-comment status semantics. Nearby support tests exist, but the new checks are mostly syntactic; behavior-level validation would give confidence without relying on external E2E job status.
  • PRA-T2 Runtime validation — missing requested job renders a not reported row and non-pass status. This PR changes workflow infrastructure and PR-comment status semantics. Nearby support tests exist, but the new checks are mostly syntactic; behavior-level validation would give confidence without relying on external E2E job status.
  • PRA-T3 Runtime validation — cancelled-only selected jobs render Run cancelled — no signal instead of pass. This PR changes workflow infrastructure and PR-comment status semantics. Nearby support tests exist, but the new checks are mostly syntactic; behavior-level validation would give confidence without relying on external E2E job status.
  • PRA-T4 Runtime validation — mixed success and cancelled selected jobs render Some jobs cancelled — partial pass. This PR changes workflow infrastructure and PR-comment status semantics. Nearby support tests exist, but the new checks are mostly syntactic; behavior-level validation would give confidence without relying on external E2E job status.
  • PRA-T5 Runtime validation — rejected selector comments omit raw requested selectors and surface generate-matrix failure. This PR changes workflow infrastructure and PR-comment status semantics. Nearby support tests exist, but the new checks are mostly syntactic; behavior-level validation would give confidence without relying on external E2E job status.
  • PRA-T6 Add behavioral coverage for selective result-comment rendering — Add focused behavior-level coverage for the changed reporter logic in this PR. Keep it local and small: either execute the `github-script` body with stubbed `github`, `core`, `context`, and synthetic `needs`, or factor just the report-entry/status calculation into a tiny testable helper used by the script. Avoid introducing a new runner or generalized workflow-testing framework.
  • PRA-T7 Acceptance clause — Extend workflow boundary checks so the compact reporting behavior does not regress — add test evidence or identify existing coverage. `tools/e2e-scenarios/workflow-boundary.mts` and the support test now require the new script tokens, but they do not yet prove the rendered report rows/statuses behave correctly.
  • PRA-T8 Selective Vitest result-comment filtering and fallback reporting — Current regression coverage only checks string presence in the embedded script. Behavior-level tests should prove row selection, missing-job rows, cancelled status buckets, and rejected-selector redaction.. `.github/workflows/e2e-vitest-scenarios.yaml` adds `selectedEntries`/`reportedEntries` fallback logic and missing/cancelled handling; `tools/e2e-scenarios/workflow-boundary.mts` validates only that key tokens appear in the script.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@cv cv added the v0.0.66 label Jun 22, 2026
@jyaunches
jyaunches merged commit 8af7f42 into main Jun 22, 2026
46 checks passed
@jyaunches
jyaunches deleted the fix/vitest-result-comment-filter branch June 22, 2026 18:29
@wscurran wscurran added area: ci CI workflows, checks, release automation, or GitHub Actions area: e2e End-to-end tests, nightly failures, or validation infrastructure chore Build, CI, dependency, or tooling maintenance labels Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ci CI workflows, checks, release automation, or GitHub Actions area: e2e End-to-end tests, nightly failures, or validation infrastructure chore Build, CI, dependency, or tooling maintenance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants