From 45d868233feb2d258d922bd28a8e5a3fd4b1cbe1 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 18:19:58 -0700 Subject: [PATCH 1/3] Move wait Out of pr_review.py's main and Check Subcommand Flags From a Table main is now parsing and dispatch. The wait body moves into run_wait unchanged. Flag ownership is read from FLAG_OWNERS, checked in two passes around the cleanup and number checks, so a command line with several bad flags names the same one first. The four negative-value checks are one loop. Over 1904 bad command lines, the first error printed and the exit are identical before and after, and a new case pins the multi-flag order. Closes #3458 Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review.py | 385 +++++++++++++++++++++------------------- tests/test_pr_review.py | 52 ++++++ 2 files changed, 251 insertions(+), 186 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 45df3307..692f155b 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -1409,192 +1409,8 @@ def utf8_console() -> None: reconfigure(encoding="utf-8", errors="replace") -def main(argv: list[str] | None = None) -> int: - args = sys.argv[1:] if argv is None else argv - pre = argparse.ArgumentParser(add_help=False) - pre.add_argument("first", nargs="?") - for flag in ("--repo", "--checkout", "--min-age-hours"): - pre.add_argument(flag) - if pre.parse_known_args(args)[0].first == "cleanup": - sys.modules.setdefault("pr_review", sys.modules[__name__]) - import pr_cleanup - - rest = list(args) - rest.remove("cleanup") - return pr_cleanup.main(rest) - utf8_console() - ap = argparse.ArgumentParser() - ap.add_argument( - "cmd", choices=["attest", "claims", "cleanup", "comment", "status", "reply", "wait"] - ) - ap.add_argument("number", type=int, nargs="?") - # No default, because a default names one repository and every run from elsewhere would read that one. - ap.add_argument( - "--repo", - required=True, - metavar="OWNER/NAME", - help="the repository the pull request is in, since a pull " - "request number identifies no repository on its own", - ) - ap.add_argument("--timeout", type=int, default=2700, help="seconds (default 45m)") - ap.add_argument( - "--pickup-grace", - type=int, - default=300, - help="deprecated compatibility option, accepted but ignored", - ) - ap.add_argument( - "--check-grace", - type=int, - default=CHECK_GRACE, - help="seconds a check may sit queued, or a required status go unposted, " - f"before either reads as unstarted (default {CHECK_GRACE // 60}m)", - ) - ap.add_argument( - "--check-stall", - type=int, - default=CHECK_STALL, - help="seconds a check may run before its duration is reported " - f"(default {CHECK_STALL // 60}m)", - ) - ap.add_argument( - "--ignore-quota-signal", - action="store_true", - help="wait: request and poll the full --timeout even where the reviewer's own most " - "recent activity elsewhere in this repository is a quota-limit refusal with nothing " - "answering it since, or this pull request's newest Copilot review is a quota or " - "error refusal, pass this once the quota is believed to have reset", - ) - ap.add_argument( - "--request", - action="store_true", - help="wait: request a Copilot round even on a pull request into a branch other than the " - "default, which an attested local pass otherwise covers", - ) - ap.add_argument( - "--min-rounds", - type=int, - default=0, - metavar="N", - help="wait: treat the head as covered only once the reviewer's total rounds on this " - "pull request, the same count `status` prints as `rounds=`, exceed N. Pass the " - "`rounds=` a prior `status` already read, after re-requesting a review on a head that " - "already carries one: `reviewed_head` alone would report that earlier round as done " - "at once, never waiting for the fresh one the re-request was for. N is the count " - "already seen, not the count to reach: on a pull request reading `rounds=3`, N=3 " - "waits for the fourth round and N=4 for a fifth (default 0, so any round counts, " - "unchanged from before this flag existed)", - ) - # `reply` takes the finding's words, and there is no argument an id fits in. - ap.add_argument( - "--match", - metavar="TEXT", - help="reply: words from the finding, matched against the thread's opening " - "comment, and required to select exactly one unresolved thread", - ) - ap.add_argument( - "--path", - metavar="FILE", - help="reply: narrow --match to one file, for a file with several findings", - ) - ap.add_argument( - "--all-identical", - action="store_true", - help="reply: where --match selects several threads whose opening comments read the " - "same, a finding posted twice, answer each of them with --body rather than refusing", - ) - ap.add_argument( - "--body", - metavar="TEXT", - help="comment or reply: the answer to post, carrying the fixing commit SHA or " - "the evidence that disproves the finding", - ) - ap.add_argument("--fingerprint", help="comment: acknowledge the unplaced set it names") - ap.add_argument( - "--resolve", - action="store_true", - help="reply: resolve the thread once the reply is confirmed", - ) - ap.add_argument( - "--checkout", - metavar="DIR", - help="attest: the checkout holding the pull request's head and its recorded local pass " - "(default the current directory)", - ) - a = ap.parse_args(args) - # Named for the command they belong to, since an ignored flag reads as one that took effect. - reply_only = { - "--match": a.match, - "--path": a.path, - "--resolve": a.resolve or None, - "--all-identical": a.all_identical or None, - } - if a.cmd != "reply": - for flag, value in reply_only.items(): - if value is not None: - ap.error(f"{flag} belongs to `reply`, not `{a.cmd}`") - if a.cmd != "wait" and a.request: - ap.error(f"--request belongs to `wait`, not `{a.cmd}`") - if a.cmd == "cleanup": - ap.error("`cleanup` takes its arguments after it, as `cleanup NUMBER --repo OWNER/NAME`") - if a.cmd != "attest" and a.checkout is not None: - ap.error(f"--checkout belongs to `attest` or `cleanup`, not `{a.cmd}`") - if a.number is None: - ap.error("the following arguments are required: number") - if a.cmd != "comment" and a.fingerprint is not None: - ap.error(f"--fingerprint belongs to `comment`, not `{a.cmd}`") - if a.cmd not in ("comment", "reply") and a.body is not None: - ap.error(f"--body belongs to `comment` or `reply`, not `{a.cmd}`") - required = ["--body"] + (["--match"] if a.cmd == "reply" else []) - if a.cmd in ("comment", "reply"): - values = {"--body": a.body, "--match": a.match} - for flag in required: - if not (values[flag] or "").strip(): - ap.error( - f"{a.cmd} requires a non-empty {flag}, since an empty answer records nothing" - ) - if a.pickup_grace < 0: - ap.error("--pickup-grace cannot be negative") - if a.min_rounds < 0: - ap.error("--min-rounds cannot be negative") - # A negative threshold would report every check in that state on every run. - for name in ("check_grace", "check_stall"): - if getattr(a, name) < 0: - ap.error(f"--{name.replace('_', '-')} cannot be negative") - # The two thresholds mean opposite things and the readings invert if the order does. - if a.check_grace >= a.check_stall: - ap.error( - "--check-grace must be less than --check-stall, since a queued check is " - f"judged sooner than a running one (got {a.check_grace} and {a.check_stall})" - ) - # A bare name is the near-miss a required argument still admits, and unpacking it raises a - # ValueError traceback rather than saying which half is missing. - owner, _, repo = a.repo.partition("/") - if not owner or not repo or "/" in repo: - ap.error(f"--repo takes OWNER/NAME, not {a.repo!r}") - - if a.cmd == "claims": - return check_claims(owner, repo, a.number) - - if a.cmd == "attest": - return attest(owner, repo, a.number, a.checkout or ".") - - if a.cmd == "comment": - marker = f"\n\n{ACK_MARKER.format(a.fingerprint)}" if a.fingerprint else "" - return comment_on_pr(owner, repo, a.number, a.body.strip() + marker) - - if a.cmd == "status": - # One payload renders the digest and decides the code, as in `wait`. - pr = full_read(owner, repo, a.number) - out, _ = digest(owner, repo, a.number, pr=pr, grace=a.check_grace, stall=a.check_stall) - print(out) - return report_verdict(pr) - - if a.cmd == "reply": - return reply_to_thread( - owner, repo, a.number, a.match, a.body, a.path, a.resolve, a.all_identical - ) - +def run_wait(owner: str, repo: str, a: argparse.Namespace) -> int: + """Request a review where one is owed, poll until it lands or the timeout, and return the code.""" # `wait` mutates through the auto-request, so a cross-owner target is refused before the read as well. ok, why = in_scope(owner) if not ok: @@ -1908,5 +1724,202 @@ def held_read() -> dict: return 30 +# Each flag a subcommand owns is refused elsewhere, since an ignored flag reads as one that took effect. +# A late row is checked after the `cleanup` and `number` checks, which fixes the first error printed. +# `cleanup` owns `--checkout` only once it has dispatched, so its row here only defers to that check. +FLAG_OWNERS = ( + ("--match", ("reply",), False), + ("--path", ("reply",), False), + ("--resolve", ("reply",), False), + ("--all-identical", ("reply",), False), + ("--request", ("wait",), False), + ("--checkout", ("attest", "cleanup"), False), + ("--fingerprint", ("comment",), True), + ("--body", ("comment", "reply"), True), +) + + +def main(argv: list[str] | None = None) -> int: + args = sys.argv[1:] if argv is None else argv + pre = argparse.ArgumentParser(add_help=False) + pre.add_argument("first", nargs="?") + for flag in ("--repo", "--checkout", "--min-age-hours"): + pre.add_argument(flag) + if pre.parse_known_args(args)[0].first == "cleanup": + sys.modules.setdefault("pr_review", sys.modules[__name__]) + import pr_cleanup + + rest = list(args) + rest.remove("cleanup") + return pr_cleanup.main(rest) + utf8_console() + ap = argparse.ArgumentParser() + ap.add_argument( + "cmd", choices=["attest", "claims", "cleanup", "comment", "status", "reply", "wait"] + ) + ap.add_argument("number", type=int, nargs="?") + # No default, because a default names one repository and every run from elsewhere would read that one. + ap.add_argument( + "--repo", + required=True, + metavar="OWNER/NAME", + help="the repository the pull request is in, since a pull " + "request number identifies no repository on its own", + ) + ap.add_argument("--timeout", type=int, default=2700, help="seconds (default 45m)") + ap.add_argument( + "--pickup-grace", + type=int, + default=300, + help="deprecated compatibility option, accepted but ignored", + ) + ap.add_argument( + "--check-grace", + type=int, + default=CHECK_GRACE, + help="seconds a check may sit queued, or a required status go unposted, " + f"before either reads as unstarted (default {CHECK_GRACE // 60}m)", + ) + ap.add_argument( + "--check-stall", + type=int, + default=CHECK_STALL, + help="seconds a check may run before its duration is reported " + f"(default {CHECK_STALL // 60}m)", + ) + ap.add_argument( + "--ignore-quota-signal", + action="store_true", + help="wait: request and poll the full --timeout even where the reviewer's own most " + "recent activity elsewhere in this repository is a quota-limit refusal with nothing " + "answering it since, or this pull request's newest Copilot review is a quota or " + "error refusal, pass this once the quota is believed to have reset", + ) + ap.add_argument( + "--request", + action="store_true", + help="wait: request a Copilot round even on a pull request into a branch other than the " + "default, which an attested local pass otherwise covers", + ) + ap.add_argument( + "--min-rounds", + type=int, + default=0, + metavar="N", + help="wait: treat the head as covered only once the reviewer's total rounds on this " + "pull request, the same count `status` prints as `rounds=`, exceed N. Pass the " + "`rounds=` a prior `status` already read, after re-requesting a review on a head that " + "already carries one: `reviewed_head` alone would report that earlier round as done " + "at once, never waiting for the fresh one the re-request was for. N is the count " + "already seen, not the count to reach: on a pull request reading `rounds=3`, N=3 " + "waits for the fourth round and N=4 for a fifth (default 0, so any round counts, " + "unchanged from before this flag existed)", + ) + # `reply` takes the finding's words, and there is no argument an id fits in. + ap.add_argument( + "--match", + metavar="TEXT", + help="reply: words from the finding, matched against the thread's opening " + "comment, and required to select exactly one unresolved thread", + ) + ap.add_argument( + "--path", + metavar="FILE", + help="reply: narrow --match to one file, for a file with several findings", + ) + ap.add_argument( + "--all-identical", + action="store_true", + help="reply: where --match selects several threads whose opening comments read the " + "same, a finding posted twice, answer each of them with --body rather than refusing", + ) + ap.add_argument( + "--body", + metavar="TEXT", + help="comment or reply: the answer to post, carrying the fixing commit SHA or " + "the evidence that disproves the finding", + ) + ap.add_argument("--fingerprint", help="comment: acknowledge the unplaced set it names") + ap.add_argument( + "--resolve", + action="store_true", + help="reply: resolve the thread once the reply is confirmed", + ) + ap.add_argument( + "--checkout", + metavar="DIR", + help="attest: the checkout holding the pull request's head and its recorded local pass " + "(default the current directory)", + ) + a = ap.parse_args(args) + + def refuse_foreign(late: bool) -> None: + for flag, owners, row_late in FLAG_OWNERS: + value = getattr(a, flag.removeprefix("--").replace("-", "_")) + if ( + row_late is late + and a.cmd not in owners + and value is not None + and value is not False + ): + ap.error( + f"{flag} belongs to {' or '.join(f'`{o}`' for o in owners)}, not `{a.cmd}`" + ) + + refuse_foreign(late=False) + if a.cmd == "cleanup": + ap.error("`cleanup` takes its arguments after it, as `cleanup NUMBER --repo OWNER/NAME`") + if a.number is None: + ap.error("the following arguments are required: number") + refuse_foreign(late=True) + required = ["--body"] + (["--match"] if a.cmd == "reply" else []) + if a.cmd in ("comment", "reply"): + values = {"--body": a.body, "--match": a.match} + for flag in required: + if not (values[flag] or "").strip(): + ap.error( + f"{a.cmd} requires a non-empty {flag}, since an empty answer records nothing" + ) + # None takes a negative value, and a negative check threshold would report every check stuck. + for name in ("pickup_grace", "min_rounds", "check_grace", "check_stall"): + if getattr(a, name) < 0: + ap.error(f"--{name.replace('_', '-')} cannot be negative") + # The two thresholds mean opposite things and the readings invert if the order does. + if a.check_grace >= a.check_stall: + ap.error( + "--check-grace must be less than --check-stall, since a queued check is " + f"judged sooner than a running one (got {a.check_grace} and {a.check_stall})" + ) + # A bare name is the near-miss a required argument still admits, and unpacking it raises a + # ValueError traceback rather than saying which half is missing. + owner, _, repo = a.repo.partition("/") + if not owner or not repo or "/" in repo: + ap.error(f"--repo takes OWNER/NAME, not {a.repo!r}") + + if a.cmd == "claims": + return check_claims(owner, repo, a.number) + + if a.cmd == "attest": + return attest(owner, repo, a.number, a.checkout or ".") + + if a.cmd == "comment": + marker = f"\n\n{ACK_MARKER.format(a.fingerprint)}" if a.fingerprint else "" + return comment_on_pr(owner, repo, a.number, a.body.strip() + marker) + + if a.cmd == "status": + # One payload renders the digest and decides the code, as in `wait`. + pr = full_read(owner, repo, a.number) + out, _ = digest(owner, repo, a.number, pr=pr, grace=a.check_grace, stall=a.check_stall) + print(out) + return report_verdict(pr) + + if a.cmd == "reply": + return reply_to_thread( + owner, repo, a.number, a.match, a.body, a.path, a.resolve, a.all_identical + ) + + return run_wait(owner, repo, a) + + if __name__ == "__main__": sys.exit(main()) diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index e1522b9b..9d00f910 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -5301,6 +5301,58 @@ def test_a_writing_option_on_a_reading_command_is_an_error(self) -> None: with self.subTest(flag=flag[0]): self.assertIn(flag[0], self.err(["status", "7", "--repo", "o/r", *flag])) + def test_a_command_line_with_two_bad_flags_names_the_same_one_first(self) -> None: + """Each expected line was printed before the ownership checks became `FLAG_OWNERS`.""" + cases = ( + ( + ["status", "7", "--repo", "o/r", "--body", "b", "--match", "x"], + "--match belongs to `reply`, not `status`", + ), + ( + ["status", "--repo", "o/r", "--fingerprint", "f", "--checkout", "."], + "--checkout belongs to `attest` or `cleanup`, not `status`", + ), + ( + ["status", "--repo", "o/r", "--fingerprint", "f", "--body", "b"], + "the following arguments are required: number", + ), + ( + [ + "--timeout", + "5", + "cleanup", + "7", + "--repo", + "o/r", + "--checkout", + ".", + "--body", + "b", + ], + "`cleanup` takes its arguments after it, as `cleanup NUMBER --repo OWNER/NAME`", + ), + ( + ["wait", "7", "--repo", "o/r", "--min-rounds", "-1", "--pickup-grace", "-1"], + "--pickup-grace cannot be negative", + ), + ( + ["wait", "7", "--repo", "o/r", "--check-stall", "-1", "--request", "--body", "b"], + "--body belongs to `comment` or `reply`, not `wait`", + ), + ) + for argv, first in cases: + with self.subTest(argv=argv): + self.assertTrue(self.err(argv).strip().endswith(f"error: {first}"), argv) + + def test_every_owner_in_the_table_is_a_subcommand(self) -> None: + """A misspelled owner would refuse its flag on the one command it belongs to.""" + commands = {"attest", "claims", "cleanup", "comment", "status", "reply", "wait"} + # A floor, since an emptied table would pass the subset check below. + self.assertGreaterEqual(len(pr_review.FLAG_OWNERS), 8) + for flag, owners, _ in pr_review.FLAG_OWNERS: + with self.subTest(flag=flag): + self.assertLessEqual(set(owners), commands) + PIN = "actions/checkout@" + "9" * 40 From d145242d55ff4561d9af3983686bf2c7d69601ac Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 18:23:19 -0700 Subject: [PATCH 2/3] Read the Subcommand Set From One Constant and Narrow the Table's Comments COMMANDS is now the parser's choices and the set the owner test reads, so a renamed subcommand fails the test. The table comment claims only the flags it lists, and the negative-value comment names its four. Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review.py | 9 ++++----- tests/test_pr_review.py | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 692f155b..21160f9e 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -1724,9 +1724,10 @@ def held_read() -> dict: return 30 -# Each flag a subcommand owns is refused elsewhere, since an ignored flag reads as one that took effect. +# Each flag listed is refused on a subcommand not owning it, since an ignored flag reads as one that took effect. # A late row is checked after the `cleanup` and `number` checks, which fixes the first error printed. # `cleanup` owns `--checkout` only once it has dispatched, so its row here only defers to that check. +COMMANDS = ("attest", "claims", "cleanup", "comment", "status", "reply", "wait") FLAG_OWNERS = ( ("--match", ("reply",), False), ("--path", ("reply",), False), @@ -1754,9 +1755,7 @@ def main(argv: list[str] | None = None) -> int: return pr_cleanup.main(rest) utf8_console() ap = argparse.ArgumentParser() - ap.add_argument( - "cmd", choices=["attest", "claims", "cleanup", "comment", "status", "reply", "wait"] - ) + ap.add_argument("cmd", choices=COMMANDS) ap.add_argument("number", type=int, nargs="?") # No default, because a default names one repository and every run from elsewhere would read that one. ap.add_argument( @@ -1880,7 +1879,7 @@ def refuse_foreign(late: bool) -> None: ap.error( f"{a.cmd} requires a non-empty {flag}, since an empty answer records nothing" ) - # None takes a negative value, and a negative check threshold would report every check stuck. + # None of these four takes a negative value, and a negative check threshold reports every check stuck. for name in ("pickup_grace", "min_rounds", "check_grace", "check_stall"): if getattr(a, name) < 0: ap.error(f"--{name.replace('_', '-')} cannot be negative") diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 9d00f910..d0cbff72 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -5346,7 +5346,7 @@ def test_a_command_line_with_two_bad_flags_names_the_same_one_first(self) -> Non def test_every_owner_in_the_table_is_a_subcommand(self) -> None: """A misspelled owner would refuse its flag on the one command it belongs to.""" - commands = {"attest", "claims", "cleanup", "comment", "status", "reply", "wait"} + commands = set(pr_review.COMMANDS) # A floor, since an emptied table would pass the subset check below. self.assertGreaterEqual(len(pr_review.FLAG_OWNERS), 8) for flag, owners, _ in pr_review.FLAG_OWNERS: From 2ca2e75de4fb4b45ba8ad9ead78b627523ae2e6e Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 18:24:41 -0700 Subject: [PATCH 3/3] Put the FLAG_OWNERS Comment Directly Above FLAG_OWNERS COMMANDS had landed between the table's comment and the table, so the comment read as describing it. Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 21160f9e..1e77171a 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -1724,10 +1724,10 @@ def held_read() -> dict: return 30 +COMMANDS = ("attest", "claims", "cleanup", "comment", "status", "reply", "wait") # Each flag listed is refused on a subcommand not owning it, since an ignored flag reads as one that took effect. # A late row is checked after the `cleanup` and `number` checks, which fixes the first error printed. # `cleanup` owns `--checkout` only once it has dispatched, so its row here only defers to that check. -COMMANDS = ("attest", "claims", "cleanup", "comment", "status", "reply", "wait") FLAG_OWNERS = ( ("--match", ("reply",), False), ("--path", ("reply",), False),