Skip to content

Commit 81903bf

Browse files
committed
Skip Stale-labeled PRs in review scan
- Carry PR labels onto OpenPR from the get_pulls payload (no extra API call) - Add STALE_LABEL/EXCLUDED_LABELS constants; case-sensitive exact "Stale" - Fold Stale-label pruning into _recency_filter via stale_labeled_numbers - Keep in-flight/retry-eligible and --pr recheck targets exempt from the skip - Refresh README/CHEATSHEET/CLAUDE.md and add github_client + review tests The pytorch stale bot bumps updated_at when it applies the "Stale" label, which drags abandoned, never-reviewed PRs back inside the recency window and gets them dispatched. Pruning on the label closes that hole: a Stale-labeled PR is skipped without fingerprinting when it is terminal or never reviewed, while a non-terminal ledger state still survives so decide can re-dispatch on timeout/retry. A single --pr target bypasses listing, so the label can never suppress a manual @greenlight recheck. Signed-off-by: Jean Schmidt <contato@jschmidt.me>
1 parent 704e982 commit 81903bf

8 files changed

Lines changed: 173 additions & 31 deletions

File tree

greenlight/CHEATSHEET.md

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,9 @@ loops as a daemon with `--loop`. It also has a one-shot `verdict` subcommand:
1111
for each, compute its fingerprint (`eval_hash`), read its latest state from
1212
`misc.greenlight_pr_state`, and dispatch the reviewer workflow
1313
(`greenlight-pr-review.yml` on `pytorch/test-infra`) for new or changed PRs. A PR whose
14-
`updated_at` is older than `PYTORCH_GREENLIGHT_REVIEW_WINDOW_HOURS` (default 24) is skipped
15-
without fingerprinting unless a review is in-flight or retry-eligible (cancelled/failed). A PR a
14+
`updated_at` is older than `PYTORCH_GREENLIGHT_REVIEW_WINDOW_HOURS` (default 24), or that carries
15+
the `Stale` label, is skipped without fingerprinting unless a review is in-flight or retry-eligible
16+
(cancelled/failed); an explicit `@greenlight recheck` (the `--pr` path) reviews it regardless. A PR a
1617
human has already decided — approved by a `merge_rules.yaml` approver (bots excluded) or with
1718
changes requested by any non-bot reviewer — is also skipped without fingerprinting or dispatch,
1819
and no state is written, so the scan resumes if that changes. Needs
@@ -113,10 +114,11 @@ logs the resolved `Config`.
113114
The end-to-end flow, per trusted-author PR:
114115

115116
1. `review` scans the open PRs, and for each computes its fingerprint (`eval_hash`) — unless
116-
the PR's `updated_at` is older than `PYTORCH_GREENLIGHT_REVIEW_WINDOW_HOURS` (default 24)
117-
and it has no in-flight or retry-eligible (cancelled/failed) review, or a human has already
118-
decided it (approved by a `merge_rules.yaml` approver with bots excluded, or changes requested
119-
by any non-bot reviewer), in which case it is skipped without fingerprinting.
117+
the PR's `updated_at` is older than `PYTORCH_GREENLIGHT_REVIEW_WINDOW_HOURS` (default 24) or it
118+
carries the `Stale` label, and it has no in-flight or retry-eligible (cancelled/failed) review,
119+
or a human has already decided it (approved by a `merge_rules.yaml` approver with bots excluded,
120+
or changes requested by any non-bot reviewer), in which case it is skipped without fingerprinting
121+
(an explicit `@greenlight recheck` via `--pr` reviews it regardless).
120122
2. It reads the PR's latest recorded state from `misc.greenlight_pr_state`.
121123
3. If the PR is new, or its fingerprint changed since that state, and no review is
122124
in-flight within the `--timeout-minutes` window, it dispatches the reviewer workflow

