ci(e2e): explain maintainer approval state - #7863
Conversation
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
📝 WalkthroughWalkthroughThe PR adds state-specific maintainer-approval diagnostics for the E2E gate, detects already-running execution, routes invalid approval states through targeted errors, and removes control-plane-specific authorization handling from the approval flow. ChangesMaintainer approval diagnostics
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Maintainer
participant startAuthorizedPrGate
participant CoordinatorCheck
participant maintainerApprovalStateError
Maintainer->>startAuthorizedPrGate: request approve-e2e
startAuthorizedPrGate->>CoordinatorCheck: inspect status and title
startAuthorizedPrGate->>maintainerApprovalStateError: classify non-pending state
maintainerApprovalStateError-->>Maintainer: return targeted error
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Sensitive-path security reviewVerdictPASS. The change is safe from a security-boundary perspective. It changes only the error selected after the existing authorization predicate has rejected a state; exact PR/head/base matching, current maintainer role, first workflow attempt, single matching check, live fork detection, pending status, and exact expected title remain unchanged. No dispatch or check mutation is reached by any rejected test case. FindingsNo security findings. Category review
Files reviewed
Reviewed exact head: |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit d74c058 in the TypeScript / code-coverage/cliThe overall coverage in commit d74c058 in the Show a code coverage summary of the most impacted files.
Updated |
cjagwani
added
area: ci
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 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. |
![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 (2)
tools/e2e/pr-e2e-gate.mts (2)
3314-3314: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not publish the untrusted review reason in check output.
reasonis dispatch input and is interpolated into a public check-run message. Backtick replacement does not prevent secret disclosure or Markdown/control-text injection. Keep this message fixed or use a tightly bounded non-sensitive identifier, and add a regression assertion that arbitrary review reasons are absent.🤖 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` at line 3314, Update the check-run message in the reviewed fork PR flow to avoid interpolating the untrusted reason value; use fixed text or a tightly bounded non-sensitive identifier instead. Add a regression assertion confirming arbitrary review reasons do not appear in check output, while preserving the existing message behavior otherwise.
3247-3249: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFail closed when the PR head repository is unavailable.
This condition rejects same-repository PRs, but
pull.head.repo?.full_namemay be absent for a deleted or otherwise unavailable fork. In that case the condition is false andapprove-e2ecan continue without proving that the PR is fork-based. Require a non-empty head repository and ensure it differs fromrepository.Proposed fix
- if (pull.head.repo?.full_name === repository) { + const headRepository = pull.head.repo?.full_name; + if (!headRepository || headRepository === repository) { throw new Error("approve-e2e is only supported for fork pull requests"); }🤖 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 3247 - 3249, The fork validation around pull.head.repo?.full_name must fail closed when the head repository is missing. Update the condition in the approve-e2e validation to require a non-empty head repository name and reject it when it equals repository, preserving the existing error behavior for invalid or same-repository pull requests.
🤖 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-gate.mts`:
- Line 3314: Update the check-run message in the reviewed fork PR flow to avoid
interpolating the untrusted reason value; use fixed text or a tightly bounded
non-sensitive identifier instead. Add a regression assertion confirming
arbitrary review reasons do not appear in check output, while preserving the
existing message behavior otherwise.
- Around line 3247-3249: The fork validation around pull.head.repo?.full_name
must fail closed when the head repository is missing. Update the condition in
the approve-e2e validation to require a non-empty head repository name and
reject it when it equals repository, preserving the existing error behavior for
invalid or same-repository pull requests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 37d23e3e-78e8-4c59-8b85-202179b08afc
📒 Files selected for processing (1)
tools/e2e/pr-e2e-gate.mts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Approve — reviewed exact head d74c058. The change preserves the existing maintainer-approval authorization predicate and replaces only its opaque rejection with fixed, state-specific fail-closed diagnostics. Tests cover preparing, queued, running, terminal, malformed, missing, null, and non-string states and prove no mutation, dispatch, or echo of observed untrusted output. Exact-head CI and E2E gate pass; no blocking defect found.
3 participants
Summary
Replace the opaque maintainer-approval state error with bounded, actionable diagnostics. Approval still fails closed on every mismatch, but maintainers can now distinguish an early, wrong-route, already-running, terminal, or malformed coordination state without exposing the observed check output.
Related Issue
Fixes #7861
Changes
Type of Change
Quality Gates
d74c058fe80154db11a856850cae9b7627783d9fagainst based5b64a72a5fcb8299e8cec1c2f22746b5c0a7f32(tree2fe73e677f0f71ec07d9b38af3709403f64fd174; stable patch11fbee7d594e9ba80e110c87d56a2e1174bd816e). All nine security categories PASS; the exact authorization predicate is unchanged, all classified states fail closed before mutation or dispatch, and observed output is never echoed.Documentation Writer Review
docs-updatedtest/e2e/README.mdto document the exact pending authorization title and fixed fail-closed remediation for preparing, executing, terminal, and malformed coordination states. Reviewed every changed diagnostic, test title, commit message, and documentation sentence against the repository writing rules and exact controller behavior. Focused fork-approval tests passed 24/24;npm run docspassed with 0 errors and 2 existing warnings;npm run validate:prpassed.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-fork-approval.test.ts(24/24)npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable to a bounded diagnostic-only controller change.npm run docsbuilds without warnings (doc changes only)Additional exact-head validation: focused fork-approval tests passed 24/24;
npm run validate:prpassed;npm run docscompleted with 0 errors and 2 existing warnings.Signed-off-by: Charan Jagwani cjagwani@nvidia.com
Summary by CodeRabbit