diff --git a/.github/scripts/pull-request-dashboard/copilot_review.py b/.github/scripts/pull-request-dashboard/copilot_review.py index 0536ae0bd835..62605a07b5c9 100644 --- a/.github/scripts/pull-request-dashboard/copilot_review.py +++ b/.github/scripts/pull-request-dashboard/copilot_review.py @@ -6,6 +6,8 @@ from typing import Any from dashboard_contracts import CopilotReviewFallback, DashboardFacts +from dashboard_status import promoted_copilot_finding_id +from copilot_review_findings import pending_copilot_summary_findings from github_cli import ( fetch_pr_reviews, fetch_review_requests, @@ -114,7 +116,12 @@ def open_copilot_findings( not thread.is_resolved and not thread.is_outdated and thread.comments - and is_copilot_reviewer(thread.comments[0].actor) + and ( + is_copilot_reviewer(thread.comments[0].actor) + or promoted_copilot_finding_id( + thread.comments[0].actor.login, thread.comments[0].body + ) + ) ) ) @@ -127,12 +134,17 @@ def open_copilot_finding_count( def open_copilot_finding_urls( review_threads: Sequence[ReviewThread], + reviews: Sequence[Review] = (), + head_sha: str = "", ) -> 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) + for finding in pending_copilot_summary_findings(reviews, head_sha, review_threads): + if finding.review_url not in urls: + urls.append(finding.review_url) return tuple(urls) @@ -159,7 +171,10 @@ def copilot_review_status( review.commit_id == head_sha for review in copilot_reviews ) - return True, stale, open_copilot_finding_count(review_threads) > 0 + return True, stale, ( + open_copilot_finding_count(review_threads) > 0 + or bool(pending_copilot_summary_findings(reviews, head_sha, review_threads)) + ) def copilot_review_outstanding(facts: DashboardFacts, *, enabled: bool) -> bool: diff --git a/.github/scripts/pull-request-dashboard/copilot_review_delivery.py b/.github/scripts/pull-request-dashboard/copilot_review_delivery.py index ceea406523cc..30b0d53623fd 100644 --- a/.github/scripts/pull-request-dashboard/copilot_review_delivery.py +++ b/.github/scripts/pull-request-dashboard/copilot_review_delivery.py @@ -20,18 +20,79 @@ stale_request_reason, ) from dashboard_contracts import CopilotReviewFallback, StoredDashboardResult -from github_cli import fetch_pr_reviews, request_copilot_review -from pull_request_source import normalize_reviews +from copilot_review_findings import pending_copilot_summary_findings +from github_cli import ( + create_pr_review_comment, + fetch_pr_review_comment_locations, + fetch_pr_reviews, + request_copilot_review, +) +from pull_request_source import fetch_pull_request_source, normalize_reviews from routing_snapshot import fetch_routing_snapshot from state import ( load_copilot_review_fallbacks, load_copilot_review_requests, + load_dashboard_state_cache, save_copilot_review_fallbacks, save_copilot_review_requests, ) from utils import format_ts +def deliver_copilot_review_findings(repo: str, pr_number: int | None = None) -> list[str]: + state = load_dashboard_state_cache() + if state is None: + return [] + owner, repo_name = repo.split("/", 1) + errors: list[str] = [] + for result in state.results: + if ( + not result.facts.copilot_review_findings_to_promote + or (pr_number is not None and result.pr_number != pr_number) + ): + continue + number = result.pr_number + try: + source = fetch_pull_request_source( + repo, owner, repo_name, number, include_commits=False, + ) + pr = source.pull_request + if ( + pr.state != "OPEN" + or pr.is_draft + or not pr.head_sha + or pr.head_sha != result.facts.head_sha + ): + print( + f"skipping summary-only Copilot findings for changed PR #{number}", + file=sys.stderr, + ) + continue + findings = pending_copilot_summary_findings( + source.reviews, pr.head_sha, source.review_threads, + ) + if not findings: + continue + locations = fetch_pr_review_comment_locations(repo, number) + for finding in findings: + if finding.path not in locations: + raise RuntimeError( + f"Copilot finding refers to a file outside PR #{number}'s diff: " + f"{finding.path}" + ) + create_pr_review_comment( + repo, + number, + pr.head_sha, + finding.path, + finding.line if finding.line in locations[finding.path] else None, + finding.comment_body(), + ) + except Exception as error: + errors.append(f"PR #{number}: {error}") + return errors + + def record_copilot_review_observation( pr_number: int, result: StoredDashboardResult | None, diff --git a/.github/scripts/pull-request-dashboard/copilot_review_findings.py b/.github/scripts/pull-request-dashboard/copilot_review_findings.py new file mode 100644 index 000000000000..00c2c8b3ca5f --- /dev/null +++ b/.github/scripts/pull-request-dashboard/copilot_review_findings.py @@ -0,0 +1,218 @@ +"""Extract summary-only Copilot findings for promotion into review threads.""" + +from __future__ import annotations + +from collections.abc import Sequence +from dataclasses import dataclass, field +import hashlib +import html +from html.parser import HTMLParser +import json +from pathlib import PurePosixPath +import re + +from dashboard_status import promoted_copilot_finding_id +from pull_request_source import Review, ReviewThread + + +@dataclass(frozen=True) +class CopilotSummaryFinding: + review_url: str + head_sha: str + path: str + line: int + title: str + severity: str + body: str + + @property + def identity(self) -> str: + value = json.dumps( + [self.head_sha, self.path, self.line, self.title], + ensure_ascii=True, + ) + return hashlib.sha256(value.encode()).hexdigest() + + def comment_body(self) -> str: + severity = f" ({self.severity})" if self.severity else "" + return ( + f"\n\n" + f"**{self.title}**{severity}\n\n" + f"`{self.path}:{self.line}`\n\n" + f"{self.body}\n\n" + f"Copied from [Copilot's review]({self.review_url}), where this " + "finding appeared only in the \"previously missed\" summary. " + "Reply here to discuss or reject it, and resolve this conversation " + "when it no longer needs attention." + ) + + +@dataclass +class _Details: + summary: str = "" + body: str = "" + summary_start: int = 0 + body_start: int = 0 + children: list[_Details] = field(default_factory=list) + + +class _DetailsParser(HTMLParser): + def __init__(self, text: str) -> None: + super().__init__(convert_charrefs=False) + self.text = text + self.offsets = [0] + for line in text.splitlines(keepends=True): + self.offsets.append(self.offsets[-1] + len(line)) + self.sections: list[_Details] = [] + self.stack: list[_Details] = [] + + def _offset(self) -> int: + line, column = self.getpos() + return self.offsets[line - 1] + column + + def handle_starttag(self, tag: str, attrs: list[tuple[str, str | None]]) -> None: + if tag == "details": + section = _Details() + if self.stack: + self.stack[-1].children.append(section) + else: + self.sections.append(section) + self.stack.append(section) + elif tag == "summary" and self.stack: + self.stack[-1].summary_start = self._offset() + len(self.get_starttag_text()) + + def handle_endtag(self, tag: str) -> None: + if tag == "summary" and self.stack: + section = self.stack[-1] + section.summary = self.text[section.summary_start:self._offset()] + section.body_start = self._offset() + len("") + elif tag == "details" and self.stack: + section = self.stack.pop() + section.body = self.text[section.body_start:self._offset()] + + +def _mask_markdown_code(text: str) -> str: + # Keep source offsets and newlines intact while hiding code from HTML parsing. + lines: list[str] = [] + fence = "" + for line in text.splitlines(keepends=True): + is_code = bool(fence) + if fence: + if re.fullmatch( + rf" {{0,3}}{re.escape(fence[0])}{{{len(fence)},}}[ \t]*", + line.rstrip("\r\n"), + ): + fence = "" + else: + opening = re.match(r" {0,3}(`{3,}|~{3,})([^\r\n]*)", line) + if opening and (opening[1][0] != "`" or "`" not in opening[2]): + fence = opening[1] + is_code = True + lines.append(re.sub(r"[^\r\n]", " ", line) if is_code else line) + return re.sub( + r"\\[^\r\n]|(? str: + parts: list[str] = [] + start = 0 + for tag in re.finditer(r"<[^>]*>", _mask_markdown_code(value)): + parts.append(value[start:tag.start()]) + start = tag.end() + parts.append(value[start:]) + return html.unescape("".join(parts)).strip() + + +def _parse_findings(review: Review) -> tuple[CopilotSummaryFinding, ...]: + if "previously missed" not in review.body.lower(): + return () + parser = _DetailsParser(review.body) + parser.feed(_mask_markdown_code(review.body)) + parser.close() + if parser.stack: + raise ValueError(f"Copilot review {review.database_id} has unclosed details") + findings: list[CopilotSummaryFinding] = [] + for section in parser.sections: + summary = _summary_text(section.summary) + if "previously missed" not in summary.lower(): + continue + count_match = re.fullmatch(r"(\d+) previously missed issues?", summary, re.I) + if count_match is None: + raise ValueError( + f"Copilot review {review.database_id} has an unsupported missed-issue summary" + ) + expected = int(count_match.group(1)) + if len(section.children) != expected: + raise ValueError( + f"Copilot review {review.database_id} lists {expected} missed issues " + f"but contains {len(section.children)} findings" + ) + for item in section.children: + location = re.search(r"(?m)^\s*`([^`\r\n]+):(\d+)`\s*$", item.body) + title = _summary_text(item.summary) + if location is None or not title: + raise ValueError( + f"Copilot review {review.database_id} has an unsupported finding location or title" + ) + path = location.group(1).replace("\u200b", "") + line = int(location.group(2)) + body = item.body[location.end():].strip() + parsed_path = PurePosixPath(path) + if ( + not path + or parsed_path.is_absolute() + or ".." in parsed_path.parts + or "\\" in path + or line <= 0 + or not body + or not review.url + ): + raise ValueError(f"Copilot review {review.database_id} has an invalid finding") + severity = re.search(r'\balt=["\']([^"\']*severity)["\']', item.summary, re.I) + findings.append(CopilotSummaryFinding( + review_url=review.url, + head_sha=review.commit_id, + path=path, + line=line, + title=title, + severity=html.unescape(severity.group(1)) if severity else "", + body=body, + )) + return tuple(findings) + + +def pending_copilot_summary_findings( + reviews: Sequence[Review], + head_sha: str, + review_threads: Sequence[ReviewThread], +) -> tuple[CopilotSummaryFinding, ...]: + current = [ + review + for review in reviews + if review.actor.is_copilot_reviewer + and head_sha + and review.commit_id == head_sha + and review.state not in ("PENDING", "DISMISSED") + and "" in review.body + ] + if not current: + return () + latest = max(current, key=lambda review: (review.submitted_at, review.database_id)) + promoted = { + promoted_copilot_finding_id(thread.comments[0].actor.login, thread.comments[0].body) + for thread in review_threads + if thread.comments + } + # Resolved and outdated copies also suppress reposting on the same head. + return tuple({ + finding.identity: finding + for finding in _parse_findings(latest) + if finding.identity not in promoted + }.values()) diff --git a/.github/scripts/pull-request-dashboard/dashboard_contracts.py b/.github/scripts/pull-request-dashboard/dashboard_contracts.py index 0ad8cd8f71d7..b4ae475b823c 100644 --- a/.github/scripts/pull-request-dashboard/dashboard_contracts.py +++ b/.github/scripts/pull-request-dashboard/dashboard_contracts.py @@ -113,6 +113,7 @@ class DashboardFacts: copilot_review_exists: bool = False copilot_review_stale: bool = False copilot_review_needed: bool = False + copilot_review_findings_to_promote: bool = False is_maintenance_bot: bool = False author_can_act: bool = True is_draft: bool = False diff --git a/.github/scripts/pull-request-dashboard/dashboard_status.py b/.github/scripts/pull-request-dashboard/dashboard_status.py index fe4e886c6045..77806e7df324 100644 --- a/.github/scripts/pull-request-dashboard/dashboard_status.py +++ b/.github/scripts/pull-request-dashboard/dashboard_status.py @@ -26,6 +26,16 @@ r"" ) DASHBOARD_APP_SLUG = "opentelemetry-pr-dashboard" +_COPILOT_FINDING_MARKER_RE = re.compile( + r"" +) + + +def promoted_copilot_finding_id(login: str, body: str) -> str: + if login != f"{DASHBOARD_APP_SLUG}[bot]": + return "" + match = _COPILOT_FINDING_MARKER_RE.search(body) + return match.group(1) if match else "" def author_nudge_episode_marker(episode_id: str) -> str: diff --git a/.github/scripts/pull-request-dashboard/delivery.py b/.github/scripts/pull-request-dashboard/delivery.py index 79dc15293136..db46053d9397 100644 --- a/.github/scripts/pull-request-dashboard/delivery.py +++ b/.github/scripts/pull-request-dashboard/delivery.py @@ -13,7 +13,10 @@ from typing import Callable from author_nudge import deliver_prepared_author_nudges -from copilot_review_delivery import deliver_copilot_review_requests +from copilot_review_delivery import ( + deliver_copilot_review_findings, + deliver_copilot_review_requests, +) from dashboard_override_delivery import deliver_dashboard_command_replies from github_cli import ( GhNotFoundError, @@ -204,6 +207,11 @@ def deliver_from_state( ), errors, ) + run_delivery_action( + "Copilot findings", + lambda: deliver_copilot_review_findings(repo, pr_number), + errors, + ) run_delivery_action( "Copilot reviews", lambda: deliver_copilot_review_requests(repo, now, copilot_retry_snapshot_path), diff --git a/.github/scripts/pull-request-dashboard/discussion_lifecycle.py b/.github/scripts/pull-request-dashboard/discussion_lifecycle.py index 23d669f91825..9de3168bb22a 100644 --- a/.github/scripts/pull-request-dashboard/discussion_lifecycle.py +++ b/.github/scripts/pull-request-dashboard/discussion_lifecycle.py @@ -187,7 +187,7 @@ def _group_review_threads(source: DiscussionInput) -> list[dict[str, Any]]: comments = [ _discussion_comment( comment.created_at, - reviewer_actor_login(comment.actor), + reviewer_actor_login(comment.actor, comment.body), source.author, reviewers, comment.body, @@ -212,7 +212,9 @@ def _group_review_threads(source: DiscussionInput) -> list[dict[str, Any]]: "discussion_kind": "review-comment-thread", "strict_author_action": bool( root_comment - and root_comment.actor.is_copilot_reviewer + and reviewer_actor_login( + root_comment.actor, root_comment.body + ) == "copilot-pull-request-reviewer[bot]" ), "path": discussion.path or None, "line": discussion.line, diff --git a/.github/scripts/pull-request-dashboard/github_cli.py b/.github/scripts/pull-request-dashboard/github_cli.py index 51bc11351060..90ed43174a15 100644 --- a/.github/scripts/pull-request-dashboard/github_cli.py +++ b/.github/scripts/pull-request-dashboard/github_cli.py @@ -2,6 +2,7 @@ import json import os +import re import subprocess import time from fnmatch import fnmatchcase @@ -80,10 +81,13 @@ def _run_gh( input_text: str | None, allowed_exit_codes: frozenset[int] | set[int], parse_stdout: Callable[[str], _T], + *, + retry: bool = True, ) -> _T: env = {**os.environ, "GH_TOKEN": token} if token else None last_stderr = "" - for attempt in range(GH_RETRY_ATTEMPTS): + attempts = GH_RETRY_ATTEMPTS if retry else 1 + for attempt in range(attempts): proc = subprocess.run( cmd, input=input_text, @@ -99,16 +103,16 @@ def _run_gh( try: return parse_stdout(proc.stdout) except json.JSONDecodeError as error: - if attempt == GH_RETRY_ATTEMPTS - 1: + if attempt == attempts - 1: message = ( f"{' '.join(cmd)} returned invalid JSON after " - f"{GH_RETRY_ATTEMPTS} attempts: {error}" + f"{attempts} attempts: {error}" ) raise TransientGhError(message) from error sleep_for_retry(attempt) continue last_stderr = proc.stderr.strip() - if attempt == GH_RETRY_ATTEMPTS - 1 or not is_retryable_gh_error(last_stderr): + if attempt == attempts - 1 or not is_retryable_gh_error(last_stderr): break sleep_for_retry(attempt) message = f"{' '.join(cmd)} failed: {last_stderr}" @@ -128,13 +132,20 @@ def run_gh( return _run_gh(cmd, token, input_text, allowed_exit_codes, lambda stdout: stdout) -def run_gh_json(cmd: list[str], token: str | None = None, input_text: str | None = None) -> Any: +def run_gh_json( + cmd: list[str], + token: str | None = None, + input_text: str | None = None, + *, + retry: bool = True, +) -> Any: return _run_gh( cmd, token, input_text, frozenset({0}), lambda stdout: json.loads(stdout or "null"), + retry=retry, ) @@ -155,6 +166,67 @@ def gh_api(path: str, paginate: bool = False, token: str | None = None) -> Any: return data +def fetch_pr_review_comment_locations(repo: str, number: int) -> dict[str, frozenset[int]]: + files = gh_api(f"/repos/{repo}/pulls/{number}/files?per_page=100", paginate=True) + locations: dict[str, frozenset[int]] = {} + for file in files: + lines: set[int] = set() + line_number: int | None = None + for line in (file.get("patch") or "").splitlines(): + hunk = re.match(r"@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@", line) + if hunk: + line_number = int(hunk.group(1)) + elif line_number is not None and line.startswith(("+", " ")): + lines.add(line_number) + line_number += 1 + locations[file["filename"]] = frozenset(lines) + return locations + + +def create_pr_review_comment( + repo: str, + number: int, + head_sha: str, + path: str, + line: int | None, + body: str, +) -> None: + pr = gh_api(f"/repos/{repo}/pulls/{number}") + if ( + pr.get("state") != "open" + or pr.get("draft") + or (pr.get("head") or {}).get("sha") != head_sha + ): + raise RuntimeError(f"PR #{number} changed before posting a Copilot finding") + payload: dict[str, Any] = { + "body": body, + "commit_id": head_sha, + "path": path, + "subject_type": "file" if line is None else "line", + } + if line is not None: + payload.update(line=line, side="RIGHT") + # Retrying an ambiguous POST can duplicate a thread. The next delivery + # reads GitHub's markers before deciding whether to post again. + result = run_gh_json( + [ + "gh", "api", "--method", "POST", + f"/repos/{repo}/pulls/{number}/comments", "--input", "-", + ], + input_text=json.dumps(payload), + retry=False, + ) + if ( + not isinstance(result, dict) + or not result.get("id") + or not result.get("html_url") + or result.get("body") != body + or result.get("path") != path + or result.get("commit_id") != head_sha + ): + raise RuntimeError(f"GitHub did not confirm the Copilot finding on PR #{number}") + + def request_copilot_review(pull_request_id: str, token: str | None = None) -> None: # Success here only means GitHub accepted the mutation. It does not mean # GitHub recorded the reviewer, so callers have to read the pull request @@ -874,6 +946,7 @@ def load_reviewer_set(org: str, approver_team_slugs: list[str]) -> set[str]: createdAt lastEditedAt author { + __typename login } reactionGroups { @@ -911,6 +984,7 @@ def load_reviewer_set(org: str, approver_team_slugs: list[str]) -> set[str]: createdAt lastEditedAt author { + __typename login } reactionGroups { diff --git a/.github/scripts/pull-request-dashboard/pull_request_activity.py b/.github/scripts/pull-request-dashboard/pull_request_activity.py index df866b935c9e..28e2f469e62c 100644 --- a/.github/scripts/pull-request-dashboard/pull_request_activity.py +++ b/.github/scripts/pull-request-dashboard/pull_request_activity.py @@ -7,6 +7,7 @@ from typing import Any from dashboard_override import dashboard_command_body_remainder +from dashboard_status import promoted_copilot_finding_id from pull_request_source import ( Actor, Commit, @@ -51,7 +52,9 @@ def role_for(login: str, author: str, reviewers: set[str]) -> str: return "outsider" -def reviewer_actor_login(actor: Actor) -> str: +def reviewer_actor_login(actor: Actor, body: str = "") -> str: + if promoted_copilot_finding_id(actor.login, body): + return "copilot-pull-request-reviewer[bot]" return actor.reviewer_login @@ -141,7 +144,7 @@ def _review_comment_event( author: str, approver_logins: set[str], ) -> dict[str, Any]: - login = reviewer_actor_login(comment.actor) + login = reviewer_actor_login(comment.actor, comment.body) timestamp = comment.updated_at or comment.created_at return { "source_id": comment.database_id or None, diff --git a/.github/scripts/pull-request-dashboard/pull_request_evaluation.py b/.github/scripts/pull-request-dashboard/pull_request_evaluation.py index ff6eb7e254f8..859e58e4e874 100644 --- a/.github/scripts/pull-request-dashboard/pull_request_evaluation.py +++ b/.github/scripts/pull-request-dashboard/pull_request_evaluation.py @@ -28,6 +28,7 @@ is_copilot_reviewer, open_copilot_finding_urls, ) +from copilot_review_findings import pending_copilot_summary_findings from dashboard_contracts import ( CopilotReviewFallback, DashboardFacts, @@ -285,6 +286,9 @@ def _compute_facts( copilot_review_exists=copilot_review_exists, copilot_review_stale=copilot_review_stale, copilot_review_needed=copilot_review_stale or copilot_review_findings, + copilot_review_findings_to_promote=bool(pending_copilot_summary_findings( + source.reviews, head_sha, source.review_threads, + )), is_maintenance_bot=_is_maintenance_bot_author(api_author), author_can_act=_author_can_act(pr.author, author), is_draft=pr.is_draft, @@ -633,7 +637,9 @@ async def evaluate_pull_request( review_threads, pending_actions, ( - open_copilot_finding_urls(pr_source.review_threads) + open_copilot_finding_urls( + pr_source.review_threads, pr_source.reviews, facts.head_sha, + ) if facts.copilot_review_outstanding else () ), diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index a04856ec6291..e83cfc61fbb5 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -728,6 +728,10 @@ def decode_dashboard_facts(value: Any) -> DashboardFacts: value.get("copilot_review_needed", _MISSING), "facts.copilot_review_needed", ), + copilot_review_findings_to_promote=_boolean( + value.get("copilot_review_findings_to_promote", _MISSING), + "facts.copilot_review_findings_to_promote", + ), is_maintenance_bot=_boolean( value.get("is_maintenance_bot", _MISSING), "facts.is_maintenance_bot", @@ -856,6 +860,7 @@ def encode_dashboard_facts(facts: DashboardFacts) -> dict[str, Any]: "copilot_review_exists": facts.copilot_review_exists, "copilot_review_stale": facts.copilot_review_stale, "copilot_review_needed": facts.copilot_review_needed, + "copilot_review_findings_to_promote": facts.copilot_review_findings_to_promote, "is_maintenance_bot": facts.is_maintenance_bot, "author_can_act": facts.author_can_act, "is_draft": facts.is_draft, diff --git a/.github/scripts/pull-request-dashboard/test_copilot_review_findings.py b/.github/scripts/pull-request-dashboard/test_copilot_review_findings.py new file mode 100644 index 000000000000..e858cd5a50da --- /dev/null +++ b/.github/scripts/pull-request-dashboard/test_copilot_review_findings.py @@ -0,0 +1,497 @@ +from __future__ import annotations + +from dataclasses import replace +import unittest +from unittest.mock import patch + +from classification_test_support import FakeClassificationOperation +from copilot_review import copilot_review_status, open_copilot_finding_urls +from copilot_review_delivery import deliver_copilot_review_findings +from copilot_review_findings import pending_copilot_summary_findings +from dashboard_status import DASHBOARD_APP_SLUG +from dashboard_contracts import DashboardRoute, EvaluationSuccess +from dashboard_test_support import ( + actor, + dashboard_facts, + dashboard_state, + pull_request_metadata, + pull_request_source, + review_source, + review_thread, + review_thread_comment, + stored_dashboard_result, +) +from discussion_lifecycle import DiscussionInput, prepare_discussions +from github_cli import fetch_review_threads +from pull_request_activity import reviewer_actor_login +from pull_request_evaluation import ( + PullRequestEvaluationConfig, + PullRequestEvaluationInput, + evaluate_pull_request, +) +from pull_request_source import normalize_review_threads + + +HEAD = "a" * 40 +REVIEW_URL = "https://github.com/owner/repo/pull/7#pullrequestreview-42" +FINDING = """
+Medium severity Restrict parsing to authority + +`src/\u200bParser.java:91` + +[General] The parser searches the database path and query too. + +Keep the helper input inside the authority. +
""" + + +def overview(*items: str) -> str: + return ( + "\n\n**No open issues**\n\n" + f"
{len(items)} previously missed " + f"{'issue' if len(items) == 1 else 'issues'}\n\n" + "In code that hasn't changed since last review\n\n" + + "\n".join(items) + + "\n
\n\n" + "
1 resolved since last review\n" + "- A fixed finding\n
" + ) + + +def missed_review(**changes): + values = { + "database_id": 42, + "commit_id": HEAD, + "url": REVIEW_URL, + "actor": actor("copilot-pull-request-reviewer[bot]"), + "body": overview(FINDING), + } + values.update(changes) + return review_source(**values) + + +def copied_thread(finding, **changes): + return review_thread( + comments=(review_thread_comment( + actor=actor(f"{DASHBOARD_APP_SLUG}[bot]"), + body=finding.comment_body(), + ),), + **changes, + ) + + +class CopilotSummaryFindingsTest(unittest.TestCase): + def test_extracts_the_summary_only_finding_and_preserves_its_text(self) -> None: + review = missed_review() + findings = pending_copilot_summary_findings((review,), HEAD, ()) + + self.assertEqual(1, len(findings)) + finding = findings[0] + self.assertEqual("src/Parser.java", finding.path) + self.assertEqual(91, finding.line) + self.assertEqual("Restrict parsing to authority", finding.title) + self.assertEqual("Medium severity", finding.severity) + self.assertEqual( + "[General] The parser searches the database path and query too.\n\n" + "Keep the helper input inside the authority.", + finding.body, + ) + body = finding.comment_body() + self.assertIn(finding.body, body) + self.assertIn("Medium severity", body) + self.assertIn(f"[Copilot's review]({REVIEW_URL})", body) + self.assertIn("Reply here to discuss or reject it", body) + self.assertEqual( + (True, False, True), + copilot_review_status((review,), HEAD, ()), + ) + self.assertEqual((REVIEW_URL,), open_copilot_finding_urls((), (review,), HEAD)) + + def test_extracts_multiple_findings_without_the_resolved_section(self) -> None: + second = FINDING.replace("authority", "the host").replace(":91", ":100") + review = missed_review(body=overview(FINDING, second)) + + findings = pending_copilot_summary_findings((review,), HEAD, ()) + + self.assertEqual([91, 100], [finding.line for finding in findings]) + self.assertNotEqual(findings[0].identity, findings[1].identity) + + def test_copies_each_identity_once(self) -> None: + review = missed_review(body=overview(FINDING, FINDING)) + + self.assertEqual(1, len(pending_copilot_summary_findings((review,), HEAD, ()))) + + def test_html_entities_are_decoded_in_the_title(self) -> None: + review = missed_review(body=overview(FINDING.replace("authority", "host & port"))) + + finding, = pending_copilot_summary_findings((review,), HEAD, ()) + + self.assertEqual("Restrict parsing to host & port", finding.title) + + def test_structural_tags_in_title_code_spans_are_preserved(self) -> None: + for title in ("Close the `
` element", "Keep `` literal"): + with self.subTest(title=title): + review = missed_review(body=overview(FINDING.replace( + "Restrict parsing to authority", title, + ))) + + finding, = pending_copilot_summary_findings((review,), HEAD, ()) + + self.assertEqual(title, finding.title) + self.assertIn(f"**{title}**", finding.comment_body()) + + def test_structural_tags_in_code_spans_preserve_the_finding(self) -> None: + examples = ( + "`
`", "`
`", "``", "``", + "``a `
` example``", "``a `
` example``", + "`an example\nwith
\nacross lines`", + ) + for example in examples: + with self.subTest(example=example): + explanation = f"Close the {example} element.\n\nKeep this explanation." + review = missed_review(body=overview(FINDING.replace( + "Keep the helper input inside the authority.", explanation, + ))) + + finding, = pending_copilot_summary_findings((review,), HEAD, ()) + + self.assertEqual( + "[General] The parser searches the database path and query too.\n\n" + + explanation, + finding.body, + ) + self.assertIn(explanation, finding.comment_body()) + + def test_structural_tags_in_fenced_code_preserve_the_finding(self) -> None: + for opening, closing in ( + ("```html", "```"), ("~~~html", "~~~~"), ("````html", "`````"), + ): + for tag in ("
", "
", "", ""): + with self.subTest(opening=opening, tag=tag): + explanation = ( + f"An example:\n\n{opening}\n{tag}\n{closing}\n\n" + "Keep this explanation." + ) + review = missed_review(body=overview(FINDING.replace( + "Keep the helper input inside the authority.", explanation, + ))) + + finding, = pending_copilot_summary_findings((review,), HEAD, ()) + + self.assertEqual( + "[General] The parser searches the database path and query too.\n\n" + + explanation, + finding.body, + ) + self.assertIn(explanation, finding.comment_body()) + + def test_uses_only_the_latest_current_head_copilot_overview(self) -> None: + old = missed_review() + clean = replace( + old, database_id=43, submitted_at="2026-08-17T07:00:00Z", body=overview(), + ) + for review in ( + clean, + replace(old, actor=actor("human")), + replace(old, commit_id="old-head"), + replace(old, state="PENDING"), + replace(old, state="DISMISSED"), + replace(old, body="Previously missed issues without an overview."), + ): + with self.subTest(review=review): + self.assertEqual((), pending_copilot_summary_findings((review,), HEAD, ())) + self.assertEqual((), pending_copilot_summary_findings((clean, old), HEAD, ())) + self.assertEqual((), pending_copilot_summary_findings((old,), "", ())) + + def test_existing_copies_suppress_reposting_even_after_resolution(self) -> None: + review = missed_review() + finding, = pending_copilot_summary_findings((review,), HEAD, ()) + for resolved, outdated in ((False, False), (True, False), (False, True)): + with self.subTest(resolved=resolved, outdated=outdated): + thread = copied_thread( + finding, is_resolved=resolved, is_outdated=outdated, + ) + self.assertEqual( + (), pending_copilot_summary_findings((review,), HEAD, (thread,)), + ) + self.assertEqual( + (True, False, not resolved and not outdated), + copilot_review_status((review,), HEAD, (thread,)), + ) + repeated = replace(review, database_id=50, url=REVIEW_URL + "-new") + self.assertEqual( + (), pending_copilot_summary_findings((repeated,), HEAD, (copied_thread(finding),)), + ) + next_head = replace(review, commit_id="b" * 40) + self.assertEqual( + 1, + len(pending_copilot_summary_findings( + (next_head,), next_head.commit_id, (copied_thread(finding),), + )), + ) + + @patch("github_cli.gh_graphql") + def test_fetched_bot_copies_suppress_reposting_and_clear_the_gate(self, graphql) -> None: + review = missed_review() + finding, = pending_copilot_summary_findings((review,), HEAD, ()) + author = {"__typename": "Bot", "login": DASHBOARD_APP_SLUG} + raw_thread = { + "id": "PRRT_1", + "comments": { + "nodes": [{"author": author, "body": finding.comment_body()}], + "pageInfo": {"hasNextPage": True, "endCursor": "comments-1"}, + }, + } + graphql.side_effect = [ + {"data": {"repository": {"pullRequest": {"reviewThreads": { + "nodes": [raw_thread], + "pageInfo": {"hasNextPage": False}, + }}}}}, + {"data": {"node": {"comments": { + "nodes": [{"author": author, "body": "Dashboard follow-up."}], + "pageInfo": {"hasNextPage": False}, + }}}}, + ] + + thread, = normalize_review_threads(fetch_review_threads("owner", "repo", 7)) + + self.assertEqual(2, graphql.call_count) + for query in graphql.call_args_list: + self.assertRegex(query.args[0], r"author\s*\{[^}]*\b__typename\b") + self.assertEqual( + [f"{DASHBOARD_APP_SLUG}[bot]"] * 2, + [comment.actor.login for comment in thread.comments], + ) + for resolved in (False, True): + with self.subTest(resolved=resolved): + current = replace(thread, is_resolved=resolved) + self.assertEqual( + (), pending_copilot_summary_findings((review,), HEAD, (current,)), + ) + self.assertEqual( + (True, False, not resolved), + copilot_review_status((review,), HEAD, (current,)), + ) + + def test_a_human_cannot_spoof_a_promoted_copilot_finding(self) -> None: + review = missed_review() + finding, = pending_copilot_summary_findings((review,), HEAD, ()) + for login in ("human", DASHBOARD_APP_SLUG): + with self.subTest(login=login): + spoof = review_thread(comments=(review_thread_comment( + actor=actor(login), body=finding.comment_body(), + ),)) + + self.assertEqual( + (finding,), pending_copilot_summary_findings((review,), HEAD, (spoof,)), + ) + self.assertEqual( + login, reviewer_actor_login(spoof.comments[0].actor, finding.comment_body()), + ) + + def test_promoted_threads_use_copilot_lifecycle_and_reviewer_identity(self) -> None: + finding, = pending_copilot_summary_findings((missed_review(),), HEAD, ()) + thread = copied_thread(finding) + thread = replace(thread, comments=(*thread.comments, review_thread_comment( + actor=actor("author"), body="This is intentional, so I disagree.", + created_at="2026-08-17T07:00:00Z", + ))) + + prepared = prepare_discussions(DiscussionInput( + (thread,), (), "author", frozenset(), "no", + )) + + self.assertTrue(prepared.review_threads[0]["strict_author_action"]) + self.assertEqual( + "copilot-pull-request-reviewer[bot]", + prepared.review_threads[0]["comments"][0]["actor"], + ) + self.assertEqual( + (thread.comments[0].url,), open_copilot_finding_urls((thread,)), + ) + + def test_unsupported_or_incomplete_findings_fail_explicitly(self) -> None: + invalid = ( + overview(FINDING).replace("1 previously", "2 previously"), + overview(FINDING).replace("1 previously", "Several previously"), + overview(FINDING).replace("src/\u200bParser.java:91", "no location"), + overview(FINDING).replace(":91`", ":0`"), + overview(FINDING).replace("src/\u200bParser.java", "../Parser.java"), + overview(FINDING).replace("src/\u200bParser.java", "/Parser.java"), + overview(FINDING).replace("
", "", 1), + ) + for body in invalid: + with self.subTest(body=body): + with self.assertRaisesRegex(ValueError, "Copilot review 42"): + pending_copilot_summary_findings((missed_review(body=body),), HEAD, ()) + + +class CopilotSummaryFindingDeliveryTest(unittest.TestCase): + def setUp(self) -> None: + self.review = missed_review() + self.source = pull_request_source( + pull_request=pull_request_metadata(head_sha=HEAD), + reviews=(self.review,), + ) + self.load = self.enterContext(patch( + "copilot_review_delivery.load_dashboard_state_cache", + return_value=dashboard_state(stored_dashboard_result( + 7, facts=dashboard_facts( + head_sha=HEAD, copilot_review_findings_to_promote=True, + ), + )), + )) + self.fetch = self.enterContext(patch( + "copilot_review_delivery.fetch_pull_request_source", + return_value=self.source, + )) + self.locations = self.enterContext(patch( + "copilot_review_delivery.fetch_pr_review_comment_locations", + return_value={"src/Parser.java": frozenset({91})}, + )) + self.create = self.enterContext(patch( + "copilot_review_delivery.create_pr_review_comment", + )) + + def test_posts_an_attributed_inline_comment(self) -> None: + self.assertEqual([], deliver_copilot_review_findings("owner/repo", 7)) + + finding, = pending_copilot_summary_findings((self.review,), HEAD, ()) + self.create.assert_called_once_with( + "owner/repo", 7, HEAD, "src/Parser.java", 91, finding.comment_body(), + ) + self.fetch.assert_called_once_with( + "owner/repo", "owner", "repo", 7, include_commits=False, + ) + + def test_uses_a_file_comment_when_the_line_is_not_in_the_diff(self) -> None: + self.locations.return_value = {"src/Parser.java": frozenset()} + + self.assertEqual([], deliver_copilot_review_findings("owner/repo")) + + self.assertIsNone(self.create.call_args.args[4]) + self.assertIn("src/Parser.java:91", self.create.call_args.args[5]) + + def test_unrelated_or_unprepared_prs_do_not_fetch_or_post(self) -> None: + self.assertEqual([], deliver_copilot_review_findings("owner/repo", 8)) + self.load.return_value = dashboard_state(stored_dashboard_result(7)) + self.assertEqual([], deliver_copilot_review_findings("owner/repo")) + self.load.return_value = None + self.assertEqual([], deliver_copilot_review_findings("owner/repo")) + self.fetch.assert_not_called() + self.create.assert_not_called() + + def test_changed_heads_drafts_and_closed_prs_are_not_posted(self) -> None: + for metadata in ( + replace(self.source.pull_request, head_sha="b" * 40), + replace(self.source.pull_request, is_draft=True), + replace(self.source.pull_request, state="CLOSED"), + replace(self.source.pull_request, head_sha=""), + ): + with self.subTest(metadata=metadata): + self.fetch.return_value = replace(self.source, pull_request=metadata) + self.assertEqual([], deliver_copilot_review_findings("owner/repo")) + self.create.assert_not_called() + self.locations.assert_not_called() + + def test_retry_reads_github_to_avoid_reposting_an_existing_copy(self) -> None: + finding, = pending_copilot_summary_findings((self.review,), HEAD, ()) + self.create.side_effect = RuntimeError("connection lost after posting") + self.assertEqual( + ["PR #7: connection lost after posting"], + deliver_copilot_review_findings("owner/repo"), + ) + self.fetch.return_value = replace( + self.source, review_threads=(copied_thread(finding, is_resolved=True),), + ) + + self.assertEqual([], deliver_copilot_review_findings("owner/repo")) + self.assertEqual(1, self.create.call_count) + + def test_partial_delivery_retries_only_the_remaining_finding(self) -> None: + review = missed_review(body=overview( + FINDING, FINDING.replace("authority", "the host").replace(":91", ":100"), + )) + self.fetch.return_value = replace(self.source, reviews=(review,)) + self.locations.return_value = {"src/Parser.java": frozenset({91, 100})} + self.create.side_effect = [None, RuntimeError("unavailable")] + + self.assertEqual( + ["PR #7: unavailable"], deliver_copilot_review_findings("owner/repo"), + ) + first, second = pending_copilot_summary_findings((review,), HEAD, ()) + self.fetch.return_value = replace( + self.source, reviews=(review,), review_threads=(copied_thread(first),), + ) + self.create.side_effect = None + + self.assertEqual([], deliver_copilot_review_findings("owner/repo")) + self.assertEqual(3, self.create.call_count) + self.assertEqual(second.comment_body(), self.create.call_args.args[-1]) + + def test_reports_a_finding_outside_the_pr_diff_without_posting(self) -> None: + self.locations.return_value = {} + + errors = deliver_copilot_review_findings("owner/repo") + + self.assertEqual(1, len(errors)) + self.assertIn("outside PR #7's diff", errors[0]) + self.create.assert_not_called() + + def test_failures_do_not_prevent_other_prs_from_being_processed(self) -> None: + result = self.load.return_value.results[0] + self.load.return_value = dashboard_state(result, replace(result, pr_number=8)) + self.fetch.side_effect = [RuntimeError("unavailable"), self.source] + + self.assertEqual( + ["PR #7: unavailable"], deliver_copilot_review_findings("owner/repo"), + ) + self.assertEqual(8, self.create.call_args.args[1]) + + +class CopilotSummaryFindingEvaluationTest(unittest.IsolatedAsyncioTestCase): + @patch("pull_request_evaluation.fetch_pull_request_source") + async def test_summary_only_findings_hold_the_gate_until_the_copy_is_resolved( + self, fetch_source, + ) -> None: + review = missed_review(body=overview(FINDING.replace( + "Keep the helper input inside the authority.", + "Close the `
` element; keep `
` literal.", + ))) + finding, = pending_copilot_summary_findings((review,), HEAD, ()) + config = PullRequestEvaluationConfig( + repo="owner/repo", + owner="owner", + repo_name="repo", + approver_logins=frozenset({"reviewer"}), + classifier_model="model", + required_approvals=1, + require_clean_copilot_review_branches=frozenset({"main"}), + ) + for resolved in (False, True): + with self.subTest(resolved=resolved): + fetch_source.return_value = pull_request_source( + pull_request=pull_request_metadata(head_sha=HEAD), + reviews=(review,), + review_threads=( + (copied_thread(finding, is_resolved=True),) + if resolved else () + ), + ) + + result = await evaluate_pull_request( + config, PullRequestEvaluationInput(7), FakeClassificationOperation(), + ) + + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual( + DashboardRoute.APPROVER if resolved else DashboardRoute.AUTHOR, + result.route, + ) + self.assertEqual(not resolved, result.facts.copilot_review_needed) + self.assertEqual(not resolved, result.facts.copilot_review_findings_to_promote) + self.assertEqual( + () if resolved else (REVIEW_URL,), + result.facts.author_action_review_thread_urls, + ) diff --git a/.github/scripts/pull-request-dashboard/test_delivery.py b/.github/scripts/pull-request-dashboard/test_delivery.py index 806e939508a9..1f91d4e65d97 100644 --- a/.github/scripts/pull-request-dashboard/test_delivery.py +++ b/.github/scripts/pull-request-dashboard/test_delivery.py @@ -12,6 +12,11 @@ class DeliveryTest(unittest.TestCase): + def setUp(self) -> None: + self.copilot_findings = self.enterContext(patch.object( + delivery, "deliver_copilot_review_findings", return_value=[], + )) + def test_canceled_full_job_is_drained_by_next_targeted_job(self) -> None: with tempfile.TemporaryDirectory() as directory: root = Path(directory) @@ -237,6 +242,7 @@ def record(label: str) -> list[str]: dashboard_command_replies.side_effect = lambda *_args: record("replies") author_nudges.side_effect = lambda *_args: record("author") copilot_reviews.side_effect = lambda *_args: record("copilot") + self.copilot_findings.side_effect = lambda *_args: record("findings") slack.side_effect = lambda *_args: record("slack") errors = delivery.deliver_from_state( "open-telemetry/example", @@ -252,6 +258,7 @@ def record(label: str) -> list[str]: call("replies"), call("author"), call("status"), + call("findings"), call("copilot"), call("slack"), ], @@ -270,6 +277,7 @@ def record(label: str) -> list[str]: Path("author"), set(), ) + self.copilot_findings.assert_called_once_with("open-telemetry/example", None) slack.assert_called_once_with( "open-telemetry/example", ANY, @@ -487,6 +495,7 @@ def test_targeted_delivery_only_processes_triggering_pr(self) -> None: list_open.assert_not_called() gh_api.assert_called_once_with("/repos/open-telemetry/example/pulls/7") self.assertEqual(7, command_replies.call_args.args[2]) + self.copilot_findings.assert_called_once_with("open-telemetry/example", 7) bulk_status.assert_not_called() targeted_status.assert_called_once_with("open-telemetry/example", 7) slack.assert_called_once_with( diff --git a/.github/scripts/pull-request-dashboard/test_github_cli.py b/.github/scripts/pull-request-dashboard/test_github_cli.py index a0b97e647982..b5930bd71c24 100644 --- a/.github/scripts/pull-request-dashboard/test_github_cli.py +++ b/.github/scripts/pull-request-dashboard/test_github_cli.py @@ -11,8 +11,10 @@ TransientGhError, check_bucket, code_scanning_tools, + create_pr_review_comment, fetch_pr_issue_comments, fetch_pr_reviews, + fetch_pr_review_comment_locations, fetch_review_requests, fetch_review_threads, fetch_latest_draft_transitions, @@ -74,6 +76,23 @@ def _rollup_page(nodes): class RunGhJsonTest(unittest.TestCase): + @patch("github_cli.sleep_for_retry") + @patch("github_cli.subprocess.run") + def test_non_idempotent_writes_can_disable_immediate_retries(self, run, sleep) -> None: + for returncode, stdout, stderr in ( + (1, "", "connection reset"), + (0, "{broken", ""), + ): + with self.subTest(returncode=returncode): + run.reset_mock() + run.return_value = subprocess.CompletedProcess( + ["gh"], returncode, stdout, stderr, + ) + with self.assertRaises(TransientGhError): + run_gh_json(["gh", "api", "--method", "POST", "/test"], retry=False) + run.assert_called_once() + sleep.assert_not_called() + @patch("github_cli.subprocess.run") def test_explicit_token_is_scoped_to_a_hidden_subprocess(self, run) -> None: run.return_value = subprocess.CompletedProcess(["gh"], 0, "{}", "") @@ -212,6 +231,80 @@ def test_process_and_decode_failures_share_attempt_budget( ) +class PromotedReviewCommentTest(unittest.TestCase): + @patch("github_cli.gh_api") + def test_maps_added_and_context_lines_in_all_diff_hunks(self, api) -> None: + api.return_value = [ + { + "filename": "src/Parser.java", + "patch": ( + "@@ -10,3 +10,4 @@\n context\n-removed\n+added\n+also added\n context\n" + "@@ -40 +41 @@\n-old\n+new\n\\ No newline at end of file" + ), + }, + {"filename": "large.java"}, + ] + + self.assertEqual( + {"src/Parser.java": frozenset({10, 11, 12, 13, 41}), "large.java": frozenset()}, + fetch_pr_review_comment_locations("owner/repo", 7), + ) + api.assert_called_once_with( + "/repos/owner/repo/pulls/7/files?per_page=100", paginate=True, + ) + + @patch("github_cli.run_gh_json") + @patch("github_cli.gh_api") + def test_posts_line_or_file_comments_with_a_fresh_head_check(self, api, run) -> None: + api.return_value = {"state": "open", "draft": False, "head": {"sha": "head"}} + run.return_value = { + "id": 123, "html_url": "https://example.test/comment/123", + "body": "Finding", "path": "file.java", "commit_id": "head", + } + for line in (91, None): + with self.subTest(line=line): + create_pr_review_comment("owner/repo", 7, "head", "file.java", line, "Finding") + + payload = json.loads(run.call_args.kwargs["input_text"]) + self.assertEqual("head", payload["commit_id"]) + self.assertEqual("file.java", payload["path"]) + self.assertEqual("Finding", payload["body"]) + self.assertFalse(run.call_args.kwargs["retry"]) + if line is None: + self.assertEqual("file", payload["subject_type"]) + self.assertNotIn("line", payload) + self.assertNotIn("side", payload) + else: + self.assertEqual("line", payload["subject_type"]) + self.assertEqual(91, payload["line"]) + self.assertEqual("RIGHT", payload["side"]) + api.assert_called_with("/repos/owner/repo/pulls/7") + + @patch("github_cli.run_gh_json") + @patch("github_cli.gh_api") + def test_refuses_to_post_to_a_changed_pr(self, api, run) -> None: + for metadata in ( + {"state": "closed", "head": {"sha": "head"}}, + {"state": "open", "draft": True, "head": {"sha": "head"}}, + {"state": "open", "head": {"sha": "new-head"}}, + ): + with self.subTest(metadata=metadata): + api.return_value = metadata + with self.assertRaisesRegex(RuntimeError, "changed before posting"): + create_pr_review_comment("owner/repo", 7, "head", "file.java", 91, "Finding") + run.assert_not_called() + + @patch("github_cli.run_gh_json") + @patch("github_cli.gh_api") + def test_requires_a_confirmed_created_comment(self, api, run) -> None: + api.return_value = {"state": "open", "head": {"sha": "head"}} + for response in (None, {}, {"id": 123, "html_url": "https://example.test/comment"}): + with self.subTest(response=response): + run.return_value = response + with self.assertRaisesRegex(RuntimeError, "did not confirm"): + create_pr_review_comment("owner/repo", 7, "head", "file.java", 91, "Finding") + + class FetchReviewRequestsTest(unittest.TestCase): def test_returns_bot_reviewers_that_gh_pr_view_omits(self) -> None: # `gh pr view --json reviewRequests` exports only User and Team nodes, diff --git a/.github/scripts/pull-request-dashboard/test_state.py b/.github/scripts/pull-request-dashboard/test_state.py index 729cca9d3079..158b65b0911a 100644 --- a/.github/scripts/pull-request-dashboard/test_state.py +++ b/.github/scripts/pull-request-dashboard/test_state.py @@ -80,6 +80,14 @@ class StateTest(unittest.TestCase): + def test_copilot_finding_promotion_intent_round_trips_and_defaults_false(self) -> None: + facts = dashboard_facts(copilot_review_findings_to_promote=True) + encoded = encode_dashboard_facts(facts) + + self.assertEqual(facts, decode_dashboard_facts(encoded)) + encoded.pop("copilot_review_findings_to_promote") + self.assertFalse(decode_dashboard_facts(encoded).copilot_review_findings_to_promote) + def test_fallback_state_survives_new_intents_and_receipt_reconciliation(self) -> None: with ( tempfile.TemporaryDirectory() as directory, @@ -899,6 +907,7 @@ def test_legacy_dashboard_state_payload_reencodes_to_current_shape(self) -> None "draft_pr_numbers": [], } expected["prs"]["123"]["facts"]["author_can_act"] = True + expected["prs"]["123"]["facts"]["copilot_review_findings_to_promote"] = False del expected["prs"]["123"]["facts"]["copilot_first_review_missing_since"] self.assertEqual(expected, encode_dashboard_state(decoded)) diff --git a/pull-request-dashboard/README.md b/pull-request-dashboard/README.md index 3ec680fdd226..5f65547cfaa5 100644 --- a/pull-request-dashboard/README.md +++ b/pull-request-dashboard/README.md @@ -181,6 +181,23 @@ first dashboard refresh where it would otherwise route to reviewers or maintainers. Conflicts, failing required checks, author-action feedback, and manual reviewer handoffs block requests; pending checks do not. +Copilot sometimes reports "previously missed" findings only inside a collapsed +review summary. The dashboard copies findings from the latest overview of the +current head into review threads under the dashboard app, preserving their +wording, severity, and Markdown code examples and linking to the original review. +When the reported line is not available in the PR diff, it creates a file-level +review comment instead. +Authors can reply to discuss or reject a finding and resolve the conversation +when it no longer needs attention. + +These copies follow the same dashboard rules as ordinary Copilot threads. +Unresolved, non-outdated copies need author attention; resolving a copy clears +that finding even if Copilot's summary still lists it. Markers in the comments +prevent reposting the same finding on the same head, including after it is +resolved. Until a copy is posted, the clean-review gate counts the summary-only +finding and the live status links to its source review. A push requires a fresh +review before summary-only findings from the new head can be copied. + Copilot's "encountered an error and was unable to review" notice does not count as a completed review. The first error allows one retry on the next eligible refresh without requiring a new commit, unless a request is already pending.