[PR #257] [MERGED] feat(assign-reviewers): add an opt-in author allowlist (BE-11575) #263

New issue

Closed

opened 2026-10-26 22:37:53 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:37:53 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-11575-assign-reviewers-author-allowlist

📝 Commits (1)

88a7e3a feat(assign-reviewers): add an opt-in author allowlist (BE-11575)

📊 Changes

3 files changed (+49 additions, -6 deletions)

View changed files

📝 .github/workflows/assign-reviewers.yml (+33 -5)
📝 README.md (+1 -1)
📝 docs/callers/assign-reviewers.md (+15 -0)

📄 Description

ELI5

Right now the PR auto-assigner runs for everyone's PRs in a repo that enrolls it. There's no middle setting between "on for the whole repo" and "off". This adds one: a repo can list the specific PR authors it wants routed, and everyone else's PRs are left alone. Nothing changes for any repo until an admin sets the variable.

Motivation

Raised in #proj-cloud: the auto-assigner pulls in people outside the PR's pod (who then aren't actually watching it) and assigns people who are out on leave — which undercuts the one guarantee the Assignee field exists to provide, that exactly one person is accountable for driving a PR to completion. The proposal on the table was to disable the automation org-wide.

The automation was deliberately shipped unrefined so the team could iterate on it, so this is the smaller knob instead: scope it to opt-in authors, and let the people who want their PRs routed keep it. It does not settle the broader question of whether the automation should exist at all — that's still an open team decision, and this just stops the answer from having to be all-or-nothing.

Provenance

Authored by: interactive session, from BE-11575.
Verified: check_workflow_pins.py (11 workflows, none defaulted, every ref checkout guarded); check_agents_md.py --root . passed (2 pre-existing warnings, unrelated); YAML parses and the new env key is present. There is no script test suite for this reusable (no test-assign-reviewers.yml, unlike agents-md-integrity or refresh-reviewers), so behavior was verified by extracting the github-script body and running it under stubbed octokit/core across six cases — see Test plan.
Deviations: one, called out below — the allowlist matches logins case-insensitively and tolerates a leading @, where REVIEWER_EXCLUDE matches exactly. Rationale in "Reviewer context".

Reviewer context

Type: additive config knob on an existing reusable. No new inputs, no new actions, no new permissions.
Slots into: assign-reviewers.yml. Merging trips bump-assign-reviewers-callers.yml's path filter, so the fleet carries it to consumers — all of them on unchanged behavior until they set the variable. No Skip-caller-bump trailer, deliberately: this is behavioral.
Not in scope: actually setting REVIEWER_AUTHOR_ALLOWLIST on any repo (a Settings → Variables change, done per repo after this ships); leave-awareness, which is the second, distinct failure mode raised in the thread and needs a data source this change doesn't have; and pod-aware routing, which is a different mechanism.

Summary

New optional caller var vars.REVIEWER_AUTHOR_ALLOWLIST (whitespace-separated logins), wired exactly like the existing REVIEWER_EXCLUDE / REVIEWER_GROWTH_POOL / REVIEWER_LOAD_CAP — caller vars → job env: → parsed in the script.
- Non-empty ⇒ only those authors' PRs are routed.
- Unset or empty ⇒ today's behavior exactly. No existing caller moves until it opts in.
The gate sits with the other early exits and ahead of the live assignee read, so a PR from an author outside the list costs zero API calls and exits with an informational log line — never a failing check. (authorLogin is hoisted up to the early-exit block for it; the later duplicate declaration is removed.)
One deliberate deviation from REVIEWER_EXCLUDE: logins are matched case-insensitively and a leading @ is stripped. The two vars fail in opposite directions. A typo in REVIEWER_EXCLUDE fails open — someone gets assigned who shouldn't, which is visible and self-correcting. A typo here fails closed: nobody matches, routing silently stops repo-wide, and it looks identical to success — green checks, no assignees, no error. Normalizing the two things people actually get wrong (@handle, capitalization) closes the likeliest way to hit that. Nothing else is guessed at — matt still does not match mattmillerai.
Docs updated in all three places the other vars appear: the workflow header comment (config list re-aligned to fit the longer name, plus an AUTHOR SCOPING note), the docs/callers/assign-reviewers.md prerequisites table and intro, and the README catalog row. The caller guide also gains a gotcha for the fail-closed behavior, with the log line to look for when confirming it's working.

Test plan

The github-script body was extracted from the workflow and executed under stubbed octokit/core with a fixture reviewers.yml. All six pass:

#	Case	Expected	Result
A	Allowlist unset	Assigns, unchanged	✅ assigns
B	Set, author absent	No assignment, no API calls, no failure	✅ `Author jacobsegal is not in REVIEWER_AUTHOR_ALLOWLIST — skipping.`, 0 API calls
C	Set, author present	Assigns as today	✅ assigns
D	Whitespace-only value	Treated as unset	✅ assigns
E	`@MattMillerAI` in list vs `mattmillerai` author	Matches	✅ assigns
F	`matt` in list vs `mattmillerai` author	Does not match	✅ skips

python3 .github/workflow-pins/check_workflow_pins.py
python3 .github/agents-md-integrity/check_agents_md.py --root .
New behavior is off by default per repo; unset ⇒ byte-for-byte today's path
After merge + fleet bump: confirm an enrolled caller with the var unset routes exactly as before
Then set REVIEWER_AUTHOR_ALLOWLIST on one repo and confirm the run logs is in REVIEWER_AUTHOR_ALLOWLIST — routing for a listed author and skips cleanly for an unlisted one

Fixes BE-11575

_{🔄 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/257 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/2/2026 **Status:** ✅ Merged **Merged:** 9/2/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-11575-assign-reviewers-author-allowlist` --- ### 📝 Commits (1) - [`88a7e3a`](https://github.com/Comfy-Org/github-workflows/commit/88a7e3a1e818c853246dae2afef065c2d542e810) feat(assign-reviewers): add an opt-in author allowlist (BE-11575) ### 📊 Changes **3 files changed** (+49 additions, -6 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/assign-reviewers.yml` (+33 -5) 📝 `README.md` (+1 -1) 📝 `docs/callers/assign-reviewers.md` (+15 -0) </details> ### 📄 Description ## ELI5 Right now the PR auto-assigner runs for **everyone's** PRs in a repo that enrolls it. There's no middle setting between "on for the whole repo" and "off". This adds one: a repo can list the specific PR authors it wants routed, and everyone else's PRs are left alone. Nothing changes for any repo until an admin sets the variable. ## Motivation Raised in #proj-cloud: the auto-assigner pulls in people outside the PR's pod (who then aren't actually watching it) and assigns people who are out on leave — which undercuts the one guarantee the Assignee field exists to provide, that exactly one person is accountable for driving a PR to completion. The proposal on the table was to disable the automation org-wide. The automation was deliberately shipped unrefined so the team could iterate on it, so this is the smaller knob instead: scope it to opt-in authors, and let the people who want their PRs routed keep it. It does **not** settle the broader question of whether the automation should exist at all — that's still an open team decision, and this just stops the answer from having to be all-or-nothing. ## Provenance - **Authored by:** interactive session, from BE-11575. - **Verified:** `check_workflow_pins.py` (11 workflows, none defaulted, every ref checkout guarded); `check_agents_md.py --root .` passed (2 pre-existing warnings, unrelated); YAML parses and the new env key is present. There is **no script test suite for this reusable** (no `test-assign-reviewers.yml`, unlike `agents-md-integrity` or `refresh-reviewers`), so behavior was verified by extracting the `github-script` body and running it under stubbed `octokit`/`core` across six cases — see Test plan. - **Deviations:** one, called out below — the allowlist matches logins case-insensitively and tolerates a leading `@`, where `REVIEWER_EXCLUDE` matches exactly. Rationale in "Reviewer context". ## Reviewer context - **Type:** additive config knob on an existing reusable. No new inputs, no new actions, no new permissions. - **Slots into:** `assign-reviewers.yml`. Merging trips `bump-assign-reviewers-callers.yml`'s path filter, so the fleet carries it to consumers — all of them on unchanged behavior until they set the variable. No `Skip-caller-bump` trailer, deliberately: this is behavioral. - **Not in scope:** actually setting `REVIEWER_AUTHOR_ALLOWLIST` on any repo (a Settings → Variables change, done per repo after this ships); leave-awareness, which is the second, distinct failure mode raised in the thread and needs a data source this change doesn't have; and pod-aware routing, which is a different mechanism. ## Summary - New optional caller var **`vars.REVIEWER_AUTHOR_ALLOWLIST`** (whitespace-separated logins), wired exactly like the existing `REVIEWER_EXCLUDE` / `REVIEWER_GROWTH_POOL` / `REVIEWER_LOAD_CAP` — caller `vars` → job `env:` → parsed in the script. - **Non-empty** ⇒ only those authors' PRs are routed. - **Unset or empty** ⇒ today's behavior exactly. No existing caller moves until it opts in. - The gate sits **with the other early exits and ahead of the live assignee read**, so a PR from an author outside the list costs zero API calls and exits with an informational log line — never a failing check. (`authorLogin` is hoisted up to the early-exit block for it; the later duplicate declaration is removed.) - **One deliberate deviation from `REVIEWER_EXCLUDE`: logins are matched case-insensitively and a leading `@` is stripped.** The two vars fail in opposite directions. A typo in `REVIEWER_EXCLUDE` fails *open* — someone gets assigned who shouldn't, which is visible and self-correcting. A typo here fails *closed*: nobody matches, routing silently stops repo-wide, and it looks identical to success — green checks, no assignees, no error. Normalizing the two things people actually get wrong (`@handle`, capitalization) closes the likeliest way to hit that. Nothing else is guessed at — `matt` still does not match `mattmillerai`. - Docs updated in all three places the other vars appear: the workflow header comment (config list re-aligned to fit the longer name, plus an `AUTHOR SCOPING` note), the `docs/callers/assign-reviewers.md` prerequisites table and intro, and the README catalog row. The caller guide also gains a gotcha for the fail-closed behavior, with the log line to look for when confirming it's working. ## Test plan The `github-script` body was extracted from the workflow and executed under stubbed `octokit`/`core` with a fixture `reviewers.yml`. All six pass: | # | Case | Expected | Result | |---|---|---|---| | A | Allowlist unset | Assigns, unchanged | ✅ assigns | | B | Set, author **absent** | No assignment, **no API calls**, no failure | ✅ `Author jacobsegal is not in REVIEWER_AUTHOR_ALLOWLIST — skipping.`, 0 API calls | | C | Set, author **present** | Assigns as today | ✅ assigns | | D | Whitespace-only value | Treated as unset | ✅ assigns | | E | `@MattMillerAI` in list vs `mattmillerai` author | Matches | ✅ assigns | | F | `matt` in list vs `mattmillerai` author | Does **not** match | ✅ skips | - [x] `python3 .github/workflow-pins/check_workflow_pins.py` - [x] `python3 .github/agents-md-integrity/check_agents_md.py --root .` - [x] New behavior is off by default per repo; unset ⇒ byte-for-byte today's path - [ ] After merge + fleet bump: confirm an enrolled caller with the var unset routes exactly as before - [ ] Then set `REVIEWER_AUTHOR_ALLOWLIST` on one repo and confirm the run logs `is in REVIEWER_AUTHOR_ALLOWLIST — routing` for a listed author and skips cleanly for an unlisted one Fixes BE-11575 --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:37:53 +00:00