fix(ci): keep PR E2E authorization pending - #6914
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.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)
📝 WalkthroughWalkthroughThe PR changes credentialed E2E authorization checks from terminal failures to pending states, restores pending coordination after pre-dispatch errors, records post-dispatch reconciliation failures, narrows completed-failure classification, and updates tests and documentation. ChangesE2E authorization coordination
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant startPrGate
participant GitHubCheckRun
participant startControlPlanePrGate
participant ChildWorkflow
startPrGate->>GitHubCheckRun: mark authorization check in_progress
startControlPlanePrGate->>GitHubCheckRun: validate matching pending state
startControlPlanePrGate->>ChildWorkflow: dispatch child E2E run
ChildWorkflow->>startControlPlanePrGate: report dispatch or reconciliation error
startControlPlanePrGate->>GitHubCheckRun: restore pending state or record reconciliation failure
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/e2e/pr-e2e-gate.mts`:
- Around line 2037-2045: Update the authorization validation around
pendingAuthorization and legacyAuthorization so completed legacy checks do not
enter the markCheckInProgress reopen path. Either route legacyAuthorization
through a completed-check-compatible handoff before dispatch, or remove it from
the accepted states until that handoff is implemented; preserve the existing
pending authorization flow.
🪄 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: 6e9c28cb-c519-43cd-b376-8150b2868857
📒 Files selected for processing (5)
test/e2e/README.mdtest/pr-e2e-gate-fork-skip.test.tstest/pr-e2e-required.test.tstools/e2e/pr-e2e-gate.mtstools/e2e/pr-e2e-required.mts
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
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)
tools/e2e/pr-e2e-gate.mts (1)
2071-2087: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDon't restore authorization on ambiguous dispatch failures. If
dispatchSelectedPrGatetimes out or returns 5xx after GitHub has already accepted the workflow dispatch, reopening the check to pending can let a retry launch duplicate E2E work. Reconcile the child run first, or only restore for failures known to occur before dispatch is accepted.🤖 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/pr-e2e-gate.mts` around lines 2071 - 2087, Update the controller-error handling around dispatchSelectedPrGate so it does not call markCheckInProgress for ambiguous timeout or 5xx failures that may follow an accepted GitHub workflow dispatch. Reconcile the child run first, or restrict authorization restoration to failures definitively confirmed to occur before dispatch acceptance, while preserving the existing error reporting for non-restored cases.
🧹 Nitpick comments (1)
test/pr-e2e-required.test.ts (1)
195-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid locking this test to exactly three polling calls.
toBe(3)asserts mock invocation count rather than the behavioral contract. Record the observed coordination states/titles and assert that pending authorization and running states were tolerated before the final success, allowing polling cadence to change.As per path instructions, this test should favor behavioral confidence over mock-call-count assertions.
🤖 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/pr-e2e-required.test.ts` at line 195, Update the polling assertions in the end-to-end test around coordinationQueries to validate observed coordination states or titles instead of requiring exactly three calls. Assert that pending authorization and running states are tolerated before the final success, while allowing the polling cadence and invocation count to change.Source: Path instructions
🤖 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/pr-e2e-required.mts`:
- Line 17: Update the classifier in pr-e2e-required.mts to recognize only exact
trusted titles matching “Running <count> E2E job” or “Running <count> E2E jobs”
as transitional, including completed checks with failure conclusions, while
leaving other terminal failures unchanged. Add a regression case covering a
completed failure titled “Running 3 E2E jobs” and verify it remains
transitional.
---
Outside diff comments:
In `@tools/e2e/pr-e2e-gate.mts`:
- Around line 2071-2087: Update the controller-error handling around
dispatchSelectedPrGate so it does not call markCheckInProgress for ambiguous
timeout or 5xx failures that may follow an accepted GitHub workflow dispatch.
Reconcile the child run first, or restrict authorization restoration to failures
definitively confirmed to occur before dispatch acceptance, while preserving the
existing error reporting for non-restored cases.
---
Nitpick comments:
In `@test/pr-e2e-required.test.ts`:
- Line 195: Update the polling assertions in the end-to-end test around
coordinationQueries to validate observed coordination states or titles instead
of requiring exactly three calls. Assert that pending authorization and running
states are tolerated before the final success, while allowing the polling
cadence and invocation count to change.
🪄 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: 3f9702df-b202-49e4-b3a0-5ee97a7b4bc5
📒 Files selected for processing (5)
test/e2e/README.mdtest/pr-e2e-gate-fork-skip.test.tstest/pr-e2e-required.test.tstools/e2e/pr-e2e-gate.mtstools/e2e/pr-e2e-required.mts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/pr-e2e-gate-fork-skip.test.ts
- test/e2e/README.md
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
wscurran
added
area: ci
2 participants
Summary
Keep control-plane E2E authorization in progress so GitHub can advance the same coordination check from authorization to running and then to its terminal verdict. This fixes the lifecycle observed on #6904, where GitHub preserved the completed failure conclusion even after the controller changed the check title to
Running 9 E2E jobs, causing the native required job to fail while E2E was still running.Changes
in_progress, validate that pending state during manual authorization, and restore it after a retryable controller failure.Type of Change
Quality Gates
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run test/pr-e2e-gate-fork-skip.test.ts test/pr-e2e-required.test.ts test/pr-e2e-gate-workflow.test.ts test/pr-e2e-gate-lifecycle.test.ts test/pr-e2e-gate.test.ts(5 files, 108 tests passed);npm run typecheck:cli;npm run source-shape:check;npm run test:projects:check;npm run test-size:checknpm run docsbuilds without warnings (doc changes only) — build passed with the same two hidden Fern warningsSigned-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests