fix(ci): publish PR E2E gate as native job - #6909
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
📝 WalkthroughWalkthroughThe PR adds a native required E2E observer job, introduces exact-diff verdict polling and validation, renames the controller check to a coordination check, updates risk/watch coverage, and documents revised coordination, authorization, evidence, and cancellation behavior. ChangesPR E2E required gate
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant RequiredJob
participant RequiredObserver
participant GitHubChecks
PullRequest->>RequiredJob: trigger pull_request_target observer
RequiredJob->>RequiredObserver: pass PR number and exact head/base SHAs
RequiredObserver->>GitHubChecks: validate PR and poll coordination check
GitHubChecks-->>RequiredObserver: return trusted terminal verdict
RequiredObserver-->>RequiredJob: write summary and set job result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review 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: Blockers
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
![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
🧹 Nitpick comments (1)
tools/advisors/risk-plan.mts (1)
74-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConstant name no longer matches its contents.
CONTROLLER_ONLY_E2E_FILESnow also contains the observer/required-gate script (tools/e2e/pr-e2e-required.mts), not just controller files. Since this set gates the trusted-boundary/auto-dispatch decision (per test/e2e/README.md), a misleading name increases the risk that a future edit to this security-relevant list misclassifies a file. Consider renaming to something likeTRUSTED_CONTROL_PLANE_ONLY_FILESto reflect that it now spans both controller and observer scripts.As per path instructions, "Derive inventories and limits from a canonical source where possible; flag duplicated lists that can silently drift."
🤖 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/advisors/risk-plan.mts` around lines 74 - 78, Rename CONTROLLER_ONLY_E2E_FILES to a name that reflects its trusted control-plane scope, such as TRUSTED_CONTROL_PLANE_ONLY_FILES, and update every reference to the set consistently. Preserve the existing file inventory and gating behavior without introducing a separate duplicate list.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 `@test/pr-e2e-required.test.ts`:
- Around line 149-167: Rewrite the fetch mock in the test “waits through
authorization and revalidates the exact PR before passing” to remove both if
statements, using the file’s existing ternary-based dispatch pattern. Preserve
the current routing for the PR URL, Coordination listing, and legacy query
responses, including the first-query failure and subsequent success behavior.
---
Nitpick comments:
In `@tools/advisors/risk-plan.mts`:
- Around line 74-78: Rename CONTROLLER_ONLY_E2E_FILES to a name that reflects
its trusted control-plane scope, such as TRUSTED_CONTROL_PLANE_ONLY_FILES, and
update every reference to the set consistently. Preserve the existing file
inventory and gating behavior without introducing a separate duplicate list.
🪄 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: 98c8fc01-c7cc-42b0-88ee-688be1f34906
📒 Files selected for processing (14)
.github/workflows/pr-e2e-gate.yamltest/e2e/README.mdtest/e2e/docs/README.mdtest/helpers/vitest-watch-triggers.tstest/pr-e2e-gate-fork-skip.test.tstest/pr-e2e-gate-lifecycle.test.tstest/pr-e2e-gate-workflow.test.tstest/pr-e2e-gate.test.tstest/pr-e2e-required.test.tstest/pr-risk-plan.test.tstest/vitest-watch-triggers.test.tstools/advisors/risk-plan.mtstools/e2e/pr-e2e-gate.mtstools/e2e/pr-e2e-required.mts
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)
tools/e2e/pr-e2e-required.mts (1)
258-273: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude the terminal verdict and trusted run link in the job summary.
appendJobSummary()now writes only static text, whileresult.conclusionandresult.detailsUrlare logged separately. This breaks the stated PR contract that the native required job’s summary publishes both values; failures currently produce the same summary as successes.Pass the validated result into
appendJobSummary, render only the validated conclusion and URL, and add regression coverage intest/pr-e2e-required.test.ts.Suggested direction
-function appendJobSummary(): void { +function appendJobSummary(result: RequiredGateResult): void { ... - "## E2E / PR Gate\n\nThis native job mirrors the trusted exact-diff E2E coordination result. See the job log for the validated controller run.\n", + `## E2E / PR Gate\n\nVerdict: \`${result.conclusion}\`\n\nTrusted E2E coordination run: ${result.detailsUrl ?? "unavailable"}\n`, ... - appendJobSummary(); + appendJobSummary(result);As per coding guidelines, add a test or diagnostic for QA-escaped defects.
Also applies to: 291-292
🤖 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-required.mts` around lines 258 - 273, Update appendJobSummary to accept the validated result object and render its terminal conclusion and trusted detailsUrl in the GitHub step summary instead of static-only text. Update its call site to pass that validated result while preserving the existing file validation behavior, and add regression coverage in test/pr-e2e-required.test.ts verifying both values appear for success and failure outcomes.Source: Coding guidelines
🤖 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 `@tools/e2e/pr-e2e-required.mts`:
- Around line 258-273: Update appendJobSummary to accept the validated result
object and render its terminal conclusion and trusted detailsUrl in the GitHub
step summary instead of static-only text. Update its call site to pass that
validated result while preserving the existing file validation behavior, and add
regression coverage in test/pr-e2e-required.test.ts verifying both values appear
for success and failure outcomes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d5b5d487-27a7-4730-bcb1-d512e1d64de9
📒 Files selected for processing (4)
test/e2e/README.mdtest/e2e/docs/README.mdtest/pr-e2e-required.test.tstools/e2e/pr-e2e-required.mts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/e2e/docs/README.md
- test/e2e/README.md
- test/pr-e2e-required.test.ts
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/advisors/risk-plan.mts (1)
74-78: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDerive the trusted inventory from one canonical source.
E2E_CONTROL_PLANE_FILESalready defines the control-plane paths, while this set duplicates entries and can silently drift. Because the set controls credentialed E2E authorization, model trust metadata in a canonical inventory or enforce the subset relationship with an invariant test.As per path instructions: derive inventories from a canonical source where possible and flag duplicated lists that can silently drift.
🤖 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/advisors/risk-plan.mts` around lines 74 - 78, Update TRUSTED_CONTROL_PLANE_ONLY_FILES to derive its entries from the canonical E2E_CONTROL_PLANE_FILES inventory instead of duplicating path literals, while preserving the trusted subset used for credentialed E2E authorization. If direct derivation is not supported, add an invariant that verifies the trusted set remains a subset of E2E_CONTROL_PLANE_FILES.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/advisors/risk-plan.mts`:
- Around line 411-413: Add focused regression tests for the authorization
predicate around the control-plane matched-file check: verify an untrusted file
returns true, a trusted-only set returns false, and a mixed trusted/untrusted
set still returns true. Use the existing test structure and
TRUSTED_CONTROL_PLANE_ONLY_FILES symbols without changing the predicate itself.
---
Outside diff comments:
In `@tools/advisors/risk-plan.mts`:
- Around line 74-78: Update TRUSTED_CONTROL_PLANE_ONLY_FILES to derive its
entries from the canonical E2E_CONTROL_PLANE_FILES inventory instead of
duplicating path literals, while preserving the trusted subset used for
credentialed E2E authorization. If direct derivation is not supported, add an
invariant that verifies the trusted set remains a subset of
E2E_CONTROL_PLANE_FILES.
🪄 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: c865da3d-535c-4aaf-96ea-03aff81f8653
📒 Files selected for processing (1)
tools/advisors/risk-plan.mts
|
Review follow-up: the static job summary is intentional after CodeQL flagged network-derived check data being written to a file. The native job conclusion remains the terminal verdict, and the strictly validated controller URL is available in the job log; the PR description and operator docs now state that boundary. The suggested trusted-set subset derivation was not applied because |
wscurran
added
area: ci
3 participants
Summary
Replace the synthetic required-check identity with a native
E2E / PR Gatejob in the base-trustedpull_request_targetworkflow. The Checks API result becomesE2E / PR Gate Coordination, so suite-association ambiguity like the behavior observed on #6887 cannot leave the required native job unreported while exact-head/base validation and credentialed E2E authorization remain unchanged.Changes
E2E / PR Gatejob that executes fromgithub.workflow_sha, waits for the trusted exact-diff coordination result, publishes the terminal verdict as its native job result, logs the validated trusted run link, and keeps the job summary static.E2E / PR Gate Coordination; authenticate it by exact external identity and GitHub Actions app, retain the old name only as a rollout bridge, and keep maintainer-authorization states pending.Type of Change
Quality Gates
github.workflow_sha, executes no PR code, and has onlychecks: read,contents: read, andpull-requests: read.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-required.test.ts test/pr-e2e-gate-workflow.test.ts(27 passed);npm run typecheck:cli;npm run test:projects:checknpm test(1,510 files passed, 3 skipped; 17,207 tests passed, 40 skipped)npm run docsbuilds without warnings (doc changes only) — build passed with two pre-existing hidden Fern warningsSigned-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Documentation
Tests