greenlight/CLAUDE.md

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,10 @@ PyTorch Green Light has one unit of work — the `review` phase:
5353
fingerprint (`eval_hash`), reads the PR's latest state from `misc.greenlight_pr_state`,
5454
and dispatches the reviewer workflow (`greenlight-pr-review.yml` on `pytorch/test-infra`)
5555
for new or changed PRs, skipping any PR untouched beyond `PYTORCH_GREENLIGHT_REVIEW_WINDOW_HOURS`
56-
(default 24) that has no in-flight or retry-eligible (cancelled/failed) review, and skipping —
57-
without fingerprinting or dispatching — any PR a human has already decided (approved by a
58-
`merge_rules.yaml` approver, bots excluded, or changes requested by any non-bot reviewer).
56+
(default 24) or carrying the `Stale` label that has no in-flight or retry-eligible
57+
(cancelled/failed) review, and skipping — without fingerprinting or dispatching — any PR a human
58+
has already decided (approved by a `merge_rules.yaml` approver, bots excluded, or changes
59+
requested by any non-bot reviewer).
5960
Requires `PYTORCH_GREENLIGHT_GITHUB_TOKEN` and `CLICKHOUSE_*` read access.
6061

6162
Approving or rejecting a PR lives in the dispatched reviewer workflow (through `verdict`),

