Skip to content

Commit c9e6a93

Browse files
authored
Use normal author-reply handoffs for Copilot feedback (#482)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a92f342-8ad1-4f64-aefd-f92558fb8a66
1 parent 6d6abec commit c9e6a93

15 files changed

Lines changed: 382 additions & 364 deletions

‎.github/scripts/pull-request-dashboard/RATIONALE.md‎

Lines changed: 29 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -414,15 +414,14 @@ the implementation understandable and operationally cheap.
414414
check suite on a PR that was already with its reviewers from looking like a
415415
stalled handoff. A push clears the clock, because new code means new checks
416416
and a review that has to run again.
417-
- Only a gate that has reported nothing on the current head runs the clock. A
418-
Copilot review that covers the head but left open findings holds the PR, yet
419-
it is not missing: it answered, and the threads it left are the author's to
420-
clear. Because the clock is carried across a trip back to the author, counting
421-
those findings would make every author who takes more than four hours over
422-
review comments look like a gate GitHub lost, and the report would say a
423-
review never arrived when it is sitting on the PR. A review that is missing or
424-
that only covers older code still counts, because Copilot has said nothing
425-
about the code being reviewed.
417+
- Only a gate that has reported nothing on the current head runs the clock.
418+
Copilot findings that still need author action hold the PR through discussion
419+
routing, but the review is not missing: it answered. Because the clock is
420+
carried across a trip back to the author, counting those findings would make
421+
every author who takes more than four hours over review comments look like a
422+
gate GitHub lost, and the report would say a review never arrived when it is
423+
sitting on the PR. A review that is missing or that only covers older code
424+
still counts, because Copilot has said nothing about the code being reviewed.
426425
- An expired hold routes the pull request and says so in its status comment,
427426
which is the whole of what the dashboard can do about it. The gates raise no
428427
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.
446445
would hold every ready PR with its author waiting for a review that never
447446
runs, so only branches with automatic review are listed and PRs targeting
448447
other branches route normally.
449-
- Every unresolved, non-outdated thread Copilot started is an author action,
450-
even after an author reply or a later clean review. This ownership bypasses
451-
generic praise and author-reply classification and ends only when the thread
452-
is resolved or outdated. The gate therefore never needs its expiration path
453-
to retain these findings.
454-
- Findings are counted from unresolved, non-outdated review threads Copilot
455-
started, not from the comment count on its review. A review's comment count
456-
never shrinks, so it keeps counting feedback the author has since addressed
457-
and holds the PR on work that is already done.
458-
- The gate's re-request path covers two states. A stale review means the author
459-
pushed, which is the one change a re-review can respond to. Findings on the
460-
current head sit on unchanged code, so asking Copilot to look at it again
461-
would reach the same verdict and be requested again on the next pass; those
462-
threads stop counting only when the author resolves them or GitHub marks them
463-
outdated after the referenced code moves. A later clean review does not close
464-
an existing thread. A push makes the review stale and triggers a re-request,
465-
but the push alone does not prove that a finding was fixed.
466-
- The other state is a missing first review. The dashboard requests it on the
467-
first eligible refresh using its GitHub App installation token. Initial reviews
468-
and re-reviews share the same delivery checks and confirmation path.
448+
- Copilot threads use the same discussion classification as human review
449+
threads. A completed author reply hands the action to reviewers; a commitment
450+
to more work keeps it with the author. Explicit resolution is not required
451+
for that handoff because a reply explains the author's response without
452+
requiring a separate administrative step. The thread remains visible as
453+
unresolved until GitHub marks it resolved or outdated.
454+
- The gate waits only for a missing or stale review. Counting open threads
455+
independently would override discussion classification and keep completed
456+
author replies assigned to the author. Actionable findings already keep the
457+
PR with its author through ordinary discussion routing.
458+
- A stale review means the author pushed, so a re-review can inspect new code.
459+
Findings on the current head do not trigger re-review requests: another
460+
review is not a substitute for responding to the finding. A later clean
461+
review alone does not close an existing thread or change its pending action.
462+
- The dashboard requests a missing first review on the first eligible refresh
463+
using its GitHub App installation token. Initial reviews and re-reviews share
464+
the same delivery checks and confirmation path.
469465
- Copilot's generic error notice is a failed attempt, not a completed review or
470466
delivery confirmation. It leaves the review missing or stale so the next
471467
eligible refresh can retry on the same head without changing the requester.
@@ -474,15 +470,15 @@ the implementation understandable and operationally cheap.
474470
from GitHub reviews, not refreshes, and a new head starts a fresh budget.
475471
Delivery rechecks the live count so an error arriving after evaluation cannot
476472
cause another request. Pending requests still prevent duplicates. Required
477-
checks and unresolved Copilot findings still apply; the status comment
478-
explains when routing stops waiting after repeated errors. The four-hour
479-
timeout remains a backstop for reviews that never report.
473+
checks and Copilot findings needing author action still apply; the status
474+
comment explains when routing stops waiting after repeated errors. The
475+
four-hour timeout remains a backstop for reviews that never report.
480476
- GitHub's quota-exhausted notice is a failed attempt even though GitHub records
481477
it as a review of the current head. It does not count as a completed review
482478
or confirm delivery. Without a configured fallback token, it immediately
483479
releases the Copilot gate and disables automatic requests for that PR,
484480
including after later pushes. Required checks and actionable feedback still
485-
apply, including unresolved, non-outdated Copilot threads.
481+
apply, including Copilot threads that still need author action.
486482
- The optional `COPILOT_REVIEW_FALLBACK_TOKEN` changes the requester after quota
487483
exhaustion and keeps the Copilot gate active under its normal rules. The
488484
fallback records existing quota notice IDs before requesting, so only a new

‎.github/scripts/pull-request-dashboard/classification_policy.py‎

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -412,7 +412,6 @@ class ClassificationDiscussion:
412412
pr_author: str = ""
413413
source_kind: str = ""
414414
candidate_feedback: tuple[CandidateFeedback, ...] = ()
415-
strict_author_action: bool = False
416415
selected_comment_index: int | None = None
417416
selected_activity_timestamp: str = ""
418417
ignored_comment_index: int | None = None
@@ -465,7 +464,6 @@ def from_record(cls, record: Mapping[str, Any]) -> ClassificationDiscussion:
465464
pr_author=str(record.get("pr_author") or ""),
466465
source_kind=str(record.get("source_kind") or ""),
467466
candidate_feedback=candidate_feedback,
468-
strict_author_action=bool(record.get("strict_author_action")),
469467
)
470468

