ci(e2e): unify maintainer approval - #7832
Conversation
📝 WalkthroughWalkthroughThe PR E2E gate now uses a unified maintainer-only ChangesUnified PR E2E approval
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Maintainer
participant PRGateWorkflow
participant PrE2EGateController
participant CoordinationCheck
Maintainer->>PRGateWorkflow: Dispatch approve-e2e with PR and SHA inputs
PRGateWorkflow->>PrE2EGateController: Invoke controller in approve-e2e mode
PrE2EGateController->>CoordinationCheck: Validate approval and revision identity
PrE2EGateController->>PRGateWorkflow: Start authorized E2E evaluation
Possibly related PRs
Suggested labels: 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 |
![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 (1)
tools/e2e/pr-e2e-gate.mts (1)
3108-3127: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFork authorization instructions omit the "gate passes only if evidence verifies" clarification.
The control-plane authorization message (Line 3156) explicitly states: "This gate passes only if the dispatched evidence references both SHAs and verifies successfully." The fork authorization message doesn't carry the equivalent clarification, only stating that no job ran and no credential was exposed. Given the path instructions explicitly require that "Approval is authorization only: it must not make the gate pass by itself," maintainers approving fork PRs should get the same clarification as maintainers approving control-plane PRs to avoid assuming
approve-e2ealone completes the gate.As per path instructions (test/e2e/README.md): "Approval is authorization only: it must not make the gate pass by itself—coordination succeeds only when the selected E2E run succeeds and every selected job shard/typed target uploads complete passing evidence."
📝 Proposed fix to align fork instructions with control-plane instructions
[ `Review scope: PR #${pull.number}; head repository \`${command.headRepository}\`; head SHA \`${command.headSha}\`; base SHA \`${ciIdentity.baseSha}\`; ${selectionSummary}; deterministic plan \`${plan.planHash}\`.`, "No selected E2E job or target ran. No repository credential was exposed to fork code.", `A repository maintainer must review the exact fork code and risk plan in ${gateRunLink}, then launch a first-attempt \`approve-e2e\` operation from the [${WORKFLOW_NAME}](${workflowUrl}) workflow.`, `Use \`pr_number=${pull.number}\`, \`expected_head_sha=${command.headSha}\`, \`expected_base_sha=${ciIdentity.baseSha}\`, and a specific \`review_reason\`. The trusted controller verifies the maintainer role and exact reviewed inputs before it dispatches this plan.`, + "This gate passes only if the dispatched evidence references both SHAs and verifies successfully.", ].join("\n\n"),🤖 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 3108 - 3127, Update the fork authorization message assembled in the markCheckInProgress call to explicitly state that approval only authorizes execution and does not make the gate pass. Add the clarification that the gate passes only after the dispatched evidence references both SHAs and verifies successfully, while preserving the existing fork credential and review instructions.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/e2e/README.md`:
- Around line 772-774: Update the shared resolver description in the README to
replace “matching fork approval state” with unified coordination-state
terminology, such as matching pending coordination or maintainer approval state.
Do not imply a separate fork-specific authorization mechanism.
---
Outside diff comments:
In `@tools/e2e/pr-e2e-gate.mts`:
- Around line 3108-3127: Update the fork authorization message assembled in the
markCheckInProgress call to explicitly state that approval only authorizes
execution and does not make the gate pass. Add the clarification that the gate
passes only after the dispatched evidence references both SHAs and verifies
successfully, while preserving the existing fork credential and review
instructions.
🪄 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: e647d18b-fbaa-40e0-aca9-c43cf7091f54
📒 Files selected for processing (12)
.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.md.github/workflows/pr-e2e-gate.yamltest/e2e/README.mdtest/maintainer-skills-policy.test.tstest/pr-e2e-gate-command.test.tstest/pr-e2e-gate-dispatch-recovery.test.tstest/pr-e2e-gate-fork-approval.test.tstest/pr-e2e-gate-lifecycle.test.tstest/pr-e2e-gate-retry-history.test.tstest/pr-e2e-gate-workflow.test.tstest/pr-e2e-required.test.tstools/e2e/pr-e2e-gate.mts
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: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
2 participants
Summary
Use one trusted
approve-e2eoperation for internal and fork PR authorization. Any account with currentmaintainoradminpermission can authorize the exact PR SHA, base SHA, and risk plan. The controller still requires passing evidence before it completes the gate.Changes
maintainoradminrepository permission for everyapprove-e2erequest.Type of Change
Quality Gates
426b12b16with no findings. The controller checks currentmaintainoradminpermission and rejectswritepermission before any credential-bearing dispatch.Documentation Writer Review
docs-updated.agents/skills/nemoclaw-maintainer-day/MERGE-GATE.mdandtest/e2e/README.mddocument the maintainer prerequisite, exact-revision review, credential risk, failure behavior, and passing-evidence criterion. The review found no actionable findings and confirmed that nodocs/source page is needed.DGX Station Hardware Evidence
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 unavailablenpm exec -- vitest run --project integration test/pr-e2e-gate*.test.ts test/pr-e2e-required.test.ts test/maintainer-skills-policy.test.tspassed 16 files and 276 tests on426b12b16.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable. This change does not modify the repository-wide test harness or validation configuration.npm run check:diffpassed, and required CI supplies the broad Linux result.npm run docsbuilds without warnings (doc changes only)npm run docspassed with zero errors and two existing Fern warnings.Signed-off-by: Julie Yaunches jyaunches@nvidia.com