ci(e2e): tier retained gateway upgrade coverage - #7956
Conversation
Document each retained gateway row's boundary and tier. Gate prepare, live execution, and upload through the row classifier. Explicit dispatch, weekly runs, and release qualification still execute retained rows. Signed-off-by: ScarabSystems <scarab.systems@yahoo.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughOpenShell gateway upgrade rows now include execution tiers and unique migration boundaries. Classification selects rows by explicit selection, schedule, or release qualification. Preparation, tests, and artifact upload follow the selection result. Boundary validators and tests verify the workflow behavior. ChangesGateway upgrade tiering
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Matrix as Gateway upgrade matrix
participant Classifier as gateway_upgrade_tier
participant E2E as E2E preparation and tests
participant Upload as Artifact upload
Matrix->>Classifier: Provide tier and migration boundary
Classifier->>Classifier: Evaluate selection and schedule
Classifier->>E2E: Emit run decision
E2E->>Upload: Upload artifacts for selected rows
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported 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.
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.yaml (1)
4396-4402: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftGate Docker Hub authentication on the tier decision.
On scheduled weekdays and default manual runs, all four
weekly-releaserows are skipped, butauth-requiredremains1, so each still performsdocker loginand can fail before classification. Move classification before auth and run the auth step only whensteps.gateway_upgrade_tier.outputs.run == '1'.🤖 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.yaml around lines 4396 - 4402, Move the upgrade-tier classification step before the *dockerhub-auth step in the weekly-release workflow, then gate Docker Hub authentication with steps.gateway_upgrade_tier.outputs.run == '1'. Ensure scheduled weekdays and default manual runs skip docker login when no weekly-release rows are selected, while eligible runs retain authentication.
🧹 Nitpick comments (3)
tools/e2e/openshell-gateway-upgrade-workflow-boundary.mts (2)
227-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClassify-step env check doesn't guard against extraneous keys.
The loop at lines 238-244 verifies each expected key/value is bound but never confirms
classifyEnvhas no other keys. Other contracts in this same module (and inprepare-e2e-workflow-boundary.mts/upload-e2e-artifacts-workflow-boundary.mts) useisDeepStrictEqualon the full key set to catch unexpected additions. Consider the same here to keep the contract airtight.♻️ Proposed refactor
- for (const [key, expected] of Object.entries(expectedClassifyEnv)) { - if (classifyEnv[key] !== expected) { - errors.push( - `${JOB_NAME} step '${CLASSIFY_STEP_NAME}' must bind ${key} from workflow context`, - ); - } - } + if (!isDeepStrictEqual(classifyEnv, expectedClassifyEnv)) { + errors.push( + `${JOB_NAME} step '${CLASSIFY_STEP_NAME}' must bind exactly the reviewed workflow context`, + ); + }🤖 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/openshell-gateway-upgrade-workflow-boundary.mts` around lines 227 - 244, Update the classify-step environment validation around classifyEnv and expectedClassifyEnv to compare their complete key sets with the module’s established deep-strict-equality approach, while retaining the existing value checks and error reporting. Ensure unexpected keys in classifyEnv are reported alongside missing or mismatched expected bindings.
245-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the
nightly-canonicalreason string.The fragment list checks for
explicit-selection,weekly-retained,release-qualification, andskipped-by-tier, but notnightly-canonical— the reason string set by theEXECUTION_TIER == "nightly"branch in e2e.yaml. A regression there (e.g., someone deletes or renames that branch) wouldn't be caught.🤖 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/openshell-gateway-upgrade-workflow-boundary.mts` around lines 245 - 254, Update the fragment list in the classification assertions around CLASSIFY_STEP_NAME to include the nightly-canonical reason string, ensuring the nightly execution-tier branch in e2e.yaml is covered while preserving all existing fragment checks.test/e2e/support/openshell-gateway-upgrade-workflow-boundary.test.ts (1)
66-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated fixture-tier literal risks silent drift from the validator's source of truth.
The
fixtures.map(...)expectation (lines 74-105) duplicatesEXPECTED_FIXTURE_TIERSfromtools/e2e/openshell-gateway-upgrade-workflow-boundary.mtsverbatim. If one copy is updated and the other isn't, this test and the boundary validator can silently diverge. Consider exportingEXPECTED_FIXTURE_TIERSfrom the boundary module and importing it here instead.🤖 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/support/openshell-gateway-upgrade-workflow-boundary.test.ts` around lines 66 - 105, Export EXPECTED_FIXTURE_TIERS from the boundary validator module and import it into the test instead of duplicating the fixture-tier objects in the fixtures.map expectation. Update the assertion to compare against the shared source-of-truth constant while preserving the existing fixture mapping and validation behavior.
🤖 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 @.github/workflows/e2e.yaml:
- Around line 4396-4402: Move the upgrade-tier classification step before the
*dockerhub-auth step in the weekly-release workflow, then gate Docker Hub
authentication with steps.gateway_upgrade_tier.outputs.run == '1'. Ensure
scheduled weekdays and default manual runs skip docker login when no
weekly-release rows are selected, while eligible runs retain authentication.
---
Nitpick comments:
In `@test/e2e/support/openshell-gateway-upgrade-workflow-boundary.test.ts`:
- Around line 66-105: Export EXPECTED_FIXTURE_TIERS from the boundary validator
module and import it into the test instead of duplicating the fixture-tier
objects in the fixtures.map expectation. Update the assertion to compare against
the shared source-of-truth constant while preserving the existing fixture
mapping and validation behavior.
In `@tools/e2e/openshell-gateway-upgrade-workflow-boundary.mts`:
- Around line 227-244: Update the classify-step environment validation around
classifyEnv and expectedClassifyEnv to compare their complete key sets with the
module’s established deep-strict-equality approach, while retaining the existing
value checks and error reporting. Ensure unexpected keys in classifyEnv are
reported alongside missing or mismatched expected bindings.
- Around line 245-254: Update the fragment list in the classification assertions
around CLASSIFY_STEP_NAME to include the nightly-canonical reason string,
ensuring the nightly execution-tier branch in e2e.yaml is covered while
preserving all existing fragment checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b1ef55b3-a007-4e96-91ca-24dcc912a499
📒 Files selected for processing (5)
.github/workflows/e2e.yamltest/e2e/support/openshell-gateway-upgrade-workflow-boundary.test.tstools/e2e/openshell-gateway-upgrade-workflow-boundary.mtstools/e2e/prepare-e2e-workflow-boundary.mtstools/e2e/upload-e2e-artifacts-workflow-boundary.mts
Record expected and row-level observed nightly runner-minute reduction from OpenShell gateway upgrade tier classification. Extend the gateway workflow boundary validator and support test so the classifier summary, outputs, and branch decisions stay covered for explicit selection, nightly, weekly, release qualification, and skipped-by-tier rows. Validation before commit: - Gateway boundary test: passed, 13 tests. - Focused workflow/report/planner bundle: passed, 128 tests. - npm run source-shape:check: passed. - npm run test:e2e-phases:check: passed. - npm run test:changed: passed, 262 tests. Signed-off-by: ScarabSystems <scarab.systems@yahoo.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 `@test/e2e/support/openshell-gateway-upgrade-workflow-boundary.test.ts`:
- Around line 234-299: Add a test case to the gateway tier classifier coverage
that sets checkoutSha and simulates an owning gateway-upgrade file change by
stubbing the changed-file command through the temporary PATH. Assert the
forced-selection output, including run, reason, and observed reduction, so
owning-file changes remain directly selectable. Use the existing
runGatewayTierClassifier test setup and output assertions.
🪄 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: 4d6455db-5e7c-48f7-b14e-e61bf8f676f5
📒 Files selected for processing (3)
.github/workflows/e2e.yamltest/e2e/support/openshell-gateway-upgrade-workflow-boundary.test.tstools/e2e/openshell-gateway-upgrade-workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/e2e/openshell-gateway-upgrade-workflow-boundary.mts
- .github/workflows/e2e.yaml
Add a gateway tier classifier scenario for a PR controller checkout that receives the upstream-selected openshell-gateway-upgrade job. The classifier remains a selector consumer. Owning-file discovery stays in the PR risk plan and workflow planner. Validation before commit: - Gateway boundary test: passed, 14 tests. - Focused workflow/report/planner bundle: passed, 129 tests. - npm run test:changed: passed, 14 tests. Signed-off-by: ScarabSystems <scarab.systems@yahoo.com>
|
Thank you for implementing the scope and tests requested by #7920. We are closing this pull request because the maintainer scheduling decision has changed: NemoClaw will not use weekly E2E schedules. Weekly compatibility coverage creates a timebomb because releases can ship with a regressed retained boundary before its next execution. Every retained compatibility boundary must run for every release candidate. If the coverage is too expensive, we must reduce setup, build, and test runtime or retire a non-unique boundary based on evidence. We must not defer required coverage beyond the release cadence. This closure is about the scheduling premise, not the implementation quality. Follow-up optimization remains in #7912, with exact-commit artifact reuse tracked by #7915. |
|
Thanks for the clear direction. That scheduling distinction makes sense. I appreciate the clarification that this closure is about the scheduling premise, not the implementation quality. I’ll treat this PR as closed on that basis and keep any follow-up focused on #7912 optimization, with #7915’s exact-commit artifact reuse as the path for reducing setup/build/test cost instead of deferring required coverage. Dorothy c/o Scarab Systems |
wscurran
added
area: ci
3 participants
Summary
Tiers the retained OpenShell gateway upgrade rows so ordinary scheduled runs keep the
current state-upgrade migration row while preserving historical and architecture
coverage for weekly, release-qualification, and explicit maintainer dispatch runs.
The change stays inside the existing
openshell-gateway-upgradejob and does notchange gateway product behavior.
Related Issue
Fixes #7920
Changes
execution_tierandunique_boundarymetadata to each retained gatewayupgrade matrix row.
v0.0.89-x86_64state-upgrade row in the ordinary nightly tier.scheduled coverage, and release qualification.
skipped rows are reported as
skipped-by-tierinstead of looking like failures.tier metadata, classifier, and reviewed prepare/upload caller gates.
Expected runner-minute reduction: ordinary scheduled Monday-Saturday runs now execute
1 of 5 retained gateway upgrade rows; weekly, release-qualification, and explicit
selection paths still execute the retained historical and architecture rows.
Type of Change
Quality Gates
selection and workflow-boundary tests only. It does not change user-facing docs,
CLI commands, configuration, or product runtime behavior.
inference, runner, sandbox, or messaging)
reviewer/approval link/justification: Reviewed against Epic: reduce E2E wall time, runner cost, and failure surface #7912 and ci(e2e): tier retained gateway compatibility coverage #7920. The
change does not delete retained assertions, add a new workflow lane, or alter
gateway product behavior. Explicit selection, weekly retained coverage, and
release qualification still run the historical and architecture rows.
approval link, and follow-up issue:
Documentation Writer Review
no-docs-neededorigin/main...HEAD; the diff changes only E2E workflowtiering and boundary-test files for
openshell-gateway-upgrade, with no changesto Fern docs, README user guidance, CLI commands, configuration options,
install/runtime behavior, or public workflow inputs. No user-facing documentation
paths need updates.
DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks wereskipped or unavailable —
npm run validate:prpassed for032e419aa.not applicable above —
npx vitest run --project e2e-support test/e2e/support/openshell-gateway-upgrade-workflow-boundary.test.tspassed 7tests; focused workflow/report/planner checks passed 122 tests;
npm run test:changedpassed 327 tests;npm run test:e2e-phases:checkvalidated 114semantic E2E phase plans.
npm testfor broad runtime/test-harnesschanges;
npm run checkfor repo-wide validation/coverage changes — notapplicable to this focused workflow-tiering change; targeted workflow checks and
npm run validate:prpassed.npm run docsbuilds without warnings (doc changes only)Disclosure: This PR was prepared with AI assistance under human direction and
review.
Signed-off-by: ScarabSystems scarab.systems@yahoo.com
Summary by CodeRabbit
New Features
Tests