Skip to content

Merge suggest-reviewers into maintainer-approval as a single workflow#4920

Merged
simonfaltum merged 2 commits intomainfrom
simonfaltum/merge-approval-reviewers
Apr 8, 2026
Merged

Merge suggest-reviewers into maintainer-approval as a single workflow#4920
simonfaltum merged 2 commits intomainfrom
simonfaltum/merge-approval-reviewers

Conversation

@simonfaltum
Copy link
Copy Markdown
Member

Why

We have two separate workflows for PR review management: one that checks approval status (maintainer-approval) and one that posts reviewer suggestions (suggest-reviewers). Having them separate means the approval check doesn't tell you who to ask, and the reviewer suggestion doesn't know about approval state. It makes more sense for a single workflow to both check approval AND communicate who's needed.

Changes

Before: Two workflows. maintainer-approval sets a commit status (pending/success) but says nothing about who to ask. suggest-reviewers posts a comment with git-history-based suggestions but doesn't know about approval state. They run on different triggers and don't share context.

Now: One workflow (PR approval) that does both. Every time it runs, it:

  1. Checks approval status and sets the commit status (same as before)
  2. Deletes any previous comment it posted (finds by marker)
  3. Posts a fresh comment at the bottom of the conversation showing:
    • Which ownership groups are approved vs pending
    • Reviewer suggestions per group (from git history scoring)
    • Maintainer footer

maintainer-approval.js

Absorbed all logic from suggest-reviewers.js: git history scoring (scoreContributors, gitLog, resolveLogin), comment building (buildComment, buildPerGroupComment), round-robin fallback, and comment management. The comment uses a <!-- MAINTAINER_APPROVAL --> marker. During migration, also cleans up legacy <!-- REVIEWER_SUGGESTION --> comments from the old workflow.

Comment management is delete-then-create: each run deletes all existing comments matching the marker, then posts a fresh one. This keeps the comment at the bottom of the conversation timeline.

maintainer-approval.yml

  • Renamed to "PR approval"
  • Added ready_for_review to pull_request_target types (so draft-to-ready gets a comment)
  • pull-requests: write (was read, needs write to post comments)
  • contents: read + fetch-depth: 0 (full history for git log scoring)
  • Checkout uses ref: ${{ github.event.pull_request.base.sha }} to ensure only trusted base-branch code runs on pull_request_review events
  • Added concurrency group per PR to prevent overlapping runs
  • Skips drafts via if condition

Deleted

  • .github/workflows/suggest-reviewers.js
  • .github/workflows/suggest-reviewers.yml

test-owners-scripts.yml

  • Removed suggest-reviewers.js from path filter and test command

Tests

  • 43 tests pass (24 owners.js + 19 maintainer-approval.js)
  • Added 7 new tests for comment posting: comment creation, delete-then-create, approval status in comments, per-group comments, wildcard handling

Test plan

  • 43 unit tests passing (node --test .github/scripts/owners.test.js .github/workflows/maintainer-approval.test.js)
  • Cursor agent review: approved with nits after 2 rounds (fixed security issue with checkout ref, draft-to-ready trigger gap, and comment cleanup)
  • Verify on a real PR that the comment appears with approval status
  • Verify that re-running after a review updates the comment (deletes old, posts new)
  • Verify legacy <!-- REVIEWER_SUGGESTION --> comments get cleaned up
  • Update branch ruleset to keep maintainer-approval commit status context

The suggest-reviewers workflow posted a comment with reviewer suggestions
based on git history. The maintainer-approval workflow checked approval
status and set a commit status. This merges both into one workflow so
every PR gets a single comment that shows both approval status and
reviewer suggestions.

Changes:
- maintainer-approval.js now scores contributors via git history and
  posts a comment with approval status per ownership group, reviewer
  suggestions, and eligible reviewers. Uses delete-then-create for
  comments so they always appear at the bottom.
- maintainer-approval.yml gets pull-requests:write, contents:read,
  fetch-depth:0 (full history for git log), and skips drafts.
- Deleted suggest-reviewers.js and suggest-reviewers.yml.
- Updated test-owners-scripts.yml to remove suggest-reviewers path.
- Added 7 new tests for comment posting (marker, delete-then-create,
  per-group, single-domain, all-approved formats).

Co-authored-by: Isaac
…oval

- Checkout base branch SHA to prevent PR-authored code execution on review events
- Add ready_for_review to pull_request_target types so draft promotions trigger
- Delete ALL matching comments (not just first) including legacy REVIEWER_SUGGESTION marker
- Add concurrency group to prevent overlapping runs per PR

Co-authored-by: Isaac
@simonfaltum simonfaltum added this pull request to the merge queue Apr 8, 2026
Merged via the queue into main with commit d503c58 Apr 8, 2026
22 checks passed
@simonfaltum simonfaltum deleted the simonfaltum/merge-approval-reviewers branch April 8, 2026 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants