From 8ee1876092952a61bd0cb4bbc4a014c21626b41b Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sat, 17 Oct 2026 11:28:38 -0700 Subject: [PATCH] Use normal author-reply handoffs for Copilot feedback Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a92f342-8ad1-4f64-aefd-f92558fb8a66 --- .../pull-request-dashboard/RATIONALE.md | 62 ++- .../classification_policy.py | 15 +- .../pull-request-dashboard/copilot_review.py | 58 +-- .../copilot_review_delivery.py | 9 +- .../pull-request-dashboard/dashboard.py | 8 +- .../discussion_lifecycle.py | 10 +- .../pull_request_evaluation.py | 19 +- .../scripts/pull-request-dashboard/state.py | 2 +- .../test_classification_execution.py | 32 +- .../test_classification_policy.py | 18 +- .../test_copilot_review.py | 96 ++--- .../pull-request-dashboard/test_dashboard.py | 406 ++++++++++++------ .../test_discussion_lifecycle.py | 1 - .../pull-request-dashboard/test_state.py | 2 +- pull-request-dashboard/README.md | 8 +- 15 files changed, 382 insertions(+), 364 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index 6fc416561f12..eda130f50019 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -414,15 +414,14 @@ the implementation understandable and operationally cheap. check suite on a PR that was already with its reviewers from looking like a stalled handoff. A push clears the clock, because new code means new checks and a review that has to run again. -- Only a gate that has reported nothing on the current head runs the clock. A - Copilot review that covers the head but left open findings holds the PR, yet - it is not missing: it answered, and the threads it left are the author's to - clear. Because the clock is carried across a trip back to the author, counting - those findings would make every author who takes more than four hours over - review comments look like a gate GitHub lost, and the report would say a - review never arrived when it is sitting on the PR. A review that is missing or - that only covers older code still counts, because Copilot has said nothing - about the code being reviewed. +- Only a gate that has reported nothing on the current head runs the clock. + Copilot findings that still need author action hold the PR through discussion + routing, but the review is not missing: it answered. Because the clock is + carried across a trip back to the author, counting those findings would make + every author who takes more than four hours over review comments look like a + gate GitHub lost, and the report would say a review never arrived when it is + sitting on the PR. A review that is missing or that only covers older code + still counts, because Copilot has said nothing about the code being reviewed. - An expired hold routes the pull request and says so in its status comment, which is the whole of what the dashboard can do about it. The gates raise no alarm of their own. Each way a gate can go missing has its own cause and none @@ -446,26 +445,23 @@ the implementation understandable and operationally cheap. would hold every ready PR with its author waiting for a review that never runs, so only branches with automatic review are listed and PRs targeting other branches route normally. -- Every unresolved, non-outdated thread Copilot started is an author action, - even after an author reply or a later clean review. This ownership bypasses - generic praise and author-reply classification and ends only when the thread - is resolved or outdated. The gate therefore never needs its expiration path - to retain these findings. -- Findings are counted from unresolved, non-outdated review threads Copilot - started, not from the comment count on its review. A review's comment count - never shrinks, so it keeps counting feedback the author has since addressed - and holds the PR on work that is already done. -- The gate's re-request path covers two states. A stale review means the author - pushed, which is the one change a re-review can respond to. Findings on the - current head sit on unchanged code, so asking Copilot to look at it again - would reach the same verdict and be requested again on the next pass; those - threads stop counting only when the author resolves them or GitHub marks them - outdated after the referenced code moves. A later clean review does not close - an existing thread. A push makes the review stale and triggers a re-request, - but the push alone does not prove that a finding was fixed. -- The other state is a missing first review. The dashboard requests it on the - first eligible refresh using its GitHub App installation token. Initial reviews - and re-reviews share the same delivery checks and confirmation path. +- Copilot threads use the same discussion classification as human review + threads. A completed author reply hands the action to reviewers; a commitment + to more work keeps it with the author. Explicit resolution is not required + for that handoff because a reply explains the author's response without + requiring a separate administrative step. The thread remains visible as + unresolved until GitHub marks it resolved or outdated. +- The gate waits only for a missing or stale review. Counting open threads + independently would override discussion classification and keep completed + author replies assigned to the author. Actionable findings already keep the + PR with its author through ordinary discussion routing. +- A stale review means the author pushed, so a re-review can inspect new code. + Findings on the current head do not trigger re-review requests: another + review is not a substitute for responding to the finding. A later clean + review alone does not close an existing thread or change its pending action. +- The dashboard requests a missing first review on the first eligible refresh + using its GitHub App installation token. Initial reviews and re-reviews share + the same delivery checks and confirmation path. - Copilot's generic error notice is a failed attempt, not a completed review or delivery confirmation. It leaves the review missing or stale so the next eligible refresh can retry on the same head without changing the requester. @@ -474,15 +470,15 @@ the implementation understandable and operationally cheap. from GitHub reviews, not refreshes, and a new head starts a fresh budget. Delivery rechecks the live count so an error arriving after evaluation cannot cause another request. Pending requests still prevent duplicates. Required - checks and unresolved Copilot findings still apply; the status comment - explains when routing stops waiting after repeated errors. The four-hour - timeout remains a backstop for reviews that never report. + checks and Copilot findings needing author action still apply; the status + comment explains when routing stops waiting after repeated errors. The + four-hour timeout remains a backstop for reviews that never report. - GitHub's quota-exhausted notice is a failed attempt even though GitHub records it as a review of the current head. It does not count as a completed review or confirm delivery. Without a configured fallback token, it immediately releases the Copilot gate and disables automatic requests for that PR, including after later pushes. Required checks and actionable feedback still - apply, including unresolved, non-outdated Copilot threads. + apply, including Copilot threads that still need author action. - The optional `COPILOT_REVIEW_FALLBACK_TOKEN` changes the requester after quota exhaustion and keeps the Copilot gate active under its normal rules. The fallback records existing quota notice IDs before requesting, so only a new diff --git a/.github/scripts/pull-request-dashboard/classification_policy.py b/.github/scripts/pull-request-dashboard/classification_policy.py index 953cc27265a6..d2c2bfe4f4ac 100644 --- a/.github/scripts/pull-request-dashboard/classification_policy.py +++ b/.github/scripts/pull-request-dashboard/classification_policy.py @@ -412,7 +412,6 @@ class ClassificationDiscussion: pr_author: str = "" source_kind: str = "" candidate_feedback: tuple[CandidateFeedback, ...] = () - strict_author_action: bool = False selected_comment_index: int | None = None selected_activity_timestamp: str = "" ignored_comment_index: int | None = None @@ -465,7 +464,6 @@ def from_record(cls, record: Mapping[str, Any]) -> ClassificationDiscussion: pr_author=str(record.get("pr_author") or ""), source_kind=str(record.get("source_kind") or ""), candidate_feedback=candidate_feedback, - strict_author_action=bool(record.get("strict_author_action")), ) def with_comments( @@ -1701,7 +1699,7 @@ def prepare_praise_candidates( return tuple( discussion for discussion in discussions - if not discussion.strict_author_action and _could_be_praise(discussion) + if _could_be_praise(discussion) ) @@ -1781,16 +1779,7 @@ def resolve_review_thread_policy( selected.activity_timestamp or selected.comment.timestamp ) - if discussion.strict_author_action: - resolved[discussion_id] = ClassificationSuccess( - discussion.identity, - ActionDecision( - DiscussionAction.AUTHOR, - "This unresolved Copilot finding remains author work until " - "the thread is resolved or outdated.", - ), - ) - elif dropped and not comments: + if dropped and not comments: resolved[discussion_id] = ClassificationSuccess( discussion.identity, ActionDecision( diff --git a/.github/scripts/pull-request-dashboard/copilot_review.py b/.github/scripts/pull-request-dashboard/copilot_review.py index 0536ae0bd835..4147ffd9abce 100644 --- a/.github/scripts/pull-request-dashboard/copilot_review.py +++ b/.github/scripts/pull-request-dashboard/copilot_review.py @@ -17,7 +17,6 @@ Check, Review, ReviewRequest, - ReviewThread, normalize_review_requests, normalize_reviews, ) @@ -100,48 +99,11 @@ def copilot_reviews_disabled(facts: DashboardFacts) -> bool: ) -def open_copilot_findings( - review_threads: Sequence[ReviewThread], -) -> tuple[ReviewThread, ...]: - # A review's own comment count never shrinks, so it still counts findings - # the author has since addressed. Unresolved threads are the live ones: - # GitHub marks a thread outdated once its anchor lines change, which is how - # the rest of the dashboard already recognises a pushed fix. - return tuple( - thread - for thread in review_threads - if ( - not thread.is_resolved - and not thread.is_outdated - and thread.comments - and is_copilot_reviewer(thread.comments[0].actor) - ) - ) - - -def open_copilot_finding_count( - review_threads: Sequence[ReviewThread], -) -> int: - return len(open_copilot_findings(review_threads)) - - -def open_copilot_finding_urls( - review_threads: Sequence[ReviewThread], -) -> tuple[str, ...]: - urls: list[str] = [] - for thread in open_copilot_findings(review_threads): - url = thread.comments[0].url - if url and url not in urls: - urls.append(url) - return tuple(urls) - - def copilot_review_status( reviews: Sequence[Review], head_sha: str, - review_threads: Sequence[ReviewThread], -) -> tuple[bool, bool, bool]: - """Return (review exists, review is stale, review left open findings).""" +) -> tuple[bool, bool]: + """Return (review exists, review is stale).""" copilot_reviews = [ review for review in reviews @@ -150,16 +112,16 @@ def copilot_review_status( and not is_copilot_error(review) ] if not copilot_reviews: - return False, False, False + return False, False if not head_sha: # Without a head to compare against, staleness is unknowable, and # acting on incomplete data would hold the pull request on a guess. - return True, False, False + return True, False stale = not any( review.commit_id == head_sha for review in copilot_reviews ) - return True, stale, open_copilot_finding_count(review_threads) > 0 + return True, stale def copilot_review_outstanding(facts: DashboardFacts, *, enabled: bool) -> bool: @@ -170,10 +132,9 @@ def copilot_review_outstanding(facts: DashboardFacts, *, enabled: bool) -> bool: def copilot_review_unreported(facts: DashboardFacts, *, enabled: bool) -> bool: # Whether the gate is still waiting for Copilot to say anything about the - # current head. Findings are an answer, not a silence: the threads they - # leave are the author's to clear, and the dashboard already routes the - # pull request to the author for them. Only a review that is missing or - # that covers older code is a report that has not arrived. + # current head. Findings are an answer, not a silence: actionable threads + # belong to discussion routing. Only a review that is missing or that + # covers older code is a report that has not arrived. if not enabled or copilot_reviews_disabled(facts): return False return not facts.copilot_review_exists or facts.copilot_review_stale @@ -269,9 +230,8 @@ def copilot_review_request_landed( # Copilot can finish a short review before the last read, which takes it # back out of the pending requests. A review of the current head proves the # request landed just as well as a pending one does. - review_exists, review_stale, _findings = copilot_review_status( + review_exists, review_stale = copilot_review_status( normalize_reviews(fetch_pr_reviews(owner, repo_name, pr_number)), head_sha, - [], ) return review_exists and not review_stale diff --git a/.github/scripts/pull-request-dashboard/copilot_review_delivery.py b/.github/scripts/pull-request-dashboard/copilot_review_delivery.py index ceea406523cc..817ca90f2ee8 100644 --- a/.github/scripts/pull-request-dashboard/copilot_review_delivery.py +++ b/.github/scripts/pull-request-dashboard/copilot_review_delivery.py @@ -149,12 +149,9 @@ def deliver_copilot_review_requests( ) requests.pop(key, None) continue - review_exists, review_stale, _review_findings = ( - copilot_review_status( - reviews, - snapshot.head_sha, - snapshot.review_threads, - ) + review_exists, review_stale = copilot_review_status( + reviews, + snapshot.head_sha, ) if review_exists and not review_stale: print( diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index 287c284bbeb8..accf6b709de4 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -163,11 +163,9 @@ reviewed; eligible PRs without a review need an initial review request. - copilot_review_needed bool The review is stale or Copilot - owns open review threads, - meaning unresolved threads - GitHub has not marked - outdated. + copilot_review_needed bool The review is stale. Findings + use normal discussion + classification and routing. created_at str (iso) last_activity_at str (iso) Latest substantive activity by a PR participant, never earlier diff --git a/.github/scripts/pull-request-dashboard/discussion_lifecycle.py b/.github/scripts/pull-request-dashboard/discussion_lifecycle.py index 23d669f91825..2e9d503eaa51 100644 --- a/.github/scripts/pull-request-dashboard/discussion_lifecycle.py +++ b/.github/scripts/pull-request-dashboard/discussion_lifecycle.py @@ -210,10 +210,6 @@ def _group_review_threads(source: DiscussionInput) -> list[dict[str, Any]]: or f"review-discussion-{len(discussions) + 1}" ), "discussion_kind": "review-comment-thread", - "strict_author_action": bool( - root_comment - and root_comment.actor.is_copilot_reviewer - ), "path": discussion.path or None, "line": discussion.line, "resolved": False, @@ -732,11 +728,7 @@ def _review_thread_pending_actions( ) discussion_id = classification.identity.discussion_id discussion = by_id.get(discussion_id) - action = ( - DiscussionAction.AUTHOR - if (discussion or {}).get("strict_author_action") - else decision.action - ) + action = decision.action comments = (discussion or {}).get("comments") or [] if action is not DiscussionAction.NONE and comments: entry = { diff --git a/.github/scripts/pull-request-dashboard/pull_request_evaluation.py b/.github/scripts/pull-request-dashboard/pull_request_evaluation.py index ff6eb7e254f8..3b9dff78a66d 100644 --- a/.github/scripts/pull-request-dashboard/pull_request_evaluation.py +++ b/.github/scripts/pull-request-dashboard/pull_request_evaluation.py @@ -26,7 +26,6 @@ copilot_review_status, is_copilot_quota_failure, is_copilot_reviewer, - open_copilot_finding_urls, ) from dashboard_contracts import ( CopilotReviewFallback, @@ -228,14 +227,9 @@ def _compute_facts( ) api_author = pr.author.login head_sha = snapshot.head_sha - ( - copilot_review_exists, - copilot_review_stale, - copilot_review_findings, - ) = copilot_review_status( + copilot_review_exists, copilot_review_stale = copilot_review_status( source.reviews, head_sha, - snapshot.review_threads, ) override = dashboard_override_facts( DashboardOverrideInput(source.issue_comments), @@ -284,7 +278,7 @@ def _compute_facts( copilot_review_error_count=copilot_review_error_count(source.reviews, head_sha), copilot_review_exists=copilot_review_exists, copilot_review_stale=copilot_review_stale, - copilot_review_needed=copilot_review_stale or copilot_review_findings, + copilot_review_needed=copilot_review_stale, is_maintenance_bot=_is_maintenance_bot_author(api_author), author_can_act=_author_can_act(pr.author, author), is_draft=pr.is_draft, @@ -317,7 +311,6 @@ def _compute_facts( def _author_action_discussion_urls( discussions: list[dict[str, Any]], pending_actions: dict[str, dict[str, Any]], - additional_urls: tuple[str, ...] = (), ) -> tuple[str, ...]: by_id = { discussion["discussion_id"]: discussion for discussion in discussions @@ -331,9 +324,6 @@ def _author_action_discussion_urls( url = (discussion or {}).get("discussion_url") or "" if url and url not in urls: urls.append(url) - for url in additional_urls: - if url and url not in urls: - urls.append(url) return tuple(urls) @@ -632,11 +622,6 @@ async def evaluate_pull_request( _author_action_discussion_urls( review_threads, pending_actions, - ( - open_copilot_finding_urls(pr_source.review_threads) - if facts.copilot_review_outstanding - else () - ), ) ), author_action_top_level_feedback_urls=( diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index a04856ec6291..c5a6a920eb24 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -58,7 +58,7 @@ STATUS_COMMENT_ROLLOUT_STATE_VERSION = 3 # Rendered status-comment behavior. Increment when existing comments need to # adopt a change; hourly runs durably roll it out to all open PRs. -STATUS_COMMENT_REVISION = 21 +STATUS_COMMENT_REVISION = 22 FULL_PUBLISH_STATE_VERSION = 1 INITIAL_BACKFILL_COMPLETE_KEY = "initial_backfill_complete" _state_dir: Path | None = None diff --git a/.github/scripts/pull-request-dashboard/test_classification_execution.py b/.github/scripts/pull-request-dashboard/test_classification_execution.py index 6eb02b785bb9..4ea9221a1500 100644 --- a/.github/scripts/pull-request-dashboard/test_classification_execution.py +++ b/.github/scripts/pull-request-dashboard/test_classification_execution.py @@ -1297,19 +1297,35 @@ async def test_long_reviewer_request_needs_no_model(self) -> None: assert isinstance(result.decision, ActionDecision) self.assertIs(result.decision.action, DiscussionAction.AUTHOR) - async def test_unresolved_copilot_finding_does_not_use_reply_classifier(self) -> None: - thread = self.thread( + async def test_copilot_finding_uses_reply_classifier(self) -> None: + for reply, verdict, action in ( + ("Fixed it.", "complete", DiscussionAction.REVIEWER), + ("I'll fix it.", "deferral", DiscussionAction.AUTHOR), + ): + with self.subTest(verdict=verdict): + result, runner = await self.classify(self.thread( + ("bot", "Please fix this.", "2026-03-12T00:00:00Z"), + ("author", reply, "2026-05-20T00:00:00Z"), + ), responder=lambda request: successful_response( + request, author_reply=verdict, + )) + + self.assertEqual(len(runner.requests), 1) + assert isinstance(result.decision, ActionDecision) + self.assertIs(result.decision.action, action) + self.assertEqual(result.since, "2026-05-20T00:00:00Z") + + async def test_copilot_reply_failure_does_not_complete_the_handoff(self) -> None: + result, runner = await self.classify(self.thread( ("bot", "Please fix this.", "2026-03-12T00:00:00Z"), ("author", "Fixed it.", "2026-05-20T00:00:00Z"), - ) - thread["strict_author_action"] = True + ), responses=(RuntimeError("model unavailable"),)) - result, runner = await self.classify(thread) - - self.assertEqual(runner.requests, []) + self.assertEqual(len(runner.requests), 1) + self.assertIsInstance(result, ClassificationFailure) + self.assertTrue(result.failed) assert isinstance(result.decision, ActionDecision) self.assertIs(result.decision.action, DiscussionAction.AUTHOR) - self.assertEqual(result.since, "2026-05-20T00:00:00Z") async def test_praise_keeps_the_previous_request_and_wait_age(self) -> None: result, runner = await self.classify(self.thread( diff --git a/.github/scripts/pull-request-dashboard/test_classification_policy.py b/.github/scripts/pull-request-dashboard/test_classification_policy.py index cc8efd397903..f79ff3ee2a2b 100644 --- a/.github/scripts/pull-request-dashboard/test_classification_policy.py +++ b/.github/scripts/pull-request-dashboard/test_classification_policy.py @@ -57,7 +57,6 @@ def discussion( requester: str = "", pr_author: str = "", candidate_feedback: tuple[tuple[str, str], ...] = (), - strict_author_action: bool = False, ) -> ClassificationDiscussion: return ClassificationDiscussion( DiscussionIdentity(discussion_id, kind), @@ -74,7 +73,6 @@ def discussion( CandidateFeedback(feedback_id, feedback_body) for feedback_id, feedback_body in candidate_feedback ), - strict_author_action=strict_author_action, ) @@ -746,7 +744,7 @@ def test_long_reviewer_comment_needs_no_model_request(self) -> None: assert isinstance(result.decision, ActionDecision) self.assertEqual(result.decision.action, DiscussionAction.AUTHOR) - def test_unresolved_copilot_finding_bypasses_author_reply_policy(self) -> None: + def test_copilot_finding_uses_author_reply_policy(self) -> None: thread = ClassificationDiscussion( DiscussionIdentity("thread-1", DiscussionKind.REVIEW_THREAD), ( @@ -761,18 +759,18 @@ def test_unresolved_copilot_finding_bypasses_author_reply_policy(self) -> None: "Done.", ), ), - strict_author_action=True, ) self.assertEqual(prepare_praise_candidates([thread]), ()) plan = resolve_review_thread_policy([thread], {}) - self.assertEqual(plan.author_replies, ()) - result = plan.resolved[0] - self.assertIsInstance(result, ClassificationSuccess) - assert isinstance(result.decision, ActionDecision) - self.assertEqual(result.decision.action, DiscussionAction.AUTHOR) - self.assertEqual(result.since, "2026-01-02T04:05:06Z") + self.assertEqual(plan.resolved, ()) + self.assertEqual(len(plan.author_replies), 1) + self.assertEqual(plan.author_replies[0].comments, thread.comments) + self.assertEqual( + plan.author_replies[0].selected_activity_timestamp, + "2026-01-02T04:05:06Z", + ) def test_praise_and_author_reply_shortcuts_preserve_handoffs(self) -> None: thread = discussion( diff --git a/.github/scripts/pull-request-dashboard/test_copilot_review.py b/.github/scripts/pull-request-dashboard/test_copilot_review.py index b4bb9d76addd..000d4233f724 100644 --- a/.github/scripts/pull-request-dashboard/test_copilot_review.py +++ b/.github/scripts/pull-request-dashboard/test_copilot_review.py @@ -20,7 +20,6 @@ copilot_review_status, is_copilot_error, is_copilot_quota_failure, - open_copilot_finding_urls, set_copilot_review_request_needed, stale_request_reason, ) @@ -32,8 +31,6 @@ actor, dashboard_facts, review_source, - review_thread, - review_thread_comment, stored_dashboard_result, ) from dashboard_contracts import CopilotReviewFallback @@ -86,57 +83,22 @@ def review_result(route: str = "approver", **fact_changes): ) -class CopilotFindingLifecycleTest(unittest.TestCase): - def test_clean_current_review_keeps_open_findings(self) -> None: - reviews = (review_source( - actor=actor("copilot"), - commit_id="current-head", - finding_count=0, - submitted_at="2026-07-20T02:00:00Z", - ),) - threads = ( - review_thread( - node_id="open", - comments=(review_thread_comment( - url="https://example.test/open", - actor=actor("copilot"), - created_at="2026-07-20T01:00:00Z", - updated_at="2026-07-20T01:00:00Z", - ),), - ), - review_thread( - node_id="resolved", - is_resolved=True, - comments=(review_thread_comment( - url="https://example.test/resolved", - actor=actor("copilot"), - created_at="2026-07-20T01:00:00Z", - updated_at="2026-07-20T01:00:00Z", - ),), - ), - review_thread( - node_id="outdated", - is_outdated=True, - comments=(review_thread_comment( - url="https://example.test/outdated", - actor=actor("copilot"), - created_at="2026-07-20T01:00:00Z", - updated_at="2026-07-20T01:00:00Z", - ),), - ), - ) - - self.assertEqual( - (True, False, True), - copilot_review_status(reviews, "current-head", threads), - ) - self.assertEqual( - ("https://example.test/open",), - open_copilot_finding_urls(threads), - ) - - class CopilotReviewStatusTest(unittest.TestCase): + def test_current_review_is_reported_regardless_of_finding_count(self) -> None: + for finding_count in (0, 1): + with self.subTest(finding_count=finding_count): + self.assertEqual( + (True, False), + copilot_review_status( + (review_source( + actor=actor("copilot"), + commit_id="current-head", + finding_count=finding_count, + ),), + "current-head", + ), + ) + def test_error_count_requires_current_head_copilot_notices(self) -> None: failure = review_source( actor=actor("copilot-pull-request-reviewer[bot]"), @@ -178,38 +140,38 @@ def test_error_notices_do_not_count_as_completed_reviews(self) -> None: completed = replace(failure, body="No issues found.") self.assertFalse(is_copilot_quota_failure(failure)) cases = ( - ("failure_only", (failure,), (False, False, False)), + ("failure_only", (failure,), (False, False)), ( "whitespace", (replace(failure, body=f"\n{ERROR_BODY}\n"),), - (False, False, False), + (False, False), ), ( "current_review_then_failure", (completed, failure), - (True, False, False), + (True, False), ), ( "failure_then_current_review", (failure, completed), - (True, False, False), + (True, False), ), ( "stale_review_then_failure", (replace(completed, commit_id="old-head"), failure), - (True, True, False), + (True, True), ), ( "review_discussing_errors", (replace(completed, body=f"Handle this failure:\n> {ERROR_BODY}"),), - (True, False, False), + (True, False), ), ) for name, reviews, expected in cases: with self.subTest(name=name): self.assertEqual( expected, - copilot_review_status(reviews, "current-head", ()), + copilot_review_status(reviews, "current-head"), ) @patch("copilot_review.sleep_for_retry") @@ -258,26 +220,26 @@ def test_quota_failures_do_not_count_as_completed_reviews(self) -> None: body="No issues found.", ) cases = ( - ("failure_only", (quota_failure,), (False, False, False)), + ("failure_only", (quota_failure,), (False, False)), ( "whitespace", (replace(quota_failure, body=f"\n{QUOTA_FAILURE_BODY}\n"),), - (False, False, False), + (False, False), ), ( "current_review_then_failure", (completed_review, quota_failure), - (True, False, False), + (True, False), ), ( "failure_then_current_review", (quota_failure, completed_review), - (True, False, False), + (True, False), ), ( "stale_review_then_failure", (replace(completed_review, commit_id="old-head"), quota_failure), - (True, True, False), + (True, True), ), ( "review_discussing_quotas", @@ -285,14 +247,14 @@ def test_quota_failures_do_not_count_as_completed_reviews(self) -> None: completed_review, body=f"Handle this failure:\n> {QUOTA_FAILURE_BODY}", ),), - (True, False, False), + (True, False), ), ) for name, reviews, expected in cases: with self.subTest(name=name): self.assertEqual( expected, - copilot_review_status(reviews, "current-head", ()), + copilot_review_status(reviews, "current-head"), ) diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index 69b406bff49a..12cc074d2028 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -920,162 +920,289 @@ async def test_copilot_pr_author_request_is_not_a_pending_review( ) @patch("pull_request_evaluation.fetch_pull_request_source") - async def test_unresolved_copilot_thread_with_completed_reply_remains_author_action( + async def test_completed_copilot_reply_resumes_approval_routing( + self, + fetch_raw: Mock, + ) -> None: + for gated in (False, True): + for approved in (False, True): + with self.subTest(gated=gated, approved=approved): + fetch_raw.return_value = pull_request_source( + reviews=( + review_source( + actor=actor("copilot"), + commit_id="abcdef123456", + finding_count=1, + ), + *( + (review_source( + actor=actor("reviewer"), + state="APPROVED", + ),) + if approved else () + ), + ), + review_threads=(review_thread( + comments=( + review_thread_comment( + actor=actor("copilot"), + body="Please add a regression test.", + ), + review_thread_comment( + node_id="PRRC_2", + body="Added the regression test.", + created_at="2026-08-16T07:30:00Z", + actor=actor("author"), + ), + ), + ),), + ) + runner = FakeModelRunner(responder=successful_response) + + result = await evaluate_pull_request( + evaluation_config( + require_clean_copilot_review_branches=( + ["main"] if gated else [] + ), + ), + PullRequestEvaluationInput(7), + ClassificationService( + runner, + MemoryClassificationCacheStore(), + ), + ) + + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual( + DashboardRoute.MAINTAINER + if approved else DashboardRoute.APPROVER, + result.route, + ) + self.assertEqual( + "reviewer", result.pending_actions["PRRT_1"]["action"], + ) + self.assertEqual((), result.facts.author_action_review_thread_urls) + self.assertFalse(result.facts.copilot_review_outstanding) + self.assertFalse(result.facts.copilot_review_request_needed) + self.assertEqual(len(runner.requests), 1) + self.assertIn( + ReviewerSummary( + login="copilot-pull-request-reviewer[bot]", + unresolved_thread=True, + ), + result.facts.reviewers, + ) + + @patch("pull_request_evaluation.fetch_pull_request_source") + async def test_copilot_findings_keep_author_work_with_the_author( self, fetch_raw: Mock, ) -> None: thread_url = "https://example.test/review-comment/copilot" - fetch_raw.return_value = pull_request_source( - pull_request=pull_request_metadata(), - reviews=( - review_source( - actor=actor("copilot"), - commit_id="old-head", - finding_count=1, - submitted_at="2026-08-16T07:00:00Z", - ), - review_source( - actor=actor("copilot"), - commit_id="abcdef123456", - finding_count=0, - submitted_at="2026-08-16T08:00:00Z", - ), - ), - review_threads=(review_thread( - comments=( - review_thread_comment( + for reply in ("", "I'll add a regression test."): + for review_state in ("completed", "quota", "errors"): + with self.subTest(reply=reply, review_state=review_state): + reviews = (review_source( + actor=actor("copilot"), + commit_id="abcdef123456", + ),) + if review_state == "quota": + reviews += (review_source( + actor=actor("copilot"), + body=( + "Copilot was unable to review this pull request " + "because the user who requested the review has " + "reached their quota limit." + ), + ),) + elif review_state == "errors": + reviews = tuple( + review_source( + database_id=review_id, + actor=actor("copilot"), + commit_id="abcdef123456", + body=( + "Copilot encountered an error and was unable " + "to review this pull request. You can try " + "again by re-requesting a review." + ), + ) + for review_id in (10, 11) + ) + comments = (review_thread_comment( url=thread_url, actor=actor("copilot"), + body="Please add a regression test.", + ),) + if reply: + comments += (review_thread_comment( + node_id="PRRC_2", + body=reply, + created_at="2026-08-16T07:30:00Z", + actor=actor("author"), + ),) + fetch_raw.return_value = pull_request_source( + reviews=reviews, + review_threads=(review_thread(comments=comments),), + ) + + result = await evaluate_pull_request( + evaluation_config( + require_clean_copilot_review_branches=["main"], + ), + PullRequestEvaluationInput(7), + ClassificationService( + FakeModelRunner(responder=lambda request: successful_response( + request, author_reply="deferral", + )), + MemoryClassificationCacheStore(), + ), + ) + + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual(DashboardRoute.AUTHOR, result.route) + self.assertEqual( + "author", result.pending_actions["PRRT_1"]["action"], + ) + self.assertEqual( + (thread_url,), result.facts.author_action_review_thread_urls, + ) + self.assertFalse(result.facts.copilot_review_request_needed) + if review_state != "completed": + self.assertFalse(result.facts.copilot_review_outstanding) + + @patch("pull_request_evaluation.fetch_pull_request_source") + async def test_new_feedback_on_answered_copilot_thread_returns_to_author( + self, + fetch_raw: Mock, + ) -> None: + for reviewer_login, edited in ( + ("copilot", False), + ("reviewer", False), + ("copilot", True), + ): + with self.subTest(reviewer_login=reviewer_login, edited=edited): + root_comment = review_thread_comment(actor=actor("copilot")) + author_reply = review_thread_comment( + node_id="PRRC_2", + actor=actor("author"), + body="Added the regression test.", + created_at="2026-08-16T07:30:00Z", + ) + thread = review_thread(comments=(root_comment, author_reply)) + source = pull_request_source( + reviews=(review_source( + actor=actor("copilot"), + commit_id="abcdef123456", + ),), + review_threads=(thread,), + ) + fetch_raw.return_value = source + classifier = ClassificationService( + FakeModelRunner(responder=successful_response), + MemoryClassificationCacheStore(), + ) + config = evaluation_config( + require_clean_copilot_review_branches=["main"], + ) + + completed = await evaluate_pull_request( + config, PullRequestEvaluationInput(7), classifier, + ) + + self.assertIsInstance(completed, EvaluationSuccess) + assert isinstance(completed, EvaluationSuccess) + self.assertEqual(DashboardRoute.APPROVER, completed.route) + + feedback = ( + "The regression test still inherits the startup context. " + "Please send a message without a parent and assert that " + "it starts a separate trace." + ) + if edited: + comments = ( + replace( + root_comment, + body=feedback, + updated_at="2026-08-16T08:00:00Z", + ), + author_reply, + ) + else: + comments = ( + *thread.comments, + review_thread_comment( + node_id="PRRC_3", + actor=actor(reviewer_login), + body=feedback, + created_at="2026-08-16T08:00:00Z", + ), + ) + fetch_raw.return_value = replace( + source, + review_threads=(replace(thread, comments=comments),), + ) + + result = await evaluate_pull_request( + config, + PullRequestEvaluationInput( + 7, StoredDashboardResult.from_evaluation(completed), ), + classifier, + ) + + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual(DashboardRoute.AUTHOR, result.route) + self.assertEqual( + "author", result.pending_actions["PRRT_1"]["action"], + ) + self.assertEqual( + (root_comment.url,), result.facts.author_action_review_thread_urls, + ) + + @patch("pull_request_evaluation.fetch_pull_request_source") + async def test_completed_copilot_reply_does_not_bypass_stale_review_gate( + self, + fetch_raw: Mock, + ) -> None: + fetch_raw.return_value = pull_request_source( + reviews=(review_source( + actor=actor("copilot"), + commit_id="old-head", + ),), + review_threads=(review_thread( + comments=( + review_thread_comment(actor=actor("copilot")), review_thread_comment( node_id="PRRC_2", - url="https://example.test/review-comment/author", - body="Handled in a follow-up.", + body="Added the regression test.", created_at="2026-08-16T07:30:00Z", actor=actor("author"), ), ), ),), ) - classifier = FakeClassificationOperation( - DiscussionClassifications( - ( - action_classification( - "PRRT_1", - DiscussionKind.REVIEW_THREAD, - DiscussionAction.NONE, - "The author replied.", - ), - ), - (), - (), - ) - ) - result = await evaluate_pr( - {"number": 7}, - require_clean_copilot_review_branches=["main"], - classification_service=classifier, + result = await evaluate_pull_request( + evaluation_config(require_clean_copilot_review_branches=["main"]), + PullRequestEvaluationInput(7), + ClassificationService( + FakeModelRunner(responder=successful_response), + MemoryClassificationCacheStore(), + ), ) self.assertIsInstance(result, EvaluationSuccess) assert isinstance(result, EvaluationSuccess) self.assertEqual(DashboardRoute.AUTHOR, result.route) - self.assertEqual( - (thread_url,), - result.facts.author_action_review_thread_urls, - ) - - ungated_result = await evaluate_pr( - {"number": 7}, - classification_service=classifier, - ) - - self.assertIsInstance(ungated_result, EvaluationSuccess) - assert isinstance(ungated_result, EvaluationSuccess) - self.assertEqual(DashboardRoute.AUTHOR, ungated_result.route) - self.assertEqual( - (thread_url,), - ungated_result.facts.author_action_review_thread_urls, - ) - self.assertEqual( - { - "PRRT_1": { - "action": "author", - "since": "2026-08-16T07:30:00Z", - }, - }, - ungated_result.pending_actions, - ) - self.assertIn( - ReviewerSummary( - login="copilot-pull-request-reviewer[bot]", - open_thread=True, - unresolved_thread=True, - ), - ungated_result.facts.reviewers, - ) - - fetch_raw.return_value = replace( - fetch_raw.return_value, - reviews=( - *fetch_raw.return_value.reviews, - review_source( - actor=actor("copilot"), - body=( - "Copilot was unable to review this pull request because " - "the user who requested the review has reached their quota limit." - ), - ), - ), - ) - quota_result = await evaluate_pr( - {"number": 7}, - require_clean_copilot_review_branches=["main"], - classification_service=classifier, - ) - - self.assertIsInstance(quota_result, EvaluationSuccess) - assert isinstance(quota_result, EvaluationSuccess) - self.assertTrue(quota_result.facts.copilot_review_quota_exhausted) - self.assertFalse(quota_result.facts.copilot_review_outstanding) - self.assertEqual(DashboardRoute.AUTHOR, quota_result.route) - self.assertEqual( - (thread_url,), - quota_result.facts.author_action_review_thread_urls, - ) - - fetch_raw.return_value = replace( - fetch_raw.return_value, - reviews=tuple( - review_source( - database_id=review_id, - actor=actor("copilot"), - commit_id="abcdef123456", - body=( - "Copilot encountered an error and was unable to review this " - "pull request. You can try again by re-requesting a review." - ), - ) - for review_id in (10, 11) - ), - ) - error_result = await evaluate_pr( - {"number": 7}, - require_clean_copilot_review_branches=["main"], - classification_service=classifier, - ) - - self.assertIsInstance(error_result, EvaluationSuccess) - assert isinstance(error_result, EvaluationSuccess) - self.assertEqual(2, error_result.facts.copilot_review_error_count) - self.assertFalse(error_result.facts.copilot_review_outstanding) - self.assertFalse(error_result.facts.copilot_review_request_needed) - self.assertEqual(DashboardRoute.AUTHOR, error_result.route) - self.assertEqual( - (thread_url,), - error_result.facts.author_action_review_thread_urls, - ) + self.assertEqual("reviewer", result.pending_actions["PRRT_1"]["action"]) + self.assertTrue(result.facts.copilot_review_stale) + self.assertTrue(result.facts.copilot_review_outstanding) + self.assertTrue(result.facts.copilot_review_request_needed) + self.assertEqual((), result.facts.author_action_review_thread_urls) @patch("pull_request_evaluation.fetch_pull_request_source") async def test_resolved_or_outdated_copilot_thread_releases_author_hold( @@ -2513,7 +2640,6 @@ def test_author_action_urls_use_thread_url_and_deduplicate(self) -> None: _author_action_discussion_urls( discussions, pending_actions, - ("https://example.test/discussion/1",), ), ) @@ -2567,7 +2693,7 @@ def test_current_head_matches_latest_clean_copilot_review(self) -> None: self.assertTrue(facts.copilot_review_exists) self.assertFalse(facts.copilot_review_needed) - def test_current_clean_review_keeps_older_open_copilot_findings( + def test_current_review_leaves_older_copilot_findings_to_discussion_routing( self, ) -> None: facts = evaluation_facts( @@ -2619,7 +2745,7 @@ def test_current_clean_review_keeps_older_open_copilot_findings( self.assertTrue(facts.copilot_review_exists) self.assertFalse(facts.copilot_review_stale) - self.assertTrue(facts.copilot_review_needed) + self.assertFalse(facts.copilot_review_needed) def test_late_stale_review_does_not_replace_clean_current_head_review(self) -> None: facts = evaluation_facts( @@ -2691,7 +2817,7 @@ def test_push_since_latest_clean_copilot_review_needs_rereview(self) -> None: self.assertFalse(facts.copilot_review_requested) self.assertTrue(facts.copilot_review_needed) - def test_unresolved_copilot_thread_on_current_head_needs_rereview(self) -> None: + def test_unresolved_copilot_thread_on_current_head_needs_no_rereview(self) -> None: facts = evaluation_facts( { "pr": { @@ -2746,7 +2872,7 @@ def test_unresolved_copilot_thread_on_current_head_needs_rereview(self) -> None: ) self.assertFalse(facts.copilot_review_stale) - self.assertTrue(facts.copilot_review_needed) + self.assertFalse(facts.copilot_review_needed) def test_resolved_copilot_findings_on_current_head_are_clean(self) -> None: # A review's comment count never shrinks, so counting it would hold the diff --git a/.github/scripts/pull-request-dashboard/test_discussion_lifecycle.py b/.github/scripts/pull-request-dashboard/test_discussion_lifecycle.py index 5e8d7de46b51..8cade3736580 100644 --- a/.github/scripts/pull-request-dashboard/test_discussion_lifecycle.py +++ b/.github/scripts/pull-request-dashboard/test_discussion_lifecycle.py @@ -210,7 +210,6 @@ def test_prepares_threads_feedback_and_author_replies(self) -> None: prepared.review_threads[0]["discussion_url"], "https://example.test/thread/root", ) - self.assertTrue(prepared.review_threads[0]["strict_author_action"]) self.assertEqual( [item["discussion_id"] for item in prepared.top_level_items], ["pr-issue-comment-201", "pr-issue-comment-202"], diff --git a/.github/scripts/pull-request-dashboard/test_state.py b/.github/scripts/pull-request-dashboard/test_state.py index 729cca9d3079..cb26686da54d 100644 --- a/.github/scripts/pull-request-dashboard/test_state.py +++ b/.github/scripts/pull-request-dashboard/test_state.py @@ -908,7 +908,7 @@ def test_notification_state_version_is_independent(self) -> None: self.assertEqual(DASHBOARD_STATE_VERSION, 19) self.assertEqual(DASHBOARD_STATE_COMPATIBLE_VERSIONS, ()) self.assertEqual(STATUS_COMMENT_ROLLOUT_STATE_VERSION, 3) - self.assertEqual(STATUS_COMMENT_REVISION, 21) + self.assertEqual(STATUS_COMMENT_REVISION, 22) self.assertEqual(AUTHOR_NUDGE_STATE_VERSION, 4) self.assertEqual(COPILOT_REVIEW_REQUEST_STATE_VERSION, 8) diff --git a/pull-request-dashboard/README.md b/pull-request-dashboard/README.md index 3ec680fdd226..a7fcc0ee7af5 100644 --- a/pull-request-dashboard/README.md +++ b/pull-request-dashboard/README.md @@ -167,7 +167,7 @@ Fields: | `required_approvals` | no | Number of active approvals required for an open PR to be marked ready to merge. An approval stops counting while that reviewer has a pending re-review request. Defaults to `1`. | | `labels_to_display` | no | Case-sensitive shell-style label name patterns to display inline after PR titles. Exact names such as `breaking change` and wildcard patterns such as `size/*` are supported. Defaults to `[]`, which displays no labels. | | `non_blocking_check_patterns` | no | Check-name globs for non-required checks whose failures should be identified in the live PR status comment. When the PR is waiting on the author, matching failures are reported only when at least one required check is failing and are noted alongside those failures. On other routes, matching failures are shown separately. Matching checks remain informational and do not affect routing or the dashboard CI column. | -| `require_clean_copilot_review_branches` | no | List of base branch names for which a Copilot review of the current head with no open Copilot review threads is normally required before automatically routing a PR to reviewers or maintainers. An effective `/dashboard route:reviewers` break-glass override bypasses this gate and every other routing blocker for the current head. A thread counts as open while it is unresolved and GitHub has not marked it outdated, so a thread whose code the author has since rewritten stops holding the PR even if nobody resolved it. The live PR status links every open Copilot thread until it is resolved or outdated, including threads where the author has already replied. The dashboard requests the first review on the first eligible refresh and re-requests review when a push has left the previous review stale. It does not duplicate a pending request. GitHub's quota-exhausted review notice does not count as a completed review, but it immediately releases the Copilot gate when no fallback token is configured or the fallback requester is exhausted; see the quota behavior below. A request counts as delivered only once GitHub confirms Copilot is a pending reviewer or finds a completed Copilot review of the current head. A gate that never reports holds the PR for at most four hours; after that the PR routes anyway, its status comment says which gate the dashboard stopped waiting for, and the run reports the stall. A conflict resets this clock because GitHub may not start checks or reviews until the conflict is resolved. PRs targeting unlisted branches are never gated. Defaults to `[]` (no branches gated). | +| `require_clean_copilot_review_branches` | no | List of base branch names for which a Copilot review of the current head is normally required before automatically routing a PR to reviewers or maintainers. Copilot findings use the same discussion rules as human feedback: a completed author reply resumes approval-based routing without requiring explicit thread resolution, while a commitment to more work keeps the action with the author. The live PR status links only threads needing author action. An effective `/dashboard route:reviewers` break-glass override bypasses this gate and every other routing blocker. The dashboard requests the first review on the first eligible refresh and re-requests review when a push has left the previous review stale. It does not duplicate a pending request. GitHub's quota-exhausted review notice does not count as a completed review, but it immediately releases the Copilot gate when no fallback token is configured or the fallback requester is exhausted; see the quota behavior below. A request counts as delivered only once GitHub confirms Copilot is a pending reviewer or finds a completed Copilot review of the current head. A gate that never reports holds the PR for at most four hours; after that the PR routes anyway, its status comment says which gate the dashboard stopped waiting for, and the run reports the stall. A conflict resets this clock because GitHub may not start checks or reviews until the conflict is resolved. PRs targeting unlisted branches are never gated. Defaults to `[]` (no branches gated). | | `slack_channel` | no | Slack channel for notifications. Omit to skip Slack processing for this repository. | | `slack_user_mapping` | no | Map of GitHub login to Slack user ID for at-mentions. | | `large_repo` | no | If `true`, apply rendering presets that keep the dashboard body under GitHub's 65,536-character issue-body limit: cap each section (each *Waiting on …* table and the *Draft pull requests* table) at 100 rows, and omit the *Draft pull requests* section entirely. Truncated sections get a `_More X PRs not shown_` footer. Defaults to `false` (no cap, drafts shown). Enable this for very large repos with hundreds of PRs. | @@ -189,7 +189,7 @@ backstop for a missed refresh. There is no additional retry delay. After two error notices for the same commit, automatic requests stop and the Copilot gate releases immediately. The status comment explains why routing no -longer waits for Copilot. Required checks and unresolved Copilot findings still +longer waits for Copilot. Required checks and findings needing author action still apply. A new commit resets the error budget. The normal four-hour gate timeout remains the backstop for reviews that never report. @@ -197,7 +197,7 @@ When Copilot reports quota exhaustion and no fallback token is configured, the dashboard immediately stops automatic review requests for that PR and releases the Copilot review gate without waiting for a completed review or the four-hour timeout. Only the Copilot gate is released: required-check gates and actionable -feedback, including unresolved, non-outdated Copilot threads, still apply. +feedback, including Copilot threads needing author action, still apply. Maintainers can optionally configure the `COPILOT_REVIEW_FALLBACK_TOKEN` workflow secret with a token allowed to request Copilot reviews. The owned drain @@ -227,7 +227,7 @@ summary that is not attached to an inline review thread. maintainers without reviewer confirmation; otherwise it waits on reviewers. A reply that commits to more work keeps the PR with the author. Author-only inline threads are treated as annotations rather than review feedback unless a - non-author joins them. + non-author joins them. These rules apply equally to human and Copilot feedback. - Top-level feedback has no resolved state. The dashboard therefore tracks each actionable top-level feedback item independently. 📌 means that one or more of those items are waiting on the author.