ci: simplify Vitest E2E selectors#5330
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:
📝 WalkthroughWalkthroughRemoves the standalone ChangesSelector Validation Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.github/workflows/e2e-vitest-scenarios.yaml (1)
1290-1294:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid labeling every
generate-matrixfailure as a selector rejection.Line 1290 treats any non-success result from
generate-matrixas "selector rejected", but that job now also does checkout,npm ci, and matrix generation. If any of those fail, the PR comment on Lines 1333 and 1338 will falsely blame the user’s selectors and suppress otherwise valid inputs. Use neutral wording here, or emit a dedicated output for actual selector-validation success before the expensive matrix work begins.Suggested minimal wording fix
- const selectorValidationPassed = needs['generate-matrix']?.result === 'success'; + const selectorValidationPassed = needs['generate-matrix']?.result === 'success'; const requestedScenarios = selectorValidationPassed ? rawRequestedScenarios : ''; const requestedJobs = selectorValidationPassed ? rawRequestedJobs : ''; const scenariosRejected = rawRequestedScenarios && !selectorValidationPassed; const jobsRejected = rawRequestedJobs && !selectorValidationPassed; @@ - ? '**Requested scenarios:** _(selector rejected by generate-matrix)_' + ? '**Requested scenarios:** _(unavailable because generate-matrix did not complete successfully)_' @@ - ? '**Requested jobs:** _(selector rejected by generate-matrix)_' + ? '**Requested jobs:** _(unavailable because generate-matrix did not complete successfully)_'Also applies to: 1332-1339
🤖 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 1290 - 1294, The current logic uses needs['generate-matrix']?.result to set selectorValidationPassed and then marks scenariosRejected/jobsRejected, which incorrectly blames selectors when generate-matrix fails for unrelated reasons; instead either (A) have the job that only validates selectors emit a dedicated output (e.g., selector_validation_passed) before checkout/npm and change selectorValidationPassed to read needs['selector-validation']?.outputs.selector_validation_passed (or read generate-matrix.outputs.selector_validation_passed if you choose to add the output there), or (B) if you can't add a dedicated validation step, change the rejection logic to use neutral wording by only treating selectors as rejected when a specific validation output exists and is false, otherwise fall back to a neutral failure path; update the variables selectorValidationPassed, scenariosRejected, and jobsRejected and the PR comment generation logic to rely on that new output or neutral condition rather than needs['generate-matrix']?.result.test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts (2)
496-497:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid the cascading
runtime-overridesrewrite.Lines 496-497 rewrite the same token twice:
runtime-overrides-vitestbecomesruntime-overrides-missing-missingafter the second.replace(). That makes the fixture less controlled than intended and weakens the diagnostic. Swap the replacement order or use bounded patterns so each literal changes once.Suggested fix
- .replace(/runtime-overrides-vitest/g, "runtime-overrides-missing") - .replace(/runtime-overrides/g, "runtime-overrides-missing"), + .replace(/\bruntime-overrides\b/g, "runtime-overrides-missing") + .replace(/\bruntime-overrides-vitest\b/g, "runtime-overrides-missing"),🤖 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/support-tests/e2e-scenarios-workflow.test.ts` around lines 496 - 497, The chained .replace calls on the string (the calls that use .replace(/runtime-overrides-vitest/g, "runtime-overrides-missing") and .replace(/runtime-overrides/g, "runtime-overrides-missing")) cause a double rewrite so the token becomes "runtime-overrides-missing-missing"; update the replacement to avoid cascading by either swapping the two replaces (replace "runtime-overrides" first then "runtime-overrides-vitest") or use bounded/anchored patterns (e.g. match exact literals or word boundaries for "runtime-overrides-vitest" and "runtime-overrides") so each literal is replaced exactly once in the code path that performs these .replace(...) calls.
486-503: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winTarget the externalized selector config in this test.
After this refactor, the runtime-overrides allowlist lives in the shared env file, but this fixture only rewrites the workflow YAML. That means the test can still pass while
tools/e2e-scenarios/free-standing-jobs.envdrifts, so it no longer verifies the contract named in the test. Mutate the loaded selector config (or mock that loader) instead of only renaming workflow literals here.🤖 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/support-tests/e2e-scenarios-workflow.test.ts` around lines 486 - 503, The test currently only mutates workflow YAML so it misses the externalized selector allowlist; update the test to either (a) mutate the in-memory selector config that validateE2eVitestScenariosWorkflowBoundary reads (so change the loaded allowlist entries to remove "runtime-overrides-vitest") or (b) mock/stub the module/function that loads the shared selector allowlist (the code that reads tools/e2e-scenarios/free-standing-jobs.env) so the validator sees the missing literal; keep the rest of the fixture and assertions the same but ensure validateE2eVitestScenariosWorkflowBoundary is invoked against the modified/mocked selector config rather than only a renamed workflow file.tools/e2e-scenarios/workflow-boundary.mts (1)
802-802:⚠️ Potential issue | 🟠 Major | ⚡ Quick winContract mismatch:
double-onboard-vitesthas a scenario mapping in the env file but validator omits it.The new
free-standing-jobs.envdefinesdouble-onboard:double-onboard-vitestinfree_standing_scenario_jobs_csv, indicating thatscenarios=double-onboardshould select this job. However, line 802 callsvalidateFreeStandingJobSelectorwithout the scenario name, causing the validator to expect anifcondition without the scenario selector.If the workflow includes the scenario selector (to match the env file contract), validation will fail.
🐛 Proposed fix
- validateFreeStandingJobSelector(errors, jobs, jobName); + validateFreeStandingJobSelector(errors, jobs, jobName, "double-onboard");🤖 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` at line 802, The validator call at validateFreeStandingJobSelector(errors, jobs, jobName) omits the scenario selector so it doesn't match entries like "double-onboard:double-onboard-vitest" from free_standing_scenario_jobs_csv; update the call to pass the scenario name (e.g., validateFreeStandingJobSelector(errors, jobs, jobName, scenarioName)) and adjust the validateFreeStandingJobSelector signature/logic to accept that scenario string and require the "if" selector to include that scenario when matching jobs (use the scenario token parsed from the env mapping such as "double-onboard" when validating).
🤖 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-scenarios/workflow-boundary.mts`:
- Around line 1463-1465: The loop over FREE_STANDING_VITEST_JOB_IDS only asserts
presence of each job but doesn't enforce the shared selector/dependency rules
for "launchable-smoke-vitest"; call validateFreeStandingJobSelector(errors,
jobs, "launchable-smoke-vitest", ...) (same signature used for other allowed
jobs) where other free-standing Vitest jobs are validated, or add a dedicated
validator that checks the job's needs: generate-matrix and the common if
selector against the jobs map and pushes descriptive errors into errors to
mirror the existing validations.
---
Outside diff comments:
In @.github/workflows/e2e-vitest-scenarios.yaml:
- Around line 1290-1294: The current logic uses needs['generate-matrix']?.result
to set selectorValidationPassed and then marks scenariosRejected/jobsRejected,
which incorrectly blames selectors when generate-matrix fails for unrelated
reasons; instead either (A) have the job that only validates selectors emit a
dedicated output (e.g., selector_validation_passed) before checkout/npm and
change selectorValidationPassed to read
needs['selector-validation']?.outputs.selector_validation_passed (or read
generate-matrix.outputs.selector_validation_passed if you choose to add the
output there), or (B) if you can't add a dedicated validation step, change the
rejection logic to use neutral wording by only treating selectors as rejected
when a specific validation output exists and is false, otherwise fall back to a
neutral failure path; update the variables selectorValidationPassed,
scenariosRejected, and jobsRejected and the PR comment generation logic to rely
on that new output or neutral condition rather than
needs['generate-matrix']?.result.
In `@test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts`:
- Around line 496-497: The chained .replace calls on the string (the calls that
use .replace(/runtime-overrides-vitest/g, "runtime-overrides-missing") and
.replace(/runtime-overrides/g, "runtime-overrides-missing")) cause a double
rewrite so the token becomes "runtime-overrides-missing-missing"; update the
replacement to avoid cascading by either swapping the two replaces (replace
"runtime-overrides" first then "runtime-overrides-vitest") or use
bounded/anchored patterns (e.g. match exact literals or word boundaries for
"runtime-overrides-vitest" and "runtime-overrides") so each literal is replaced
exactly once in the code path that performs these .replace(...) calls.
- Around line 486-503: The test currently only mutates workflow YAML so it
misses the externalized selector allowlist; update the test to either (a) mutate
the in-memory selector config that validateE2eVitestScenariosWorkflowBoundary
reads (so change the loaded allowlist entries to remove
"runtime-overrides-vitest") or (b) mock/stub the module/function that loads the
shared selector allowlist (the code that reads
tools/e2e-scenarios/free-standing-jobs.env) so the validator sees the missing
literal; keep the rest of the fixture and assertions the same but ensure
validateE2eVitestScenariosWorkflowBoundary is invoked against the
modified/mocked selector config rather than only a renamed workflow file.
In `@tools/e2e-scenarios/workflow-boundary.mts`:
- Line 802: The validator call at validateFreeStandingJobSelector(errors, jobs,
jobName) omits the scenario selector so it doesn't match entries like
"double-onboard:double-onboard-vitest" from free_standing_scenario_jobs_csv;
update the call to pass the scenario name (e.g.,
validateFreeStandingJobSelector(errors, jobs, jobName, scenarioName)) and adjust
the validateFreeStandingJobSelector signature/logic to accept that scenario
string and require the "if" selector to include that scenario when matching jobs
(use the scenario token parsed from the env mapping such as "double-onboard"
when validating).
🪄 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: 2c1088f0-2513-431a-9aea-056c0b681e62
📒 Files selected for processing (4)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstools/e2e-scenarios/free-standing-jobs.envtools/e2e-scenarios/workflow-boundary.mts
|
This is a useful cleanup direction, especially because #5098 needs one deterministic way to answer “which live E2E Vitest jobs exist?”, but I’m requesting changes before this becomes the workflow cleanup base. Required before merge:
Net: this moves the needle, but it needs to be rebased and tied into the advisor/dispatch contract so it actually simplifies #5098 execution rather than adding another place to keep in sync. |
jyaunches
force-pushed
the
chore/e2e-live-job-inventory
branch
from
d2355e5 to
6ee68c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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)
1423-1429:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale reference to
validate-jobsin PR comment text.The rejection messages still reference
validate-jobs, but validation is now performed bygenerate-matrix. Update the user-facing text for consistency.📝 Proposed fix
scenariosRejected - ? '**Requested scenarios:** _(selector rejected by validate-jobs)_' + ? '**Requested scenarios:** _(selector rejected by generate-matrix)_' : requestedScenarios ? `**Requested scenarios:** \`${requestedScenarios}\`` : '**Requested scenarios:** _(default — all supported)_', jobsRejected - ? '**Requested jobs:** _(selector rejected by validate-jobs)_' + ? '**Requested jobs:** _(selector rejected by generate-matrix)_' : requestedJobs🤖 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 1423 - 1429, Update the user-facing rejection strings that currently mention "validate-jobs" to instead reference "generate-matrix": locate the template branches using the symbols scenariosRejected and jobsRejected and replace the text '**Requested scenarios:** _(selector rejected by validate-jobs)_' and any similar '**Requested jobs:** _(selector rejected by validate-jobs)_' occurrences with '**Requested scenarios:** _(selector rejected by generate-matrix)_' and '**Requested jobs:** _(selector rejected by generate-matrix)_' respectively so the PR comment accurately reflects the new validator.
🤖 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 1083-1088: The job-level env uses runner.temp (DOCKER_CONFIG)
which is invalid at job scope; update the workflow by removing DOCKER_CONFIG
from the job-level env and either (a) replace it with a job-safe value like
using github.workspace for the path (e.g., set DOCKER_CONFIG based on
github.workspace) or (b) move DOCKER_CONFIG into the env: of the specific steps
that need it (steps that use NEMOCLAW_CLI_BIN / E2E_ARTIFACT_DIR /
NEMOCLAW_RUN_E2E_SCENARIOS / OPENSHELL_GATEWAY), or compute DOCKER_CONFIG in a
prior step and export it for later steps.
---
Outside diff comments:
In @.github/workflows/e2e-vitest-scenarios.yaml:
- Around line 1423-1429: Update the user-facing rejection strings that currently
mention "validate-jobs" to instead reference "generate-matrix": locate the
template branches using the symbols scenariosRejected and jobsRejected and
replace the text '**Requested scenarios:** _(selector rejected by
validate-jobs)_' and any similar '**Requested jobs:** _(selector rejected by
validate-jobs)_' occurrences with '**Requested scenarios:** _(selector rejected
by generate-matrix)_' and '**Requested jobs:** _(selector rejected by
generate-matrix)_' respectively so the PR comment accurately reflects the new
validator.
🪄 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: 1c93eb51-c041-4d27-bd47-b327dde8920e
📒 Files selected for processing (4)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstools/e2e-scenarios/free-standing-jobs.envtools/e2e-scenarios/workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (3)
- tools/e2e-scenarios/free-standing-jobs.env
- 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
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)
1423-1429:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale reference to
validate-jobsin PR comment text.The rejection messages still reference
validate-jobs, but validation is now performed bygenerate-matrix. Update the user-facing text for consistency.📝 Proposed fix
scenariosRejected - ? '**Requested scenarios:** _(selector rejected by validate-jobs)_' + ? '**Requested scenarios:** _(selector rejected by generate-matrix)_' : requestedScenarios ? `**Requested scenarios:** \`${requestedScenarios}\`` : '**Requested scenarios:** _(default — all supported)_', jobsRejected - ? '**Requested jobs:** _(selector rejected by validate-jobs)_' + ? '**Requested jobs:** _(selector rejected by generate-matrix)_' : requestedJobs🤖 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 1423 - 1429, Update the user-facing rejection strings that currently mention "validate-jobs" to instead reference "generate-matrix": locate the template branches using the symbols scenariosRejected and jobsRejected and replace the text '**Requested scenarios:** _(selector rejected by validate-jobs)_' and any similar '**Requested jobs:** _(selector rejected by validate-jobs)_' occurrences with '**Requested scenarios:** _(selector rejected by generate-matrix)_' and '**Requested jobs:** _(selector rejected by generate-matrix)_' respectively so the PR comment accurately reflects the new validator.
🤖 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 1083-1088: The job-level env uses runner.temp (DOCKER_CONFIG)
which is invalid at job scope; update the workflow by removing DOCKER_CONFIG
from the job-level env and either (a) replace it with a job-safe value like
using github.workspace for the path (e.g., set DOCKER_CONFIG based on
github.workspace) or (b) move DOCKER_CONFIG into the env: of the specific steps
that need it (steps that use NEMOCLAW_CLI_BIN / E2E_ARTIFACT_DIR /
NEMOCLAW_RUN_E2E_SCENARIOS / OPENSHELL_GATEWAY), or compute DOCKER_CONFIG in a
prior step and export it for later steps.
---
Outside diff comments:
In @.github/workflows/e2e-vitest-scenarios.yaml:
- Around line 1423-1429: Update the user-facing rejection strings that currently
mention "validate-jobs" to instead reference "generate-matrix": locate the
template branches using the symbols scenariosRejected and jobsRejected and
replace the text '**Requested scenarios:** _(selector rejected by
validate-jobs)_' and any similar '**Requested jobs:** _(selector rejected by
validate-jobs)_' occurrences with '**Requested scenarios:** _(selector rejected
by generate-matrix)_' and '**Requested jobs:** _(selector rejected by
generate-matrix)_' respectively so the PR comment accurately reflects the new
validator.
🪄 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: 1c93eb51-c041-4d27-bd47-b327dde8920e
📒 Files selected for processing (4)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstools/e2e-scenarios/free-standing-jobs.envtools/e2e-scenarios/workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (3)
- tools/e2e-scenarios/free-standing-jobs.env
- 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)
1083-1088:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
runnercontext is not available at job-levelenv:— workflow dispatch will fail.The
runnercontext (includingrunner.temp) is only available within steps, not at the job level. GitHub will reject this workflow at parse time when manually dispatched. The PR objectives explicitly flag this as a blocker to address.Move
DOCKER_CONFIGinto step-levelenv:blocks for steps that need it, or compute it within a step.🔧 Proposed fix
Option 1 — Use
github.workspacewhich is available at job level:env: - DOCKER_CONFIG: ${{ runner.temp }}/docker-config-model-router-provider-routed-inference + DOCKER_CONFIG: ${{ github.workspace }}/.docker-config-model-router-provider-routed-inferenceOption 2 — Move to step-level for steps that need it:
env: - DOCKER_CONFIG: ${{ runner.temp }}/docker-config-model-router-provider-routed-inference E2E_ARTIFACT_DIR: ${{ github.workspace }}/e2e-artifacts/vitest/model-router-provider-routed-inference ... steps: ... - 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: | ...📝 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: ${{ github.workspace }}/.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] 1084-1084: 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 1083 - 1088, The job-level env uses runner.temp (DOCKER_CONFIG) which is invalid at job scope; update the workflow by removing DOCKER_CONFIG from the job-level env and either (a) replace it with a job-safe value like using github.workspace for the path (e.g., set DOCKER_CONFIG based on github.workspace) or (b) move DOCKER_CONFIG into the env: of the specific steps that need it (steps that use NEMOCLAW_CLI_BIN / E2E_ARTIFACT_DIR / NEMOCLAW_RUN_E2E_SCENARIOS / OPENSHELL_GATEWAY), or compute DOCKER_CONFIG in a prior step and export it for later steps.Source: Linters/SAST tools
jyaunches
force-pushed
the
chore/e2e-live-job-inventory
branch
from
6ee68c5 to
6c37476
Compare
wscurran
added
area: ci
|
✨ |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/e2e-scenarios/workflow-boundary.mts (1)
1732-1758:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe new selector registry is still only half-dynamic.
evaluateE2eVitestWorkflowDispatchSelectors()now accepts every job id fromFREE_STANDING_VITEST_JOB_IDS, but boundary enforcement andreport-to-prcoverage are still maintained as separate hardcoded lists here. That means adding a new id tofree-standing-jobs.envcan make the selector valid and part of the default run set without also requiring a workflow validator, existence check, shared gating check, orreport-to-prdependency for that job.Please derive these checks from the same registry (for example, a single validator table keyed by job id, with
report-to-prneeds derived from the same keys) so the env file cannot become an unvalidated secondary inventory.Also applies to: 1764-1783
🤖 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 1732 - 1758, The various hardcoded validator lists (calls like validateFreeStandingJobSelector, validateRuntimeOverridesVitestJob, validateTokenRotationVitestJob, and validateRebuildOpenClawVitestJob) must be derived from the same registry used by evaluateE2eVitestWorkflowDispatchSelectors / FREE_STANDING_VITEST_JOB_IDS so the env file cannot introduce unvalidated jobs; refactor by creating a single registry/table keyed by job id that includes metadata flags (needsValidation, requiresReportToPr, sharedGating, etc.), update evaluateE2eVitestWorkflowDispatchSelectors to consume that registry, and replace the separate hardcoded validator calls in workflow-boundary.mts with a loop that iterates the registry and invokes the appropriate validation logic based on each job's metadata (use the existing validator functions like validateFreeStandingJobSelector and validate*VitestJob by referencing their names when dispatching).
🤖 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 59-65: The free-standing selector contract is split between
generate-matrix and the workflow guards (is_free_standing_scenario(), each job's
if: and report-to-pr.needs), causing drift; fix by centralizing the inventory:
have the workflow source and parse free_standing_jobs from
free-standing-jobs.env into a single canonical variable (e.g.
free_standing_scenarios_csv) and change every per-job guard (the
is_free_standing_scenario() function and all job if: expressions) and
report-to-pr.needs aggregation to reference that same canonical variable, or
alternatively add a strict boundary test that enumerates every free-standing job
mapping and fails if any job guard or report-to-pr.needs entry does not match
generate-matrix output (use generate-matrix output as the single source of
truth).
In `@tools/e2e-scenarios/workflow-boundary.mts`:
- Around line 23-39: The module assumes keys exist and dereferences
FREE_STANDING_JOBS_ENV.allowed_jobs and
FREE_STANDING_JOBS_ENV.free_standing_scenario_jobs_csv during import, causing
crashes on malformed free-standing-jobs.env; update readFreeStandingJobsEnv and
the initialization of FREE_STANDING_VITEST_JOB_IDS and
FREE_STANDING_VITEST_SCENARIO_TO_JOB to validate the parsed env contract: after
building FREE_STANDING_JOBS_ENV (in readFreeStandingJobsEnv or immediately
after) assert the required keys exist and are non-empty, throw a clear,
actionable error if missing, and default to safe empty values (e.g., [] or empty
Map) when appropriate so consumers of FREE_STANDING_JOBS_ENV don't crash on
import; reference the functions/consts readFreeStandingJobsEnv,
FREE_STANDING_JOBS_ENV, FREE_STANDING_VITEST_JOB_IDS,
FREE_STANDING_VITEST_SCENARIO_TO_JOB and the keys allowed_jobs and
free_standing_scenario_jobs_csv when making the checks.
---
Outside diff comments:
In `@tools/e2e-scenarios/workflow-boundary.mts`:
- Around line 1732-1758: The various hardcoded validator lists (calls like
validateFreeStandingJobSelector, validateRuntimeOverridesVitestJob,
validateTokenRotationVitestJob, and validateRebuildOpenClawVitestJob) must be
derived from the same registry used by
evaluateE2eVitestWorkflowDispatchSelectors / FREE_STANDING_VITEST_JOB_IDS so the
env file cannot introduce unvalidated jobs; refactor by creating a single
registry/table keyed by job id that includes metadata flags (needsValidation,
requiresReportToPr, sharedGating, etc.), update
evaluateE2eVitestWorkflowDispatchSelectors to consume that registry, and replace
the separate hardcoded validator calls in workflow-boundary.mts with a loop that
iterates the registry and invokes the appropriate validation logic based on each
job's metadata (use the existing validator functions like
validateFreeStandingJobSelector and validate*VitestJob by referencing their
names when dispatching).
🪄 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: deac211c-89ee-47a0-b1ad-6a27cde97747
📒 Files selected for processing (4)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstools/e2e-scenarios/free-standing-jobs.envtools/e2e-scenarios/workflow-boundary.mts
✅ Files skipped from review due to trivial changes (1)
- tools/e2e-scenarios/free-standing-jobs.env
jyaunches
force-pushed
the
chore/e2e-live-job-inventory
branch
from
18156ec to
00adf10
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts (1)
269-289: ⚡ Quick winAdd a regression test for the empty-selector default path.
This new coverage exercises explicit
jobs=andscenarios=inputs, but it never pins the no-input behavior that now derives “all registry scenarios + all free-standing jobs” from the inventory. That default is one of the main contract changes in this PR, so a regression there would still pass this suite.Please add one assertion for
evaluateE2eVitestWorkflowDispatchSelectors({})and onegenerateMatrixForDispatch({ JOBS: "", SCENARIOS: "" })check to lock down the empty-selector behavior. Based on PR objectives, an empty selector must dispatch the full E2E Vitest suite.🤖 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/support-tests/e2e-scenarios-workflow.test.ts` around lines 269 - 289, Add assertions to lock the empty-selector default behavior: call evaluateE2eVitestWorkflowDispatchSelectors({}) and assert it returns valid: true, liveScenariosRuns: false, selectedFreeStandingJobs equals the inventory's allowedJobs, and registryScenarios equals the inventory's registry scenarios; and call generateMatrixForDispatch({ JOBS: "", SCENARIOS: "" }) and assert it dispatches the full E2E Vitest suite (i.e., hermes_selected true for hermes-e2e-vitest and matrix contains all registry scenarios and free-standing jobs per readFreeStandingJobsInventory()). Use the existing helpers evaluateE2eVitestWorkflowDispatchSelectors, generateMatrixForDispatch, and readFreeStandingJobsInventory to construct the expected values.
🤖 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-scenarios/workflow-boundary.mts`:
- Around line 368-395: The validator validateFreeStandingInventoryCoverage
currently only checks the hardcoded FREE_STANDING_VITEST_JOB_IDS and
FREE_STANDING_VITEST_SCENARIO_TO_JOB; update it to iterate the canonical
FREE_STANDING_JOBS_INVENTORY (or its equivalent registry) and run the
selector/needs checks for each inventory entry (or call the existing
validateFreeStandingJobSelector per job) instead of only the subset, so every
inventory-defined job (e.g., sandbox-survival-vitest) is validated for presence,
report-to-pr coverage, and correct needs/shared-if selectors; ensure you
reference FREE_STANDING_JOBS_INVENTORY, validateFreeStandingJobSelector (or the
selector-validation logic), and the job keys used in asRecord/jobIf when
implementing this change.
---
Nitpick comments:
In `@test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts`:
- Around line 269-289: Add assertions to lock the empty-selector default
behavior: call evaluateE2eVitestWorkflowDispatchSelectors({}) and assert it
returns valid: true, liveScenariosRuns: false, selectedFreeStandingJobs equals
the inventory's allowedJobs, and registryScenarios equals the inventory's
registry scenarios; and call generateMatrixForDispatch({ JOBS: "", SCENARIOS: ""
}) and assert it dispatches the full E2E Vitest suite (i.e., hermes_selected
true for hermes-e2e-vitest and matrix contains all registry scenarios and
free-standing jobs per readFreeStandingJobsInventory()). Use the existing
helpers evaluateE2eVitestWorkflowDispatchSelectors, generateMatrixForDispatch,
and readFreeStandingJobsInventory to construct the expected values.
🪄 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: a7a24c14-9c68-49b6-a296-fbd66c5863ab
📒 Files selected for processing (4)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstools/e2e-scenarios/free-standing-jobs.envtools/e2e-scenarios/workflow-boundary.mts
✅ Files skipped from review due to trivial changes (1)
- tools/e2e-scenarios/free-standing-jobs.env
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/e2e-vitest-scenarios.yaml
jyaunches
force-pushed
the
chore/e2e-live-job-inventory
branch
2 times, most recently
from
cc64cc9 to
4862adb
Compare
jyaunches
force-pushed
the
chore/e2e-live-job-inventory
branch
from
4862adb to
783312b
Compare
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27428412159
|
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27428410843
|
3 participants
Summary
Diff budget
Validation
Refs #5098
Summary by CodeRabbit
Chores
Tests