greenlight/README.md

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,11 @@ trusted authors in `pytorch/pytorch` and, for each one, computes its fingerprint
2626
`misc.greenlight_pr_state`, and dispatches the reviewer workflow
2727
(`greenlight-pr-review.yml` on `pytorch/test-infra`) for PRs that are new or changed since
2828
their last review. A PR whose `updated_at` is older than
29-
`PYTORCH_GREENLIGHT_REVIEW_WINDOW_HOURS` (default 24) is skipped without fingerprinting unless
30-
it has an in-flight or retry-eligible (cancelled/failed) review to re-check. An in-flight review — one marked
31-
`AI_REVIEW_STARTED` — is left alone until the `--timeout-minutes` re-dispatch window elapses.
29+
`PYTORCH_GREENLIGHT_REVIEW_WINDOW_HOURS` (default 24), or that carries the `Stale` label, is
30+
skipped without fingerprinting unless it has an in-flight or retry-eligible (cancelled/failed)
31+
review to re-check; an explicit `@greenlight recheck` (the `--pr` path) reviews it regardless. An
32+
in-flight review — one marked `AI_REVIEW_STARTED` — is left alone until the `--timeout-minutes`
33+
re-dispatch window elapses.
3234
The listing scan also skips a PR a human has already decided — approved by a merge-authorized
3335
login (any `approved_by` in `pytorch/pytorch`'s `merge_rules.yaml`, taken across all rules
3436
regardless of the PR's changed paths, bots excluded) or with changes requested by any non-bot
@@ -152,8 +154,9 @@ from a fixed set of trusted authors in `pytorch/pytorch`, computes each PR's fin
152154
(`eval_hash`), reads the PR's latest recorded state from `misc.greenlight_pr_state`, and
153155
dispatches the reviewer workflow (`greenlight-pr-review.yml` on `pytorch/test-infra`) for
154156
PRs that are new or changed. PRs whose `updated_at` is older than the review window
155-
(`PYTORCH_GREENLIGHT_REVIEW_WINDOW_HOURS`, default 24) are skipped without fingerprinting
156-
unless a review is in-flight or retry-eligible (cancelled/failed). PRs a human has already decided
157+
(`PYTORCH_GREENLIGHT_REVIEW_WINDOW_HOURS`, default 24), or that carry the `Stale` label, are
158+
skipped without fingerprinting unless a review is in-flight or retry-eligible (cancelled/failed);
159+
an explicit `@greenlight recheck` (the `--pr` path) reviews such a PR regardless. PRs a human has already decided
157160
— approved by a merge-authorized login (bots excluded) or with changes requested by any non-bot
158161
reviewer — are also skipped without fingerprinting, and no state is written, so the scan resumes
159162
if that changes. An `AI_REVIEW_STARTED` marker is treated as an

greenlight/src/greenlight/constants.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,10 @@
1515
RETRY_STATUSES: frozenset[str] = frozenset({STATUS_CANCELLED, STATUS_FAILED})
1616
VERDICT_STATUSES: frozenset[str] = TERMINAL_STATUSES | IN_FLIGHT_STATUSES | RETRY_STATUSES
1717

18+
# GitHub labels are case-sensitive; the pytorch stale bot uses the exact name "Stale".
19+
STALE_LABEL = "Stale"
20+
EXCLUDED_LABELS: frozenset[str] = frozenset({STALE_LABEL})
21+
1822
S3_KEY_PREFIX = "greenlight_pr_state"
1923
EVAL_HASH_RE = re.compile(r"[0-9a-f]{64}")
2024
HEAD_SHA_RE = re.compile(r"[0-9a-fA-F]{40}")

greenlight/src/greenlight/github_client.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,10 @@ class _PRUser(Protocol):
2525
@property
2626
def login(self) -> str | None: ...
2727

28+
class _Label(Protocol):
29+
@property
30+
def name(self) -> str: ...
31+
2832
class _PullRequest(Protocol):
2933
@property
3034
def number(self) -> int: ...
@@ -38,6 +42,8 @@ def html_url(self) -> str: ...
3842
def head(self) -> _PRBase: ...
3943
@property
4044
def updated_at(self) -> datetime | None: ...
45+
@property
46+
def labels(self) -> Iterable[_Label]: ...
4147

4248
class _Repo(Protocol):
4349
def get_pulls(self, state: str) -> Iterable[_PullRequest]: ...
@@ -142,6 +148,7 @@ class OpenPR:
142148
url: str
143149
head_sha: str
144150
updated_at: datetime | None
151+
labels: tuple[str, ...] = ()
145152

146153

147154
# Pin the request timeout so a future PyGithub default change can't let
@@ -175,6 +182,7 @@ def list_open_prs_by_authors(client: _RepoClient, repo: str, authors: Iterable[s
175182
url=pr.html_url,
176183
head_sha=pr.head.sha,
177184
updated_at=naive_utc(updated_at) if updated_at is not None else None,
185+
labels=tuple(label.name for label in pr.labels),
178186
)
179187
)
180188
return sorted(prs, key=lambda p: p.number)

greenlight/src/greenlight/review.py

Lines changed: 39 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,13 @@
2424

2525
from greenlight import dispatch as dispatch_module
2626
from greenlight import github_client, scan_runner, state
27-
from greenlight.constants import DEFAULT_DISPATCH_REF, DEFAULT_TIMEOUT_MINUTES, TARGET_REPO, TERMINAL_STATUSES
27+
from greenlight.constants import (
28+
DEFAULT_DISPATCH_REF,
29+
DEFAULT_TIMEOUT_MINUTES,
30+
EXCLUDED_LABELS,
31+
TARGET_REPO,
32+
TERMINAL_STATUSES,
33+
)
2834

2935
if TYPE_CHECKING:
3036
from collections.abc import Callable, Sequence
@@ -102,15 +108,22 @@ def _close_client(client: Github) -> None:
102108

103109
def _candidate_numbers(
104110
client: Github, *, pr: int | None, fetch: Callable[[Github], list[OpenPR]]
105-
) -> tuple[list[int], dict[int, datetime | None]]:
111+
) -> tuple[list[int], dict[int, datetime | None], frozenset[int]]:
106112
if pr is not None:
107113
logger.info("targeting single PR #%d in %s", pr, TARGET_REPO)
108-
return [pr], {}
114+
return [pr], {}, frozenset()
109115
open_prs = fetch(client)
110116
logger.info("found %d open PR(s) from %d author(s) in %s", len(open_prs), len(TRUSTED_AUTHORS), TARGET_REPO)
111117
for open_pr in open_prs:
112118
logger.info("open PR #%d by %s: %s (%s)", open_pr.number, open_pr.author, open_pr.title, open_pr.url)
113-
return [open_pr.number for open_pr in open_prs], {open_pr.number: open_pr.updated_at for open_pr in open_prs}
119+
stale_labeled_numbers = frozenset(
120+
open_pr.number for open_pr in open_prs if not EXCLUDED_LABELS.isdisjoint(open_pr.labels)
121+
)
122+
return (
123+
[open_pr.number for open_pr in open_prs],
124+
{open_pr.number: open_pr.updated_at for open_pr in open_prs},
125+
stale_labeled_numbers,
126+
)
114127

