ci(e2e): use jobs selector only#5329
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 removes the ChangesE2E Vitest Dispatch Selector Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/e2e-vitest-scenarios.yaml (1)
56-98:⚠️ Potential issue | 🟠 MajorMake
generate-matrixtruly no-op forworkflow_dispatchruns that supplyinputs.jobs.Even when
inputs.jobsis non-empty (so thelive-scenariosjob is skipped viaif: inputs.jobs == '' ...andgenerate-matrixsetsmatrix="[]"), thegenerate-matrixjob still runsactions/checkout,actions/setup-node, andnpm cibefore reaching that logic. A transient failure in those steps will failgenerate-matrixand then cause the PRreport-to-prcomment to showgenerate-matrixas failed (it hasneeds: [...] generate-matrixandif: always()), despite no registry scenarios being requested.Gate the checkout/node/npm steps behind
inputs.jobs == '', or splitgenerate-matrixinto a lightweight path that only outputs[]wheninputs.jobsis provided.🤖 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 56 - 98, The generate-matrix job currently always runs heavy setup steps even when inputs.jobs is provided; update it so the checkout/setup-node/npm steps are skipped when inputs.jobs is non-empty by gating those steps on the inputs.jobs check (e.g., add an if condition that runs them only when inputs.jobs == ''), or split generate-matrix into a lightweight branch that immediately emits matrix="[]" when inputs.jobs is provided and only runs the npx/tsx generation path otherwise; reference the job name generate-matrix and the steps actions/checkout, actions/setup-node, and the "Install root dependencies" npm ci step to locate where to add the condition.
🧹 Nitpick comments (1)
tools/e2e-scenarios/workflow-boundary.mts (1)
340-345: ⚡ Quick winReuse
validateFreeStandingJobSelectorfor the remaining free-standing jobs.
network-policy-vitest,hermes-e2e-vitest, andhermes-root-entrypoint-smoke-viteststill duplicate the sharedneeds/ifcontract, and the Hermes root variant hardcodes the full selector expression string. Keeping these copies alongsidefreeStandingJobIf()/validateFreeStandingJobSelector()makes the jobs-only rule easy to drift again.♻️ Proposed simplification
- if (job.needs !== "validate-jobs") { - errors.push("hermes-root-entrypoint-smoke-vitest job must depend on validate-jobs"); - } - const expectedIf = - "${{ inputs.jobs == '' || contains(format(',{0},', inputs.jobs), ',hermes-root-entrypoint-smoke-vitest,') }}"; - if (job.if !== expectedIf) { - errors.push( - "hermes-root-entrypoint-smoke-vitest job must use the shared jobs selector condition", - ); - } + validateFreeStandingJobSelector(errors, jobs, jobName);Also applies to: 973-978, 1073-1080
🤖 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 `@tools/e2e-scenarios/workflow-boundary.mts` around lines 340 - 345, The checks for the free-standing jobs are duplicated; replace the manual comparisons for network-policy-vitest, hermes-e2e-vitest, and hermes-root-entrypoint-smoke-vitest with a single call to the existing validateFreeStandingJobSelector helper and stop hardcoding the full selector string (use freeStandingJobIf(jobName) where needed). Locate the blocks that currently assert job.needs and job.if (the ones using the literal selector or separate checks) and call validateFreeStandingJobSelector(jobName, job) or otherwise delegate to that function to validate both needs and if consistently; remove the duplicated needs/if assertions and the hardcoded selector in hermes-root-entrypoint-smoke-vitest so all free-standing jobs use the shared validation logic.
🤖 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:
- Line 24: The concurrency.group currently uses the raw `${{ inputs.jobs }}`
string which is order-sensitive; update the workflow so the concurrency key is
order-insensitive by canonicalizing the inputs.jobs string (e.g., split, sort,
and rejoin the job names) before using it in concurrency.group or alternatively
switch the concurrency key to a coarser identifier like `${{ github.ref }}` for
jobs-only runs; locate the concurrency.group usage in e2e-vitest-scenarios.yaml
and modify the expression that references `inputs.jobs` (or replace it) so that
dispatches with the same job set in different orders map to the same concurrency
group.
In `@test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts`:
- Around line 473-477: The arrayContaining assertion in the test is missing the
check that the "report-to-pr" step passes jobs via the JOBS env, which lets a
broken boundary slip through; add the string "report-to-pr step must pass jobs
through JOBS env" into the arrayContaining list used when validating
validateE2eVitestScenariosWorkflowBoundary() so the test will fail when the
fixture (which sets report-to-pr.env.JOBS to "bad") violates the JOBS wiring
contract for the report-to-pr step.
- Around line 69-76: The test currently uses the selector string
"network-policy,../escape" which mixes a traversal token with a non-existent job
id and can mask whether selector-pattern validation or allowed-job validation is
failing; update the test to use a valid job id plus the traversal token (e.g.
"network-policy-vitest,../escape") and change the expectation to assert the
specific error path by matching the "Invalid jobs input" failure from
evaluateE2eVitestWorkflowDispatchSelectors so the malformed-selector (traversal)
rejection is isolated; locate the test case in e2e-scenarios-workflow.test.ts
and update the input string and expected result accordingly.
In `@tools/e2e-scenarios/workflow-boundary.mts`:
- Around line 21-37: ALLOWED_FREE_STANDING_JOBS has drifted from the downstream
validators: either remove the two outdated ids ("launchable-smoke-vitest" and
"issue-4434-tui-unreachable-inference-vitest") from the
ALLOWED_FREE_STANDING_JOBS set or make validate-jobs, generate-matrix, the
per-job selector assertions (allowed_jobs), and report-to-pr.needs derive their
allowed list from ALLOWED_FREE_STANDING_JOBS or the same shared source used by
evaluateE2eVitestWorkflowDispatchSelectors() so all checks use a single source
of truth (update references in evaluateE2eVitestWorkflowDispatchSelectors(),
validate-jobs, generate-matrix, allowed_jobs assertions, and report-to-pr.needs
accordingly).
---
Outside diff comments:
In @.github/workflows/e2e-vitest-scenarios.yaml:
- Around line 56-98: The generate-matrix job currently always runs heavy setup
steps even when inputs.jobs is provided; update it so the
checkout/setup-node/npm steps are skipped when inputs.jobs is non-empty by
gating those steps on the inputs.jobs check (e.g., add an if condition that runs
them only when inputs.jobs == ''), or split generate-matrix into a lightweight
branch that immediately emits matrix="[]" when inputs.jobs is provided and only
runs the npx/tsx generation path otherwise; reference the job name
generate-matrix and the steps actions/checkout, actions/setup-node, and the
"Install root dependencies" npm ci step to locate where to add the condition.
---
Nitpick comments:
In `@tools/e2e-scenarios/workflow-boundary.mts`:
- Around line 340-345: The checks for the free-standing jobs are duplicated;
replace the manual comparisons for network-policy-vitest, hermes-e2e-vitest, and
hermes-root-entrypoint-smoke-vitest with a single call to the existing
validateFreeStandingJobSelector helper and stop hardcoding the full selector
string (use freeStandingJobIf(jobName) where needed). Locate the blocks that
currently assert job.needs and job.if (the ones using the literal selector or
separate checks) and call validateFreeStandingJobSelector(jobName, job) or
otherwise delegate to that function to validate both needs and if consistently;
remove the duplicated needs/if assertions and the hardcoded selector in
hermes-root-entrypoint-smoke-vitest so all free-standing jobs use the shared
validation logic.
🪄 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: ec6a73c2-eb2a-48e1-82ea-fdc00efd7068
📒 Files selected for processing (3)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstools/e2e-scenarios/workflow-boundary.mts
|
Thanks for pushing this simplification. I think the direction is right, but I’m requesting changes because this needs a little more cleanup before it becomes the base for #5098 execution. Required before merge:
This would help a lot with #5098 because it removes the confusing |
jyaunches
force-pushed
the
issue-5098-jobs-selector
branch
from
cc9d953 to
34c072a
Compare
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/e2e-vitest-scenarios.yaml:
- Around line 1062-1067: The job-level env block uses runner.temp (invalid at
job scope) for DOCKER_CONFIG; fix by either (A) replacing the runner.temp
reference with a workspace-based fixed path such as using ${{ github.workspace
}} for DOCKER_CONFIG, or (B) remove DOCKER_CONFIG from the job-level env and set
it in the specific step that requires it (e.g., the "Authenticate to Docker Hub"
step) using a step-level env or by exporting it to $GITHUB_ENV so subsequent
steps see it; leave the other job env entries (E2E_ARTIFACT_DIR,
NEMOCLAW_CLI_BIN, NEMOCLAW_RUN_E2E_SCENARIOS, OPENSHELL_GATEWAY) unchanged.
🪄 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: fa907bd3-3c45-4e86-bf73-1f845f83480e
📒 Files selected for processing (3)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstools/e2e-scenarios/workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts
- tools/e2e-scenarios/workflow-boundary.mts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 1062-1067: The job-level env block uses runner.temp (invalid at
job scope) for DOCKER_CONFIG; fix by either (A) replacing the runner.temp
reference with a workspace-based fixed path such as using ${{ github.workspace
}} for DOCKER_CONFIG, or (B) remove DOCKER_CONFIG from the job-level env and set
it in the specific step that requires it (e.g., the "Authenticate to Docker Hub"
step) using a step-level env or by exporting it to $GITHUB_ENV so subsequent
steps see it; leave the other job env entries (E2E_ARTIFACT_DIR,
NEMOCLAW_CLI_BIN, NEMOCLAW_RUN_E2E_SCENARIOS, OPENSHELL_GATEWAY) unchanged.
🪄 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: fa907bd3-3c45-4e86-bf73-1f845f83480e
📒 Files selected for processing (3)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstools/e2e-scenarios/workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts
- tools/e2e-scenarios/workflow-boundary.mts
🛑 Comments failed to post (1)
.github/workflows/e2e-vitest-scenarios.yaml (1)
1062-1067:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
runnercontext unavailable in job-levelenv:block.The
runnercontext is only available within step contexts, not in job-levelenv:blocks. Per GitHub Actions documentation, the available contexts at this level are:github,inputs,matrix,needs,secrets,strategy,vars. This will cause the workflow to fail at parse/runtime.🔧 Proposed fix: move DOCKER_CONFIG to step-level or use a fixed path
Option 1: Use a fixed path under
$GITHUB_WORKSPACEenv: - DOCKER_CONFIG: ${{ runner.temp }}/docker-config-model-router-provider-routed-inference + DOCKER_CONFIG: /tmp/docker-config-model-router-provider-routed-inference E2E_ARTIFACT_DIR: ${{ github.workspace }}/e2e-artifacts/vitest/model-router-provider-routed-inferenceOption 2: Set DOCKER_CONFIG in the step that needs it
RemoveDOCKER_CONFIGfrom job-levelenv:and add it to the "Authenticate to Docker Hub" step:- name: Authenticate to Docker Hub env: DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }} DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }} + DOCKER_CONFIG: ${{ runner.temp }}/docker-config-model-router-provider-routed-inference shell: bash run: | set -euo pipefail + export DOCKER_CONFIGThen export
DOCKER_CONFIGin subsequent steps that need it, or use$GITHUB_ENVto persist it.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.env: DOCKER_CONFIG: /tmp/docker-config-model-router-provider-routed-inference E2E_ARTIFACT_DIR: ${{ github.workspace }}/e2e-artifacts/vitest/model-router-provider-routed-inference NEMOCLAW_CLI_BIN: ${{ github.workspace }}/bin/nemoclaw.js NEMOCLAW_RUN_E2E_SCENARIOS: "1" OPENSHELL_GATEWAY: "nemoclaw"🧰 Tools
🪛 actionlint (1.7.12)
[error] 1063-1063: context "runner" is not allowed here. available contexts are "github", "inputs", "matrix", "needs", "secrets", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🤖 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 1062 - 1067, The job-level env block uses runner.temp (invalid at job scope) for DOCKER_CONFIG; fix by either (A) replacing the runner.temp reference with a workspace-based fixed path such as using ${{ github.workspace }} for DOCKER_CONFIG, or (B) remove DOCKER_CONFIG from the job-level env and set it in the specific step that requires it (e.g., the "Authenticate to Docker Hub" step) using a step-level env or by exporting it to $GITHUB_ENV so subsequent steps see it; leave the other job env entries (E2E_ARTIFACT_DIR, NEMOCLAW_CLI_BIN, NEMOCLAW_RUN_E2E_SCENARIOS, OPENSHELL_GATEWAY) unchanged.Source: Linters/SAST tools
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e-scenario-advisor.test.ts (1)
357-376:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a negative case for an invalid free-standing job selector.
These fixtures now use the shared jobs-only predicate, but there is still no test for the malformed variant that omits the
inputs.jobs == ''arm. That matters becauseextractFreeStandingVitestJobs()currently accepts any body containinginputs.jobsand,token-rotation-vitest,, so it can treat a boundary-invalid selector as workflow-wired. A negative case here would keep the advisor aligned with the workflow-boundary contract. Based on workflow-boundary support logic, free-standing jobs must use the shared${{ inputs.jobs == '' || contains(format(',{0},', inputs.jobs), ',<job>,') }}selector.Also applies to: 378-418
🤖 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-advisor.test.ts` around lines 357 - 376, Add a negative test case that verifies extractFreeStandingVitestJobs rejects selectors that omit the leading "inputs.jobs == ''" arm; create a fixture string in test/e2e-scenario-advisor.test.ts where the job selector only contains "contains(format(',{0},', inputs.jobs), ',token-rotation-vitest,')" (no inputs.jobs == '' check) and assert the function returns an empty array. Update both places noted (the existing positive case around the current diff and the similar block at 378-418) to include this malformed-selector test so the advisor stays aligned with the workflow-boundary contract; reference extractFreeStandingVitestJobs when locating the implementation under test.
🧹 Nitpick comments (1)
tools/e2e-advisor/scenarios.mts (1)
76-77: ⚡ Quick winClarify or collapse duplicate fan-out dispatches for multi-scenario recommendations.
After this change, every typed scenario recommendation gets the same fan-out command. If the advisor returns two targeted scenarios,
renderScenarioSummary()will print that identical dispatch line twice, which reads like two separate runs even though one fan-out run covers both. Consider collapsing typed-scenario recommendations onto one shared fan-out dispatch, or at least making the “run once” contract explicit in the rendered output.🤖 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 `@tools/e2e-advisor/scenarios.mts` around lines 76 - 77, The generated fan-out command is identical for each scenario id causing duplicate dispatch lines; update the logic so duplicates are collapsed or an explicit "run once for all scenarios" message is emitted. Specifically, deduplicate scenario ids before mapping to commands (use SCENARIO_ID_PATTERN to validate then unique the ids) or change renderScenarioSummary() to group typed-scenario recommendations and print a single shared dispatch line (or add an explicit note that one fan-out covers all listed scenarios) rather than repeating the same `gh workflow run ${SCENARIO_WORKFLOW} --ref <pr-head-ref>` for each id.
🤖 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.
Outside diff comments:
In `@test/e2e-scenario-advisor.test.ts`:
- Around line 357-376: Add a negative test case that verifies
extractFreeStandingVitestJobs rejects selectors that omit the leading
"inputs.jobs == ''" arm; create a fixture string in
test/e2e-scenario-advisor.test.ts where the job selector only contains
"contains(format(',{0},', inputs.jobs), ',token-rotation-vitest,')" (no
inputs.jobs == '' check) and assert the function returns an empty array. Update
both places noted (the existing positive case around the current diff and the
similar block at 378-418) to include this malformed-selector test so the advisor
stays aligned with the workflow-boundary contract; reference
extractFreeStandingVitestJobs when locating the implementation under test.
---
Nitpick comments:
In `@tools/e2e-advisor/scenarios.mts`:
- Around line 76-77: The generated fan-out command is identical for each
scenario id causing duplicate dispatch lines; update the logic so duplicates are
collapsed or an explicit "run once for all scenarios" message is emitted.
Specifically, deduplicate scenario ids before mapping to commands (use
SCENARIO_ID_PATTERN to validate then unique the ids) or change
renderScenarioSummary() to group typed-scenario recommendations and print a
single shared dispatch line (or add an explicit note that one fan-out covers all
listed scenarios) rather than repeating the same `gh workflow run
${SCENARIO_WORKFLOW} --ref <pr-head-ref>` for each id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4f8ec727-acb3-42d9-bb29-492eb50348b3
📒 Files selected for processing (2)
test/e2e-scenario-advisor.test.tstools/e2e-advisor/scenarios.mts
wscurran
added
area: ci
|
✨ |
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27425512277
|
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27427156289
|
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27428450458
|
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27432588145
|
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27432590206
|
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27435916094
|
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27435217295
|
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27435918149
|
# Conflicts: # .github/workflows/e2e-vitest-scenarios.yaml # test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts # tools/e2e-scenarios/workflow-boundary.mts
…o pr-5329-jobs-selector
3 participants
Summary
scenariosworkflow_dispatch input from the Vitest E2E workflowjobsonlyhermes_selectedoutputValidation
npm test -- --project e2e-vitest-support test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tsnpm run build:cli && npm run typecheck -- --pretty falseRefs #5098
Summary by CodeRabbit
Chores
Documentation / UI