Commit d503c58
authored
Merge suggest-reviewers into maintainer-approval as a single workflow (#4920)
## 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
- [x] 43 unit tests passing (`node --test .github/scripts/owners.test.js
.github/workflows/maintainer-approval.test.js`)
- [x] 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
context1 parent d3ba591 commit d503c58
6 files changed
Lines changed: 571 additions & 554 deletions
0 commit comments