115128

116129
def _within_recency_window(updated_at: datetime | None, now: datetime, window: timedelta) -> bool:
@@ -124,28 +137,37 @@ def _recency_filter(
124137
pr_numbers: Sequence[int],
125138
updated_at_by_number: dict[int, datetime | None],
126139
states: dict[int, PRState],
140+
stale_labeled_numbers: frozenset[int],
127141
*,
128142
now: datetime,
129143
window: timedelta,
130144
) -> list[int]:
131-
"""Drop stale PRs the scan can safely leave alone this iteration.
132-
133-
A PR is kept when it was updated within ``window`` OR its recorded state is non-terminal
134-
(in-flight or retry-eligible), so ``decide`` can still re-dispatch it on timeout/retry. A
135-
stale PR is skipped without fingerprinting when it is terminal (its eval_hash cannot have
136-
changed) or never reviewed (an untouched PR outside the window is not worth a first review).
145+
"""Drop PRs the scan can safely leave alone this iteration.
146+
147+
A PR is kept when it was updated within ``window`` AND is not ``Stale``-labeled, OR its
148+
recorded state is non-terminal (in-flight or retry-eligible), so ``decide`` can still
149+
re-dispatch it on timeout/retry. A PR is skipped without fingerprinting when it is stale or
150+
``Stale``-labeled and either terminal (its eval_hash cannot have changed) or never reviewed
151+
(an untouched PR is not worth a first review). The ``Stale`` label matters because the pytorch
152+
stale bot bumps ``updated_at`` when it applies the label, which would otherwise drag an
153+
abandoned never-reviewed PR back into the window.
137154
"""
138155
kept: list[int] = []
139156
for number in pr_numbers:
140-
if _within_recency_window(updated_at_by_number.get(number), now, window):
157+
active = _within_recency_window(updated_at_by_number.get(number), now, window)
158+
stale_labeled = number in stale_labeled_numbers
159+
if active and not stale_labeled:
141160
kept.append(number)
142161
continue
143162
recorded = states.get(number)
144-
if recorded is None or recorded.status in TERMINAL_STATUSES:
145-
detail = recorded.status if recorded is not None else "never reviewed"
146-
logger.info("skipping stale PR #%d: no recent activity (%s)", number, detail)
163+
if recorded is not None and recorded.status not in TERMINAL_STATUSES:
164+
kept.append(number)
147165
continue
148-
kept.append(number)
166+
detail = recorded.status if recorded is not None else "never reviewed"
167+
if stale_labeled:
168+
logger.info("skipping PR #%d: Stale label (%s)", number, detail)
169+
else:
170+
logger.info("skipping stale PR #%d: no recent activity (%s)", number, detail)
149171
return kept
150172

151173

@@ -199,7 +221,7 @@ def run(
199221
# exits non-zero, daemon backs off) rather than silently revert to hashing all human comments.
200222
authorized_logins = resolve_authorized()
201223
logger.info("filtering fingerprint comments to %d merge-authorized login(s)", len(authorized_logins))
202-
pr_numbers, updated_at_by_number = _candidate_numbers(client, pr=pr, fetch=fetch)
224+
pr_numbers, updated_at_by_number, stale_labeled_numbers = _candidate_numbers(client, pr=pr, fetch=fetch)
203225
states = read_state(TARGET_REPO, pr_numbers)
204226
evaluated_at = now()
205227
timeout = timedelta(minutes=timeout_minutes)
@@ -213,6 +235,7 @@ def run(
213235
pr_numbers,
214236
updated_at_by_number,
215237
states,
238+
stale_labeled_numbers,
216239
now=evaluated_at,
217240
window=timedelta(hours=config.review_window_hours),
218241
)

greenlight/tests/test_github_client.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,11 @@ def __init__(self, login: str) -> None:
1515
self.login = login
1616

1717

18+
class _FakeLabel:
19+
def __init__(self, name: str) -> None:
20+
self.name = name
21+
22+
1823
class _FakePull:
1924
def __init__(
2025
self,
@@ -24,13 +29,15 @@ def __init__(
2429
html_url: str,
2530
head_sha: str = "head-sha",
2631
updated_at: datetime | None = None,
32+
labels: list[str] | None = None,
2733
) -> None:
2834
self.number = number
2935
self.user = _FakeUser(login) if login is not None else None
3036
self.title = title
3137
self.html_url = html_url
3238
self.head = _FakeBase(head_sha)
3339
self.updated_at = updated_at
40+
self.labels = [_FakeLabel(name) for name in (labels or [])]
3441

3542

3643
class _FakeRepo:
@@ -174,10 +181,21 @@ def test_list_open_prs_by_authors_maps_pull_fields():
174181
url="https://github.com/pytorch/pytorch/pull/42",
175182
head_sha="abc123",
176183
updated_at=datetime(2026, 7, 30, 9, 0, 0),
184+
labels=(),
177185
)
178186
]
179187

180188

189+
def test_list_open_prs_by_authors_maps_labels():
190+
client = _client_with_pulls([_FakePull(1, "alice", "fix", "https://example.test/1", labels=["Stale", "ci-no-td"])])
191+
192+
prs = github_client.list_open_prs_by_authors(client, "pytorch/pytorch", ["alice"])
193+
194+
# Each label's name is carried onto OpenPR.labels in listing order, read from the get_pulls
195+
# payload with no extra API call.
196+
assert prs[0].labels == ("Stale", "ci-no-td")
197+
198+
181199
def test_list_open_prs_by_authors_normalizes_tz_aware_updated_at():
182200
client = _client_with_pulls(
183201
[_FakePull(1, "alice", "fix", "https://example.test/1", updated_at=datetime(2026, 7, 30, 9, 0, 0, tzinfo=UTC))]

greenlight/tests/test_review.py

Lines changed: 84 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,13 @@
4545
_NEW = _NOW - timedelta(hours=1)
4646

4747

48-
def _open_pr(number: int, *, head_sha: str | None = None, updated_at: datetime | None = None) -> OpenPR:
48+
def _open_pr(
49+
number: int,
50+
*,
51+
head_sha: str | None = None,
52+
updated_at: datetime | None = None,
53+
labels: tuple[str, ...] = (),
54+
) -> OpenPR:
4955
return OpenPR(
5056
repo=TARGET_REPO,
5157
number=number,
@@ -54,6 +60,7 @@ def _open_pr(number: int, *, head_sha: str | None = None, updated_at: datetime |
5460
url=f"https://example.test/{number}",
5561
head_sha=head_sha or f"headsha{number}",
5662
updated_at=updated_at,
63+
labels=labels,
5764
)
5865

5966

@@ -373,6 +380,82 @@ def test_recency_window_exactly_at_edge_is_out_of_window(make_config):
373380
assert scan.dispatched == []
374381

375382

383+
def test_stale_label_in_window_never_reviewed_is_skipped(make_config, caplog):
384+
with caplog.at_level(logging.INFO, logger="greenlight"):
385+
scan = _run_scan(
386+
make_config,
387+
listed=[_open_pr(1, updated_at=_NEW, labels=("Stale",))],
388+
fingerprints={1: ("headsha1", _HASH_A)},
389+
)
390+
391+
# The stale bot bumps updated_at when it applies "Stale", so this PR is inside the window yet
392+
# abandoned. The label prunes it -- never fingerprinted, never dispatched.
393+
assert scan.fingerprinted == []
394+
assert scan.dispatched == []
395+
assert "skipping PR #1: Stale label (never reviewed)" in caplog.text
396+
397+
398+
def test_stale_label_in_window_terminal_is_skipped(make_config, caplog):
399+
with caplog.at_level(logging.INFO, logger="greenlight"):
400+
scan = _run_scan(
401+
make_config,
402+
listed=[_open_pr(1, updated_at=_NEW, labels=("Stale",))],
403+
fingerprints={1: ("headsha1", _HASH_A)},
404+
states={1: _state(1, STATUS_LAND, _HASH_A, _NEW)},
405+
)
406+
407+
# Stale-labeled and terminal (decided): its eval_hash cannot have changed, so the label prunes it
408+
# without fingerprinting just as the never-reviewed case does -- but the skip log carries the
409+
# recorded status, exercising the detail = recorded.status branch for a Stale-labeled PR.
410+
assert scan.fingerprinted == []
411+
assert scan.dispatched == []
412+
assert f"skipping PR #1: Stale label ({STATUS_LAND})" in caplog.text
413+
414+
415+
@pytest.mark.parametrize("status", [STATUS_AI_REVIEW_STARTED, STATUS_CANCELLED, STATUS_FAILED])
416+
def test_stale_label_nonterminal_state_is_still_processed(make_config, status):
417+
scan = _run_scan(
418+
make_config,
419+
listed=[_open_pr(1, updated_at=_NEW, labels=("Stale",))],
420+
fingerprints={1: ("headsha1", _HASH_A)},
421+
states={1: _state(1, status, _HASH_A, _STALE)},
422+
)
423+
424+
# A non-terminal ledger state (in-flight / retry) survives the Stale label exactly as it survives
425+
# the recency window: the PR is re-checked so decide can re-dispatch it on timeout/retry.
426+
assert scan.fingerprinted == [1]
427+
assert scan.dispatched == [(1, "headsha1", _HASH_A, DEFAULT_DISPATCH_REF)]
428+
429+
430+
@pytest.mark.parametrize("label", ["enhancement", "stale", "STALE"])
431+
def test_non_stale_label_in_window_is_not_skipped(make_config, label):
432+
scan = _run_scan(
433+
make_config,
434+
listed=[_open_pr(1, updated_at=_NEW, labels=(label,))],
435+
fingerprints={1: ("headsha1", _HASH_A)},
436+
)
437+
438+
# Control: only the exact case-sensitive "Stale" prunes. Any other label -- including the
439+
# lowercase/uppercase variants -- leaves an in-window never-reviewed PR fingerprinted.
440+
assert scan.fingerprinted == [1]
441+
assert scan.dispatched == [(1, "headsha1", _HASH_A, DEFAULT_DISPATCH_REF)]
442+
443+
444+
def test_stale_label_pr_target_is_exempt(make_config):
445+
scan = _run_scan(
446+
make_config,
447+
pr=5,
448+
listed=[_open_pr(5, updated_at=_OLD, labels=("Stale",))],
449+
fingerprints={5: ("headsha5", _HASH_A)},
450+
)
451+
452+
# A single --pr target bypasses listing entirely (listed_calls == 0), so the Stale label -- which
453+
# only ever comes from the listing -- can never suppress a manual recheck.
454+
assert scan.listed_calls == 0
455+
assert scan.fingerprinted == [5]
456+
assert scan.dispatched == [(5, "headsha5", _HASH_A, DEFAULT_DISPATCH_REF)]
457+
458+
376459
def test_max_caps_only_dispatches(make_config):
377460
scan = _run_scan(
378461
make_config,

0 commit comments

Comments
 (0)