diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index 0f2fefa1..d8ca2ce6 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -78,6 +78,8 @@ is never carried into a downstream repository. 7. Reply to each thread and resolve what was addressed. 8. Re-run the loop after every fix push until the checks are green and no finding remains open. +The review effort setting is user-controlled. The workflow never selects or changes it. `status` reports `Lite`, `Balanced`, or `Max` when the completed review exposes that metadata, and distinguishes an inherited `Default ()` from an explicit choice. Missing effort metadata reports `unknown` and does not change coverage or completion. A pending effort-labeled request can complete without a `copilot_work_started` timeline event, so absence of that event never proves the request is abandoned. The bounded timeout reports `PENDING` when no review or terminal answer arrives. After a timeout with `requested=yes`, rerun `wait` for another bounded interval by default because the request may still be active. If the maintainer directs a retry, remove Copilot in the pull request UI, add it again, and rerun `wait`. This recovery replaces only the review request and never changes the effort setting. + Drive to green, a review confirmed on the latest head SHA and every actionable finding closed, then apply the Merge Gate above. **Never exit the loop early.** A round count is not a stopping condition, and neither is patience running out. Reporting only that the PR was opened is an early diff --git a/.claude-plugin/fleet-skills/.source-digest b/.claude-plugin/fleet-skills/.source-digest index 7adf27da..69d7199d 100644 --- a/.claude-plugin/fleet-skills/.source-digest +++ b/.claude-plugin/fleet-skills/.source-digest @@ -1 +1 @@ -5c6b9955c54c1e80 +74a2d7f8def278aa diff --git a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md index 0f2fefa1..d8ca2ce6 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -78,6 +78,8 @@ is never carried into a downstream repository. 7. Reply to each thread and resolve what was addressed. 8. Re-run the loop after every fix push until the checks are green and no finding remains open. +The review effort setting is user-controlled. The workflow never selects or changes it. `status` reports `Lite`, `Balanced`, or `Max` when the completed review exposes that metadata, and distinguishes an inherited `Default ()` from an explicit choice. Missing effort metadata reports `unknown` and does not change coverage or completion. A pending effort-labeled request can complete without a `copilot_work_started` timeline event, so absence of that event never proves the request is abandoned. The bounded timeout reports `PENDING` when no review or terminal answer arrives. After a timeout with `requested=yes`, rerun `wait` for another bounded interval by default because the request may still be active. If the maintainer directs a retry, remove Copilot in the pull request UI, add it again, and rerun `wait`. This recovery replaces only the review request and never changes the effort setting. + Drive to green, a review confirmed on the latest head SHA and every actionable finding closed, then apply the Merge Gate above. **Never exit the loop early.** A round count is not a stopping condition, and neither is patience running out. Reporting only that the PR was opened is an early diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 8c80c52a..8febbd7f 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -48,6 +48,8 @@ diff coverage. A refusal, partial or absent coverage statement, unrecognized out unresolved thread, or body-only finding blocks the review loop. Re-run the loop after every fix push. Never infer review completion from `mergeStateStatus: CLEAN`. +Review effort is user-controlled. The automation observes `Lite`, `Balanced`, or `Max`, including an inherited `Default ()`, and never selects or changes the setting. Effort does not determine coverage or completion. A request can complete without a `copilot_work_started` event, so absence of that event is not a stalled-review verdict. When `wait` returns `PENDING` with `requested=yes`, report the state and rerun `wait` for another bounded interval by default. Do not clear the request automatically because it may be active. If the maintainer directs a retry, remove Copilot in the pull request UI, add it again, and rerun `wait`. This recovery replaces only the review request and never changes the effort setting. + ### Disproved Claims **A disproof is proof about this repository, and the thread it was written in is not where the next round looks.** [GOVERNANCE.md "PR Review Etiquette"](../GOVERNANCE.md#pr-review-etiquette), which routes to the `pr-review-conduct` Skill, closes a false finding by disproving it in the thread, addressed to the reviewer so it does not raise the same thing again, and while the pull request is open that is the right place for it. Afterwards it is the wrong one. The pull request merges, the next round begins with no memory of the last, and the second occurrence reaches a maintainer with no way to tell it from a first. Each entry below is a claim that was tested against this repository and found false, kept so the proof is read rather than built twice. diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md index 0f2fefa1..d8ca2ce6 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -78,6 +78,8 @@ is never carried into a downstream repository. 7. Reply to each thread and resolve what was addressed. 8. Re-run the loop after every fix push until the checks are green and no finding remains open. +The review effort setting is user-controlled. The workflow never selects or changes it. `status` reports `Lite`, `Balanced`, or `Max` when the completed review exposes that metadata, and distinguishes an inherited `Default ()` from an explicit choice. Missing effort metadata reports `unknown` and does not change coverage or completion. A pending effort-labeled request can complete without a `copilot_work_started` timeline event, so absence of that event never proves the request is abandoned. The bounded timeout reports `PENDING` when no review or terminal answer arrives. After a timeout with `requested=yes`, rerun `wait` for another bounded interval by default because the request may still be active. If the maintainer directs a retry, remove Copilot in the pull request UI, add it again, and rerun `wait`. This recovery replaces only the review request and never changes the effort setting. + Drive to green, a review confirmed on the latest head SHA and every actionable finding closed, then apply the Merge Gate above. **Never exit the loop early.** A round count is not a stopping condition, and neither is patience running out. Reporting only that the PR was opened is an early diff --git a/scripts/README.md b/scripts/README.md index 02f39a6e..e41b6f18 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -151,7 +151,9 @@ python3 scripts/pr_review.py reply 452 --repo ptr727/ProjectTemplate \ `wait` exits `40` when Copilot answers the request with a plain comment rather than a review, meaning a comment of its own that postdates its newest review on the pull request. The test is the **shape** of that answer and not its cause, which the script reads nothing of: a comment carries no commit, so it satisfies no coverage check whatever it says, and a wait reading formal reviews alone treats it as an unmet condition and then polls out its whole timeout against an answer that already arrived. A refusal is the case that makes this worth catching, a quota or rate-limit message among them, and `40` neither asserts nor detects one. The comment prints whole because its wording is the only thing separating a refusal, which is terminal since no review follows it and re-requesting does not clear it, from an ordinary remark that is not, so `40` ends the wait and hands the text to the reader who can tell them apart. A comment **older** than the newest review is spent rather than terminal, because the review it preceded did land. Every connection reads the newest `WINDOW` nodes rather than the reviewer's own, since GraphQL offers no author filter, so ordinary traffic is what pushes theirs out of reach. `window_blind` is the one guard over both sides, and each side fails differently. Blind on **comments** means an answer could be back there unseen, which reads as `answered_outside_review=unknown` rather than `no`. Blind on **reviews** is worse, because the newest review in view is then not the newest there is, and an empty baseline dates every comment as newer so each one reads as an answer: a false `40` that stops the loop on a pull request whose review actually landed. That case reports nothing and lets the wait keep polling, since a wait that runs on is visible where a wrong terminal is not. -Everything else is decidable and says so. One of the reviewer's own nodes in view, even a **spent** one, settles the question, because nodes arrive in creation order, so anything behind the window is older than everything inside it. A window holding every node the pull request has is settled too, which is why the guard reads `pageInfo.hasPreviousPage` rather than the node count: a full window and a complete one are the same length, so length alone would report a gap where none exists. Cases hold `WINDOW` equal across all four windows and hold all four to asking for `hasPreviousPage`, since a connection that stops asking reports `no` instead of `unknown`, the silent narrowing one level up. `wait` exits `50` when the reviewer sits in the pending request set and no `copilot_work_started` follows the newest request, meaning nothing is acting on it and waiting on will not start it. That state is invisible from the reviews alone and indistinguishable from patience: one request sat thirteen and a half hours while the pull request read as waiting on the reviewer. Elapsed time cannot separate it from a slow round either, so the pickup event decides. It is the one thing here read over REST, since no GraphQL timeline item carries it, and it runs on its own interval rather than per poll: the first read comes after `--pickup-grace` (default five minutes), because inside that window a pending request is simply a review being worked on, and each later read waits another interval. One reading settles the request in front of it, and the next covers a request a push raises mid-wait, so a long wait costs a handful of REST calls instead of one per poll. The pickup is checked **before** the timeout, so the stall reports as itself instead of as `PENDING` once the clock runs out. Recovery stays out of this script: the digest names the state and the runbook carries the two mutations that clear and re-raise the request, neither of which has an id to hide and so neither of which is worth wrapping. The pending set is read through GraphQL rather than `gh pr view --json reviewRequests`, which omits a Bot reviewer outright and reports an empty set while Copilot sits in it. +Everything else is decidable and says so. One of the reviewer's own nodes in view, even a **spent** one, settles the question because nodes arrive in creation order. A window holding every node is settled too, which is why the guard reads `pageInfo.hasPreviousPage` instead of the node count. `wait` does not classify a request from `copilot_work_started`. GitHub can convert a request to an effort-labeled review and complete it without that event. Absence of the event therefore cannot distinguish queued work from abandoned work. A request with no review or terminal answer remains `PENDING` until the bounded timeout. After a timeout with `requested=yes`, rerun `wait` for another bounded interval by default because the request may still be active. If the maintainer directs a retry, remove Copilot in the pull request UI, add it again, and rerun `wait`. That recovery replaces only the review request and never changes the effort setting. `--pickup-grace` remains accepted for command compatibility but has no effect. The pending set still comes from GraphQL because `gh pr view --json reviewRequests` omits a Bot reviewer. + +The digest reports the completed head review's effective effort as `lite`, `balanced`, or `max` when its metadata provides one. It reports `effort_source=default` for `Default ()` and `effort_source=explicit` for a bare level. Missing metadata reports `unknown` for both fields. Effort is informational and never changes the coverage or completion verdict. The workflow never selects or changes the user-controlled setting. `wait` exits `41` when the review carrying the head is a **refusal**, meaning its body opens by saying it did not review. That answer arrives as a formal review, `state: COMMENTED`, with the correct commit and zero inline threads, so it satisfies every coverage check a clean pass does and renders a digest byte for byte identical to one. The `40` reasoning does not reach it, because that reasoning rests on a comment carrying no commit, and this carries the right one. A pull request of 301 changed files, one over the reviewer's limit of 300, read as `rounds=1 review_on_head=yes threads=0 unresolved=0 merge=CLEAN` and was one command from merging on a review that never ran. A refusal is therefore not coverage: `review_on_head` reports `NO`, the summary line carries a `refusal=YES` of its own, since `rounds=1 review_on_head=NO` is equally what a stale round looks like and the two want opposite responses, and the body prints whole because its wording is the only thing separating a file-count refusal, cleared by splitting the pull request, from a quota one, cleared by waiting. The script reads neither cause, only that the round declined. The match is on the body's **opening line**, since a refusal is the whole body where a review that merely quotes the wording carries it below its own overview, and this script and this file are exactly that quotation. One line rather than two, because a review's first line is its heading and its second is the overview prose: reading two passed every case except the review describing this check, which reported itself as a refusal of itself. The cost is the other direction, that a refusal introduced by a heading would sit below the opening and be missed, and answering that shape means telling a refusal from an overview rather than reading one line further. It is an alternation over the runbook's phrasings for the same reason the suppressed heading is, and a case asserts the script's pattern is the one the runbook publishes. The reading is **head-scoped**, unlike a suppressed finding, because a refusal is a statement about one commit that a push retires, and a genuine review of that same head outranks it, coverage that landed being coverage. The field is spent by that coverage as well as the exit code is, or the summary line reads `review_on_head=yes refusal=YES` and tells a reader to split a pull request the reviewer has just reviewed. The liveness query carries no bodies, so a refusal reads there as ordinary coverage. That is deliberate: it ends the wait, which is what a terminal outcome should do, and the full read every wait finishes with is what tells the two apart, so no exit code comes from the cheaper reading. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index d747bc0f..777758e9 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -62,8 +62,9 @@ not this and exits 0, and neither is a stuck check on a merge that is not BLOCKED, since the rollup carries checks no ruleset requires. The digest reports the check in both cases, so a shape outside 44 is still named rather than lost. - 50 = the request is pending and nothing picked it up, which no amount of - waiting changes. Recovery is two mutations, and they stay in the runbook. + A pending request remains pending until a review, an answer, or the timeout. GitHub's + effort-labeled review lifecycle does not always emit `copilot_work_started`, so that + event is not evidence that distinguishes queued work from abandoned work. Reading is the bulk of this and the writing commands are a trade rather than a free win. A mutation spelled as a `gh` command in a shell is read by the gh-write-guard PreToolUse hook, and @@ -220,6 +221,11 @@ MARKDOWN_HEADING = re.compile(r"\s*#{1,6}\s") # The `Review details` metadata bullets, of which the coverage line is one. LABEL_LINE = re.compile(r"\s*[-*]\s+\*\*([^*]+):\*\*") +EFFORT_LINE = re.compile( + r"\s*[-*]\s+\*\*Review effort level:\*\*\s*" + r"(?:(Default)\s*\(\s*(Lite|Balanced|Max)\s*\)|(Lite|Balanced|Max))\s*$", + re.IGNORECASE, +) # A login that reads as this reviewer without being the spelling every query here filters on. # A rename leaves every filter matching nothing, so a review that landed reads as none at all. # A wait then polls out its whole timeout against a review sitting in plain sight. @@ -256,18 +262,6 @@ # A case is not the guarantee though, since it holds only where someone runs it. CHECKS_WINDOW = 100 -# The timeline spells the reviewer a third way, as login `Copilot` with type `Bot`. -# GraphQL says `copilot-pull-request-reviewer`, and REST user objects add a `[bot]` suffix. -# The predicate is the type plus a loose login match rather than any one spelling. -# Requests are the reviewer's own, since a human requested later is a different request. -# Reading one as the newest reports a picked-up review as never picked up. -TIMELINE_JQ = ( - '.[] | select(.event == "copilot_work_started" or (.event == "review_requested"' - ' and .requested_reviewer.type == "Bot"' - ' and ((.requested_reviewer.login // "") | ascii_downcase | test("copilot"))))' - ' | "\\(.event) \\(.created_at)"' -) - # Liveness query: timestamps and ids only, no comment or review bodies. # A liveness check does not need the finding text, and re-fetching bodies was 76% of polls. # It does need the reviewer's non-review answers. @@ -429,52 +423,6 @@ def gql(query: str, owner: str, repo: str, num: int) -> dict: return gh_graphql(query, o=owner, r=repo, n=num)["repository"]["pullRequest"] -def timeline(owner: str, repo: str, num: int) -> list[tuple[str, str]]: - """The request and pickup events, oldest first, as (event, timestamp). - - GraphQL carries no `copilot_work_started`, so this is the one REST reader here. - `--jq` projects inside gh rather than after it, since `--paginate` without one emits a - concatenated array per page that is not valid JSON on every gh a fleet machine may carry. - `per_page` is the page size the pagination actually costs, and the default of 30 turns a - long-running pull request into six requests a reading where the maximum makes it two. - """ - r = subprocess.run( - [ - "gh", - "api", - "--paginate", - f"repos/{owner}/{repo}/issues/{num}/timeline?per_page=100", - "--jq", - TIMELINE_JQ, - ], - capture_output=True, - text=True, - encoding="utf-8", - check=False, - ) - if r.returncode != 0: - sys.stderr.write(r.stderr[:800]) - raise SystemExit(f"gh timeline failed rc={r.returncode}") - return [(ln.split(" ", 1)[0], ln.split(" ", 1)[1]) for ln in r.stdout.splitlines() if " " in ln] - - -def never_picked_up(events: list[tuple[str, str]]) -> str: - """The newest request's timestamp where no pickup followed it, otherwise the empty string. - - A request the reviewer accepts raises `copilot_work_started` within about half a minute, so - a request with no pickup after it is not a slow review, it is a request nothing is acting on. - The two states look identical from the reviews alone, which is how one sat for thirteen hours - reading as pending. Elapsed time cannot separate them either, since a genuinely slow round - also produces no review, and only the pickup event says whether anything is working. - """ - requested = [t for e, t in events if e == "review_requested"] - started = [t for e, t in events if e == "copilot_work_started"] - if not requested: - return "" - newest = max(requested) - return "" if any(t >= newest for t in started) else newest - - def reviewer_requested(pr: dict) -> bool: """True where the reviewer sits in the pending request set. @@ -575,18 +523,6 @@ def window_blind(pr: dict, field: str) -> bool: return bool(older) and not reviewer_nodes(pr, field) -def stall_of(owner: str, repo: str, num: int, pr: dict) -> str: - """The stalled request's timestamp for this payload, or the empty string where none. - - Derived from the payload it is reported beside, since a stall read earlier describes a - pull request that has since moved: a request picked up after the reading still reports as - picked up by nothing. A covered head or no pending request settles it without a REST call. - """ - if reviewed_head(pr) or not reviewer_requested(pr): - return "" - return never_picked_up(timeline(owner, repo, num)) - - def refusal_of(node: dict) -> str: """The review's body where its opening line says the reviewer did not review, otherwise empty. @@ -659,6 +595,26 @@ def reviewed_head(pr: dict) -> bool: return bool(head_reviews(pr)) +def review_effort(pr: dict) -> tuple[str, str]: + """The newest head review's effective effort and selection source. + + GitHub can render an inherited choice as `Default (Lite)`, `Default (Balanced)`, or + `Default (Max)`. A bare level is explicit. The setting remains user-controlled, and this + reader only reports metadata that the completed review body exposes. + """ + reviews = head_reviews(pr) + if not reviews: + return "unknown", "unknown" + newest = max(reviews, key=lambda n: n.get("submittedAt") or "") + plain = FENCE.sub("", newest.get("body") or "") + for line in plain.splitlines(): + match = EFFORT_LINE.fullmatch(line) + if match: + source = "default" if match.group(1) else "explicit" + return (match.group(2) or match.group(3)).lower(), source + return "unknown", "unknown" + + def is_coverage_line(line: str) -> bool: """Whether this line is the reviewer stating its file coverage, rather than prose about it.""" if FLEET_REVIEW.fullmatch(line): @@ -1247,24 +1203,26 @@ def digest( stall: float = CHECK_STALL, checks: list[dict] | None = None, ) -> tuple[str, int]: - """Render the digest, from a caller's payload and stall reading where those are given. + """Render the digest from a caller's payload and normalized checks when supplied. The caller passes its own readings when the exit code has to agree with what was printed, since a review landing between two reads makes a fresh fetch describe a different pull - request than the one the code was decided from. Passing the stall also spends one REST - call between the caller and the digest rather than one each, and `checks` spends the rollup - parse the same way, since the wait decides an exit code from the reading it just printed. + request than the one the code was decided from. `checks` spends the rollup parse once, + since the wait decides an exit code from the reading it just printed. `now` is a parameter for the same reason, so a case can hold a check at a known age rather than at whatever the clock says when the suite runs. + + `stalled` remains as an ignored compatibility parameter for callers that supplied the old + pickup reading. GitHub's effort-labeled lifecycle makes that reading inconclusive. """ pr = gql(Q_FULL, owner, repo, num) if pr is None else pr - stalled = stall_of(owner, repo, num, pr) if stalled is None else stalled now = datetime.now(UTC) if now is None else now head = pr["headRefOid"] revs = reviewer_nodes(pr, "reviews") # `revs` is every round and `on_head` is the ones that reviewed this commit. # A refusal sits in the first and not the second, being a round that covered nothing. on_head = head_reviews(pr) + effort, effort_source = review_effort(pr) cover, cover_line = head_coverage(pr) unknown = unrecognized_shapes(pr) threads = pr["reviewThreads"]["nodes"] @@ -1308,6 +1266,7 @@ def digest( # A digest of the wrong pull request is well-formed, so naming it is what shows the miss. f"repo={owner}/{repo} pr={num} head={head[:8]} rounds={len(revs)} " f"review_on_head={'yes' if on_head else 'NO'} " + f"effort={effort} effort_source={effort_source} " # A field of its own beside that one, since a round can cover the head and read part. # Those two readings are what `review_on_head=yes` alone conflates. f"coverage={COVERAGE_FIELD[cover]} " @@ -1427,11 +1386,6 @@ def digest( "head, so no rollup here describes this head and `checks=0/0` is this " "reading failing rather than a pull request with no checks" ) - if stalled: - lines.append( - f" REQUEST NOT PICKED UP (requested {stalled}, no copilot_work_started " - "since): clear the request and re-request, per the runbook" - ) if blind: lines.append( f" BEHIND THE WINDOW ({' and '.join(blind)}): the newest {WINDOW} carry " @@ -1879,7 +1833,7 @@ def main(argv: list[str] | None = None) -> int: "--pickup-grace", type=int, default=300, - help="seconds before the first pickup read, and between reads (default 5m)", + help="deprecated compatibility option, accepted but ignored", ) ap.add_argument( "--check-grace", @@ -1942,8 +1896,6 @@ def main(argv: list[str] | None = None) -> int: ap.error( f"{a.cmd} requires a non-empty {flag}, since an empty answer records nothing" ) - # A negative grace leaves the next reading permanently behind the clock. - # That is the per-poll REST pattern the interval exists to prevent. if a.pickup_grace < 0: ap.error("--pickup-grace cannot be negative") # A negative threshold reports every check in that state, on every run, from the first read. @@ -1998,20 +1950,9 @@ def main(argv: list[str] | None = None) -> int: print(f"auto-request: {request_copilot_review(owner, repo, pr['id'])}") # No re-read here: Copilot never resolves within the round trip that just issued the request. # The loop below picks up fresh state on its own first iteration instead of this spending a second call to learn nothing new. - stalled = "" i = 0 - next_pickup = a.pickup_grace while not done and not answer and not drift: elapsed = time.monotonic() - start - # Read the pickup before the clock, so a request nothing acted on reports as itself. - # Running the clock out instead would report it exactly as a slow reviewer. - # The read costs a second call over REST, so it runs on its own interval, not per poll. - # One reading settles the current request, and the next covers a request a push raises. - if elapsed > next_pickup and reviewer_requested(pr): - next_pickup = elapsed + a.pickup_grace - stalled = never_picked_up(timeline(owner, repo, a.number)) - if stalled: - break if elapsed > a.timeout: break time.sleep(delays[min(i, len(delays) - 1)]) @@ -2022,14 +1963,11 @@ def main(argv: list[str] | None = None) -> int: drift = reviewer_login_drift(pr) # One payload decides the digest and the exit code together. - # Read separately, a review landing between them prints coverage and returns a stalled code. + # Read separately, a review landing between them prints coverage and returns a timeout code. # A reader resolves that by believing the code, dropping the review it was just shown. # The digest also earns its call at the timeout. # A bare PENDING line reports a broken wait and a slow reviewer identically. final = gql(Q_FULL, owner, repo, a.number) - # The stall is re-read here rather than carried out of the loop. - # A request picked up since that reading would still report as picked up by nothing. - stalled = stall_of(owner, repo, a.number, final) now = datetime.now(UTC) # Parsed here and handed down, so the digest and the exit code share one read of the rollup. # Deriving the stuck shapes from that list costs no parse, which is what was doubled. @@ -2040,7 +1978,6 @@ def main(argv: list[str] | None = None) -> int: repo, a.number, pr=final, - stalled=stalled, now=now, grace=a.check_grace, stall=a.check_stall, @@ -2108,13 +2045,6 @@ def main(argv: list[str] | None = None) -> int: "no review follows and re-requesting does not clear it" ) return 40 - if stalled: - print( - f"status=REQUEST_NOT_PICKED_UP requested {stalled} and no copilot_work_started " - "followed it, so nothing is working on this and waiting on will not start it: " - "clear the request and re-request, per the runbook" - ) - return 50 print("status=PENDING") return 30 diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py index 2dd75c5c..da46c33f 100755 --- a/scripts/tests/test_pr_review.py +++ b/scripts/tests/test_pr_review.py @@ -19,7 +19,6 @@ import sys import unittest from datetime import UTC, datetime, timedelta -from itertools import count from pathlib import Path from unittest import mock @@ -92,6 +91,7 @@ def nested( heading: str = "### Suppressed comments (2)", finding: str = "**a.py:12**\n* The retry count is off by one.", covers: str = "- **Files reviewed:** 1/1 changed files", + effort: str = "Lite", ) -> str: """The section as a Markdown heading nested inside the `Review details` wrapper. @@ -108,7 +108,7 @@ def nested( "| File | Description |\n\n\n\n" f"
\nReview details\n\n{heading}\n\n{finding}\n\n" f"{covers}\n" - "- **Review effort level:** Lite\n
\n" + f"- **Review effort level:** {effort}\n\n" ) @@ -392,39 +392,28 @@ def test_one_spent_reviewer_comment_in_view_settles_the_question(self) -> None: self.assertFalse(pr_review.window_blind(pr, "comments")) -class TestPickup(unittest.TestCase): - """A request nothing acted on and a review being worked on are one reading from the reviews.""" +class TestReviewEffort(unittest.TestCase): + """Review effort is observed from completed review metadata and never selected here.""" - def test_a_request_with_no_pickup_after_it_is_named_by_its_timestamp(self) -> None: - """The shape that sat thirteen hours reading as pending: requested, never started.""" - events = [ - ("review_requested", "2026-08-02T22:58:15Z"), - ("copilot_work_started", "2026-08-02T22:58:45Z"), - ("review_requested", "2026-08-03T00:15:00Z"), - ] - self.assertEqual("2026-08-03T00:15:00Z", pr_review.never_picked_up(events)) + def test_inherited_effort_reports_the_effective_level_and_default_source(self) -> None: + for level in ("Lite", "Balanced", "Max"): + with self.subTest(level=level): + pr = payload([review(body=nested(effort=f"Default ({level})"))]) + self.assertEqual((level.lower(), "default"), pr_review.review_effort(pr)) - def test_a_request_the_reviewer_took_up_is_not_stalled(self) -> None: - """Slow is not stuck, and only the pickup event tells them apart.""" - events = [ - ("review_requested", "2026-08-03T13:09:19Z"), - ("copilot_work_started", "2026-08-03T13:09:54Z"), - ] - self.assertEqual("", pr_review.never_picked_up(events)) + def test_explicit_effort_reports_the_level_and_explicit_source(self) -> None: + for level in ("Lite", "Balanced", "Max"): + with self.subTest(level=level): + pr = payload([review(body=nested(effort=level))]) + self.assertEqual((level.lower(), "explicit"), pr_review.review_effort(pr)) - def test_an_earlier_pickup_does_not_cover_a_later_request(self) -> None: - """Answering the last request is not answering this one, and order is what says so.""" - events = [ - ("copilot_work_started", "2026-08-02T22:58:45Z"), - ("review_requested", "2026-08-02T23:31:44Z"), - ] - self.assertEqual("2026-08-02T23:31:44Z", pr_review.never_picked_up(events)) + def test_absent_effort_metadata_is_unknown(self) -> None: + self.assertEqual(("unknown", "unknown"), pr_review.review_effort(payload([review()]))) - def test_no_request_at_all_is_not_a_stall(self) -> None: - self.assertEqual( - "", pr_review.never_picked_up([("copilot_work_started", "2026-08-02T22:58:45Z")]) - ) - self.assertEqual("", pr_review.never_picked_up([])) + def test_newest_head_review_does_not_borrow_older_effort_metadata(self) -> None: + older = review(at=EARLY, body=nested(effort="Balanced")) + newer = review(at=LATE) + self.assertEqual(("unknown", "unknown"), pr_review.review_effort(payload([older, newer]))) def test_the_pending_set_is_read_where_a_bot_reviewer_is_visible(self) -> None: """`gh pr view --json reviewRequests` omits a Bot outright and reports an empty set.""" @@ -470,6 +459,12 @@ def test_review_on_head_reports_no_when_every_round_is_stale(self) -> None: out, _ = pr_review.digest("o", "r", 7) self.assertIn("review_on_head=NO", out) + def test_digest_reports_effective_effort_without_changing_the_verdict(self) -> None: + self.answer(payload([review(body=nested(effort="Default (Balanced)"))])) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("effort=balanced effort_source=default", out) + self.assertIn("coverage=full", out) + def test_a_thread_from_a_deleted_account_does_not_crash_the_digest(self) -> None: """GraphQL sends `author` present and null, which a defaulted lookup returns as None.""" orphan = thread("T1") @@ -2108,82 +2103,28 @@ def test_a_landed_review_wins_over_an_older_answer(self) -> None: with mock.patch.object(pr_review.time, "sleep"): self.assertEqual(0, self.cli(["wait", "7"])) - def test_wait_stops_on_a_request_nothing_picked_up(self) -> None: - """Waiting on cannot start a request nothing is acting on, so the wait says so and ends. - - The zero timeout is what this fails on rather than hangs on, and it also pins the order: - the pickup is read before the clock, so the stall reports as itself instead of as PENDING. - """ + def test_a_pending_effort_labeled_request_reaches_the_timeout(self) -> None: + """Missing pickup telemetry cannot prove that an effort-labeled request is abandoned.""" self.answer(payload([review(oid=OLD)], pending=True)) - with ( - mock.patch.object(pr_review, "timeline", return_value=[("review_requested", LATE)]), - mock.patch.object(pr_review.time, "sleep"), - ): - self.assertEqual(50, self.cli(["wait", "7", "--pickup-grace", "0", "--timeout", "0"])) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(30, self.cli(["wait", "7", "--timeout", "0"])) out = self.out.getvalue() - self.assertIn("status=REQUEST_NOT_PICKED_UP", out) - self.assertIn(LATE, out) - - def test_a_request_being_worked_on_is_not_stopped_on(self) -> None: - """A slow round is the case the grace exists for, and stopping on it loses the review.""" - self.answer(payload([review(oid=OLD)], pending=True), payload([review()], pending=True)) - with ( - mock.patch.object( - pr_review, - "timeline", - return_value=[("review_requested", EARLY), ("copilot_work_started", LATE)], - ), - mock.patch.object(pr_review.time, "sleep"), - ): - self.assertEqual(0, self.cli(["wait", "7", "--pickup-grace", "0", "--timeout", "600"])) + self.assertIn("status=PENDING", out) + self.assertNotIn("REQUEST NOT PICKED UP", out) - def test_the_pickup_read_waits_out_the_grace_rather_than_running_per_poll(self) -> None: - """It costs a second call, and inside the grace a pending request is just work in flight.""" + def test_a_pending_request_can_complete_without_pickup_telemetry(self) -> None: + """The PR #873 lifecycle reaches a review without `copilot_work_started`.""" self.answer(payload([review(oid=OLD)], pending=True), payload([review()], pending=True)) - with ( - mock.patch.object(pr_review, "timeline", return_value=[]) as seen, - mock.patch.object(pr_review.time, "sleep"), - ): - self.assertEqual( - 0, self.cli(["wait", "7", "--pickup-grace", "9999", "--timeout", "600"]) - ) - seen.assert_not_called() - - def test_the_pickup_read_runs_on_its_own_interval_once_the_grace_is_out(self) -> None: - """Every poll past the grace is what the comment ruled out and the code did anyway. - - The clock advances a fixed step per reading, so the interval is counted rather than - waited: a long wait must not turn one REST reader into one per poll. - """ - picked_up = [("review_requested", EARLY), ("copilot_work_started", LATE)] - self.answer(payload([review(oid=OLD)], pending=True)) - with ( - mock.patch.object(pr_review.time, "monotonic", side_effect=count(0, 30)), - mock.patch.object(pr_review, "timeline", return_value=picked_up) as seen, - mock.patch.object(pr_review.time, "sleep"), - ): - self.assertEqual( - 30, self.cli(["wait", "7", "--pickup-grace", "300", "--timeout", "1200"]) - ) - # Roughly one read per grace interval over the wait, never one per poll. - self.assertGreaterEqual(seen.call_count, 1) - self.assertLessEqual(seen.call_count, 1200 // 300 + 1) - - def test_a_review_landing_during_the_last_read_wins_over_the_stalled_code(self) -> None: - """The digest and the exit code come from one payload, or they describe different PRs. + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(0, self.cli(["wait", "7", "--timeout", "600"])) - An automated reader resolves a digest saying covered against a code saying stalled by - believing the code, so the review it just printed is the thing that gets dropped. - """ + def test_a_pending_review_landing_during_the_final_read_wins_over_timeout(self) -> None: + """The digest and exit code come from one payload when the review lands at timeout.""" self.answer(payload([review(oid=OLD)], pending=True), payload([review()], pending=True)) - with ( - mock.patch.object(pr_review, "timeline", return_value=[("review_requested", LATE)]), - mock.patch.object(pr_review.time, "sleep"), - ): - self.assertEqual(0, self.cli(["wait", "7", "--pickup-grace", "0", "--timeout", "0"])) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(0, self.cli(["wait", "7", "--timeout", "0"])) out = self.out.getvalue() self.assertIn("review_on_head=yes", out) - self.assertNotIn("status=REQUEST_NOT_PICKED_UP", out) def test_a_review_landing_during_the_last_read_wins_over_the_timeout(self) -> None: """Same disagreement at the other exit: printing coverage and returning PENDING.""" @@ -2194,29 +2135,19 @@ def test_a_review_landing_during_the_last_read_wins_over_the_timeout(self) -> No self.assertIn("review_on_head=yes", out) self.assertNotIn("status=PENDING", out) - def test_a_request_picked_up_after_the_loop_read_it_is_not_reported_as_stalled(self) -> None: - """The stall is re-read at the end, or a request taken up since still reports as dead.""" + def test_pickup_grace_remains_an_ignored_compatibility_option(self) -> None: + """Existing callers keep parsing while the option makes no liveness claim.""" self.answer(payload([review(oid=OLD)], pending=True)) - picked_up = [("review_requested", EARLY), ("copilot_work_started", LATE)] - with ( - mock.patch.object( - pr_review, "timeline", side_effect=[[("review_requested", LATE)], picked_up] - ), - mock.patch.object(pr_review.time, "sleep"), - ): + with mock.patch.object(pr_review.time, "sleep"): self.assertEqual(30, self.cli(["wait", "7", "--pickup-grace", "0", "--timeout", "0"])) out = self.out.getvalue() - self.assertNotIn("status=REQUEST_NOT_PICKED_UP", out) - self.assertNotIn("REQUEST NOT PICKED UP", out) + self.assertIn("status=PENDING", out) - def test_an_answer_outranks_a_stall_when_both_are_true(self) -> None: - """The reviewer saying something outranks it saying nothing, and the digest shows both.""" + def test_an_answer_ends_a_pending_request(self) -> None: + """An answer remains terminal regardless of absent pickup telemetry.""" self.answer(payload([review(oid=OLD)], comments=[comment()], pending=True)) - with ( - mock.patch.object(pr_review, "timeline", return_value=[("review_requested", LATE)]), - mock.patch.object(pr_review.time, "sleep"), - ): - self.assertEqual(40, self.cli(["wait", "7", "--pickup-grace", "0", "--timeout", "0"])) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(40, self.cli(["wait", "7", "--timeout", "0"])) def test_the_repo_argument_splits_into_owner_and_name(self) -> None: self.answer(payload([review()])) @@ -3052,63 +2983,8 @@ def test_the_guard_tests_the_window_the_queries_actually_read(self) -> None: self.assertEqual(4, source.count("pageInfo{ hasPreviousPage }")) self.assertEqual(4, len(re.findall(r"(?:comments|reviews)\(last:\d+\)", source))) - def test_the_timeline_reader_asks_for_the_largest_page(self) -> None: - """The page size is what pagination costs, and the default of 30 triples the requests.""" - done = subprocess.CompletedProcess(args=[], returncode=0, stdout="", stderr="") - with mock.patch.object(pr_review.subprocess, "run", return_value=done) as run: - pr_review.timeline("o", "r", 7) - argv = run.call_args.args[0] - self.assertIn("repos/o/r/issues/7/timeline?per_page=100", argv) - self.assertIn("--paginate", argv) - # A read, and the guard against a write creeping into the one REST call here. - self.assertEqual(["gh", "api"], argv[:2]) - self.assertFalse({"-X", "--method"} & set(argv)) - - def test_the_timeline_filter_takes_the_reviewer_s_own_requests_only(self) -> None: - """A human requested later is not this request, and reading it as one reports a stall. - - The filter runs inside gh, so this drives the real `jq` over a crafted timeline rather - than asserting on the filter's text, which would pass on a filter that matches nothing. - The timeline spells the reviewer `Copilot` with type `Bot`, a third form after GraphQL's - `copilot-pull-request-reviewer` and REST's `[bot]` suffix on that, so a filter keyed to - either of those two selects nothing here and the whole state reads as no request at all. - """ - events = [ - { - "event": "review_requested", - "created_at": "01", - "requested_reviewer": {"login": "Copilot", "type": "Bot"}, - }, - {"event": "copilot_work_started", "created_at": "02"}, - { - "event": "review_requested", - "created_at": "03", - "requested_reviewer": {"login": "ptr727", "type": "User"}, - }, - { - "event": "review_requested", - "created_at": "04", - "requested_reviewer": {"login": "some-other-bot", "type": "Bot"}, - }, - {"event": "commented", "created_at": "05"}, - ] - run = subprocess.run( - ["jq", "-r", pr_review.TIMELINE_JQ], - input=json.dumps(events), - capture_output=True, - text=True, - check=False, - ) - self.assertEqual(0, run.returncode, run.stderr) - self.assertEqual( - ["review_requested 01", "copilot_work_started 02"], run.stdout.split("\n")[:-1] - ) - # The reading that matters: the human request must not become the newest request. - parsed = [(ln.split(" ", 1)[0], ln.split(" ", 1)[1]) for ln in run.stdout.splitlines()] - self.assertEqual("", pr_review.never_picked_up(parsed)) - def test_a_negative_pickup_grace_is_rejected_rather_than_read_as_every_poll(self) -> None: - """It leaves the next reading behind the clock, which is the per-poll pattern returning.""" + """Compatibility accepts old callers without accepting a nonsensical negative value.""" with contextlib.redirect_stderr(io.StringIO()) as err, self.assertRaises(SystemExit): pr_review.main(["wait", "7", "--repo", "o/r", "--pickup-grace", "-1"]) # The repository is named, or this exits on the missing argument and proves nothing. @@ -3178,17 +3054,6 @@ def test_an_inverted_pair_of_check_thresholds_is_rejected_at_the_flags_too(self) ) self.assertIn("check-grace", err.getvalue()) - def test_a_failed_timeline_read_raises_rather_than_reading_as_no_events(self) -> None: - """An empty list reads as no request pending, which is the false clean one level up.""" - failed = subprocess.CompletedProcess(args=[], returncode=1, stdout="", stderr="boom") - with ( - mock.patch.object(pr_review.subprocess, "run", return_value=failed), - contextlib.redirect_stderr(io.StringIO()) as err, - self.assertRaises(SystemExit), - ): - pr_review.timeline("o", "r", 7) - self.assertIn("boom", err.getvalue()) - def test_the_backoff_is_bounded_and_non_decreasing(self) -> None: """A wait that sleeps zero seconds is a busy loop, and one that shrinks polls harder later.""" source = (REPO / "scripts" / "pr_review.py").read_text(encoding="utf-8")