[PR #283] [MERGED] feat(assign-reviewers): optional REVIEWER_SKIP_BASE_BRANCHES to skip stacked-PR lanes #285

New issue

Closed

opened 2026-10-26 22:38:04 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:38:04 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/283
Author: @mattmillerai
Created: 9/9/2026
Status: ✅ Merged
Merged: 9/9/2026
Merged by: @mattmillerai

Base: main ← Head: matt/skip-assign-on-stacked-prs

📝 Commits (1)

42c8320 feat(assign-reviewers): optional REVIEWER_SKIP_BASE_BRANCHES to skip stacked-PR lanes

📊 Changes

3 files changed (+122 additions, -24 deletions)

View changed files

📝 .github/assign-reviewers/tests/assignment.test.cjs (+35 -0)
📝 .github/workflows/assign-reviewers.yml (+62 -24)
📝 docs/callers/assign-reviewers.md (+25 -0)

📄 Description

ELI-5

Auto-assign currently puts a human on every PR. If you're building something as a stack of ten small PRs onto one integration branch, that's ten people pinged for reviews nobody plans to do there — the real review happens once, at the end, on the promotion PR. This adds a repo setting: list the branches you don't want routed (stack/**) and PRs onto them get no assignee. Everything aimed at main behaves exactly as it does today.

Why

A stacked-PR lane defers review to a single promotion PR by design. Routing an owner onto each child pages a human per commit for a review that isn't intended there, and that kind of noise is what gets an automation muted for the whole repo rather than tuned.

Existing options don't fit. skip_label works per-PR and manually, so it means remembering to label every child. REVIEWER_AUTHOR_ALLOWLIST is author-scoped, which is the wrong axis — the same author wants routing on their main PRs and off on their stacked ones.

What changed

vars.REVIEWER_SKIP_BASE_BRANCHES — whitespace-separated globs, matched against the PR's base ref. A match skips the PR with an informational log, never a failing check.

Unset or empty keeps today's behaviour, so no existing caller changes until it sets the var — the same opt-in shape as REVIEWER_AUTHOR_ALLOWLIST.

Two deliberate differences from that var:

This gate fails open. A pattern matching nothing routes as usual, so a typo costs you the exemption rather than silently disabling routing repo-wide. REVIEWER_AUTHOR_ALLOWLIST fails closed because getting it wrong there is the dangerous direction; here it's the reverse.
It's per-lane and automatic, where skip_label is per-PR and manual. Both remain — they're for different jobs, and the header and caller doc say which to reach for.

The glob helper moves above the early-exit block so the check runs before any API call; a skipped PR now costs zero requests. Same function, unchanged semantics, and it's the one already used for the path rules: ** spans segments, * stays inside one, and a wildcard-free pattern is an exact match.

How has this been tested?

node --test .github/assign-reviewers/tests/*.test.cjs — 68 pass, 0 fail (was 64; +4 new). The harness executes the shipped inline script rather than a copy, so these exercise the real thing.

The default-off case is asserted first and hardest — unset, empty, and whitespace-only all route normally. It's every existing caller.
A PR onto stack/router-queue-poc with stack/** set is not routed; the same PR onto the default branch still is.
Glob semantics pinned in both directions, because a false positive here silently exempts branches nobody meant to: release matches release but not release/1.2 or releases; stack/* matches stack/a but not stack/a/b; stack/** wip/** shows multiple patterns.
One test asserts the skip performs no file listing and no config read, so the early exit stays free.

actionlint clean on the reusable; YAML parses; the 64 pre-existing tests are untouched and still green, which is what covers the helper move.

Residual

Not exercised against a live PR. The tests drive the shipped script with a stubbed octokit, so the vars plumbing itself (vars.REVIEWER_SKIP_BASE_BRANCHES reaching env) is verified only by reading the workflow, as with every other var here.
Only assign-reviewers.yml gained this. assign-prs-to-author.yml is a separate workflow and may want the same knob; not assumed, not added.
Callers pin this reusable by SHA, so nothing changes for anyone until their pin moves — via the ASSIGN_REVIEWERS_CALLERS roster bump or by hand.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/283 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/9/2026 **Status:** ✅ Merged **Merged:** 9/9/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/skip-assign-on-stacked-prs` --- ### 📝 Commits (1) - [`42c8320`](https://github.com/Comfy-Org/github-workflows/commit/42c83203ca6ab8c7344cf4b2278e1262e637a516) feat(assign-reviewers): optional REVIEWER_SKIP_BASE_BRANCHES to skip stacked-PR lanes ### 📊 Changes **3 files changed** (+122 additions, -24 deletions) <details> <summary>View changed files</summary> 📝 `.github/assign-reviewers/tests/assignment.test.cjs` (+35 -0) 📝 `.github/workflows/assign-reviewers.yml` (+62 -24) 📝 `docs/callers/assign-reviewers.md` (+25 -0) </details> ### 📄 Description ## ELI-5 Auto-assign currently puts a human on every PR. If you're building something as a stack of ten small PRs onto one integration branch, that's ten people pinged for reviews nobody plans to do there — the real review happens once, at the end, on the promotion PR. This adds a repo setting: list the branches you don't want routed (`stack/**`) and PRs onto them get no assignee. Everything aimed at `main` behaves exactly as it does today. ## Why A stacked-PR lane defers review to a single promotion PR by design. Routing an owner onto each child pages a human per commit for a review that isn't intended there, and that kind of noise is what gets an automation muted for the whole repo rather than tuned. Existing options don't fit. `skip_label` works per-PR and manually, so it means remembering to label every child. `REVIEWER_AUTHOR_ALLOWLIST` is author-scoped, which is the wrong axis — the same author wants routing on their `main` PRs and off on their stacked ones. ## What changed `vars.REVIEWER_SKIP_BASE_BRANCHES` — whitespace-separated globs, matched against the PR's **base** ref. A match skips the PR with an informational log, never a failing check. Unset or empty keeps today's behaviour, so **no existing caller changes until it sets the var** — the same opt-in shape as `REVIEWER_AUTHOR_ALLOWLIST`. Two deliberate differences from that var: 1. **This gate fails open.** A pattern matching nothing routes as usual, so a typo costs you the exemption rather than silently disabling routing repo-wide. `REVIEWER_AUTHOR_ALLOWLIST` fails closed because getting it wrong there is the dangerous direction; here it's the reverse. 2. **It's per-lane and automatic**, where `skip_label` is per-PR and manual. Both remain — they're for different jobs, and the header and caller doc say which to reach for. The glob helper moves above the early-exit block so the check runs **before any API call**; a skipped PR now costs zero requests. Same function, unchanged semantics, and it's the one already used for the path rules: `**` spans segments, `*` stays inside one, and a wildcard-free pattern is an exact match. ## How has this been tested? `node --test .github/assign-reviewers/tests/*.test.cjs` — **68 pass, 0 fail** (was 64; +4 new). The harness executes the shipped inline script rather than a copy, so these exercise the real thing. - The **default-off** case is asserted first and hardest — unset, empty, and whitespace-only all route normally. It's every existing caller. - A PR onto `stack/router-queue-poc` with `stack/**` set is not routed; the same PR onto the default branch still is. - Glob semantics pinned in **both** directions, because a false positive here silently exempts branches nobody meant to: `release` matches `release` but **not** `release/1.2` or `releases`; `stack/*` matches `stack/a` but not `stack/a/b`; `stack/** wip/**` shows multiple patterns. - One test asserts the skip performs **no** file listing and **no** config read, so the early exit stays free. `actionlint` clean on the reusable; YAML parses; the 64 pre-existing tests are untouched and still green, which is what covers the helper move. ## Residual - Not exercised against a live PR. The tests drive the shipped script with a stubbed octokit, so the `vars` plumbing itself (`vars.REVIEWER_SKIP_BASE_BRANCHES` reaching `env`) is verified only by reading the workflow, as with every other var here. - Only `assign-reviewers.yml` gained this. `assign-prs-to-author.yml` is a separate workflow and may want the same knob; not assumed, not added. - Callers pin this reusable by SHA, so nothing changes for anyone until their pin moves — via the `ASSIGN_REVIEWERS_CALLERS` roster bump or by hand. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:38:04 +00:00