diff --git a/plugins/magpie-pr-management/skills/code-review/SKILL.md b/plugins/magpie-pr-management/skills/code-review/SKILL.md index 98d9282dd..80b61846a 100644 --- a/plugins/magpie-pr-management/skills/code-review/SKILL.md +++ b/plugins/magpie-pr-management/skills/code-review/SKILL.md @@ -24,7 +24,7 @@ argument-hint: "[pr:N] [area:LBL] [collab:true|false] [team:NAME] [ready] [dry-r capability: capability:review surface_hash: sha256:871da2459ce53192 license: Apache-2.0 -measured_tokens: 4119 +measured_tokens: 4142 --- @@ -260,7 +260,7 @@ last update with PRs whose real CI never ran last. Selectors (`pr:`, `area:`, `collab:true|false`, `team:`, `ready`, `*-only` / `no-*`, `since:`, `max:`, `dry-run`, `inline:off`, `with-reviewers:`, `with-reviewer:`, `no-adversarial`, `repo:/`, -`lookahead:`, `no-prefetch`) compose by AND; `pr:` overrides the rest. `code-review +`lookahead:`, `no-prefetch`) compose by AND; `pr:` (or a list, `pr:,` / `pr:-`) overrides the rest. `code-review resolve ` parses them; the full reference with examples is [`invocation.md`](invocation.md). If the resolved queue is empty, say so and exit — never widen it silently. diff --git a/plugins/magpie-pr-management/skills/code-review/classifications/selector-single-pr.md b/plugins/magpie-pr-management/skills/code-review/classifications/selector-single-pr.md index c3376e8fb..103499c31 100644 --- a/plugins/magpie-pr-management/skills/code-review/classifications/selector-single-pr.md +++ b/plugins/magpie-pr-management/skills/code-review/classifications/selector-single-pr.md @@ -1,7 +1,8 @@ -# Selector `pr:` — one PR +# Selector `pr:` — named PRs -[`code-review`](../../../../../tools/pr-management/README.md#code-review--pr-management-code-review) `queue pr:` puts exactly that PR on the list. It bypasses every other filter — `collab:`, drafts, the five signals: the maintainer asked for this PR, the skill reviews it. -Read the PR with `gql-cr-pr `; a draft or a "WIP" / "do not merge" title still asks before the review starts ([`skip-draft.md`](skip-draft.md), [`skip-wip-title.md`](skip-wip-title.md)). +[`code-review`](../../../../../tools/pr-management/README.md#code-review--pr-management-code-review) `queue pr:` puts exactly that PR on the list; `pr:,`, `pr:-` (inclusive) and repeated `pr:` tokens put each named PR on it, in the order named (duplicates dropped, at most 50). It bypasses every other filter — `collab:`, drafts, the five signals: the maintainer asked for this PR, the skill reviews it. +A named PR that is not in the open-PR sweep (closed, merged, or a typo) is listed in the queue's `not_open`: say so and go on with the rest. +Read each PR with `gql-cr-pr `; a draft or a "WIP" / "do not merge" title still asks before the review starts ([`skip-draft.md`](skip-draft.md), [`skip-wip-title.md`](skip-wip-title.md)). diff --git a/plugins/magpie-pr-management/skills/code-review/invocation.md b/plugins/magpie-pr-management/skills/code-review/invocation.md index 41aa61007..69a8e6939 100644 --- a/plugins/magpie-pr-management/skills/code-review/invocation.md +++ b/plugins/magpie-pr-management/skills/code-review/invocation.md @@ -10,6 +10,7 @@ examples a maintainer can paste: |---|---| | Walk through everything in **"my reviews"**, newest first | `pr-management-code-review` | | Review a single PR (the most common ad-hoc trigger) | `pr-management-code-review pr:65981` | +| Review several named PRs in one session, in that order | `pr-management-code-review pr:1929,1944,2013` (or `pr:2005-2008`) | | Just the PRs where I'm a CODEOWNER, ignore the rest | `pr-management-code-review codeowner-only` | | PRs that explicitly `@`-mention me, skip the noise | `pr-management-code-review mentioned-only` | | Re-look at the PRs I already reviewed (follow-ups after author push) | `pr-management-code-review reviewed-before-only` | @@ -38,7 +39,7 @@ everything you'd reasonably be expected to look at. | Selector / flag | Effect | |---|---| -| `pr:` | review only PR `` | +| `pr:` | review only PR ``; `pr:,`, `pr:-` and repeated `pr:` tokens name several (at most 50), reviewed in the order given | | `area:` | restrict to PRs carrying `area:` (wildcards supported) | | `collab:true|false` | restrict to PRs whose author is / isn't a collaborator | | `team:` | restrict to PRs requesting review from a team `` is on | diff --git a/plugins/magpie-security-tools/tools/pr-management/README.md b/plugins/magpie-security-tools/tools/pr-management/README.md index 8aededf0c..a5d85c03a 100644 --- a/plugins/magpie-security-tools/tools/pr-management/README.md +++ b/plugins/magpie-security-tools/tools/pr-management/README.md @@ -224,7 +224,7 @@ uv run --project /tools/pr-management pr-management code-review dict[str, Any]: "queue": [r for r in rows if r["skip"] is None], "auto_skipped": [r for r in rows if r["skip"] is not None], "count": sum(1 for r in rows if r["skip"] is None), + "not_open": [n for n in sel.pr_numbers if n not in {r["number"] for r in rows}], "empty_message": None if rows else f"No PRs match {' '.join(args.selector) or 'my reviews'}. Nothing to review.", diff --git a/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/selectors.py b/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/selectors.py index 57aaed246..e3b3c0002 100644 --- a/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/selectors.py +++ b/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/selectors.py @@ -54,6 +54,8 @@ "no-reviewed-before": "reviewed-before", } _WINDOW = re.compile(r"^(\d+)([dw])$") +_PR_ITEM = re.compile(r"^#?(\d+)(?:-#?(\d+))?$") +MAX_PRS = 50 # an explicit list longer than this is a fat-fingered range _WIP = re.compile(r"\b(wip|do not merge|don't merge|dnm)\b", re.IGNORECASE) @@ -64,7 +66,7 @@ class SelectorError(ValueError): @dataclass class Selector: mode: str = "my-reviews" - pr_number: int | None = None + pr_numbers: list[int] = field(default_factory=list) area_label: str | None = None collab: str | None = None team: str | None = None @@ -81,10 +83,16 @@ class Selector: lookahead: int = 3 prefetch: bool = True + @property + def pr_number(self) -> int | None: + """The PR when exactly one was named (kept for single-PR callers).""" + return self.pr_numbers[0] if len(self.pr_numbers) == 1 else None + def as_dict(self) -> dict[str, Any]: return { "mode": self.mode, "pr_number": self.pr_number, + "pr_numbers": list(self.pr_numbers), "area_label": self.area_label, "collab": self.collab, "team": self.team, @@ -103,6 +111,24 @@ def as_dict(self) -> dict[str, Any]: } +def parse_pr_list(arg: str) -> list[int]: + """`pr:N`, `pr:N,M`, `pr:N-M` (inclusive range), `#` prefixes allowed.""" + value = arg.partition(":")[2] + numbers: list[int] = [] + for item in value.split(","): + match = _PR_ITEM.match(item.strip()) + if not match: + raise SelectorError(f"pr:[,…|-] needs PR numbers, got {arg!r}") + low = int(match.group(1)) + high = int(match.group(2)) if match.group(2) else low + if high < low: + raise SelectorError(f"pr: range runs backwards in {arg!r}") + if high - low + 1 > MAX_PRS: + raise SelectorError(f"pr: range in {arg!r} names more than {MAX_PRS} PRs") + numbers.extend(range(low, high + 1)) + return numbers + + def parse(args: list[str]) -> Selector: sel = Selector() only: list[str] = [] @@ -110,9 +136,11 @@ def parse(args: list[str]) -> Selector: for arg in args: key, _, value = arg.partition(":") if arg.startswith("pr:"): - if not value.isdigit(): - raise SelectorError(f"pr: needs a number, got {arg!r}") - sel.pr_number = int(value) + for number in parse_pr_list(arg): + if number not in sel.pr_numbers: + sel.pr_numbers.append(number) + if len(sel.pr_numbers) > MAX_PRS: + raise SelectorError(f"pr: selectors name more than {MAX_PRS} PRs") elif key == "area" and value: sel.area_label = arg elif key == "collab" and value in ("true", "false"): @@ -153,7 +181,7 @@ def parse(args: list[str]) -> Selector: if only: sel.signals = [s for s in SIGNALS if s in only] sel.signals = [s for s in sel.signals if s not in drop] - if sel.pr_number is not None: + if sel.pr_numbers: sel.mode = "single-pr" elif sel.team: sel.mode = "team" @@ -280,7 +308,7 @@ def queue( entries: list[Entry] = [] for pr in prs: if sel.mode == "single-pr": - if pr.number != sel.pr_number: + if pr.number not in sel.pr_numbers: continue chips = signals_for(pr, sel, viewer, active_set, rules, viewer_teams, now) else: @@ -313,7 +341,10 @@ def queue( skip, ask = _skip(pr, viewer) entries.append(Entry(pr, chips, skip=skip, ask=ask, real_ci=real_ci(pr, cfg))) oldest = dt.datetime.min.replace(tzinfo=dt.UTC) - entries.sort(key=lambda e: (not e.real_ci, -(e.pr.updated or oldest).timestamp())) + if sel.mode == "single-pr": + entries.sort(key=lambda e: sel.pr_numbers.index(e.pr.number)) # the order the maintainer named + else: + entries.sort(key=lambda e: (not e.real_ci, -(e.pr.updated or oldest).timestamp())) if sel.max is not None: reviewable = [e for e in entries if e.skip is None][: sel.max] entries = reviewable + [e for e in entries if e.skip is not None] diff --git a/tools/pr-management/README.md b/tools/pr-management/README.md index 88db69e50..e1e837efc 100644 --- a/tools/pr-management/README.md +++ b/tools/pr-management/README.md @@ -224,7 +224,7 @@ uv run --project /tools/pr-management pr-management code-review dict[str, Any]: "queue": [r for r in rows if r["skip"] is None], "auto_skipped": [r for r in rows if r["skip"] is not None], "count": sum(1 for r in rows if r["skip"] is None), + "not_open": [n for n in sel.pr_numbers if n not in {r["number"] for r in rows}], "empty_message": None if rows else f"No PRs match {' '.join(args.selector) or 'my reviews'}. Nothing to review.", diff --git a/tools/pr-management/src/pr_management/code_review/selectors.py b/tools/pr-management/src/pr_management/code_review/selectors.py index 57aaed246..e3b3c0002 100644 --- a/tools/pr-management/src/pr_management/code_review/selectors.py +++ b/tools/pr-management/src/pr_management/code_review/selectors.py @@ -54,6 +54,8 @@ "no-reviewed-before": "reviewed-before", } _WINDOW = re.compile(r"^(\d+)([dw])$") +_PR_ITEM = re.compile(r"^#?(\d+)(?:-#?(\d+))?$") +MAX_PRS = 50 # an explicit list longer than this is a fat-fingered range _WIP = re.compile(r"\b(wip|do not merge|don't merge|dnm)\b", re.IGNORECASE) @@ -64,7 +66,7 @@ class SelectorError(ValueError): @dataclass class Selector: mode: str = "my-reviews" - pr_number: int | None = None + pr_numbers: list[int] = field(default_factory=list) area_label: str | None = None collab: str | None = None team: str | None = None @@ -81,10 +83,16 @@ class Selector: lookahead: int = 3 prefetch: bool = True + @property + def pr_number(self) -> int | None: + """The PR when exactly one was named (kept for single-PR callers).""" + return self.pr_numbers[0] if len(self.pr_numbers) == 1 else None + def as_dict(self) -> dict[str, Any]: return { "mode": self.mode, "pr_number": self.pr_number, + "pr_numbers": list(self.pr_numbers), "area_label": self.area_label, "collab": self.collab, "team": self.team, @@ -103,6 +111,24 @@ def as_dict(self) -> dict[str, Any]: } +def parse_pr_list(arg: str) -> list[int]: + """`pr:N`, `pr:N,M`, `pr:N-M` (inclusive range), `#` prefixes allowed.""" + value = arg.partition(":")[2] + numbers: list[int] = [] + for item in value.split(","): + match = _PR_ITEM.match(item.strip()) + if not match: + raise SelectorError(f"pr:[,…|-] needs PR numbers, got {arg!r}") + low = int(match.group(1)) + high = int(match.group(2)) if match.group(2) else low + if high < low: + raise SelectorError(f"pr: range runs backwards in {arg!r}") + if high - low + 1 > MAX_PRS: + raise SelectorError(f"pr: range in {arg!r} names more than {MAX_PRS} PRs") + numbers.extend(range(low, high + 1)) + return numbers + + def parse(args: list[str]) -> Selector: sel = Selector() only: list[str] = [] @@ -110,9 +136,11 @@ def parse(args: list[str]) -> Selector: for arg in args: key, _, value = arg.partition(":") if arg.startswith("pr:"): - if not value.isdigit(): - raise SelectorError(f"pr: needs a number, got {arg!r}") - sel.pr_number = int(value) + for number in parse_pr_list(arg): + if number not in sel.pr_numbers: + sel.pr_numbers.append(number) + if len(sel.pr_numbers) > MAX_PRS: + raise SelectorError(f"pr: selectors name more than {MAX_PRS} PRs") elif key == "area" and value: sel.area_label = arg elif key == "collab" and value in ("true", "false"): @@ -153,7 +181,7 @@ def parse(args: list[str]) -> Selector: if only: sel.signals = [s for s in SIGNALS if s in only] sel.signals = [s for s in sel.signals if s not in drop] - if sel.pr_number is not None: + if sel.pr_numbers: sel.mode = "single-pr" elif sel.team: sel.mode = "team" @@ -280,7 +308,7 @@ def queue( entries: list[Entry] = [] for pr in prs: if sel.mode == "single-pr": - if pr.number != sel.pr_number: + if pr.number not in sel.pr_numbers: continue chips = signals_for(pr, sel, viewer, active_set, rules, viewer_teams, now) else: @@ -313,7 +341,10 @@ def queue( skip, ask = _skip(pr, viewer) entries.append(Entry(pr, chips, skip=skip, ask=ask, real_ci=real_ci(pr, cfg))) oldest = dt.datetime.min.replace(tzinfo=dt.UTC) - entries.sort(key=lambda e: (not e.real_ci, -(e.pr.updated or oldest).timestamp())) + if sel.mode == "single-pr": + entries.sort(key=lambda e: sel.pr_numbers.index(e.pr.number)) # the order the maintainer named + else: + entries.sort(key=lambda e: (not e.real_ci, -(e.pr.updated or oldest).timestamp())) if sel.max is not None: reviewable = [e for e in entries if e.skip is None][: sel.max] entries = reviewable + [e for e in entries if e.skip is not None] diff --git a/tools/pr-management/tests/code_review/test_selectors.py b/tools/pr-management/tests/code_review/test_selectors.py index 89aaa454c..4b869ba72 100644 --- a/tools/pr-management/tests/code_review/test_selectors.py +++ b/tools/pr-management/tests/code_review/test_selectors.py @@ -98,6 +98,29 @@ def test_since_windows(arg: str, days: int) -> None: assert selectors.parse([arg]).since_days == days +@pytest.mark.parametrize( + ("args", "numbers"), + [ + (["pr:1929,1944"], [1929, 1944]), + (["pr:2005-2008"], [2005, 2006, 2007, 2008]), + (["pr:#1929,#1944-#1945"], [1929, 1944, 1945]), + (["pr:1944", "pr:1929", "pr:1944"], [1944, 1929]), + ], +) +def test_pr_lists(args: list[str], numbers: list[int]) -> None: + _subset(selectors.parse(args), mode="single-pr", pr_numbers=numbers, pr_number=None, signals=[]) + + +def test_one_pr_keeps_pr_number() -> None: + _subset(selectors.parse(["pr:65981"]), pr_number=65981, pr_numbers=[65981]) + + +@pytest.mark.parametrize("arg", ["pr:", "pr:abc", "pr:1929,", "pr:10-2", "pr:1-100", "pr:1;2"]) +def test_bad_pr_lists_are_refused(arg: str) -> None: + with pytest.raises(selectors.SelectorError): + selectors.parse([arg]) + + def test_unknown_flags_are_refused() -> None: with pytest.raises(selectors.SelectorError): selectors.parse(["approve-everything"]) @@ -248,3 +271,13 @@ def test_team_selector_matches_the_requested_team() -> None: def test_commits_are_scanned_for_mentions() -> None: (entry,) = _queue(pr(commits=[commit("Thanks @alice for the idea")])) assert entry.chips[0] == "mentioned-in: commit" + + +def test_named_prs_keep_the_named_order_and_bypass_filters() -> None: + entries = _queue( + pr(number=11, files=["a.py"]), + pr(number=12, draft=True, files=["b.py"]), + pr(number=13, files=["c.py"]), + sel=selectors.parse(["pr:13,12", "collab:true"]), + ) + assert [e.pr.number for e in entries] == [13, 12]