471469
def with_comments(
@@ -1701,7 +1699,7 @@ def prepare_praise_candidates(
17011699
return tuple(
17021700
discussion
17031701
for discussion in discussions
1704-
if not discussion.strict_author_action and _could_be_praise(discussion)
1702+
if _could_be_praise(discussion)
17051703
)
17061704

17071705

@@ -1781,16 +1779,7 @@ def resolve_review_thread_policy(
17811779
selected.activity_timestamp
17821780
or selected.comment.timestamp
17831781
)
1784-
if discussion.strict_author_action:
1785-
resolved[discussion_id] = ClassificationSuccess(
1786-
discussion.identity,
1787-
ActionDecision(
1788-
DiscussionAction.AUTHOR,
1789-
"This unresolved Copilot finding remains author work until "
1790-
"the thread is resolved or outdated.",
1791-
),
1792-
)
1793-
elif dropped and not comments:
1782+
if dropped and not comments:
17941783
resolved[discussion_id] = ClassificationSuccess(
17951784
discussion.identity,
17961785
ActionDecision(

‎.github/scripts/pull-request-dashboard/copilot_review.py‎

Lines changed: 9 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
Check,
1818
Review,
1919
ReviewRequest,
20-
ReviewThread,
2120
normalize_review_requests,
2221
normalize_reviews,
2322
)
@@ -100,48 +99,11 @@ def copilot_reviews_disabled(facts: DashboardFacts) -> bool:
10099
)
101100

102101

103-
def open_copilot_findings(
104-
review_threads: Sequence[ReviewThread],
105-
) -> tuple[ReviewThread, ...]:
106-
# A review's own comment count never shrinks, so it still counts findings
107-
# the author has since addressed. Unresolved threads are the live ones:
108-
# GitHub marks a thread outdated once its anchor lines change, which is how
109-
# the rest of the dashboard already recognises a pushed fix.
110-
return tuple(
111-
thread
112-
for thread in review_threads
113-
if (
114-
not thread.is_resolved
115-
and not thread.is_outdated
116-
and thread.comments
117-
and is_copilot_reviewer(thread.comments[0].actor)
118-
)
119-
)
120-
121-
122-
def open_copilot_finding_count(
123-
review_threads: Sequence[ReviewThread],
124-
) -> int:
125-
return len(open_copilot_findings(review_threads))
126-
127-
128-
def open_copilot_finding_urls(
129-
review_threads: Sequence[ReviewThread],
130-
) -> tuple[str, ...]:
131-
urls: list[str] = []
132-
for thread in open_copilot_findings(review_threads):
133-
url = thread.comments[0].url
134-
if url and url not in urls:
135-
urls.append(url)
136-
return tuple(urls)
137-
138-
139102
def copilot_review_status(
140103
reviews: Sequence[Review],
141104
head_sha: str,
142-
review_threads: Sequence[ReviewThread],
143-
) -> tuple[bool, bool, bool]:
144-
"""Return (review exists, review is stale, review left open findings)."""
105+
) -> tuple[bool, bool]:
106+
"""Return (review exists, review is stale)."""
145107
copilot_reviews = [
146108
review
147109
for review in reviews
@@ -150,16 +112,16 @@ def copilot_review_status(
150112
and not is_copilot_error(review)
151113
]
152114
if not copilot_reviews:
153-
return False, False, False
115+
return False, False
154116
if not head_sha:
155117
# Without a head to compare against, staleness is unknowable, and
156118
# acting on incomplete data would hold the pull request on a guess.
157-
return True, False, False
119+
return True, False
158120
stale = not any(
159121
review.commit_id == head_sha
160122
for review in copilot_reviews
161123
)
162-
return True, stale, open_copilot_finding_count(review_threads) > 0
124+
return True, stale
163125

164126

165127
def copilot_review_outstanding(facts: DashboardFacts, *, enabled: bool) -> bool:
@@ -170,10 +132,9 @@ def copilot_review_outstanding(facts: DashboardFacts, *, enabled: bool) -> bool:
170132

171133
def copilot_review_unreported(facts: DashboardFacts, *, enabled: bool) -> bool:
172134
# Whether the gate is still waiting for Copilot to say anything about the
173-
# current head. Findings are an answer, not a silence: the threads they
174-
# leave are the author's to clear, and the dashboard already routes the
175-
# pull request to the author for them. Only a review that is missing or
176-
# that covers older code is a report that has not arrived.
135+
# current head. Findings are an answer, not a silence: actionable threads
136+
# belong to discussion routing. Only a review that is missing or that
137+
# covers older code is a report that has not arrived.
177138
if not enabled or copilot_reviews_disabled(facts):
178139
return False
179140
return not facts.copilot_review_exists or facts.copilot_review_stale
@@ -269,9 +230,8 @@ def copilot_review_request_landed(
269230
# Copilot can finish a short review before the last read, which takes it
270231
# back out of the pending requests. A review of the current head proves the
271232
# request landed just as well as a pending one does.
272-
review_exists, review_stale, _findings = copilot_review_status(
233+
review_exists, review_stale = copilot_review_status(
273234
normalize_reviews(fetch_pr_reviews(owner, repo_name, pr_number)),
274235
head_sha,
275-
[],
276236
)
277237
return review_exists and not review_stale

‎.github/scripts/pull-request-dashboard/copilot_review_delivery.py‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -149,12 +149,9 @@ def deliver_copilot_review_requests(
149149
)
150150
requests.pop(key, None)
151151
continue
152-
review_exists, review_stale, _review_findings = (
153-
copilot_review_status(
154-
reviews,
155-
snapshot.head_sha,
156-
snapshot.review_threads,
157-
)
152+
review_exists, review_stale = copilot_review_status(
153+
reviews,
154+
snapshot.head_sha,
158155
)
159156
if review_exists and not review_stale:
160157
print(

‎.github/scripts/pull-request-dashboard/dashboard.py‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -163,11 +163,9 @@
163163
reviewed; eligible PRs
164164
without a review need an
165165
initial review request.
166-
copilot_review_needed bool The review is stale or Copilot
167-
owns open review threads,
168-
meaning unresolved threads
169-
GitHub has not marked
170-
outdated.
166+
copilot_review_needed bool The review is stale. Findings
167+
use normal discussion
168+
classification and routing.
171169
created_at str (iso)
172170
last_activity_at str (iso) Latest substantive activity by a
173171
PR participant, never earlier

‎.github/scripts/pull-request-dashboard/discussion_lifecycle.py‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -210,10 +210,6 @@ def _group_review_threads(source: DiscussionInput) -> list[dict[str, Any]]:
210210
or f"review-discussion-{len(discussions) + 1}"
211211
),
212212
"discussion_kind": "review-comment-thread",
213-
"strict_author_action": bool(
214-
root_comment
215-
and root_comment.actor.is_copilot_reviewer
216-
),
217213
"path": discussion.path or None,
218214
"line": discussion.line,
219215
"resolved": False,
@@ -732,11 +728,7 @@ def _review_thread_pending_actions(
732728
)
733729
discussion_id = classification.identity.discussion_id
734730
discussion = by_id.get(discussion_id)
735-
action = (
736-
DiscussionAction.AUTHOR
737-
if (discussion or {}).get("strict_author_action")
738-
else decision.action
739-
)
731+
action = decision.action
740732
comments = (discussion or {}).get("comments") or []
741733
if action is not DiscussionAction.NONE and comments:
742734
entry = {

‎.github/scripts/pull-request-dashboard/pull_request_evaluation.py‎

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
copilot_review_status,
2727
is_copilot_quota_failure,
2828
is_copilot_reviewer,
29-
open_copilot_finding_urls,
3029
)
3130
from dashboard_contracts import (
3231
CopilotReviewFallback,
@@ -228,14 +227,9 @@ def _compute_facts(
228227
)
229228
api_author = pr.author.login
230229
head_sha = snapshot.head_sha
231-
(
232-
copilot_review_exists,
233-
copilot_review_stale,
234-
copilot_review_findings,
235-
) = copilot_review_status(
230+
copilot_review_exists, copilot_review_stale = copilot_review_status(
236231
source.reviews,
237232
head_sha,
238-
snapshot.review_threads,
239233
)
240234
override = dashboard_override_facts(
241235
DashboardOverrideInput(source.issue_comments),
@@ -284,7 +278,7 @@ def _compute_facts(
284278
copilot_review_error_count=copilot_review_error_count(source.reviews, head_sha),
285279
copilot_review_exists=copilot_review_exists,
286280
copilot_review_stale=copilot_review_stale,
287-
copilot_review_needed=copilot_review_stale or copilot_review_findings,
281+
copilot_review_needed=copilot_review_stale,
288282
is_maintenance_bot=_is_maintenance_bot_author(api_author),
289283
author_can_act=_author_can_act(pr.author, author),
290284
is_draft=pr.is_draft,
@@ -317,7 +311,6 @@ def _compute_facts(
317311
def _author_action_discussion_urls(
318312
discussions: list[dict[str, Any]],
319313
pending_actions: dict[str, dict[str, Any]],
320-
additional_urls: tuple[str, ...] = (),
321314
) -> tuple[str, ...]:
322315
by_id = {
323316
discussion["discussion_id"]: discussion for discussion in discussions
@@ -331,9 +324,6 @@ def _author_action_discussion_urls(
331324
url = (discussion or {}).get("discussion_url") or ""
332325
if url and url not in urls:
333326
urls.append(url)
334-
for url in additional_urls:
335-
if url and url not in urls:
336-
urls.append(url)
337327
return tuple(urls)
338328

339329

@@ -632,11 +622,6 @@ async def evaluate_pull_request(
632622
_author_action_discussion_urls(
633623
review_threads,
634624
pending_actions,
635-
(
636-
open_copilot_finding_urls(pr_source.review_threads)
637-
if facts.copilot_review_outstanding
638-
else ()
639-
),
640625
)
641626
),
642627
author_action_top_level_feedback_urls=(

‎.github/scripts/pull-request-dashboard/state.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@
5858
STATUS_COMMENT_ROLLOUT_STATE_VERSION = 3
5959
# Rendered status-comment behavior. Increment when existing comments need to
6060
# adopt a change; hourly runs durably roll it out to all open PRs.
61-
STATUS_COMMENT_REVISION = 21
61+
STATUS_COMMENT_REVISION = 22
6262
FULL_PUBLISH_STATE_VERSION = 1
6363
INITIAL_BACKFILL_COMPLETE_KEY = "initial_backfill_complete"
6464
_state_dir: Path | None = None

0 commit comments

Comments
 (0)