Skip to content

bug: Maintainer Approval check fails and emails on every review comment #4559

Closed

Description

@purp

User Story

I maintain OpenShell and review pull requests daily. Since #3176 landed, contributors and reviewers get a "Run failed: Maintainer Approval" email every time they comment on a PR that has no maintainer approval yet, so the gate meant to be silent has become one of our noisiest notifications.

Problem Statement

.github/workflows/maintainer-approval.yml runs on every pull_request_review submitted and dismissed event. Most of those events cannot change the outcome:

  • Comment reviews. Every comment review, including each reply in a review thread, is a submitted review with state commented. check_maintainer_approval.py ignores COMMENTED, so the result never changes, but the job re-runs, fails again, and GitHub emails whoever posted the comment.
  • Pushes. dismiss_stale_reviews_on_push dismisses approvals on push, which fires dismissed. The new head commit has no approvals, so the outcome is already known to be a failure.

Since the check is the job's own exit code, every one of these runs fails, and GitHub emails a failure notice to the actor who triggered it. Kris Hicks reported getting these emails while replying to review threads on their own PR (#4500). Their two failures at 17:44 and 17:45 UTC on 2026-10-14 were both their own COMMENTED reviews.

On 2026-10-14 (UTC) the workflow failed 35 times. For example, rhuss got 8 failures in 25 minutes commenting on one RFC PR, and alangou got 6 on their own PR.

Why the obvious fix is unsafe

Skipping comment reviews (or push dismissals) with a job-level if: would make the noise stop, but the job's name is the required status context. A skipped job reports skipped, and GitHub treats skipped as passing for a required check. #4496 relies on exactly that to clear the merge queue. Once the context is required:

  • A comment on an unapproved PR would replace the earlier failure on that commit with skipped, which passes the gate.
  • A skipped run after a push would put skipped on the new head, which passes the gate with zero reviews.

The fix therefore cannot be a skip as long as the job's own result is the gate.

Impact / Why This Matters

There is no per-workflow way to silence Actions failure emails. The only workaround is for each person to turn off Actions email notifications entirely.

Acceptance Criteria

  • Submitting a comment-only review, or replying in a review thread, does not re-evaluate the gate and does not send anyone a failure email.
  • Approval, request-changes, and manual dismissal still re-evaluate the gate.
  • A push leaves the new head commit blocked until a maintainer approves again, without sending a failure email.
  • No event (comment, skip, merge queue, fork PR) can turn a blocked PR green without a current maintainer approval.
  • The gate works on fork PRs, where review-triggered runs get a read-only GITHUB_TOKEN.
  • The merge queue is still satisfied, as it is today after fix(ci): skip maintainer approval in merge queue #4496.
  • The required status context name to add to ruleset 13332227 is documented in CONTRIBUTING.md.

Reproduction Steps

  1. Open a PR against NVIDIA/OpenShell with no maintainer approval.
  2. As anyone, submit a comment review or reply to an existing review thread.
  3. The Maintainer Approval workflow runs, OpenShell / Maintainer Approval fails, and the commenter receives a "Run failed" email.
  4. Repeat step 2. Each comment produces another failed run and another email.

Environment

Suggested Approach

One approach that would meet the criteria is to stop using the job's own result as the gate:

  • The review-triggered workflow would skip commented reviews and never fail, under a job name that is not the required context.
  • A workflow_run workflow, which always runs main's definition with a write-capable token (fork PRs included), would evaluate approval and write an OpenShell / Maintainer Approval commit status on the PR head.
  • A push would leave the new head without a status, so the required context stays pending.
  • Merge-queue runs would write a passing status on the merge group commit.

Commit status failures do not send email. As a side effect, the decision would always run main's copy of the workflow, which closes the workflow-tampering path that #3176 currently covers with CODEOWNERS.

Logs

Example runs, all pull_request_review events on 2026-10-14:

Activity

  1. purp commented on Oct 20, 2026

    @purp
    CollaboratorAuthor

    Not easily possible to quiesce these. GitHub wraps every response comment in its own one-comment COMMENT type review, so it fires the pull_request_review event each time. Gross.

    We should look at returning to a codeowner solution after the repo move and some area/topics dust settles.

    Comments from the agent below, session name was maintainer comment alerts for future me if I need to pick it back up.

    Short answer: Kris's replies trigger the check because GitHub wraps every inline reply in its own one-comment review, and that review submission is exactly what pull_request_review: submitted fires on. There is no trigger-level way to exclude those. Filtering inside the job would open a merge bypass, so the options are to live with the runs or change the enforcement primitive.

    What the PR shows. PR #4500 has 9 review objects and 9 workflow runs, one per review. Every reply Kris made has its own pull_request_review_id and state COMMENTED. GitHub records Kris as the triggering actor on those runs, which means Kris also received a "workflow failed" email for each reply until Taylor approved. Taylor's first two comments share one review because Taylor used "Start a review" and batched them. Each reply, on both sides, was a single-click comment, and single-click comments get an implicit review created and submitted on the spot.

    Why the events are separate. They describe different objects with different cardinality and lifecycle, not redundant views of the same thing.

    • pull_request_review fires once per review container: submitted, edited, or dismissed. A batched review with five comments is one event. An approval with no comments is one event and zero comment events.
    • pull_request_review_comment fires once per inline comment: created, edited, or deleted. Editing or deleting a comment later touches the comment but not the review, so only this event fires.
    • pull_request_comment does not exist. Top-level conversation comments are issue_comment, and those are not reviews at all.

    The surprise is only that a single reply produces both a comment event and a review event. The on: block has no review-state filter. GitHub's docs say to filter on github.event.review.state in a job-level if: instead, and that is where the trap is.

    Why a job-level if: is not safe here. GitHub's documented rule is that a job skipped by a conditional reports Success, and required checks look at the latest check run for that name on the head SHA. So with if: github.event.review.state != 'commented', any comment on an unapproved PR would produce a skipped run that overwrites the earlier failure and turns the gate green. The two-check pattern you rejected is the standard workaround for precisely this, so rejecting it leaves no in-workflow fix.

    One finding you should know. The main ruleset's required status checks are currently Branch Checks, Helm Lint, E2E, and GPU E2E. "OpenShell / Maintainer Approval" is not in that list, and classic branch protection on main requires no status checks either. The workflow comment still describes it as the required context. I did not look into when or why it dropped out.

    Remaining options:

    • Accept the runs. Each takes about ten seconds and costs nothing. A failed run before approval blocks exactly as a pending check would, so the extra runs are cosmetic plus the reviewer emails. Reviewers can silence those only by turning off Actions notifications for themselves.
    • Move enforcement to a native primitive. The ruleset's pull-request rule supports required_reviewers with file patterns, and CODEOWNERS could carry a wildcard rule. Both were set aside when the workflow was written because outside collaborators cannot join org teams and because native rules request reviews and notify people. Those constraints still hold, so this is a trade of one kind of noise for another.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions