ci(e2e): publish direct PR gate check - #7935
Conversation
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
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 makes ChangesController-owned required check
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 848d79a in the TypeScript / code-coverage/cliThe overall coverage in commit 848d79a in the Show a code coverage summary of the most impacted files.
Updated |
VerdictPASS — safe to merge after required CI. The change removes a read-only polling/mirroring job and makes the existing trusted controller publish the already-required Reviewed head: 217b10a FindingsNo security findings. Detailed analysis
Files reviewed
|
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 1 semantic terminology decisionTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
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: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
![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)
test/pr-e2e-gate.test.ts (1)
478-501: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the order-sensitive fixture contain multiple commits.
This test now claims to verify GitHub comparison commit order, but
ahead_by: 1with one commit cannot catch regressions in selecting the final commit. Useahead_by: 2,total_commits: 2, and place a non-main SHA beforeADVANCED_WORKFLOW_SHA.As per path instructions, tests should prove the observable behavior rather than only exercise a one-element fixture.
Suggested fixture update
- total_commits: 1, + total_commits: 2, ... - commits: [{ sha: ADVANCED_WORKFLOW_SHA }], + commits: [ + { sha: "f".repeat(40) }, + { sha: ADVANCED_WORKFLOW_SHA }, + ],Also update the surrounding
ahead_byvalue to2.🤖 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-gate.test.ts` around lines 478 - 501, Update the comparison fixture in the “dispatches from a safe descendant in GitHub comparison commit order” test to represent two commits: set ahead_by and total_commits to 2, and place a non-main commit SHA before ADVANCED_WORKFLOW_SHA in commits. Keep the expected observable dispatch behavior unchanged.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.
Outside diff comments:
In `@test/pr-e2e-gate.test.ts`:
- Around line 478-501: Update the comparison fixture in the “dispatches from a
safe descendant in GitHub comparison commit order” test to represent two
commits: set ahead_by and total_commits to 2, and place a non-main commit SHA
before ADVANCED_WORKFLOW_SHA in commits. Keep the expected observable dispatch
behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 62d2aa47-1235-44b0-bd73-f0fb26ee4726
📒 Files selected for processing (3)
test/e2e/README.mdtest/pr-e2e-gate.test.tstools/e2e/pr-e2e-gate.mts
wscurran
added
area: ci
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Approve — reviewed exact head 217b10a. The direct controller-owned E2E / PR Gate preserves exact PR head/base identity, GitHub Actions App ownership, retry lineage, live-state revalidation, and fail-closed child authorization while removing the duplicate polling mirror. Focused gate and workflow-boundary coverage is extensive, and I found no blocking correctness, security, compatibility, or regression defect. The recorded gate failure correctly propagated an unrelated flaky messaging cleanup test rather than exposing a defect in this PR.
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> # Conflicts: # .agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md # test/e2e/README.md
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Security review — exact head
|
| Category | Verdict | Evidence |
|---|---|---|
| Secrets and credentials | PASS | No secrets or credential material added; diff-scoped gitleaks passed. |
| Input validation and data sanitization | PASS | Existing strict PR number, SHA, repository, workflow, check ID, title, and external-identity validation remains in the trusted controller. |
| Authentication and authorization | PASS | Fork execution still requires current maintain/admin permission and an exact pending fork-approval state; child dispatch authenticates the controller-owned check. |
| Dependencies and third-party libraries | PASS | No dependency or action-pin changes. |
| Error handling and logging | PASS | Terminal, malformed, stale, ambiguous, and lost-response states remain fail closed; refreshed diagnostics identify the required check without echoing untrusted output. |
| Cryptography and data protection | PASS | Existing SHA-256 state binding and HTTPS GitHub API transport are unchanged; no custom cryptography added. |
| Configuration and security headers | PASS | Workflow permissions remain job-scoped and least-privilege; the removed observer had read-only permissions. |
| Security testing | PASS | Exact-head validation passed 508 focused integration tests plus 45 E2E-support boundary tests, including fork approval, retry history, runner-loss recovery, rollout compatibility, and workflow contracts. |
| System security | PASS | No TOCTOU relaxation: the controller still revalidates live PR/head/base/check identity before dispatch and finalization, and superseded revisions cannot satisfy the current gate. |
Files reviewed: all 28 files in the exact diff against origin/main, covering the two workflows, controller and merge-gate code, maintainer/E2E documentation, deletion of the polling observer, and all changed tests.
Local verification: npm run check:diff passed, including workflow/config validation, gitleaks, E2E phase plans, source-shape and test-size budgets, CLI typecheck, and pre-push checks.
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Security review refresh — exact head
|
4 participants
Summary
Publish
E2E / PR Gatedirectly from the trusted controller instead of mirroring the controller verdict through a six-hour polling job. The required check keeps the exact PR head, base, GitHub App, retry-history, and final-state validation already enforced by the controller.Related Issue
Closes #7451
Changes
requiredworkflow job and deletetools/e2e/pr-e2e-required.mtswith its observer tests.E2E / PR Gatecontext and update the child-workflow authorization boundary.E2E / PR Gate Coordinationas a rollout-only maintainer-inspection fallback when the current check is absent.approve-e2eroute for selected fork work established by ci(e2e): let maintainers authorize internal runs #7826 and ci(e2e): unify maintainer approval #7832.Type of Change
Quality Gates
Documentation Writer Review
docs-updated.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md,test/e2e/README.md, andtest/e2e/docs/README.mdaccurately document the direct check, rollout fallback, automatic internal dispatch, and fork-onlyapprove-e2eroute. The exact-head documentation review corrected the seeded-check identity rule; a follow-up review confirmed the final test-only fixture change needs no additional prose. Focused documentation-policy validation passed 19 tests.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 were skipped or unavailablenpm exec -- vitest run --project integration test/pr-e2e-gate*.test.ts test/maintainer-skills-policy.test.ts test/pr-risk-plan.test.ts test/skills/check-gates*.test.ts test/vitest-watch-triggers.test.tspassed 23 files and 508 tests; the final ordered-comparison fixture passed 26/26 controller tests;npm exec -- vitest run --project e2e-support test/e2e/support/e2e-operations-workflow-boundary.test.tspassed 45 tests;npm run test:changedpassed with no source-backed tests selected.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable. This is a focused controller/check-name migration; the targeted integration and workflow-boundary suites cover the changed behavior, and required CI supplies the broad result.npm run check:diffandnpm run typecheck:clipassed against currentmain.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Charan Jagwani cjagwani@nvidia.com
Summary by CodeRabbit
Bug Fixes
E2E / PR Gatecheck, with clearer retry semantics and fail-closed behavior.Documentation
Tests