Promote summary-only Copilot findings to review threads

.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:

.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,

.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"<!-- pull-request-dashboard-copilot-finding:{self.identity} -->\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("</summary>")
+        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]|(?<!`)(`+)(?!`)"
+        r"(?:(?!\n[ \t]*\r?\n).)*?(?<!`)\1(?!`)",
+        lambda match: (
+            re.sub(r"[^\r\n]", " ", match[0]) if match[1] else match[0]
+        ),
+        "".join(lines),
+        flags=re.S,
+    )
+
+
+def _summary_text(value: str) -> 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 "<!-- ccr-overview-v2 -->" 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())

.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

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

@@ -26,6 +26,16 @@
     r"<!-- pull-request-dashboard-reviewer-handoff-cleared:(\d+):([^\s>]+) -->"
 )
 DASHBOARD_APP_SLUG = "opentelemetry-pr-dashboard"
+_COPILOT_FINDING_MARKER_RE = re.compile(
+    r"<!-- pull-request-dashboard-copilot-finding:([a-f0-9]{64}) -->"
+)
+
+
+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: