diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index dc01c4a23b5..9c030ba6083 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -11,10 +11,10 @@ the implementation understandable and operationally cheap. - Target repositories only need GitHub App access and an entry in `repositories.json`. - The top-level workflow resolves target repositories, then calls a reusable - per-repository workflow for each target. The per-repository workflow runs the - update, notification, and publishing jobs top to bottom for one repository, so - one repository's update failure does not block publishing or notifications for - repositories whose updates succeeded. + per-repository workflow for each target. The per-repository workflow separates + read-only calculation from a repository-wide publisher, so one repository's + update failure does not block delivery for repositories whose updates + succeeded. - The top-level repository matrix runs one repository at a time. Backfills do not benefit enough from cross-repository parallelism to justify extra aggregate API and LLM demand. @@ -43,6 +43,14 @@ the implementation understandable and operationally cheap. - Concurrency bounds pending jobs per target; it does not debounce webhook delivery or workflow dispatch. Different repositories and PRs can still run independently, and every accepted webhook still creates a workflow run. +- Publishers use one concurrency group per target repository. GitHub preserves + the running publisher but may replace an older pending publisher with a newer + one even when `cancel-in-progress` is false. This is safe because accepted + work lives on the state branch and every surviving publisher drains + repository-wide work rather than serving only its triggering invocation. +- The top-level hourly health check treats a replaced pending publisher as + successful. Matrix failures take precedence over cancellation, so genuine + update or delivery failures still open the failure issue. ## GitHub Actions Instead Of Netlify For Scheduled Backfills @@ -82,8 +90,8 @@ the implementation understandable and operationally cheap. queue. Incrementing the implementation revision snapshots all open PRs, then hourly runs update at most 50 queued comments until the rollout completes. Dashboard refreshes atomically queue comments only when their persisted result - changes; targeted refreshes update immediately and leave no completed PR - pending in the rollout queue. + changes. Every publisher drains up to 50 queued comments, so a newer publisher + also delivers work left by an older pending publisher that GitHub replaced. - Selected PRs are processed one at a time through the same single-PR merge path as targeted refreshes. Each accepted PR update pushes structured state before the next selected PR is processed. @@ -106,9 +114,9 @@ the implementation understandable and operationally cheap. - A selected PR failure is recorded outside dashboard state, advances the cursor, and does not stop later selected PRs. The backfill still exits nonzero while any open PR is still recorded as having failed processing, keeping - scheduled failure reporting active. Publish and notification jobs consume - only accepted state, so untrusted PR content cannot deny service to the rest - of the repository. + scheduled failure reporting active. The publisher consumes only accepted + state, so untrusted PR content cannot deny service to the rest of the + repository. - The cursor deliberately does not rely on PR `updatedAt`; prior testing showed `updatedAt` is not a safe freshness key for every comment, review-comment, or thread event the dashboard needs. @@ -167,6 +175,40 @@ the implementation understandable and operationally cheap. respond to a dashboard action. Pending required checks affect the CI column but do not change who owns the next action. +## Copilot Review Gate + +- `require_clean_copilot_review_branches` is a final safety net applied only when a PR + would otherwise route to reviewers or maintainers — that is, after the author + has addressed the actionable discussions. It is not a routing input while the + author still owns actions. +- The setting lists the base branches to gate rather than a single on/off + switch, because automatic Copilot review is itself configured per branch + (often only the default branch). Gating a branch with no automatic review + would park every ready PR on the copilot route waiting for a review that never + runs, so only branches with automatic review are listed and PRs targeting + other branches route normally. +- Copilot findings normally return a PR to the author through ordinary + discussion routing: an inline finding is an unresolved review thread, and an + actionable one routes the PR to "waiting on author." In that common path the + gate never fires and no re-review is requested. +- The gate's re-request path is deliberately narrow: it triggers when the + current head has no Copilot review yet (a push made the prior review stale) or + the author resolved Copilot's threads without a code change. Re-requesting the + same head is intentional — it asks Copilot to re-review after the author + responded, mirroring a human review cycle. Copilot's answer either clears the + gate or produces fresh actionable threads that route the PR back to the + author, so re-requesting an unchanged commit is self-correcting rather than a + re-request loop. +- "Clean" means no inline comments on the current head, counted from the + review, not from the classifier's actionability judgment. Accepted + limitation: if Copilot leaves comments the classifier treats as + non-actionable while they stay unresolved, routing sits at reviewers but the + gate holds the PR on the copilot route and re-requests until Copilot returns a + comment-free review or the author pushes. The strict count is intentional — + the gate is a conservative "Copilot had nothing to say about this exact code" + check, and folding in classifier judgment could let a real-but-non-actionable + comment slip a PR to humans. + ## Live PR Status Comments - Feedback totals in the live comment count the canonical author-action links @@ -261,20 +303,21 @@ the implementation understandable and operationally cheap. - Slack notification state is PR-granular. It does not track notification history separately for each assignee. - When notification state is first created, existing approver-routed PRs may - receive initial notifications on a later targeted refresh. Avoiding that - bootstrap case would require storing separate seen-but-not-notified state. + receive initial notifications from a later publisher. Avoiding that bootstrap + case would require storing separate seen-but-not-notified state. - When a mapped assignee is added after a PR was already notified during the same waiting period, that assignee may wait until the next follow-up cadence instead of receiving an immediate initial notification. -- Scheduled runs send only due follow-up reminders. Targeted PR refreshes send - only the triggering PR's initial notification. This keeps webhook-driven - refreshes from sweeping unrelated PR reminders while preserving the hourly - reminder pass. +- Every publisher evaluates all accepted repository state for eligible initial + and follow-up notifications. The sent-notification ledger and weekday + 24-hour follow-up cadence bound delivery. Repository-wide evaluation ensures + that a newer publisher drains Slack work whose older pending publisher GitHub + replaced. - Slack notifications are sent only for dashboard state that has already been accepted on the state branch. A newer dashboard update can land after the - notification job checks out state, so a notification can be slightly late + publisher checks out state, so a notification can be slightly late relative to the newest state. -- The notification job preserves just-written notification state across normal +- The publisher preserves just-written notification state across normal state-branch CAS retries. If Slack delivery succeeds and every state-branch push attempt is rejected, a later run can send the same notification again. Recording state before sending Slack would avoid that duplicate window, but @@ -282,11 +325,15 @@ the implementation understandable and operationally cheap. ## Publishing -- Dashboard publishing is serialized per target repository. -- Each publish job fetches the accepted state branch while holding the publish - slot, lists the target repository's current open PRs, renders markdown from - `dashboard-state.json`, and publishes the issue body. -- Publish jobs are superseded by newer publish jobs for the same repository; - only the newest queued publish needs to render the latest accepted state. -- If another update advances the state branch while a publish job is already - editing the issue, the live issue can briefly lag until the next publish job. +- Dashboard publishing is serialized per target repository. The publisher owns + target-repository writes for status comments, author reminders, Copilot + re-review requests, Slack notifications, and the dashboard issue. +- Each publisher fetches accepted state while holding the publish slot and + drains repository-wide pending work. Status comments are bounded to 50 per + publisher; author reminders and Copilot requests use explicit durable + ledgers; Slack eligibility is reconstructed from accepted dashboard and + notification state. +- The dashboard issue is rendered from `dashboard-state.json` and the target + repository's current open PR list after delivery. If another update advances + the state branch while a publisher is already working, external views can + briefly lag until the next publisher. diff --git a/.github/scripts/pull-request-dashboard/author_nudge.py b/.github/scripts/pull-request-dashboard/author_nudge.py new file mode 100644 index 00000000000..8e187093000 --- /dev/null +++ b/.github/scripts/pull-request-dashboard/author_nudge.py @@ -0,0 +1,346 @@ +#!/usr/bin/env python3 +"""Track and post reminders for pull requests waiting on authors.""" + +from __future__ import annotations + +from concurrent.futures import ThreadPoolExecutor +from datetime import datetime, timedelta +import hashlib +import json +from pathlib import Path +import sys +from typing import Any + +from github_cli import ( + fetch_pr_issue_comments, + fetch_pr_review_data, + fetch_review_threads, + gh_api, + gh_pr_check_rollup, + gh_required_check_contexts, + include_missing_required_checks, + run_gh, +) +from dashboard_override import author_override_guidance +from dashboard_override import DASHBOARD_OVERRIDE_LABEL +from pr_status_comment import ( + DASHBOARD_APP_SLUG, + managed_status_comments, + publish_pr_status, +) +from state import ( + load_author_nudges, + load_dashboard_state_cache, + save_author_nudges, +) +from utils import format_ts, parse_ts + + +NUDGE_AFTER = timedelta(weeks=1) +NUDGE_MARKER_PREFIX = "" + + +def routing_input_fingerprint(raw: dict[str, Any]) -> str: + dashboard_login = f"{DASHBOARD_APP_SLUG}[bot]" + pr = raw.get("pr") or raw + labels = raw.get("labels") + if labels is None: + labels = pr.get("labels") or [] + issue_comments = [ + comment + for comment in raw.get("issue_comments") or [] + if (comment.get("user") or {}).get("login") != dashboard_login + ] + routing_inputs = { + "base_branch": str( + pr.get("baseRefName") + or (pr.get("base") or {}).get("ref") + or "" + ), + "checks": raw.get("checks"), + "issue_comments": issue_comments, + "labels": sorted( + label.get("name") or "" + for label in labels + if isinstance(label, dict) + and label.get("name") == DASHBOARD_OVERRIDE_LABEL + ), + "pr_text": { + "body": str(pr.get("body") or "").replace("\r\n", "\n"), + "title": str(pr.get("title") or ""), + }, + "review_comments": raw.get("review_comments") or [], + "reviews": raw.get("reviews") or [], + "review_threads": raw.get("review_threads") or [], + } + encoded = json.dumps( + routing_inputs, + sort_keys=True, + separators=(",", ":"), + ).encode() + return hashlib.sha256(encoded).hexdigest() + + +def fetch_current_pr_routing_state( + repo: str, + pr_number: int, +) -> tuple[dict[str, Any], str]: + owner, repo_name = repo.split("/", 1) + with ThreadPoolExecutor() as pool: + pr_future = pool.submit(gh_api, f"/repos/{repo}/pulls/{pr_number}") + issue_comments_future = pool.submit( + fetch_pr_issue_comments, + owner, + repo_name, + pr_number, + ) + review_comments_future = pool.submit( + gh_api, + f"/repos/{repo}/pulls/{pr_number}/comments?per_page=100", + True, + ) + review_data_future = pool.submit( + fetch_pr_review_data, + owner, + repo_name, + pr_number, + ) + review_threads_future = pool.submit( + fetch_review_threads, + owner, + repo_name, + pr_number, + ) + pr = pr_future.result() or {} + check_rollup_future = pool.submit( + gh_pr_check_rollup, + repo, + pr.get("node_id") or "", + [], + ) + required_contexts_future = pool.submit( + gh_required_check_contexts, + repo, + ((pr.get("base") or {}).get("ref") or ""), + ) + review_data = review_data_future.result() or {} + check_rollup = check_rollup_future.result() + fingerprint = routing_input_fingerprint({ + "checks": include_missing_required_checks( + None if check_rollup is None else check_rollup["required"], + required_contexts_future.result(), + ), + "issue_comments": issue_comments_future.result() or [], + "labels": pr.get("labels") or [], + "pr": pr, + "review_comments": review_comments_future.result() or [], + "reviews": review_data.get("reviews") or [], + "review_threads": review_threads_future.result() or [], + }) + return pr, fingerprint + + +def plan_nudge( + result: dict[str, Any] | None, + previous: dict[str, Any] | None, + now: datetime, +) -> tuple[bool, dict[str, Any] | None]: + entry = dict(previous or {}) + nudged_at = entry.get("nudged_at") or "" + if result and ( + result.get("failed") + or result.get("route") in ("transient-failure", "unknown") + ): + return False, entry or None + if not result or result.get("route") != "author": + return False, None + if nudged_at: + return False, entry + + waiting_since = parse_ts(entry.get("waiting_since") or "") + if waiting_since is None: + return False, { + "waiting_since": format_ts(now), + "nudged_at": "", + } + return now - waiting_since >= NUDGE_AFTER, entry + + +def existing_nudge_comment( + repo: str, + pr_number: int, + episode_id: str, +) -> dict[str, Any] | None: + marker = nudge_marker(episode_id) + comments = gh_api( + f"/repos/{repo}/issues/{pr_number}/comments?per_page=100", + paginate=True, + ) + return next( + ( + comment + for comment in comments or [] + if (comment.get("performed_via_github_app") or {}).get("slug") + == DASHBOARD_APP_SLUG + and marker in (comment.get("body") or "") + ), + None, + ) + + +def render_nudge(author: str, status_url: str, episode_id: str) -> str: + return "\n".join([ + nudge_marker(episode_id), + f"Hi @{author} — just a friendly reminder that this pull request has " + "been waiting on you for a week.", + "", + f"There are still items that need your attention. See the " + f"[dashboard status comment]({status_url}) for the full list. You don't " + "need to push a code change to hand it back — replying to move each " + "discussion forward is enough, whether that's answering a question, " + "explaining why no change is needed, or asking a follow-up. The " + "dashboard then automatically routes it back to reviewers.", + "", + author_override_guidance(), + "", + ]) + + +def ensure_nudge( + repo: str, + pr_number: int, + result: dict[str, Any], + dashboard_state: dict[str, Any], + waiting_since: str, + now: datetime, +) -> str | None: + episode_id = str( + ((result.get("facts") or {}).get("author_nudge_episode_id") or "") + ) + if not episode_id: + raise RuntimeError(f"author nudge episode not found for PR #{pr_number}") + existing = existing_nudge_comment(repo, pr_number, episode_id) + if existing: + return existing.get("created_at") or format_ts(now) + + pr = gh_api(f"/repos/{repo}/pulls/{pr_number}") or {} + if pr.get("state") != "open" or pr.get("draft"): + return None + + publish_pr_status(repo, pr_number, dashboard_state) + status_comments = managed_status_comments(repo, pr_number) + if not status_comments or not status_comments[0].get("html_url"): + raise RuntimeError(f"dashboard status comment not found for PR #{pr_number}") + author = str(((result.get("facts") or {}).get("author") or "")).strip() + author = author or str((pr.get("user") or {}).get("login") or "").strip() + if not author: + raise RuntimeError(f"author not found for PR #{pr_number}") + run_gh([ + "gh", "api", "--method", "POST", + f"repos/{repo}/issues/{pr_number}/comments", + "-f", f"body={render_nudge(author, status_comments[0]['html_url'], episode_id)}", + ]) + return format_ts(now) + + +def record_author_nudge_observation( + pr_number: int, + result: dict[str, Any] | None, + now: datetime, + *, + prepare_due: bool = False, +) -> None: + updated = dict(load_author_nudges()) + key = str(pr_number) + due, entry = plan_nudge(result, updated.get(key), now) + if due and prepare_due and entry is not None: + facts = (result or {}).get("facts") or {} + head_sha = facts.get("head_sha") or "" + routing_fingerprint = facts.get("routing_input_fingerprint") or "" + if head_sha and routing_fingerprint: + entry = { + **entry, + "pending_at": format_ts(now), + "head_sha": head_sha, + "routing_input_fingerprint": routing_fingerprint, + } + if entry is None: + updated.pop(key, None) + else: + updated[key] = entry + save_author_nudges(updated) + + +def deliver_prepared_author_nudges( + repo: str, + now: datetime, + retry_snapshot_path: Path | None = None, +) -> list[str]: + dashboard_state = load_dashboard_state_cache() + if dashboard_state is None: + print("dashboard state not found; skipping author nudges", file=sys.stderr) + return [] + updated = dict(load_author_nudges(retry_snapshot_path)) + dashboard_prs = dashboard_state.get("prs") or {} + errors: list[str] = [] + for key, entry in sorted(updated.items(), key=lambda item: int(item[0])): + if not (entry or {}).get("pending_at"): + continue + pr_number = int(key) + result = dashboard_prs.get(key) + if not result or result.get("route") != "author": + _due, reset_entry = plan_nudge(result, entry, now) + if reset_entry is None: + updated.pop(key, None) + else: + updated[key] = reset_entry + continue + try: + pr, current_routing_fingerprint = fetch_current_pr_routing_state( + repo, + pr_number, + ) + expected_head = entry.get("head_sha") or "" + expected_routing_fingerprint = entry.get("routing_input_fingerprint") or "" + current_head = ((pr.get("head") or {}).get("sha") or "") + if pr.get("state") != "open" or pr.get("draft"): + updated.pop(key, None) + continue + if ( + not expected_head + or current_head != expected_head + or not expected_routing_fingerprint + or current_routing_fingerprint != expected_routing_fingerprint + ): + updated[key] = { + name: value + for name, value in entry.items() + if name not in ( + "pending_at", + "head_sha", + "routing_input_fingerprint", + ) + } + continue + nudged_at = ensure_nudge( + repo, + pr_number, + result, + dashboard_state, + entry.get("waiting_since") or "", + now, + ) + except Exception as e: + errors.append(f"PR #{pr_number}: {e}") + continue + if nudged_at: + updated[key] = { + "waiting_since": entry.get("waiting_since") or "", + "nudged_at": nudged_at, + } + save_author_nudges(updated) + return errors diff --git a/.github/scripts/pull-request-dashboard/copilot_review.py b/.github/scripts/pull-request-dashboard/copilot_review.py new file mode 100644 index 00000000000..0040e2a745c --- /dev/null +++ b/.github/scripts/pull-request-dashboard/copilot_review.py @@ -0,0 +1,154 @@ +"""Track Copilot re-review requests for delivery by the publisher job.""" + +from __future__ import annotations + +from datetime import datetime +from pathlib import Path +from typing import Any + +from author_nudge import fetch_current_pr_routing_state +from github_cli import ( + fetch_pr_review_data, + request_copilot_review, +) +from state import load_copilot_review_requests, save_copilot_review_requests +from utils import actor_login, format_ts + + +_COPILOT_REVIEWER_LOGINS = { + "copilot", + "copilot-pull-request-reviewer", + "copilot-pull-request-reviewer[bot]", +} + + +def is_copilot_reviewer(obj: dict[str, Any] | None) -> bool: + return actor_login(obj).lower() in _COPILOT_REVIEWER_LOGINS + + +def copilot_review_status( + reviews: list[dict[str, Any]], + head_sha: str, +) -> tuple[bool, bool]: + copilot_reviews = [ + review + for review in reviews + if is_copilot_reviewer(review.get("user")) + ] + if not copilot_reviews: + return False, False + if not head_sha: + return True, False + current_head_reviews = [ + review + for review in copilot_reviews + if (review.get("commit_id") or "") == head_sha + ] + if not current_head_reviews: + return True, True + latest_review = max( + current_head_reviews, + key=lambda review: review.get("submitted_at") or "", + ) + return True, bool(latest_review.get("finding_count")) + + +def apply_copilot_review_gate( + facts: dict[str, Any], + route: str, + *, + enabled: bool, +) -> str: + facts["copilot_review_request_needed"] = False + if not enabled or route not in ("approver", "maintainer"): + return route + if not facts.get("copilot_review_exists"): + return "copilot" + if not facts.get("copilot_review_needed"): + return route + if not facts.get("copilot_review_requested"): + facts["copilot_review_request_needed"] = True + return "copilot" + + +def record_copilot_review_observation( + pr_number: int, + result: dict[str, Any] | None, + observed_at: datetime, +) -> None: + requests = dict(load_copilot_review_requests()) + key = str(pr_number) + facts = (result or {}).get("facts") or {} + head_sha = str(facts.get("head_sha") or "") + routing_fingerprint = str(facts.get("routing_input_fingerprint") or "") + if ( + not result + or result.get("failed") + or result.get("route") != "copilot" + or not facts.get("copilot_review_request_needed") + or not head_sha + or not routing_fingerprint + ): + requests.pop(key, None) + else: + requests[key] = { + "head_sha": head_sha, + "observed_at": format_ts(observed_at), + "requested_at": "", + "routing_input_fingerprint": routing_fingerprint, + } + save_copilot_review_requests(requests) + + +def deliver_copilot_review_requests( + repo: str, + now: datetime, + retry_snapshot_path: Path | None = None, +) -> list[str]: + requests = dict(load_copilot_review_requests(retry_snapshot_path)) + owner, repo_name = repo.split("/", 1) + errors: list[str] = [] + for key, entry in sorted(requests.items(), key=lambda item: int(item[0])): + if (entry or {}).get("requested_at"): + continue + pr_number = int(key) + try: + pr, current_routing_fingerprint = fetch_current_pr_routing_state( + repo, + pr_number, + ) + current_head = ((pr.get("head") or {}).get("sha") or "") + if ( + pr.get("state") != "open" + or pr.get("draft") + or current_head != entry.get("head_sha") + or not entry.get("routing_input_fingerprint") + or current_routing_fingerprint + != entry.get("routing_input_fingerprint") + ): + requests.pop(key, None) + continue + if any( + is_copilot_reviewer(request) + for request in (pr.get("requested_reviewers") or []) + ): + requests[key] = {**entry, "requested_at": format_ts(now)} + continue + review_data = fetch_pr_review_data(owner, repo_name, pr_number) or {} + review_exists, review_needed = copilot_review_status( + review_data.get("reviews") or [], + current_head, + ) + if not review_exists or not review_needed: + requests.pop(key, None) + continue + pull_request_id = pr.get("node_id") or "" + if not pull_request_id: + raise RuntimeError(f"GitHub did not return a node ID for PR #{pr_number}") + request_copilot_review(pull_request_id) + except Exception as e: + errors.append(f"PR #{pr_number}: {e}") + continue + requests[key] = {**entry, "requested_at": format_ts(now)} + save_copilot_review_requests(requests) + return errors \ No newline at end of file diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index 48e787ab928..88f25dc11f2 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -44,10 +44,11 @@ v save_dashboard_state_cache -Slack notifications are sent by notify_slack.py in a separate serialized -workflow job. That job loads the latest accepted dashboard state and -notification state, sends any due notifications, and pushes the updated -notification state with the same git CAS pattern. +Status comments, author nudges, Copilot re-review requests, and Slack +notifications are delivered by delivery.py in one serialized publishing job. +That job loads the latest accepted dashboard state and delivery ledgers, sends +due updates, and pushes successful acknowledgements with the same git CAS +pattern. State files are committed and pushed first. Only after that state branch push succeeds does a follow-up publishing job fetch the accepted dashboard state, @@ -160,6 +161,7 @@ import argparse import sys import traceback +import uuid from concurrent.futures import ThreadPoolExecutor from dataclasses import dataclass, replace from datetime import datetime @@ -191,6 +193,20 @@ normalize_discussion_action, prune_classification_cache, ) +from author_nudge import record_author_nudge_observation, routing_input_fingerprint +from copilot_review import ( + apply_copilot_review_gate, + copilot_review_status, + is_copilot_reviewer, + record_copilot_review_observation, +) +from dashboard_override import ( + apply_dashboard_override, + append_route_noop_reply, + dashboard_command_body_remainder, + dashboard_override_facts, +) +from pr_status_comment import status_author_nudge_episode_id from state import ( INITIAL_BACKFILL_COMPLETE_KEY, empty_state, @@ -206,7 +222,7 @@ update_dashboard_state_for_pr, ) import state_branch -from utils import actor_login, format_ts, parse_ts, truncate +from utils import actor_login, format_ts, parse_ts, truncate, utc_now # --- CLI defaults ---------------------------------------------------------- DEFAULT_MODEL = "gpt-5.4-mini" @@ -236,13 +252,12 @@ def role_for(login: str, author: str, reviewers: set[str]) -> str: # human author behind a Copilot-authored PR. _COPILOT_COMMITTER_LOGINS = {"copilot"} _COPILOT_PR_AUTHORS = {"app/copilot-swe-agent", "copilot"} -_COPILOT_REVIEWER_LOGINS = {"copilot", "copilot-pull-request-reviewer", "copilot-pull-request-reviewer[bot]"} _MAINTENANCE_BOT_PR_AUTHORS = {"app/otelbot", "app/renovate"} def reviewer_actor_login(obj: dict[str, Any] | None) -> str: login = actor_login(obj) - if login.lower() in _COPILOT_REVIEWER_LOGINS: + if is_copilot_reviewer(obj): return "copilot-pull-request-reviewer[bot]" return login @@ -376,6 +391,13 @@ def normalize_events(raw: dict[str, Any], author: str, reviewers: set[str]) -> l for c in raw["issue_comments"]: if c.get("minimized"): continue + command_remainder = dashboard_command_body_remainder(c) + # A `/dashboard` command line is control metadata, not discussion. Skip + # the comment only when it is command-only; otherwise keep the author's + # explanation that follows the command as an event. + if command_remainder is not None and not command_remainder: + continue + body = command_remainder if command_remainder is not None else (c.get("body") or "") login = reviewer_actor_login(c.get("user") or {}) timestamp = ( c.get("content_updated_at") @@ -392,7 +414,7 @@ def normalize_events(raw: dict[str, Any], author: str, reviewers: set[str]) -> l "updated_timestamp": timestamp, "actor": login, "actor_role": role_for(login, author, reviewers), - "body": c.get("body") or "", + "body": body, "state": None, "path": None, "sha": None, @@ -521,6 +543,7 @@ def compute_facts( raw: dict[str, Any], author: str, events: list[dict[str, Any]], + reviewers: set[str] | None = None, ) -> dict[str, Any]: pr = raw["pr"] checks = raw["checks"] @@ -536,9 +559,31 @@ def compute_facts( api_author = actor_login(pr.get("author") or {}) assignees = [reviewer_actor_login(a) for a in (pr.get("assignees") or [])] assignees = [a for a in assignees if a] + # Read the head OID straight from the PR object. Deriving it from + # raw["commits"] is wrong for PRs with more than 250 commits, where the + # commits REST endpoint truncates and the last entry is not the real head. + head_sha = pr.get("headRefOid") or "" + copilot_review_exists, copilot_review_needed = copilot_review_status( + raw.get("reviews") or [], + head_sha, + ) + labels = { + label.get("name") or "" + for label in pr.get("labels") or [] + if isinstance(label, dict) + } facts = { "author": author, "assignees": assignees, + "head_sha": head_sha, + "routing_input_fingerprint": routing_input_fingerprint(raw), + **dashboard_override_facts(raw, author, labels, reviewers or set()), + "copilot_review_requested": any( + is_copilot_reviewer(request) + for request in (pr.get("reviewRequests") or []) + ), + "copilot_review_exists": copilot_review_exists, + "copilot_review_needed": copilot_review_needed, "is_maintenance_bot": api_author.lower() in _MAINTENANCE_BOT_PR_AUTHORS, "is_draft": bool(pr.get("isDraft")), "approval_count": current_approval_count(events), @@ -1226,7 +1271,7 @@ def oldest_pending_action_ts( def fallback_wait_ts(route: str, facts: dict[str, Any]) -> tuple[datetime | None, str]: - if route in ("approver", "maintainer"): + if route in ("approver", "maintainer", "copilot"): return parse_ts(facts.get("last_author_activity_at") or ""), "last_author_activity" if route == "author": if facts.get("ci_failing_count", 0) > 0: @@ -1359,6 +1404,50 @@ def add_reviewers( ] +def resolve_pr_route( + facts: dict[str, Any], + pending_actions: dict[str, dict[str, Any]], + required_approvals: int, + require_clean_copilot_review: bool, +) -> str: + # Apply the manual reviewer-routing override before the Copilot review gate + # so an overridden route (for example author -> reviewers) is still held for + # a required clean Copilot review instead of bypassing it. + return apply_copilot_review_gate( + facts, + apply_dashboard_override( + facts, + route_pr(facts, pending_actions, required_approvals), + ), + enabled=require_clean_copilot_review, + ) + + +def assign_author_nudge_episode( + facts: dict[str, Any], + route: str, + previous_result: dict[str, Any] | None, + issue_comments: list[dict[str, Any]], +) -> None: + if route != "author": + facts.pop("author_nudge_episode_id", None) + return + previous_facts = (previous_result or {}).get("facts") or {} + previous_episode_id = ( + previous_facts.get("author_nudge_episode_id") + if (previous_result or {}).get("route") == "author" + else "" + ) + recovered_episode_id = ( + status_author_nudge_episode_id(issue_comments) + if previous_result is None + else "" + ) + facts["author_nudge_episode_id"] = str( + previous_episode_id or recovered_episode_id or uuid.uuid4().hex + ) + + # ---------------------------------------------------------------- main @@ -1372,6 +1461,8 @@ def build_pr_result( required_approvals: int, non_blocking_check_patterns: list[str], previous_top_level_history: dict[str, dict[str, Any]] | None = None, + require_clean_copilot_review_branches: list[str] | None = None, + previous_result: dict[str, Any] | None = None, ) -> dict[str, Any] | None: number = pr_summary["number"] try: @@ -1386,7 +1477,7 @@ def build_pr_result( return None author = effective_author(raw) events = normalize_events(raw, author, reviewers) - facts = compute_facts(raw, author, events) + facts = compute_facts(raw, author, events, reviewers) review_threads = group_review_threads(raw, author, reviewers, facts) top_level_items = derive_top_level_items(events, facts) top_level_author_comment_items = derive_top_level_author_comment_items( @@ -1462,7 +1553,22 @@ def build_pr_result( "route": "unknown", "error": f"{len(failed_classifications)} discussion classification(s) failed", } - route = route_pr(facts, pending_actions, required_approvals) + require_clean_copilot_review = (raw["pr"].get("baseRefName") or "") in ( + require_clean_copilot_review_branches or [] + ) + route = resolve_pr_route( + facts, + pending_actions, + required_approvals, + require_clean_copilot_review, + ) + assign_author_nudge_episode( + facts, + route, + previous_result, + raw.get("issue_comments") or [], + ) + append_route_noop_reply(raw, facts, route) add_wait_age_facts(facts, route, pending_actions) facts["author_action_review_thread_urls"] = author_action_discussion_urls( review_threads, pending_actions @@ -1543,6 +1649,7 @@ def build_dashboard_update_for_pr( required_approvals: int, non_blocking_check_patterns: list[str], dashboard_state: dict[str, Any], + require_clean_copilot_review_branches: list[str] | None = None, ) -> DashboardUpdate: print(f"refreshing dashboard state for PR #{pr_number}", file=sys.stderr) results = results_from_dashboard_state(dashboard_state, open_pr_numbers) @@ -1557,6 +1664,8 @@ def build_dashboard_update_for_pr( required_approvals, non_blocking_check_patterns, previous_top_level_history=(starting_pr_result or {}).get("top_level_history") or {}, + require_clean_copilot_review_branches=require_clean_copilot_review_branches, + previous_result=starting_pr_result, ) if trigger_pr_result is None: results.pop(pr_number, None) @@ -1859,14 +1968,18 @@ def clear_backfill_pr_failure(pr_number: int) -> None: def remove_cached_dashboard_prs( args: argparse.Namespace, pr_numbers_to_remove: set[int], + observed_at: datetime | None = None, ) -> int: if not pr_numbers_to_remove: return 0 dashboard_state = load_dashboard_state_cache() or empty_state() state_prs = dict(dashboard_state.get("prs") or {}) + observed_at = observed_at or utc_now() for number in pr_numbers_to_remove: state_prs.pop(str(number), None) enqueue_status_comment_update(number) + record_author_nudge_observation(number, None, observed_at) + record_copilot_review_observation(number, None, observed_at) dashboard_state["prs"] = state_prs return save_dashboard_update_state(args, dashboard_state, False) @@ -1895,10 +2008,15 @@ def build_targeted_dashboard_update(args: argparse.Namespace) -> DashboardUpdate args.required_approvals, args.non_blocking_check_pattern, loaded_dashboard_state, + getattr(args, "require_clean_copilot_review_branches", []), ) -def apply_targeted_dashboard_update(args: argparse.Namespace, calculation: DashboardUpdate) -> int: +def apply_targeted_dashboard_update( + args: argparse.Namespace, + calculation: DashboardUpdate, + observed_at: datetime | None = None, +) -> int: merged_calculation, dashboard_state_unchanged = merge_dashboard_update_with_latest_state( calculation, args.pr_number, @@ -1912,6 +2030,18 @@ def apply_targeted_dashboard_update(args: argparse.Namespace, calculation: Dashb clear_backfill_pr_failure(args.pr_number) if not dashboard_state_unchanged and args.pr_number is not None: enqueue_status_comment_update(args.pr_number) + if args.pr_number is not None: + observed_at = observed_at or utc_now() + accepted_result = (merged_calculation.dashboard_state.get("prs") or {}).get( + str(args.pr_number) + ) + record_author_nudge_observation( + args.pr_number, + accepted_result, + observed_at, + prepare_due=getattr(args, "prepare_author_nudges", False), + ) + record_copilot_review_observation(args.pr_number, accepted_result, observed_at) return save_dashboard_update_state( args, @@ -1934,10 +2064,11 @@ def update_dashboard_for_pr_number(args: argparse.Namespace, state_dir: Path) -> if update is None: return 0 + observed_at = utc_now() return state_branch.push_state_changes( state_dir, "Update dashboard state", - lambda: apply_targeted_dashboard_update(args, update), + lambda: apply_targeted_dashboard_update(args, update, observed_at), state_branch=args.state_branch, ) @@ -1965,18 +2096,19 @@ def update_dashboard_for_backfill(args: argparse.Namespace, state_dir: Path) -> ) if selection.cached_pr_numbers_to_remove: + observed_at = utc_now() status = state_branch.push_state_changes( state_dir, "Update dashboard state", lambda: remove_cached_dashboard_prs( args, selection.cached_pr_numbers_to_remove, + observed_at, ), state_branch=args.state_branch, ) if status != 0: return status - print( f"backfill selected {len(selection.selected_prs)} PR(s) " f"in {repo} (max={DEFAULT_BACKFILL_MAX_PRS})", @@ -1999,6 +2131,8 @@ def save_current_dashboard_state() -> int: ) for pr_summary in selection.selected_prs: + observed_at = utc_now() + def update_selected_pr(pr_summary: dict[str, Any] = pr_summary) -> int: pr_number = pr_summary["number"] dashboard_state = load_dashboard_state_cache() or empty_state() @@ -2013,6 +2147,7 @@ def update_selected_pr(pr_summary: dict[str, Any] = pr_summary) -> int: args.required_approvals, args.non_blocking_check_pattern, dashboard_state, + getattr(args, "require_clean_copilot_review_branches", []), ) calculation, dashboard_state_unchanged = merge_dashboard_update_with_latest_state( calculation, @@ -2031,6 +2166,17 @@ def update_selected_pr(pr_summary: dict[str, Any] = pr_summary) -> int: dashboard_state, not initial_backfill_completed, ) + record_author_nudge_observation( + pr_number, + (calculation.dashboard_state.get("prs") or {}).get(str(pr_number)), + observed_at, + prepare_due=getattr(args, "prepare_author_nudges", False), + ) + record_copilot_review_observation( + pr_number, + (calculation.dashboard_state.get("prs") or {}).get(str(pr_number)), + observed_at, + ) failed_pr_numbers = update_backfill_progress(pr_number, failed=False) if not dashboard_state_unchanged: enqueue_status_comment_update(pr_number) @@ -2106,6 +2252,19 @@ def main() -> int: default=[], help="glob matching a non-required check to mention when it fails; repeat as needed", ) + parser.add_argument( + "--require-clean-copilot-review-branch", + action="append", + default=[], + dest="require_clean_copilot_review_branches", + metavar="BRANCH", + help="require a clean Copilot review before reviewer or maintainer handoff for PRs targeting this base branch; repeat as needed", + ) + parser.add_argument( + "--prepare-author-nudges", + action="store_true", + help="queue due author nudges from accepted dashboard results", + ) parser.add_argument("--model", default=DEFAULT_MODEL, help=f"copilot model (default: {DEFAULT_MODEL})") parser.add_argument( "--github-output", diff --git a/.github/scripts/pull-request-dashboard/dashboard_override.py b/.github/scripts/pull-request-dashboard/dashboard_override.py new file mode 100644 index 00000000000..196bb58a449 --- /dev/null +++ b/.github/scripts/pull-request-dashboard/dashboard_override.py @@ -0,0 +1,451 @@ +"""Recognize and deliver explicit reviewer-routing overrides.""" + +from __future__ import annotations + +import re +from typing import Any +from urllib.parse import quote + +from github_cli import gh_api, run_gh +from state import load_dashboard_state_cache +from utils import actor_login + + +DASHBOARD_OVERRIDE_LABEL = "dashboard:route-overridden" +DASHBOARD_COMMAND_PREFIX = "/dashboard" +DASHBOARD_OVERRIDE_COMMAND = "/dashboard route:reviewers" +DASHBOARD_OVERRIDE_SUBCOMMAND = "route:reviewers" +DASHBOARD_OVERRIDE_LABEL_COLOR = "1D76DB" +DASHBOARD_OVERRIDE_LABEL_DESCRIPTION = "Routing manually overridden to reviewers" +# Mirrors pr_status_comment.DASHBOARD_APP_SLUG; duplicated here to avoid a +# circular import between the two modules. +DASHBOARD_APP_SLUG = "opentelemetry-pr-dashboard" +COMMAND_REPLY_MARKER_PREFIX = "" +) +OVERRIDE_ACK_MARKER_PREFIX = "" +) +PRE_REVIEW_ROUTES = ("author", "external") +REVIEWERS_OR_LATER_ROUTES = ("approver", "maintainer") + + +def author_override_guidance( + staleness_note: str = "", + *, + route: str = "author", +) -> str: + waiting_on = ( + "an external dependency or decision" + if route == "external" + else "the author" + ) + guidance = ( + "If you believe this pull request is incorrectly routed as waiting on " + f"{waiting_on}, comment `/dashboard route:reviewers` to route it from " + f"waiting on {waiting_on} to waiting on reviewers." + ) + if staleness_note: + guidance = f"{guidance} {staleness_note}" + return guidance + + +def parse_dashboard_command(comment: dict[str, Any]) -> str | None: + """Return the subcommand of a `/dashboard` command, or None. + + Returns the (possibly empty) subcommand token when the comment's first + line is a `/dashboard` command, and None when it is not a command at all. + """ + if comment.get("minimized"): + return None + lines = (comment.get("body") or "").strip().splitlines() + if not lines: + return None + tokens = lines[0].strip().split() + if not tokens or tokens[0] != DASHBOARD_COMMAND_PREFIX: + return None + return tokens[1] if len(tokens) > 1 else "" + + +def dashboard_command_body_remainder(comment: dict[str, Any]) -> str | None: + """Return the comment body after a leading `/dashboard` command. + + Returns None when the comment is not a `/dashboard` command, and the + (possibly empty) text after the subcommand otherwise. This lets callers + keep an author's explanation on the same or later lines while treating the + command tokens themselves as control metadata. + """ + if parse_dashboard_command(comment) is None: + return None + lines = (comment.get("body") or "").strip().splitlines() + first_line = lines[0].strip().split(maxsplit=2) + return "\n".join([first_line[2] if len(first_line) > 2 else "", *lines[1:]]).strip() + + +def is_authorized_commander(login: str, author: str, reviewers: set[str] | None) -> bool: + low = (login or "").lower() + return bool(low) and (low == author.lower() or low in (reviewers or set())) + + +def latest_authorized_command( + raw: dict[str, Any], + author: str, + reviewers: set[str] | None, +) -> tuple[int, str]: + acknowledged_id = _acknowledged_override_command_id(raw.get("issue_comments")) + best_id = 0 + best_user = "" + for comment in raw.get("issue_comments") or []: + if parse_dashboard_command(comment) != DASHBOARD_OVERRIDE_SUBCOMMAND: + continue + commenter = actor_login(comment.get("user") or {}) + if not is_authorized_commander(commenter, author, reviewers): + continue + try: + comment_id = int(comment.get("id")) + except (TypeError, ValueError): + continue + if comment_id <= acknowledged_id: + continue + if comment_id > best_id: + best_id, best_user = comment_id, commenter + return best_id, best_user + + +def dashboard_override_facts( + raw: dict[str, Any], + author: str, + labels: set[str], + reviewers: set[str] | None = None, +) -> dict[str, Any]: + label_applied = DASHBOARD_OVERRIDE_LABEL in labels + command_id, command_user = latest_authorized_command(raw, author, reviewers) + command_pending = bool(command_id) + return { + "dashboard_override": label_applied, + "dashboard_override_label_applied": label_applied, + "dashboard_override_command_id": command_id, + "dashboard_override_command_user": command_user, + "dashboard_override_requested": command_pending and not label_applied, + "dashboard_override_release_requested": False, + "dashboard_command_replies": pending_command_replies(raw, author, reviewers), + } + + +def _is_dashboard_app_comment(comment: dict[str, Any]) -> bool: + """Whether a comment was authored by the dashboard GitHub App. + + Handles both the REST shape (``performed_via_github_app.slug``) and the + normalized GraphQL shape from ``fetch_pr_issue_comments`` (a bot + ``user.login`` of ``[bot]``). + """ + if (comment.get("performed_via_github_app") or {}).get("slug") == DASHBOARD_APP_SLUG: + return True + return (comment.get("user") or {}).get("login") == f"{DASHBOARD_APP_SLUG}[bot]" + + +def _replied_command_ids(comments: list[dict[str, Any]] | None) -> set[int]: + """Command ids already answered by a dashboard-app reply comment. + + Reply markers are matched only in comments authored by the dashboard app, so + a user cannot forge a `command-reply` marker to suppress an owed reply. + """ + replied_ids: set[int] = set() + for comment in comments or []: + if not _is_dashboard_app_comment(comment): + continue + for match in _COMMAND_REPLY_MARKER_RE.findall(comment.get("body") or ""): + replied_ids.add(int(match)) + return replied_ids + + +def _acknowledged_override_command_id( + comments: list[dict[str, Any]] | None, +) -> int: + acknowledged_id = 0 + for comment in comments or []: + if not _is_dashboard_app_comment(comment): + continue + for match in _OVERRIDE_ACK_MARKER_RE.findall(comment.get("body") or ""): + acknowledged_id = max(acknowledged_id, int(match)) + return acknowledged_id + + +def pending_command_replies( + raw: dict[str, Any], + author: str, + reviewers: set[str] | None = None, +) -> list[dict[str, Any]]: + """Return replies owed to unsupported or unauthorized `/dashboard` commands. + + `/dashboard route:reviewers` from the author or an approver is handled by the + override flow and never gets a reply here. The same command from anyone else + gets an unauthorized reply, and any unrecognized `/dashboard` subcommand gets + an unknown-command reply. Commands that already have a reply comment are + skipped so replies are posted at most once. + """ + comments = raw.get("issue_comments") or [] + replied_ids = _replied_command_ids(comments) + + replies: list[dict[str, Any]] = [] + for comment in comments: + subcommand = parse_dashboard_command(comment) + if subcommand is None: + continue + try: + comment_id = int(comment.get("id")) + except (TypeError, ValueError): + continue + if comment_id in replied_ids: + continue + commenter = actor_login(comment.get("user") or {}) + if subcommand == DASHBOARD_OVERRIDE_SUBCOMMAND: + if is_authorized_commander(commenter, author, reviewers): + continue + kind = "unauthorized" + else: + kind = "unknown_command" + replies.append({ + "comment_id": comment_id, + "kind": kind, + "user": commenter, + "subcommand": subcommand, + }) + return replies + + +def command_reply_marker(comment_id: int) -> str: + return f"{COMMAND_REPLY_MARKER_PREFIX}{comment_id} -->" + + +def override_ack_marker(comment_id: int) -> str: + return f"{OVERRIDE_ACK_MARKER_PREFIX}{comment_id} -->" + + +ROUTE_ALREADY_ROUTED_PHRASE = { + "approver": "already waiting on reviewers", + "maintainer": "already past review and waiting on maintainers", + "external": "waiting on an external dependency, not on you", + "copilot": "waiting on an automated Copilot review", +} + + +def render_command_reply(reply: dict[str, Any]) -> str: + user = reply.get("user") or "" + mention = f"@{user} " if user else "" + kind = reply.get("kind") + if kind == "unauthorized": + message = ( + "only the pull request author or a member of an approving team can " + "use `/dashboard route:reviewers`." + ) + elif kind == "routed": + if reply.get("route") == "copilot": + message = ( + "accepted the reviewer-routing override; the reviewer handoff " + "is waiting on Copilot." + ) + else: + message = "routed this pull request to reviewers." + elif kind == "already_routed": + where = ROUTE_ALREADY_ROUTED_PHRASE.get( + reply.get("route") or "", "not currently waiting on you" + ) + message = ( + f"this pull request is {where}, so `/dashboard route:reviewers` had " + "no effect. The command only applies while the pull request is " + "waiting on you." + ) + else: + subcommand = reply.get("subcommand") or "" + attempted = DASHBOARD_COMMAND_PREFIX + (f" {subcommand}" if subcommand else "") + message = ( + f"`{attempted}` is not a recognized dashboard command. The only " + "supported command is `/dashboard route:reviewers`, which the pull " + "request author can use to move a pull request from waiting on the " + "author to waiting on reviewers." + ) + comment_id = int(reply["comment_id"]) + markers = [command_reply_marker(comment_id)] + if kind in ("routed", "already_routed"): + markers.append(override_ack_marker(comment_id)) + return "\n".join([ + *markers, + f"{mention}{message}", + "", + ]) + + +def command_reply_exists( + comments: list[dict[str, Any]] | None, + comment_id: int, +) -> bool: + marker = command_reply_marker(comment_id) + return any( + (comment.get("performed_via_github_app") or {}).get("slug") == DASHBOARD_APP_SLUG + and marker in (comment.get("body") or "") + for comment in comments or [] + ) + + +def ensure_command_reply( + repo: str, + pr_number: int, + reply: dict[str, Any], +) -> None: + comments = gh_api( + f"/repos/{repo}/issues/{pr_number}/comments?per_page=100", + paginate=True, + ) + if command_reply_exists(comments, int(reply["comment_id"])): + return + run_gh([ + "gh", "api", "--method", "POST", + f"repos/{repo}/issues/{pr_number}/comments", + "-f", f"body={render_command_reply(reply)}", + ]) + + +def deliver_dashboard_command_replies(repo: str) -> list[str]: + dashboard_state = load_dashboard_state_cache() + if dashboard_state is None: + return [] + errors: list[str] = [] + for key, result in sorted( + (dashboard_state.get("prs") or {}).items(), + key=lambda item: int(item[0]), + ): + replies = ((result or {}).get("facts") or {}).get("dashboard_command_replies") or [] + if not replies: + continue + pr_number = int(key) + try: + comments = gh_api( + f"/repos/{repo}/issues/{pr_number}/comments?per_page=100", + paginate=True, + ) + except Exception as e: + errors.append(f"PR #{pr_number}: {e}") + continue + for reply in replies: + try: + if command_reply_exists(comments, int(reply["comment_id"])): + continue + run_gh([ + "gh", "api", "--method", "POST", + f"repos/{repo}/issues/{pr_number}/comments", + "-f", f"body={render_command_reply(reply)}", + ]) + except Exception as e: + errors.append(f"PR #{pr_number}: {e}") + return errors + + +def apply_dashboard_override(facts: dict[str, Any], route: str) -> str: + label_applied = bool(facts.get("dashboard_override_label_applied")) + requested = bool(facts.get("dashboard_override_requested")) + command_pending = bool(facts.get("dashboard_override_command_id")) + # The override only takes effect before review, while automatic routing waits + # on the author or an external dependency. On every later route the natural + # routing stands. + override_applies = route in PRE_REVIEW_ROUTES and (label_applied or requested) + # A command that does not newly move the pull request to reviewers is a no-op; + # the author is told where it is routed. This covers both a non-overridable + # route and an existing label that already provides the reviewer handoff. + facts["dashboard_override_noop"] = command_pending and ( + label_applied or not override_applies + ) + if requested and not override_applies: + facts["dashboard_override_requested"] = False + facts["dashboard_override"] = override_applies + # Release the label once automatic routing reaches or passes reviewers, so a + # forgotten override cannot pin the pull request at reviewers or drag it back + # from maintainers. + facts["dashboard_override_release_requested"] = ( + label_applied and route in REVIEWERS_OR_LATER_ROUTES + ) + return "approver" if override_applies else route + + +def append_route_noop_reply( + raw: dict[str, Any], + facts: dict[str, Any], + route: str, +) -> None: + if not facts.get("dashboard_override_noop"): + return + command_id = int(facts.get("dashboard_override_command_id") or 0) + if not command_id: + return + replied_ids = _replied_command_ids(raw.get("issue_comments") or []) + if command_id in replied_ids: + return + replies = facts.setdefault("dashboard_command_replies", []) + if any(reply.get("comment_id") == command_id for reply in replies): + return + replies.append({ + "comment_id": command_id, + "kind": "already_routed", + "user": facts.get("dashboard_override_command_user") or facts.get("author") or "", + "route": route, + }) + + +def deliver_dashboard_override_requests(repo: str) -> list[str]: + dashboard_state = load_dashboard_state_cache() + if dashboard_state is None: + return [] + pr_results = sorted( + (dashboard_state.get("prs") or {}).items(), + key=lambda item: int(item[0]), + ) + if any( + ((result or {}).get("facts") or {}).get("dashboard_override_requested") + for _key, result in pr_results + ): + try: + run_gh([ + "gh", "label", "create", DASHBOARD_OVERRIDE_LABEL, + "--repo", repo, + "--color", DASHBOARD_OVERRIDE_LABEL_COLOR, + "--description", DASHBOARD_OVERRIDE_LABEL_DESCRIPTION, + "--force", + ]) + except Exception as e: + return [f"label: {e}"] + + errors: list[str] = [] + for key, result in pr_results: + facts = (result or {}).get("facts") or {} + pr_number = int(key) + if facts.get("dashboard_override_requested"): + try: + run_gh([ + "gh", "api", "--method", "POST", + f"repos/{repo}/issues/{pr_number}/labels", + "-f", f"labels[]={DASHBOARD_OVERRIDE_LABEL}", + ]) + ensure_command_reply( + repo, + pr_number, + { + "comment_id": facts["dashboard_override_command_id"], + "kind": "routed", + "route": (result or {}).get("route") or "", + "user": facts.get("dashboard_override_command_user") or "", + }, + ) + except Exception as e: + errors.append(f"PR #{pr_number}: {e}") + elif facts.get("dashboard_override_release_requested"): + try: + run_gh([ + "gh", "api", "--method", "DELETE", + f"repos/{repo}/issues/{pr_number}/labels/{quote(DASHBOARD_OVERRIDE_LABEL)}", + ]) + except Exception as e: + if "404" not in str(e): + errors.append(f"PR #{pr_number}: {e}") + return errors \ No newline at end of file diff --git a/.github/scripts/pull-request-dashboard/delivery.py b/.github/scripts/pull-request-dashboard/delivery.py new file mode 100644 index 00000000000..131f0ad8bd0 --- /dev/null +++ b/.github/scripts/pull-request-dashboard/delivery.py @@ -0,0 +1,168 @@ +#!/usr/bin/env python3 +"""Deliver dashboard side effects from accepted state in one CAS transaction.""" + +from __future__ import annotations + +import argparse +import os +from pathlib import Path +import sys +import traceback +from typing import Callable + +from author_nudge import deliver_prepared_author_nudges +from copilot_review import deliver_copilot_review_requests +from dashboard_override import ( + deliver_dashboard_command_replies, + deliver_dashboard_override_requests, +) +from github_cli import detect_repo, list_open_prs, normalize_repo, repo_state_key +from notify_slack import notify_slack_from_state +from pr_status_comment import update_status_comments_from_state +from state import ( + author_nudge_state_path, + copilot_review_request_state_path, + notification_state_path, + set_state_dir, +) +import state_branch +from utils import utc_now + + +def runner_temp_path(name: str) -> Path: + return Path(os.environ.get("RUNNER_TEMP", ".")) / name + + +def run_delivery_action( + label: str, + action: Callable[[], list[str]], + errors: list[str], +) -> None: + try: + errors.extend(f"{label}: {error}" for error in action()) + except Exception as e: + # Keep the traceback in the job log so a failed stage is diagnosable; + # the short message alone is rarely enough in production. + print(f"{label} raised an exception:", file=sys.stderr) + traceback.print_exc() + errors.append(f"{label}: {e}") + + +def deliver_from_state( + repo: str, + author_retry_snapshot_path: Path, + copilot_retry_snapshot_path: Path, + notification_retry_snapshot_path: Path, +) -> list[str]: + now = utc_now() + errors: list[str] = [] + try: + open_prs = list_open_prs(repo) + except Exception as e: + errors.append(f"open pull requests: {e}") + open_prs = None + run_delivery_action( + "dashboard overrides", + lambda: deliver_dashboard_override_requests(repo), + errors, + ) + run_delivery_action( + "dashboard command replies", + lambda: deliver_dashboard_command_replies(repo), + errors, + ) + run_delivery_action( + "author nudges", + lambda: deliver_prepared_author_nudges( + repo, + now, + author_retry_snapshot_path, + ), + errors, + ) + if open_prs is not None: + run_delivery_action( + "status comments", + lambda: update_status_comments_from_state( + repo, + {pr["number"] for pr in open_prs}, + ), + errors, + ) + run_delivery_action( + "Copilot reviews", + lambda: deliver_copilot_review_requests(repo, now, copilot_retry_snapshot_path), + errors, + ) + if open_prs is not None: + run_delivery_action( + "Slack notifications", + lambda: notify_slack_from_state( + repo, + notification_retry_snapshot_path, + open_prs, + now, + ), + errors, + ) + return errors + + +def deliver_with_state( + repo: str, + state_branch_name: str, + state_dir: Path, +) -> int: + repo_key = repo_state_key(repo) + author_retry = runner_temp_path("prior-author-nudge-state.json") + copilot_retry = runner_temp_path("prior-copilot-review-request-state.json") + notification_retry = runner_temp_path("prior-notification-state.json") + errors: list[str] = [] + + def deliver() -> int: + errors[:] = deliver_from_state( + repo, + author_retry, + copilot_retry, + notification_retry, + ) + return 0 + + status = state_branch.push_state_changes( + state_dir, + "Deliver pull request dashboard updates", + deliver, + state_branch=state_branch_name, + add_paths=[repo_key], + retry_snapshots=[ + (author_nudge_state_path(), author_retry), + (copilot_review_request_state_path(), copilot_retry), + (notification_state_path(), notification_retry), + ], + ) + if status != 0: + return status + if not errors: + return 0 + print("Dashboard delivery failed:", file=sys.stderr) + print("\n".join(errors), file=sys.stderr) + return 1 + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--repo", help="target repository name") + parser.add_argument("--state-branch", required=True, help="git branch used for workflow state") + args = parser.parse_args() + repo = normalize_repo(args.repo) if args.repo else detect_repo() + with state_branch.temporary_state_dir() as state_dir: + set_state_dir(state_dir / repo_state_key(repo)) + return deliver_with_state( + repo, + args.state_branch, + state_dir, + ) + + +if __name__ == "__main__": + sys.exit(main()) \ No newline at end of file diff --git a/.github/scripts/pull-request-dashboard/github_cli.py b/.github/scripts/pull-request-dashboard/github_cli.py index 5e9559a1eed..8f6e719ce06 100644 --- a/.github/scripts/pull-request-dashboard/github_cli.py +++ b/.github/scripts/pull-request-dashboard/github_cli.py @@ -12,6 +12,22 @@ GH_RETRY_ATTEMPTS = 4 GH_RETRY_DELAY_SECONDS = 1.5 DEFAULT_OWNER = "open-telemetry" +COPILOT_REVIEWER_BOT_ID = "BOT_kgDOCnlnWA" + + +REQUEST_COPILOT_REVIEW_MUTATION = """ +mutation($pullRequestId: ID!, $botId: ID!) { + requestReviews(input: { + pullRequestId: $pullRequestId + botIds: [$botId] + union: true + }) { + pullRequest { + id + } + } +} +""" def normalize_repo(repo: str) -> str: @@ -98,6 +114,16 @@ def gh_api(path: str, paginate: bool = False, token: str | None = None) -> Any: return data +def request_copilot_review(pull_request_id: str) -> None: + gh_graphql( + REQUEST_COPILOT_REVIEW_MUTATION, + { + "pullRequestId": pull_request_id, + "botId": COPILOT_REVIEWER_BOT_ID, + }, + ) + + def gh_graphql(query: str, fields: dict[str, Any], token: str | None = None) -> dict[str, Any]: cmd = ["gh", "api", "graphql", "-f", f"query={query}"] for name, value in fields.items(): @@ -109,9 +135,9 @@ def gh_graphql(query: str, fields: dict[str, Any], token: str | None = None) -> def gh_pr_view(repo: str, number: int) -> dict[str, Any]: fields = ",".join([ - "id", "number", "title", "url", "author", "state", "isDraft", - "mergeable", "mergeStateStatus", "createdAt", "updatedAt", - "reviewDecision", "assignees", "baseRefName", + "id", "number", "title", "body", "url", "author", "state", "isDraft", + "mergeable", "mergeStateStatus", "createdAt", "updatedAt", "headRefOid", + "reviewDecision", "reviewRequests", "assignees", "baseRefName", "labels", ]) cmd = ["gh", "pr", "view", str(number), "--repo", repo, "--json", fields] last: dict[str, Any] = {} @@ -139,6 +165,12 @@ def gh_pr_view(repo: str, number: int) -> dict[str, Any]: } nodes { fullDatabaseId + commit { + oid + } + comments { + totalCount + } url body state @@ -307,6 +339,10 @@ def fetch_pr_review_data(owner: str, repo_name: str, number: int) -> dict[str, A continue reviews.append({ "id": database_id, + "commit_id": ((review.get("commit") or {}).get("oid") or ""), + "finding_count": int( + (review.get("comments") or {}).get("totalCount") or 0 + ), "url": review.get("url") or "", "user": review.get("author") or {}, "state": review.get("state") or "", @@ -581,14 +617,6 @@ def list_open_prs(repo: str) -> list[dict[str, Any]]: ] -def list_all_open_pr_numbers(repo: str) -> set[int]: - return { - pull["number"] - for pull in _list_open_pulls(repo) - if isinstance(pull, dict) and isinstance(pull.get("number"), int) - } - - def detect_repo() -> str: proc = subprocess.run( ["gh", "repo", "view", "--json", "nameWithOwner", "-q", ".nameWithOwner"], diff --git a/.github/scripts/pull-request-dashboard/netlify/functions/github-webhook.js b/.github/scripts/pull-request-dashboard/netlify/functions/github-webhook.js index 13df9bdfd99..0fa37ab3358 100644 --- a/.github/scripts/pull-request-dashboard/netlify/functions/github-webhook.js +++ b/.github/scripts/pull-request-dashboard/netlify/functions/github-webhook.js @@ -15,11 +15,15 @@ const ALLOWED_ACTIONS = { "closed", "converted_to_draft", "edited", + "labeled", "opened", "ready_for_review", "reopened", + "review_request_removed", + "review_requested", "synchronize", "unassigned", + "unlabeled", ]), issue_comment: new Set(["created", "edited", "deleted"]), pull_request_review: new Set(["submitted", "edited", "dismissed"]), @@ -37,6 +41,7 @@ exports.handler = async (event) => { }; exports.isDashboardSelfTriggeredCommentEvent = isDashboardSelfTriggeredCommentEvent; +exports.isAllowedAction = isAllowedAction; async function handle(event) { if (event.httpMethod !== "POST") { @@ -64,7 +69,7 @@ async function handle(event) { const payload = parseJson(rawBody); const action = payload.action; - if (!ALLOWED_ACTIONS[eventName].has(action)) { + if (!isAllowedAction(eventName, action)) { return response(202, { status: "ignored", reason: `unsupported action: ${eventName}.${action || "missing"}` }); } if (isDashboardSelfTriggeredCommentEvent(eventName, payload)) { @@ -101,6 +106,10 @@ async function handle(event) { }); } +function isAllowedAction(eventName, action) { + return Boolean(ALLOWED_ACTIONS[eventName] && ALLOWED_ACTIONS[eventName].has(action)); +} + function isDashboardSelfTriggeredCommentEvent(eventName, payload) { if (eventName !== "issue_comment") { return false; diff --git a/.github/scripts/pull-request-dashboard/notifications.py b/.github/scripts/pull-request-dashboard/notifications.py index 0fb5dc2d4bb..8044ee0e651 100644 --- a/.github/scripts/pull-request-dashboard/notifications.py +++ b/.github/scripts/pull-request-dashboard/notifications.py @@ -190,8 +190,6 @@ def next_notifications( results: dict[int, dict[str, Any]], last_notifications: dict[str, Any] | None, now: datetime, - notification_numbers: set[int] | None = None, - notification_kinds: set[str] | None = None, ) -> tuple[dict[str, Any], list[str]]: has_last_notifications = last_notifications is not None previous_notifications = last_notifications or {} @@ -208,11 +206,6 @@ def next_notifications( pr_key = str(number) last_pr_notification = migrated_pr_notification(previous_notifications.get(pr_key) or {}) - if notification_numbers is not None and number not in notification_numbers: - if last_pr_notification: - updated_notifications[pr_key] = last_pr_notification - continue - route = result.get("route") or "unknown" if result.get("failed") or route in ("transient-failure", "unknown"): if last_pr_notification: @@ -238,8 +231,6 @@ def next_notifications( kind = pending_notification_kind( notification_baseline, current_waiting_since, now, ) - if kind and notification_kinds is not None and kind not in notification_kinds: - kind = None updated_pr_notification: dict[str, Any] = { "last_notified_at": last_pr_notification.get("last_notified_at") or "", diff --git a/.github/scripts/pull-request-dashboard/notify_slack.py b/.github/scripts/pull-request-dashboard/notify_slack.py index aeed3fbb1d3..d0bd319382e 100644 --- a/.github/scripts/pull-request-dashboard/notify_slack.py +++ b/.github/scripts/pull-request-dashboard/notify_slack.py @@ -3,26 +3,20 @@ from __future__ import annotations -import argparse -import os import sys from pathlib import Path from typing import Any +from datetime import datetime -from github_cli import detect_repo, list_open_prs, normalize_repo, repo_state_key from notifications import next_notifications from state import ( load_dashboard_state_cache, load_notification_state_file, load_notifications, - notification_state_path, results_from_dashboard_state, save_notifications, - set_state_dir, union_merge_notifications, ) -import state_branch -from utils import utc_now def last_notifications( @@ -45,18 +39,17 @@ def last_notifications( def notify_slack_from_state( repo: str, retry_snapshot_path: Path | None, - notification_numbers: set[int] | None, - notification_kinds: set[str] | None, + open_prs: list[dict[str, Any]], + now: datetime, ) -> list[str]: dashboard_state = load_dashboard_state_cache() if dashboard_state is None: print("dashboard state not found; skipping Slack notifications", file=sys.stderr) return [] - prs = list_open_prs(repo) - open_pr_numbers = {p["number"] for p in prs if not p.get("isDraft")} + open_pr_numbers = {p["number"] for p in open_prs if not p.get("isDraft")} results = results_from_dashboard_state(dashboard_state, open_pr_numbers) - current_prs = {p["number"]: p for p in prs} + current_prs = {p["number"]: p for p in open_prs} for number, result in results.items(): result["pr_title"] = current_prs.get(number, {}).get("title") or "" @@ -66,9 +59,7 @@ def notify_slack_from_state( repo, results, last_notifications(saved_notifications, retry_snapshot_path), - utc_now(), - notification_numbers=notification_numbers, - notification_kinds=notification_kinds, + now, ) notifications_changed = updated_notifications != (saved_notifications or {}) if not notifications_changed and saved_notifications is not None: @@ -77,79 +68,3 @@ def notify_slack_from_state( save_notifications(updated_notifications) return delivery_errors - - -def notification_retry_snapshot_path() -> Path: - return Path(os.environ.get("RUNNER_TEMP", ".")) / "prior-notification-state.json" - - -def delivery_errors_path() -> Path: - return Path(os.environ.get("RUNNER_TEMP", ".")) / "notification-errors.txt" - - -def notify_slack( - repo: str, - retry_snapshot_path: Path, - delivery_errors_file: Path, - notification_numbers: set[int] | None, - notification_kinds: set[str] | None, -) -> int: - errors = notify_slack_from_state(repo, retry_snapshot_path, notification_numbers, notification_kinds) - if errors: - delivery_errors_file.write_text("\n".join(errors) + "\n", encoding="utf-8") - else: - delivery_errors_file.unlink(missing_ok=True) - return 0 - - -def notify_slack_with_state(args: argparse.Namespace, state_dir: Path) -> int: - repo_key = repo_state_key(args.repo) if args.repo else repo_state_key(detect_repo()) - retry_snapshot_path = notification_retry_snapshot_path() - delivery_errors_file = delivery_errors_path() - delivery_errors_file.unlink(missing_ok=True) - notification_numbers = {args.pr_number} if args.pr_number is not None else None - notification_kinds = {"initial"} if args.pr_number is not None else {"follow-up"} - status = state_branch.push_state_changes( - state_dir, - "Update dashboard notification state", - lambda: notify_slack( - normalize_repo(args.repo) if args.repo else detect_repo(), - retry_snapshot_path, - delivery_errors_file, - notification_numbers, - notification_kinds, - ), - state_branch=args.state_branch, - add_paths=[f"{repo_key}/notification-state.json"], - retry_snapshots=[(notification_state_path(), retry_snapshot_path)], - ) - if status != 0: - return status - if not delivery_errors_file.exists(): - return 0 - print("Slack notification delivery failed:", file=sys.stderr) - print(delivery_errors_file.read_text(encoding="utf-8").rstrip(), file=sys.stderr) - return 1 - - -def main() -> int: - parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument("--repo", help="target repository name, e.g. opentelemetry-java-instrumentation") - parser.add_argument( - "--state-branch", - required=True, - help="git branch used for workflow state", - ) - parser.add_argument( - "--pr-number", - type=int, - help="send initial notifications for one pull request; omit to send due follow-up notifications only", - ) - args = parser.parse_args() - with state_branch.temporary_state_dir() as state_dir: - set_state_dir(state_dir / (repo_state_key(args.repo) if args.repo else repo_state_key(detect_repo()))) - return notify_slack_with_state(args, state_dir) - - -if __name__ == "__main__": - sys.exit(main()) diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index ec1f124a7f4..23ad315c663 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -3,38 +3,36 @@ from __future__ import annotations -import argparse -import os +import re import sys -from pathlib import Path from typing import Any from urllib.parse import urlencode from github_cli import ( - detect_repo, gh_api, - list_all_open_pr_numbers, - normalize_repo, - repo_state_key, run_gh, ) +from dashboard_override import PRE_REVIEW_ROUTES, author_override_guidance from route_presentation import route_status_summary from state import ( load_dashboard_state_cache, load_status_comment_rollout_state, save_status_comment_rollout_state, - set_state_dir, - status_comment_rollout_state_path, ) from utils import markdown_escape, truncate -import state_branch from utils import utc_now STATUS_MARKER = "" +AUTHOR_NUDGE_EPISODE_MARKER_PREFIX = ( + "" +) # Increment whenever render_status_comment changes in a way existing comments # need to adopt. Hourly runs durably roll the revision out to all open PRs. -STATUS_COMMENT_REVISION = 9 +STATUS_COMMENT_REVISION = 12 STATUS_COMMENT_ROLLOUT_BATCH_SIZE = 50 AUTHOR_ACTION_FEEDBACK_LINK_LIMIT = 20 NON_BLOCKING_CHECK_FAILURE_LIMIT = 20 @@ -42,8 +40,8 @@ STATUS_REPORT_ISSUE_URL = "https://github.com/open-telemetry/shared-workflows/issues/new" STATUS_REPORT_ISSUE_TEMPLATE = "incorrect-pr-dashboard-result.md" AUTHOR_GUIDANCE = ( - "For each item, link to the commit that addresses it, explain why no change is needed, " - "or ask a follow-up question." + "For each item, reply to move the discussion forward, e.g. link to the commit " + "that addresses it, explain why no change is needed, or ask a follow-up question." ) DASHBOARD_APP_SLUG = "opentelemetry-pr-dashboard" # Remove after migrating open PRs as described by the post-rollout @@ -54,17 +52,49 @@ ) -def accuracy_note(pr: dict[str, Any]) -> str: +def author_nudge_episode_marker(episode_id: str) -> str: + return f"{AUTHOR_NUDGE_EPISODE_MARKER_PREFIX}{episode_id} -->" + + +def is_dashboard_app_comment(comment: dict[str, Any]) -> bool: + app_slug = (comment.get("performed_via_github_app") or {}).get("slug") or "" + author_login = (comment.get("user") or {}).get("login") or "" + return app_slug == DASHBOARD_APP_SLUG or author_login == f"{DASHBOARD_APP_SLUG}[bot]" + + +def status_author_nudge_episode_id( + comments: list[dict[str, Any]] | None, +) -> str: + for comment in comments or []: + body = comment.get("body") or "" + match = _AUTHOR_NUDGE_EPISODE_MARKER_RE.search(body) + if ( + match + and STATUS_MARKER in body + and is_dashboard_app_comment(comment) + ): + return match.group(1) + return "" + + +def accuracy_note(pr: dict[str, Any], override_route: str = "") -> str: query = urlencode({ "template": STATUS_REPORT_ISSUE_TEMPLATE, "title": "PR dashboard result looks incorrect", "body": f"PR: {pr.get('html_url') or ''}\n\nWhat looks incorrect:\n", }) report_url = f"{STATUS_REPORT_ISSUE_URL}?{query}" - return ( - "_This automated status or its linked feedback items may be incorrect. " - f"If something looks wrong, [report it]({report_url}) with the result you expected._" + note = ( + "This automated status or its linked feedback items may be incorrect. " + f"If something looks wrong, please [report it]({report_url}) with the result you expected." ) + if override_route: + note += " " + author_override_guidance( + "If the last refreshed time above predates your latest reply or " + "push, the dashboard hasn't processed it yet.", + route=override_route, + ) + return f"_{note}_" def render_status_comment( @@ -106,6 +136,7 @@ def render_status_comment( ) feedback_indent: str | None = None + override_route = "" if pr.get("merged"): summary = ["- **Status:** Merged."] @@ -123,6 +154,8 @@ def render_status_comment( ] else: route = result.get("route") or "unknown" + if route in PRE_REVIEW_ROUTES: + override_route = route if route == "author": waiting_on, fallback_next_step = route_status_summary(route) check_action = None @@ -187,6 +220,9 @@ def render_status_comment( "", *summary, ] + episode_id = str(facts.get("author_nudge_episode_id") or "") + if (result or {}).get("route") == "author" and episode_id: + lines.insert(2, author_nudge_episode_marker(episode_id)) if feedback_indent is not None and feedback_count: lines.extend( @@ -197,7 +233,7 @@ def render_status_comment( ) ) lines.append("") - lines.append(accuracy_note(pr)) + lines.append(accuracy_note(pr, override_route)) lines.append("") return "\n".join(lines) @@ -318,8 +354,7 @@ def prepare_rollout_state( def update_status_comments_from_state( repo: str, - pr_number: int | None, - open_pr_numbers: set[int] | None, + open_pr_numbers: set[int], ) -> list[str]: dashboard_state = load_dashboard_state_cache() if dashboard_state is None: @@ -327,21 +362,10 @@ def update_status_comments_from_state( return [] saved_rollout_state = load_status_comment_rollout_state() - if open_pr_numbers is None: - raise RuntimeError("open PR numbers are required for a status comment update") + queued_pr_numbers = set(saved_rollout_state.get("pending_pr_numbers") or []) rollout_state = prepare_rollout_state(saved_rollout_state, open_pr_numbers) - if pr_number is not None: - publish_pr_status(repo, pr_number, dashboard_state) - pending = set(rollout_state["pending_pr_numbers"]) - pending.discard(pr_number) - rollout_state["pending_pr_numbers"] = sorted(pending) - if not pending and rollout_state["target_revision"] == STATUS_COMMENT_REVISION: - rollout_state["completed_revision"] = STATUS_COMMENT_REVISION - if rollout_state != saved_rollout_state: - save_status_comment_rollout_state(rollout_state) - return [] - - rollout_pr_numbers = rollout_state["pending_pr_numbers"][:STATUS_COMMENT_ROLLOUT_BATCH_SIZE] + pending_pr_numbers = set(rollout_state["pending_pr_numbers"]) | queued_pr_numbers + rollout_pr_numbers = sorted(pending_pr_numbers)[:STATUS_COMMENT_ROLLOUT_BATCH_SIZE] successful_pr_numbers: set[int] = set() errors: list[str] = [] for number in rollout_pr_numbers: @@ -352,81 +376,9 @@ def update_status_comments_from_state( else: successful_pr_numbers.add(number) - pending = set(rollout_state["pending_pr_numbers"]) - successful_pr_numbers + pending = pending_pr_numbers - successful_pr_numbers rollout_state["pending_pr_numbers"] = sorted(pending) if not pending: rollout_state["completed_revision"] = STATUS_COMMENT_REVISION save_status_comment_rollout_state(rollout_state) return errors - - -def rollout_errors_path() -> Path: - return Path(os.environ.get("RUNNER_TEMP", ".")) / "status-comment-rollout-errors.txt" - - -def update_status_comments( - repo: str, - pr_number: int | None, - open_pr_numbers: set[int] | None, - errors_file: Path, -) -> int: - errors = update_status_comments_from_state(repo, pr_number, open_pr_numbers) - if errors: - errors_file.write_text("\n".join(errors) + "\n", encoding="utf-8") - else: - errors_file.unlink(missing_ok=True) - return 0 - - -def update_status_comments_with_state( - repo: str, - state_branch_name: str, - state_dir: Path, - pr_number: int | None, -) -> int: - open_pr_numbers = list_all_open_pr_numbers(repo) - repo_key = repo_state_key(repo) - errors_file = rollout_errors_path() - errors_file.unlink(missing_ok=True) - status = state_branch.push_state_changes( - state_dir, - "Update status comment rollout state", - lambda: update_status_comments( - repo, - pr_number, - open_pr_numbers, - errors_file, - ), - state_branch=state_branch_name, - add_paths=[f"{repo_key}/{status_comment_rollout_state_path().name}"], - ) - if status != 0: - return status - if not errors_file.exists(): - return 0 - print("Status comment rollout failed:", file=sys.stderr) - print(errors_file.read_text(encoding="utf-8").rstrip(), file=sys.stderr) - return 1 - - -def main() -> int: - parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument("--repo", help="target repository name, e.g. opentelemetry-java-instrumentation") - parser.add_argument("--state-branch", required=True, help="git branch used for workflow state") - parser.add_argument("--pr-number", type=int, help="targeted pull request to update") - args = parser.parse_args() - - repo = normalize_repo(args.repo) if args.repo else detect_repo() - - with state_branch.temporary_state_dir() as state_dir: - set_state_dir(state_dir / repo_state_key(repo)) - return update_status_comments_with_state( - repo, - args.state_branch, - state_dir, - args.pr_number, - ) - - -if __name__ == "__main__": - sys.exit(main()) \ No newline at end of file diff --git a/.github/scripts/pull-request-dashboard/route_presentation.py b/.github/scripts/pull-request-dashboard/route_presentation.py index bd57c3f0e64..20b204aa06d 100644 --- a/.github/scripts/pull-request-dashboard/route_presentation.py +++ b/.github/scripts/pull-request-dashboard/route_presentation.py @@ -7,6 +7,11 @@ "status_waiting_on": "Maintainers", "status_next_step": "Merge when ready.", }, + "copilot": { + "dashboard_label": "Waiting on Copilot", + "status_waiting_on": "Copilot", + "status_next_step": "Wait for the pending review to complete.", + }, "approver": { "dashboard_label": "Waiting on reviewers", "status_waiting_on": "Reviewers", diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index 186e66868ce..64db5c46017 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -12,12 +12,16 @@ DASHBOARD_MARKDOWN_FILE = "pull-request-dashboard.md" BACKFILL_STATE_FILE = "backfill-state.json" +AUTHOR_NUDGE_STATE_FILE = "author-nudge-state.json" +COPILOT_REVIEW_REQUEST_STATE_FILE = "copilot-review-request-state.json" STATUS_COMMENT_ROLLOUT_STATE_FILE = "status-comment-rollout-state.json" # State files are disposable workflow caches, not durable user data. Bump only # the version for the state shape whose meaning changed. DASHBOARD_STATE_VERSION = 5 BACKFILL_STATE_VERSION = 3 NOTIFICATION_STATE_VERSION = 3 +AUTHOR_NUDGE_STATE_VERSION = 2 +COPILOT_REVIEW_REQUEST_STATE_VERSION = 3 STATUS_COMMENT_ROLLOUT_STATE_VERSION = 1 INITIAL_BACKFILL_COMPLETE_KEY = "initial_backfill_complete" _state_dir: Path | None = None @@ -42,6 +46,14 @@ def notification_state_path() -> Path: return state_dir() / "notification-state.json" +def author_nudge_state_path() -> Path: + return state_dir() / AUTHOR_NUDGE_STATE_FILE + + +def copilot_review_request_state_path() -> Path: + return state_dir() / COPILOT_REVIEW_REQUEST_STATE_FILE + + def backfill_state_path() -> Path: return state_dir() / BACKFILL_STATE_FILE @@ -235,6 +247,93 @@ def save_notifications(notifications: dict[str, Any]) -> None: _save_notification_state_file({"prs": notifications}) +def load_author_nudge_state_file(path: Path) -> dict[str, Any]: + state = load_state_file(path, AUTHOR_NUDGE_STATE_VERSION) + if state is None or not isinstance(state.get("prs"), dict): + return {} + return state["prs"] + + +def union_merge_author_nudges( + baseline_nudges: dict[str, Any], + retry_snapshot_nudges: dict[str, Any], +) -> dict[str, Any]: + merged = dict(baseline_nudges) + for key, retry_entry in retry_snapshot_nudges.items(): + nudged_at = (retry_entry or {}).get("nudged_at") or "" + waiting_since = (retry_entry or {}).get("waiting_since") or "" + baseline_waiting_since = (baseline_nudges.get(key) or {}).get("waiting_since") or "" + if nudged_at and waiting_since and waiting_since == baseline_waiting_since: + merged[key] = { + "waiting_since": waiting_since, + "nudged_at": nudged_at, + } + return merged + + +def load_author_nudges(retry_snapshot_path: Path | None = None) -> dict[str, Any]: + nudges = load_author_nudge_state_file(author_nudge_state_path()) + if retry_snapshot_path and retry_snapshot_path.exists(): + nudges = union_merge_author_nudges( + nudges, + load_author_nudge_state_file(retry_snapshot_path), + ) + return nudges + + +def save_author_nudges(nudges: dict[str, Any]) -> None: + save_state_file( + author_nudge_state_path(), + {"prs": nudges}, + AUTHOR_NUDGE_STATE_VERSION, + ) + + +def load_copilot_review_request_state_file(path: Path) -> dict[str, Any]: + state = load_state_file( + path, + COPILOT_REVIEW_REQUEST_STATE_VERSION, + ) + if state is None or not isinstance(state.get("prs"), dict): + return {} + return state["prs"] + + +def union_merge_copilot_review_requests( + baseline_requests: dict[str, Any], + retry_snapshot_requests: dict[str, Any], +) -> dict[str, Any]: + merged = dict(baseline_requests) + for key, retry_entry in retry_snapshot_requests.items(): + baseline_entry = merged.get(key) or {} + if ( + (retry_entry or {}).get("requested_at") + and retry_entry.get("head_sha") == baseline_entry.get("head_sha") + and retry_entry.get("observed_at") + and retry_entry.get("observed_at") == baseline_entry.get("observed_at") + ): + merged[key] = retry_entry + return merged + + +def load_copilot_review_requests(retry_snapshot_path: Path | None = None) -> dict[str, Any]: + requests = load_copilot_review_request_state_file(copilot_review_request_state_path()) + if retry_snapshot_path and retry_snapshot_path.exists(): + requests = union_merge_copilot_review_requests( + requests, + load_copilot_review_request_state_file(retry_snapshot_path), + ) + return requests + + +def save_copilot_review_requests(requests: dict[str, Any]) -> None: + save_state_file( + copilot_review_request_state_path(), + {"prs": requests}, + COPILOT_REVIEW_REQUEST_STATE_VERSION, + ) + + def union_merge_notifications( baseline_notifications: dict[str, Any], retry_snapshot_notifications: dict[str, Any] ) -> dict[str, Any]: diff --git a/.github/scripts/pull-request-dashboard/test_author_nudge.py b/.github/scripts/pull-request-dashboard/test_author_nudge.py new file mode 100644 index 00000000000..a8991a028dc --- /dev/null +++ b/.github/scripts/pull-request-dashboard/test_author_nudge.py @@ -0,0 +1,644 @@ +from __future__ import annotations + +from datetime import datetime, timezone +import unittest +from unittest.mock import patch + +import author_nudge + + +NOW = datetime(2026, 7, 17, tzinfo=timezone.utc) + + +def author_result(route: str = "author") -> dict: + return { + "route": route, + "facts": { + "author": "alice", + "author_nudge_episode_id": "episode-1", + "head_sha": "current-head", + "routing_input_fingerprint": "current-fingerprint", + }, + } + + +class AuthorNudgePolicyTest(unittest.TestCase): + def test_routing_fingerprint_ignores_dashboard_comments_and_tracks_author_activity(self) -> None: + raw = { + "checks": [], + "issue_comments": [], + "labels": [], + "review_comments": [], + "reviews": [], + "review_threads": [], + } + baseline = author_nudge.routing_input_fingerprint(raw) + + raw["issue_comments"].append({ + "id": 1, + "user": {"login": "opentelemetry-pr-dashboard[bot]"}, + "body": "dashboard status", + }) + self.assertEqual(baseline, author_nudge.routing_input_fingerprint(raw)) + + raw["issue_comments"].append({ + "id": 2, + "user": {"login": "alice"}, + "body": "I addressed the feedback.", + }) + self.assertNotEqual(baseline, author_nudge.routing_input_fingerprint(raw)) + + def test_routing_fingerprint_tracks_normalized_labels(self) -> None: + raw = { + "checks": [], + "issue_comments": [], + "pr": {"labels": [{"name": "needs-triage"}]}, + "review_comments": [], + "reviews": [], + "review_threads": [], + } + baseline = author_nudge.routing_input_fingerprint(raw) + raw["pr"]["labels"].append({"name": "documentation"}) + + self.assertEqual(baseline, author_nudge.routing_input_fingerprint(raw)) + + raw["pr"]["labels"].append({"name": "dashboard:route-overridden"}) + overridden = author_nudge.routing_input_fingerprint(raw) + + self.assertNotEqual(baseline, overridden) + raw["pr"]["labels"].reverse() + self.assertEqual(overridden, author_nudge.routing_input_fingerprint(raw)) + + def test_routing_fingerprint_tracks_required_check_state(self) -> None: + raw = { + "checks": [{"name": "build", "bucket": "fail"}], + "issue_comments": [], + "review_comments": [], + "reviews": [], + "review_threads": [], + } + failing = author_nudge.routing_input_fingerprint(raw) + + raw["checks"][0]["bucket"] = "pass" + + self.assertNotEqual(failing, author_nudge.routing_input_fingerprint(raw)) + + def test_routing_fingerprint_tracks_pr_title_and_body(self) -> None: + raw = { + "checks": [], + "issue_comments": [], + "pr": {"title": "Original title", "body": "Original body"}, + "review_comments": [], + "reviews": [], + "review_threads": [], + } + baseline = author_nudge.routing_input_fingerprint(raw) + + raw["pr"]["title"] = "Updated title" + title_updated = author_nudge.routing_input_fingerprint(raw) + raw["pr"]["title"] = "Original title" + raw["pr"]["body"] = "Updated body" + + self.assertNotEqual(baseline, title_updated) + self.assertNotEqual(baseline, author_nudge.routing_input_fingerprint(raw)) + + def test_routing_fingerprint_normalizes_and_tracks_base_branch(self) -> None: + accepted = { + "pr": {"baseRefName": "main"}, + } + live = { + "pr": {"base": {"ref": "main"}}, + } + + self.assertEqual( + author_nudge.routing_input_fingerprint(accepted), + author_nudge.routing_input_fingerprint(live), + ) + live["pr"]["base"]["ref"] = "release" + self.assertNotEqual( + author_nudge.routing_input_fingerprint(accepted), + author_nudge.routing_input_fingerprint(live), + ) + + @patch.object(author_nudge, "gh_required_check_contexts", return_value=[]) + @patch.object( + author_nudge, + "gh_pr_check_rollup", + return_value={ + "required": [{"name": "build", "bucket": "fail"}], + "non_blocking_failures": [], + }, + ) + @patch.object(author_nudge, "fetch_review_threads", return_value=[]) + @patch.object(author_nudge, "fetch_pr_review_data", return_value={}) + @patch.object(author_nudge, "fetch_pr_issue_comments", return_value=[]) + @patch.object(author_nudge, "gh_api") + def test_fetch_current_routing_state_includes_required_checks( + self, + gh_api, + _fetch_issue_comments, + _fetch_review_data, + _fetch_review_threads, + gh_pr_check_rollup, + gh_required_check_contexts, + ) -> None: + pr = { + "node_id": "PR_node", + "base": {"ref": "main"}, + "body": "Current body", + "head": {"sha": "current-head"}, + "labels": [ + {"name": "needs-triage"}, + {"name": "dashboard:route-overridden"}, + ], + "title": "Current title", + } + gh_api.side_effect = lambda path, paginate=False: ( + pr if path.endswith("/pulls/1") else [] + ) + + current_pr, fingerprint = author_nudge.fetch_current_pr_routing_state( + "open-telemetry/example", + 1, + ) + + self.assertEqual(pr, current_pr) + self.assertEqual( + author_nudge.routing_input_fingerprint({ + "checks": [{"name": "build", "bucket": "fail"}], + "issue_comments": [], + "labels": [ + {"name": "needs-triage"}, + {"name": "dashboard:route-overridden"}, + ], + "pr": pr, + "review_comments": [], + "reviews": [], + "review_threads": [], + }), + fingerprint, + ) + gh_pr_check_rollup.assert_called_once_with( + "open-telemetry/example", + "PR_node", + [], + ) + gh_required_check_contexts.assert_called_once_with( + "open-telemetry/example", + "main", + ) + + def test_nudge_advertises_dashboard_override_command(self) -> None: + body = author_nudge.render_nudge( + "alice", + "https://example.test/status", + "episode-1", + ) + + self.assertIn( + "comment `/dashboard route:reviewers` to route it from waiting on the " + "author to waiting on reviewers", + body, + ) + + def test_first_author_route_observation_starts_clock(self) -> None: + due, entry = author_nudge.plan_nudge(author_result(), None, NOW) + + self.assertFalse(due) + self.assertEqual( + entry, + {"waiting_since": "2026-07-17T00:00:00+00:00", "nudged_at": ""}, + ) + + def test_nudge_is_due_after_one_week(self) -> None: + due, _entry = author_nudge.plan_nudge( + author_result(), + {"waiting_since": "2026-07-10T00:00:00+00:00", "nudged_at": ""}, + NOW, + ) + + self.assertTrue(due) + + def test_nudge_is_not_due_before_one_week(self) -> None: + due, _entry = author_nudge.plan_nudge( + author_result(), + {"waiting_since": "2026-07-10T00:00:01+00:00", "nudged_at": ""}, + NOW, + ) + + self.assertFalse(due) + + def test_leaving_author_route_resets_unnudged_clock(self) -> None: + due, entry = author_nudge.plan_nudge( + author_result("approver"), + {"waiting_since": "2026-07-10T00:00:00+00:00", "nudged_at": ""}, + NOW, + ) + + self.assertFalse(due) + self.assertIsNone(entry) + + def test_returning_to_author_route_starts_new_episode(self) -> None: + previous = { + "waiting_since": "2026-07-01T00:00:00+00:00", + "nudged_at": "2026-07-10T00:00:00+00:00", + } + + due, entry = author_nudge.plan_nudge(author_result("approver"), previous, NOW) + self.assertFalse(due) + self.assertIsNone(entry) + + due, entry = author_nudge.plan_nudge(author_result(), entry, NOW) + self.assertFalse(due) + self.assertEqual( + entry, + {"waiting_since": "2026-07-17T00:00:00+00:00", "nudged_at": ""}, + ) + + def test_failed_refresh_preserves_clock(self) -> None: + previous = {"waiting_since": "2026-07-10T00:00:00+00:00", "nudged_at": ""} + + due, entry = author_nudge.plan_nudge( + {"failed": True, "route": "unknown"}, + previous, + NOW, + ) + + self.assertFalse(due) + self.assertEqual(entry, previous) + + +class AuthorNudgeProcessingTest(unittest.TestCase): + @patch.object(author_nudge, "save_author_nudges") + @patch.object(author_nudge, "load_author_nudges", return_value={}) + def test_observation_starts_clock( + self, + _load_nudges, + save_nudges, + ) -> None: + author_nudge.record_author_nudge_observation(2, author_result(), NOW) + + self.assertEqual( + save_nudges.call_args.args[0], + {"2": {"waiting_since": "2026-07-17T00:00:00+00:00", "nudged_at": ""}}, + ) + + @patch.object(author_nudge, "save_author_nudges") + @patch.object( + author_nudge, + "load_author_nudges", + return_value={ + "1": { + "waiting_since": "2026-07-10T00:00:00+00:00", + "nudged_at": "", + } + }, + ) + def test_departure_observation_resets_clock( + self, + _load_nudges, + save_nudges, + ) -> None: + author_nudge.record_author_nudge_observation(1, None, NOW) + + self.assertEqual(save_nudges.call_args.args[0], {}) + + @patch.object(author_nudge, "save_author_nudges") + @patch.object( + author_nudge, + "load_author_nudges", + return_value={"1": {"waiting_since": "2026-07-01T00:00:00+00:00", "nudged_at": ""}}, + ) + def test_due_accepted_observation_records_pending_nudge( + self, + _load_nudges, + save_nudges, + ) -> None: + author_nudge.record_author_nudge_observation( + 1, + author_result(), + NOW, + prepare_due=True, + ) + + self.assertEqual( + save_nudges.call_args.args[0], + { + "1": { + "waiting_since": "2026-07-01T00:00:00+00:00", + "nudged_at": "", + "pending_at": "2026-07-17T00:00:00+00:00", + "head_sha": "current-head", + "routing_input_fingerprint": "current-fingerprint", + } + }, + ) + + @patch.object( + author_nudge, + "ensure_nudge", + return_value="2026-07-17T00:00:00+00:00", + ) + @patch.object(author_nudge, "save_author_nudges") + @patch.object( + author_nudge, + "load_author_nudges", + return_value={ + "1": { + "waiting_since": "2026-07-01T00:00:00+00:00", + "nudged_at": "", + "pending_at": "2026-07-17T00:00:00+00:00", + "head_sha": "current-head", + "routing_input_fingerprint": "current-fingerprint", + } + }, + ) + @patch.object( + author_nudge, + "load_dashboard_state_cache", + return_value={"prs": {"1": author_result()}}, + ) + @patch.object( + author_nudge, + "fetch_current_pr_routing_state", + return_value=({ + "state": "open", + "draft": False, + "head": {"sha": "current-head"}, + }, "current-fingerprint"), + ) + def test_delivery_records_posted_nudge( + self, + fetch_current_state, + _load_dashboard_state, + _load_nudges, + save_nudges, + ensure_nudge, + ) -> None: + errors = author_nudge.deliver_prepared_author_nudges( + "open-telemetry/example", + NOW, + ) + + self.assertEqual([], errors) + fetch_current_state.assert_called_once_with("open-telemetry/example", 1) + ensure_nudge.assert_called_once() + save_nudges.assert_called_once_with({ + "1": { + "waiting_since": "2026-07-01T00:00:00+00:00", + "nudged_at": "2026-07-17T00:00:00+00:00", + }, + }) + + @patch.object(author_nudge, "ensure_nudge") + @patch.object(author_nudge, "save_author_nudges") + @patch.object( + author_nudge, + "load_author_nudges", + return_value={ + "1": { + "waiting_since": "2026-07-01T00:00:00+00:00", + "nudged_at": "", + "pending_at": "2026-07-17T00:00:00+00:00", + "head_sha": "current-head", + "routing_input_fingerprint": "current-fingerprint", + } + }, + ) + @patch.object( + author_nudge, + "load_dashboard_state_cache", + return_value={"prs": {"1": author_result()}}, + ) + @patch.object( + author_nudge, + "fetch_current_pr_routing_state", + return_value=({ + "state": "open", + "draft": False, + "head": {"sha": "new-head"}, + }, "current-fingerprint"), + ) + def test_delivery_defers_when_head_advanced( + self, + _gh_api, + _load_dashboard_state, + _load_nudges, + save_nudges, + ensure_nudge, + ) -> None: + errors = author_nudge.deliver_prepared_author_nudges( + "open-telemetry/example", + NOW, + ) + + self.assertEqual([], errors) + ensure_nudge.assert_not_called() + save_nudges.assert_called_once_with({ + "1": { + "waiting_since": "2026-07-01T00:00:00+00:00", + "nudged_at": "", + }, + }) + + @patch.object(author_nudge, "ensure_nudge") + @patch.object(author_nudge, "save_author_nudges") + @patch.object( + author_nudge, + "load_author_nudges", + return_value={ + "1": { + "waiting_since": "2026-07-01T00:00:00+00:00", + "nudged_at": "", + "pending_at": "2026-07-17T00:00:00+00:00", + "head_sha": "current-head", + "routing_input_fingerprint": "accepted-fingerprint", + } + }, + ) + @patch.object( + author_nudge, + "load_dashboard_state_cache", + return_value={"prs": {"1": author_result()}}, + ) + @patch.object( + author_nudge, + "fetch_current_pr_routing_state", + return_value=({ + "state": "open", + "draft": False, + "head": {"sha": "current-head"}, + }, "new-fingerprint"), + ) + def test_delivery_defers_when_routing_inputs_changed( + self, + _fetch_current_state, + _load_dashboard_state, + _load_nudges, + save_nudges, + ensure_nudge, + ) -> None: + errors = author_nudge.deliver_prepared_author_nudges( + "open-telemetry/example", + NOW, + ) + + self.assertEqual([], errors) + ensure_nudge.assert_not_called() + save_nudges.assert_called_once_with({ + "1": { + "waiting_since": "2026-07-01T00:00:00+00:00", + "nudged_at": "", + }, + }) + + def test_delivery_clears_episode_for_closed_or_draft_pr(self) -> None: + pending = { + "1": { + "waiting_since": "2026-07-01T00:00:00+00:00", + "nudged_at": "", + "pending_at": "2026-07-17T00:00:00+00:00", + "head_sha": "current-head", + "routing_input_fingerprint": "current-fingerprint", + } + } + for state, draft in (("closed", False), ("open", True)): + with ( + self.subTest(state=state, draft=draft), + patch.object(author_nudge, "load_author_nudges", return_value=pending), + patch.object(author_nudge, "save_author_nudges") as save_nudges, + patch.object( + author_nudge, + "load_dashboard_state_cache", + return_value={"prs": {"1": author_result()}}, + ), + patch.object( + author_nudge, + "fetch_current_pr_routing_state", + return_value=({ + "state": state, + "draft": draft, + "head": {"sha": "current-head"}, + }, "current-fingerprint"), + ), + patch.object(author_nudge, "ensure_nudge") as ensure_nudge, + ): + errors = author_nudge.deliver_prepared_author_nudges( + "open-telemetry/example", + NOW, + ) + + self.assertEqual([], errors) + ensure_nudge.assert_not_called() + save_nudges.assert_called_once_with({}) + + def test_rendered_nudge_mentions_author_and_links_status(self) -> None: + body = author_nudge.render_nudge( + "alice", + "https://example.test/status", + "episode-1", + ) + + self.assertIn("@alice", body) + self.assertIn("[dashboard status comment](https://example.test/status)", body) + self.assertIn( + author_nudge.nudge_marker("episode-1"), + body, + ) + + @patch.object( + author_nudge, + "gh_api", + return_value=[ + { + "performed_via_github_app": {"slug": "opentelemetry-pr-dashboard"}, + "body": author_nudge.nudge_marker("previous-episode"), + }, + { + "performed_via_github_app": {"slug": "opentelemetry-pr-dashboard"}, + "body": author_nudge.nudge_marker("episode-1"), + "created_at": "2026-07-17T00:00:00Z", + }, + ], + ) + def test_existing_nudge_matches_current_episode(self, _gh_api) -> None: + comment = author_nudge.existing_nudge_comment( + "open-telemetry/example", + 1, + "episode-1", + ) + + self.assertEqual(comment["created_at"], "2026-07-17T00:00:00Z") + + @patch.object(author_nudge, "run_gh") + @patch.object(author_nudge, "publish_pr_status") + @patch.object(author_nudge, "managed_status_comments") + @patch.object( + author_nudge, + "gh_api", + side_effect=[ + [], + {"state": "open", "draft": False, "user": {"login": "alice"}}, + ], + ) + def test_posts_nudge_after_ensuring_status_comment( + self, + _gh_api, + managed_status_comments, + publish_status, + run_gh, + ) -> None: + managed_status_comments.return_value = [ + {"html_url": "https://example.test/status"} + ] + dashboard_state = {"prs": {"1": author_result()}} + + nudged_at = author_nudge.ensure_nudge( + "open-telemetry/example", + 1, + author_result(), + dashboard_state, + "2026-07-10T00:00:00+00:00", + NOW, + ) + + self.assertEqual(nudged_at, "2026-07-17T00:00:00+00:00") + publish_status.assert_called_once_with( + "open-telemetry/example", 1, dashboard_state + ) + self.assertIn("@alice", run_gh.call_args.args[0][-1]) + + @patch.object(author_nudge, "run_gh") + @patch.object(author_nudge, "publish_pr_status") + @patch.object( + author_nudge, + "existing_nudge_comment", + return_value={"created_at": "2026-07-11T00:00:00Z"}, + ) + def test_existing_episode_marker_prevents_duplicate_after_state_loss( + self, + existing_comment, + publish_status, + run_gh, + ) -> None: + nudged_at = author_nudge.ensure_nudge( + "open-telemetry/example", + 1, + author_result(), + {"prs": {"1": author_result()}}, + "2026-07-17T00:00:00+00:00", + NOW, + ) + + self.assertEqual(nudged_at, "2026-07-11T00:00:00Z") + existing_comment.assert_called_once_with( + "open-telemetry/example", + 1, + "episode-1", + ) + publish_status.assert_not_called() + run_gh.assert_not_called() + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/.github/scripts/pull-request-dashboard/test_copilot_review.py b/.github/scripts/pull-request-dashboard/test_copilot_review.py new file mode 100644 index 00000000000..ddc5bab009d --- /dev/null +++ b/.github/scripts/pull-request-dashboard/test_copilot_review.py @@ -0,0 +1,303 @@ +from __future__ import annotations + +import unittest +from unittest.mock import patch + +from datetime import datetime, timezone + +from copilot_review import deliver_copilot_review_requests, record_copilot_review_observation + + +NOW = datetime(2026, 7, 20, 2, tzinfo=timezone.utc) + + +class CopilotReviewRequestStateTest(unittest.TestCase): + @patch("copilot_review.save_copilot_review_requests") + @patch("copilot_review.load_copilot_review_requests", return_value={}) + def test_records_request_for_current_head(self, _load_requests, save_requests) -> None: + record_copilot_review_observation( + 7, + { + "route": "copilot", + "facts": { + "head_sha": "current-head", + "copilot_review_request_needed": True, + "routing_input_fingerprint": "accepted-fingerprint", + }, + }, + NOW, + ) + + save_requests.assert_called_once_with({ + "7": { + "head_sha": "current-head", + "observed_at": "2026-07-20T02:00:00+00:00", + "requested_at": "", + "routing_input_fingerprint": "accepted-fingerprint", + }, + }) + + @patch("copilot_review.save_copilot_review_requests") + @patch( + "copilot_review.load_copilot_review_requests", + return_value={"7": {"head_sha": "old-head", "requested_at": "old-request"}}, + ) + def test_new_head_replaces_previous_request(self, _load_requests, save_requests) -> None: + record_copilot_review_observation( + 7, + { + "route": "copilot", + "facts": { + "head_sha": "current-head", + "copilot_review_request_needed": True, + "routing_input_fingerprint": "accepted-fingerprint", + }, + }, + NOW, + ) + + save_requests.assert_called_once_with({ + "7": { + "head_sha": "current-head", + "observed_at": "2026-07-20T02:00:00+00:00", + "requested_at": "", + "routing_input_fingerprint": "accepted-fingerprint", + }, + }) + + @patch("copilot_review.save_copilot_review_requests") + @patch( + "copilot_review.load_copilot_review_requests", + return_value={ + "7": { + "head_sha": "current-head", + "requested_at": "2026-07-20T01:00:00Z", + } + }, + ) + def test_same_head_request_needed_resets_acknowledgement( + self, + _load_requests, + save_requests, + ) -> None: + record_copilot_review_observation( + 7, + { + "route": "copilot", + "facts": { + "head_sha": "current-head", + "copilot_review_request_needed": True, + "routing_input_fingerprint": "accepted-fingerprint", + }, + }, + NOW, + ) + + save_requests.assert_called_once_with({ + "7": { + "head_sha": "current-head", + "observed_at": "2026-07-20T02:00:00+00:00", + "requested_at": "", + "routing_input_fingerprint": "accepted-fingerprint", + }, + }) + + @patch("copilot_review.save_copilot_review_requests") + @patch( + "copilot_review.load_copilot_review_requests", + return_value={"7": {"head_sha": "current-head", "requested_at": ""}}, + ) + def test_clears_request_when_no_longer_needed(self, _load_requests, save_requests) -> None: + record_copilot_review_observation( + 7, + { + "route": "maintainer", + "facts": { + "head_sha": "current-head", + "copilot_review_request_needed": False, + }, + }, + NOW, + ) + + save_requests.assert_called_once_with({}) + + @patch("copilot_review.save_copilot_review_requests") + @patch("copilot_review.load_copilot_review_requests", return_value={}) + def test_initial_automatic_review_does_not_enqueue_request( + self, + _load_requests, + save_requests, + ) -> None: + record_copilot_review_observation( + 7, + { + "route": "copilot", + "facts": { + "head_sha": "current-head", + "copilot_review_exists": False, + "copilot_review_request_needed": False, + }, + }, + NOW, + ) + + save_requests.assert_called_once_with({}) + + @patch("copilot_review.request_copilot_review") + @patch("copilot_review.fetch_pr_review_data") + @patch("copilot_review.fetch_current_pr_routing_state") + @patch("copilot_review.save_copilot_review_requests") + @patch( + "copilot_review.load_copilot_review_requests", + return_value={ + "7": { + "head_sha": "current-head", + "observed_at": "2026-07-20T01:00:00+00:00", + "requested_at": "", + "routing_input_fingerprint": "accepted-fingerprint", + } + }, + ) + def test_delivers_request_for_current_stale_review( + self, + _load_requests, + save_requests, + fetch_current_state, + fetch_review_data, + request_review, + ) -> None: + pr = { + "state": "open", + "draft": False, + "head": {"sha": "current-head"}, + "node_id": "PR_node_id", + "requested_reviewers": [], + } + fetch_current_state.return_value = (pr, "accepted-fingerprint") + fetch_review_data.return_value = { + "reviews": [{ + "id": 20, + "commit_id": "reviewed-head", + "finding_count": 0, + "user": {"login": "copilot"}, + "submitted_at": "2026-07-20T01:00:00Z", + }], + } + + errors = deliver_copilot_review_requests( + "open-telemetry/example", + NOW, + ) + + self.assertEqual([], errors) + fetch_current_state.assert_called_once_with("open-telemetry/example", 7) + fetch_review_data.assert_called_once_with("open-telemetry", "example", 7) + request_review.assert_called_once_with("PR_node_id") + save_requests.assert_called_once_with({ + "7": { + "head_sha": "current-head", + "observed_at": "2026-07-20T01:00:00+00:00", + "requested_at": "2026-07-20T02:00:00+00:00", + "routing_input_fingerprint": "accepted-fingerprint", + }, + }) + + @patch("copilot_review.request_copilot_review") + @patch("copilot_review.fetch_pr_review_data") + @patch("copilot_review.fetch_current_pr_routing_state") + @patch("copilot_review.save_copilot_review_requests") + @patch( + "copilot_review.load_copilot_review_requests", + return_value={ + "7": { + "head_sha": "current-head", + "observed_at": "2026-07-20T01:00:00+00:00", + "requested_at": "", + "routing_input_fingerprint": "accepted-fingerprint", + } + }, + ) + def test_pending_request_is_acknowledged_from_pull_response( + self, + _load_requests, + save_requests, + fetch_current_state, + fetch_review_data, + request_review, + ) -> None: + pr = { + "state": "open", + "draft": False, + "head": {"sha": "current-head"}, + "requested_reviewers": [ + {"login": "copilot-pull-request-reviewer[bot]"}, + ], + } + fetch_current_state.return_value = (pr, "accepted-fingerprint") + + errors = deliver_copilot_review_requests( + "open-telemetry/example", + NOW, + ) + + self.assertEqual([], errors) + fetch_current_state.assert_called_once_with("open-telemetry/example", 7) + fetch_review_data.assert_not_called() + request_review.assert_not_called() + save_requests.assert_called_once_with({ + "7": { + "head_sha": "current-head", + "observed_at": "2026-07-20T01:00:00+00:00", + "requested_at": "2026-07-20T02:00:00+00:00", + "routing_input_fingerprint": "accepted-fingerprint", + }, + }) + + @patch("copilot_review.request_copilot_review") + @patch("copilot_review.fetch_pr_review_data") + @patch( + "copilot_review.fetch_current_pr_routing_state", + return_value=( + { + "state": "open", + "draft": False, + "head": {"sha": "current-head"}, + "requested_reviewers": [], + }, + "new-fingerprint", + ), + ) + @patch("copilot_review.save_copilot_review_requests") + @patch( + "copilot_review.load_copilot_review_requests", + return_value={ + "7": { + "head_sha": "current-head", + "observed_at": "2026-07-20T01:00:00+00:00", + "requested_at": "", + "routing_input_fingerprint": "accepted-fingerprint", + } + }, + ) + def test_drops_request_when_live_routing_inputs_changed( + self, + _load_requests, + save_requests, + _fetch_current_state, + fetch_review_data, + request_review, + ) -> None: + errors = deliver_copilot_review_requests( + "open-telemetry/example", + NOW, + ) + + self.assertEqual([], errors) + fetch_review_data.assert_not_called() + request_review.assert_not_called() + save_requests.assert_called_once_with({}) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index f821f62a47f..783f976adbf 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -5,14 +5,16 @@ from pathlib import Path import tempfile import unittest -from unittest.mock import Mock, call, patch +from unittest.mock import ANY, Mock, call, patch from classification import discussion_prompt_input +from copilot_review import apply_copilot_review_gate from dashboard import ( BACKFILL_RECORDED_FAILURE_STATUS, DashboardUpdate, add_wait_age_facts, apply_targeted_dashboard_update, + assign_author_nudge_episode, author_action_discussion_urls, backfill_failed_pr_numbers, complete_initial_backfill_if_ready, @@ -21,6 +23,7 @@ group_review_threads, main, remove_cached_dashboard_prs, + resolve_pr_route, route_pr, set_backfill_pr_failed, update_dashboard_for_backfill, @@ -28,6 +31,100 @@ ) +class ResolvePrRouteTest(unittest.TestCase): + def _author_route_facts(self, **overrides: object) -> dict[str, object]: + # A failing required check routes a human-authored PR to the author. + facts: dict[str, object] = { + "ci_failing_count": 1, + "dashboard_override_label_applied": True, + "dashboard_override_requested": False, + } + facts.update(overrides) + return facts + + def test_override_is_still_gated_by_required_copilot_review(self) -> None: + facts = self._author_route_facts( + copilot_review_exists=True, + copilot_review_needed=True, + copilot_review_requested=False, + ) + + route = resolve_pr_route(facts, {}, 1, True) + + self.assertEqual("copilot", route) + + def test_override_reaches_reviewers_when_copilot_review_is_clean(self) -> None: + facts = self._author_route_facts( + copilot_review_exists=True, + copilot_review_needed=False, + ) + + route = resolve_pr_route(facts, {}, 1, True) + + self.assertEqual("approver", route) + + def test_override_reaches_reviewers_when_gate_disabled(self) -> None: + facts = self._author_route_facts() + + route = resolve_pr_route(facts, {}, 1, False) + + self.assertEqual("approver", route) + + +class AuthorNudgeEpisodeTest(unittest.TestCase): + def test_preserves_episode_while_route_remains_author(self) -> None: + facts: dict[str, object] = {} + + assign_author_nudge_episode( + facts, + "author", + { + "route": "author", + "facts": {"author_nudge_episode_id": "abc123"}, + }, + [], + ) + + self.assertEqual("abc123", facts["author_nudge_episode_id"]) + + @patch("dashboard.uuid.uuid4") + def test_starts_new_episode_after_known_route_departure(self, uuid4: Mock) -> None: + uuid4.return_value.hex = "def456" + facts: dict[str, object] = {} + + assign_author_nudge_episode( + facts, + "author", + {"route": "approver", "facts": {}}, + [{ + "performed_via_github_app": {"slug": "opentelemetry-pr-dashboard"}, + "body": ( + "\n" + "" + ), + }], + ) + + self.assertEqual("def456", facts["author_nudge_episode_id"]) + + def test_recovers_episode_from_status_comment_after_cache_loss(self) -> None: + facts: dict[str, object] = {} + + assign_author_nudge_episode( + facts, + "author", + None, + [{ + "performed_via_github_app": {"slug": "opentelemetry-pr-dashboard"}, + "body": ( + "\n" + "" + ), + }], + ) + + self.assertEqual("abc123", facts["author_nudge_episode_id"]) + class FetchPrRawTest(unittest.TestCase): def test_uses_graphql_issue_comments_without_rest_join(self) -> None: issue_comments = [{"id": 101, "body": "GraphQL comment"}] @@ -181,6 +278,316 @@ def test_author_action_urls_use_thread_url_and_deduplicate(self) -> None: author_action_discussion_urls(discussions, pending_actions), ) + +class CopilotReviewGateTest(unittest.TestCase): + def test_current_head_matches_latest_clean_copilot_review(self) -> None: + facts = compute_facts( + { + "pr": { + "updatedAt": "2026-07-20T03:00:00Z", + "createdAt": "2026-07-20T01:00:00Z", + "author": {"login": "author"}, + "assignees": [], + "reviewRequests": [ + {"login": "copilot-pull-request-reviewer"}, + ], + "mergeStateStatus": "CLEAN", + "mergeable": "MERGEABLE", + "headRefOid": "current-head", + }, + "reviews": [ + { + "id": 10, + "commit_id": "old-head", + "finding_count": 1, + "user": {"login": "copilot-pull-request-reviewer[bot]"}, + "submitted_at": "2026-07-20T01:30:00Z", + }, + { + "id": 20, + "commit_id": "current-head", + "finding_count": 0, + "user": {"login": "copilot-pull-request-reviewer"}, + "submitted_at": "2026-07-20T02:30:00Z", + }, + ], + "commits": [{"sha": "old-head"}, {"sha": "current-head"}], + "review_comments": [ + {"pull_request_review_id": 10}, + ], + "checks": [], + }, + "author", + [], + ) + + self.assertTrue(facts["copilot_review_requested"]) + self.assertTrue(facts["copilot_review_exists"]) + self.assertFalse(facts["copilot_review_needed"]) + + def test_late_stale_review_does_not_replace_clean_current_head_review(self) -> None: + facts = compute_facts( + { + "pr": { + "updatedAt": "2026-07-20T03:00:00Z", + "createdAt": "2026-07-20T01:00:00Z", + "author": {"login": "author"}, + "assignees": [], + "reviewRequests": [], + "mergeStateStatus": "CLEAN", + "mergeable": "MERGEABLE", + "headRefOid": "current-head", + }, + "reviews": [ + { + "id": 10, + "commit_id": "current-head", + "finding_count": 0, + "user": {"login": "copilot"}, + "submitted_at": "2026-07-20T02:30:00Z", + }, + { + "id": 20, + "commit_id": "old-head", + "finding_count": 1, + "user": {"login": "copilot"}, + "submitted_at": "2026-07-20T03:00:00Z", + }, + ], + "commits": [{"sha": "old-head"}, {"sha": "current-head"}], + "review_comments": [{"pull_request_review_id": 20}], + "checks": [], + }, + "author", + [], + ) + + self.assertFalse(facts["copilot_review_needed"]) + + def test_push_since_latest_clean_copilot_review_needs_rereview(self) -> None: + facts = compute_facts( + { + "pr": { + "updatedAt": "2026-07-20T03:00:00Z", + "createdAt": "2026-07-20T01:00:00Z", + "author": {"login": "author"}, + "assignees": [], + "reviewRequests": [], + "mergeStateStatus": "CLEAN", + "mergeable": "MERGEABLE", + "headRefOid": "current-head", + }, + "reviews": [ + { + "id": 20, + "commit_id": "reviewed-head", + "finding_count": 0, + "user": {"login": "copilot"}, + "submitted_at": "2026-07-20T02:30:00Z", + }, + ], + "commits": [{"sha": "reviewed-head"}, {"sha": "current-head"}], + "review_comments": [], + "checks": [], + }, + "author", + [], + ) + + self.assertFalse(facts["copilot_review_requested"]) + self.assertTrue(facts["copilot_review_needed"]) + + def test_latest_findings_review_replaces_clean_review_on_same_head(self) -> None: + facts = compute_facts( + { + "pr": { + "updatedAt": "2026-07-20T03:00:00Z", + "createdAt": "2026-07-20T01:00:00Z", + "author": {"login": "author"}, + "assignees": [], + "reviewRequests": [], + "mergeStateStatus": "CLEAN", + "mergeable": "MERGEABLE", + "headRefOid": "current-head", + }, + "reviews": [ + { + "id": 10, + "commit_id": "current-head", + "finding_count": 0, + "user": {"login": "copilot"}, + "submitted_at": "2026-07-20T01:30:00Z", + }, + { + "id": 20, + "commit_id": "current-head", + "finding_count": 1, + "user": {"login": "copilot"}, + "submitted_at": "2026-07-20T02:30:00Z", + }, + ], + "commits": [{"sha": "current-head"}], + "review_comments": [{"pull_request_review_id": 20}], + "checks": [], + }, + "author", + [], + ) + + self.assertTrue(facts["copilot_review_needed"]) + + def test_findings_only_history_needs_rereview(self) -> None: + facts = compute_facts( + { + "pr": { + "updatedAt": "2026-07-20T03:00:00Z", + "createdAt": "2026-07-20T01:00:00Z", + "author": {"login": "author"}, + "assignees": [], + "reviewRequests": [], + "mergeStateStatus": "CLEAN", + "mergeable": "MERGEABLE", + "headRefOid": "current-head", + }, + "reviews": [ + { + "id": 20, + "commit_id": "reviewed-head", + "finding_count": 1, + "user": {"login": "copilot"}, + "submitted_at": "2026-07-20T02:30:00Z", + }, + ], + "commits": [{"sha": "reviewed-head"}, {"sha": "current-head"}], + "review_comments": [{"pull_request_review_id": 20}], + "checks": [], + }, + "author", + [], + ) + + self.assertTrue(facts["copilot_review_needed"]) + + def test_waits_for_automatic_initial_copilot_review(self) -> None: + facts = compute_facts( + { + "pr": { + "updatedAt": "2026-07-20T03:00:00Z", + "createdAt": "2026-07-20T01:00:00Z", + "author": {"login": "author"}, + "assignees": [], + "reviewRequests": [], + "mergeStateStatus": "CLEAN", + "mergeable": "MERGEABLE", + }, + "reviews": [], + "commits": [{"sha": "current-head"}], + "review_comments": [], + "checks": [], + }, + "author", + [], + ) + + self.assertFalse(facts["copilot_review_exists"]) + self.assertFalse(facts["copilot_review_needed"]) + + def test_initial_automatic_review_blocks_human_handoff(self) -> None: + facts = { + "copilot_review_requested": True, + "copilot_review_exists": False, + "copilot_review_needed": False, + } + + route = apply_copilot_review_gate( + facts, + "approver", + enabled=True, + ) + + self.assertEqual(route, "copilot") + self.assertFalse(facts["copilot_review_request_needed"]) + + def test_marks_re_review_needed_after_push_since_clean_review(self) -> None: + facts = { + "copilot_review_requested": False, + "copilot_review_exists": True, + "copilot_review_needed": True, + } + + route = apply_copilot_review_gate( + facts, + "maintainer", + enabled=True, + ) + + self.assertEqual(route, "copilot") + self.assertTrue(facts["copilot_review_request_needed"]) + + def test_marks_re_review_needed_before_reviewer_handoff(self) -> None: + facts = { + "copilot_review_requested": False, + "copilot_review_exists": True, + "copilot_review_needed": True, + } + + route = apply_copilot_review_gate( + facts, + "approver", + enabled=True, + ) + + self.assertEqual(route, "copilot") + self.assertTrue(facts["copilot_review_request_needed"]) + + def test_pending_re_review_waits_without_duplicate_request(self) -> None: + facts = { + "copilot_review_requested": True, + "copilot_review_exists": True, + "copilot_review_needed": True, + } + + route = apply_copilot_review_gate( + facts, + "maintainer", + enabled=True, + ) + + self.assertEqual(route, "copilot") + self.assertFalse(facts["copilot_review_request_needed"]) + + def test_current_head_clean_review_moves_to_maintainers(self) -> None: + facts = { + "copilot_review_requested": False, + "copilot_review_exists": True, + "copilot_review_needed": False, + } + + route = apply_copilot_review_gate( + facts, + "maintainer", + enabled=True, + ) + + self.assertEqual(route, "maintainer") + self.assertFalse(facts["copilot_review_request_needed"]) + + def test_disabled_gate_preserves_maintainer_route(self) -> None: + facts = { + "copilot_review_requested": False, + "copilot_review_exists": True, + "copilot_review_needed": True, + } + + route = apply_copilot_review_gate( + facts, + "maintainer", + enabled=False, + ) + + self.assertEqual(route, "maintainer") + self.assertFalse(facts["copilot_review_request_needed"]) + def test_discussion_url_is_excluded_from_classifier_input(self) -> None: prompt_input = discussion_prompt_input({ "discussion_id": "thread-1", @@ -192,6 +599,44 @@ def test_discussion_url_is_excluded_from_classifier_input(self) -> None: self.assertNotIn("discussion_url", prompt_input) +class HeadShaSourceTest(unittest.TestCase): + def test_head_sha_prefers_pr_head_ref_oid_over_truncated_commits(self) -> None: + facts = compute_facts( + { + "pr": { + "updatedAt": "2026-07-20T03:00:00Z", + "createdAt": "2026-07-20T01:00:00Z", + "author": {"login": "author"}, + "assignees": [], + "reviewRequests": [], + "mergeStateStatus": "CLEAN", + "mergeable": "MERGEABLE", + "headRefOid": "real-head", + }, + "reviews": [ + { + "id": 10, + "commit_id": "real-head", + "finding_count": 0, + "user": {"login": "copilot"}, + "submitted_at": "2026-07-20T02:30:00Z", + }, + ], + # The commits REST endpoint is bounded at 250 entries, so its + # last element is not the real head for a large PR. + "commits": [{"sha": "commit-1"}, {"sha": "commit-250"}], + "review_comments": [], + "checks": [], + }, + "author", + [], + ) + + self.assertEqual(facts["head_sha"], "real-head") + self.assertTrue(facts["copilot_review_exists"]) + self.assertFalse(facts["copilot_review_needed"]) + + class InitialBackfillCompletionTest(unittest.TestCase): def test_marks_complete_only_after_all_open_prs_are_cached(self) -> None: state = {"initial_backfill_complete": False, "prs": {"1": {}}} @@ -227,6 +672,8 @@ def test_writes_initial_backfill_status_to_github_output(self) -> None: ) class StatusCommentQueueTest(unittest.TestCase): + @patch("dashboard.record_copilot_review_observation") + @patch("dashboard.record_author_nudge_observation") @patch("dashboard.save_dashboard_update_state", return_value=0) @patch("dashboard.enqueue_status_comment_update") @patch( @@ -238,6 +685,8 @@ def test_removed_dashboard_results_enqueue_status_comments( _load_state: Mock, enqueue_update: Mock, save_state: Mock, + record_nudge: Mock, + _record_copilot: Mock, ) -> None: args = Namespace(pr_number=None) @@ -250,7 +699,13 @@ def test_removed_dashboard_results_enqueue_status_comments( ) saved_state = save_state.call_args.args[1] self.assertEqual({"56": {}}, saved_state["prs"]) + self.assertEqual( + [call(12, None, ANY), call(34, None, ANY)], + sorted(record_nudge.call_args_list, key=lambda value: value.args[0]), + ) + @patch("dashboard.record_copilot_review_observation") + @patch("dashboard.record_author_nudge_observation") @patch("dashboard.clear_backfill_pr_failure") @patch("dashboard.save_dashboard_update_state", return_value=0) @patch("dashboard.enqueue_status_comment_update") @@ -261,15 +716,38 @@ def test_targeted_state_change_enqueues_status_comment( enqueue_update: Mock, _save_state: Mock, _clear_backfill_failure: Mock, + record_nudge: Mock, + record_copilot: Mock, ) -> None: - calculation = DashboardUpdate(results={}, dashboard_state={}, trigger_pr_result={}) + accepted_result = {"route": "author"} + calculation = DashboardUpdate( + results={}, + dashboard_state={"prs": {"12": accepted_result}}, + trigger_pr_result={"route": "approver"}, + ) merge_update.return_value = (calculation, False) - status = apply_targeted_dashboard_update(Namespace(pr_number=12), calculation) + status = apply_targeted_dashboard_update( + Namespace(pr_number=12, prepare_author_nudges=True), + calculation, + ) self.assertEqual(0, status) enqueue_update.assert_called_once_with(12) + record_nudge.assert_called_once_with( + 12, + accepted_result, + ANY, + prepare_due=True, + ) + record_copilot.assert_called_once_with( + 12, + accepted_result, + record_nudge.call_args.args[2], + ) + @patch("dashboard.record_copilot_review_observation") + @patch("dashboard.record_author_nudge_observation") @patch("dashboard.clear_backfill_pr_failure") @patch("dashboard.save_dashboard_update_state", return_value=0) @patch("dashboard.enqueue_status_comment_update") @@ -280,14 +758,27 @@ def test_unchanged_targeted_state_does_not_enqueue_status_comment( enqueue_update: Mock, _save_state: Mock, _clear_backfill_failure: Mock, + record_nudge: Mock, + _record_copilot: Mock, ) -> None: - calculation = DashboardUpdate(results={}, dashboard_state={}, trigger_pr_result={}) + accepted_result = {"route": "approver"} + calculation = DashboardUpdate( + results={}, + dashboard_state={"prs": {"12": accepted_result}}, + trigger_pr_result={"route": "author"}, + ) merge_update.return_value = (calculation, True) status = apply_targeted_dashboard_update(Namespace(pr_number=12), calculation) self.assertEqual(0, status) enqueue_update.assert_not_called() + record_nudge.assert_called_once_with( + 12, + accepted_result, + ANY, + prepare_due=False, + ) class RequiredCiRoutingTest(unittest.TestCase): @@ -496,6 +987,7 @@ def push_state_changes(_state_dir, _message, update_state, **_kwargs) -> int: side_effect=lambda result: result["failed"], ), patch("dashboard.save_dashboard_update_state", side_effect=save_dashboard_state), + patch("dashboard.record_author_nudge_observation") as record_nudge, patch("dashboard.state_branch.configure_git"), patch("dashboard.state_branch.checkout_state"), patch("dashboard.state_branch.remove_existing_state_dir"), @@ -504,6 +996,7 @@ def push_state_changes(_state_dir, _message, update_state, **_kwargs) -> int: status = update_dashboard_for_backfill(args, Path("state")) self.assertEqual(refreshed_pr_numbers, [1, 2]) + record_nudge.assert_called_once_with(2, ANY, ANY, prepare_due=False) self.assertEqual(status, BACKFILL_RECORDED_FAILURE_STATUS) self.assertEqual(dashboard_state["prs"], {"2": {"pr_number": 2, "failed": False, "route": "reviewer"}}) self.assertTrue(dashboard_state["initial_backfill_complete"]) diff --git a/.github/scripts/pull-request-dashboard/test_dashboard_override.py b/.github/scripts/pull-request-dashboard/test_dashboard_override.py new file mode 100644 index 00000000000..43178a1e6a0 --- /dev/null +++ b/.github/scripts/pull-request-dashboard/test_dashboard_override.py @@ -0,0 +1,710 @@ +from __future__ import annotations + +import unittest +from unittest.mock import call, patch + +import dashboard_override + + +class DashboardOverrideTest(unittest.TestCase): + def test_override_guidance_matches_pre_review_route(self) -> None: + self.assertIn( + "waiting on the author to waiting on reviewers", + dashboard_override.author_override_guidance(), + ) + self.assertIn( + "waiting on an external dependency or decision to waiting on reviewers", + dashboard_override.author_override_guidance(route="external"), + ) + + def test_dashboard_command_body_remainder(self) -> None: + self.assertIsNone( + dashboard_override.dashboard_command_body_remainder( + {"body": "just a normal comment"} + ) + ) + self.assertEqual( + "", + dashboard_override.dashboard_command_body_remainder( + {"body": "/dashboard route:reviewers"} + ), + ) + self.assertEqual( + "I addressed everything by doing X.", + dashboard_override.dashboard_command_body_remainder( + {"body": "/dashboard route:reviewers\n\nI addressed everything by doing X."} + ), + ) + self.assertEqual( + "I addressed the feedback.\nAdditional context follows.", + dashboard_override.dashboard_command_body_remainder({ + "body": ( + "/dashboard route:reviewers I addressed the feedback.\n" + "Additional context follows." + ) + }), + ) + + def test_latest_authorized_command_accepts_author_and_approvers(self) -> None: + raw = { + "issue_comments": [ + {"id": 1, "user": {"login": "outsider"}, "body": "/dashboard route:reviewers"}, + {"id": 2, "user": {"login": "author"}, "body": "text /dashboard route:reviewers"}, + {"id": 3, "user": {"login": "author"}, "body": "/dashboard route:reviewers\nplease review"}, + {"id": 4, "user": {"login": "Approver"}, "body": "/dashboard route:reviewers"}, + ] + } + + self.assertEqual( + (4, "Approver"), + dashboard_override.latest_authorized_command(raw, "author", {"approver"}), + ) + + def test_latest_authorized_command_ignores_app_acknowledged_command(self) -> None: + raw = { + "issue_comments": [ + {"id": 3, "user": {"login": "author"}, "body": "/dashboard route:reviewers"}, + { + "id": 4, + "user": {"login": "opentelemetry-pr-dashboard[bot]"}, + "body": dashboard_override.override_ack_marker(3), + }, + ] + } + + self.assertEqual( + (0, ""), + dashboard_override.latest_authorized_command(raw, "author", set()), + ) + + def test_is_authorized_commander_matches_author_or_approver(self) -> None: + self.assertTrue( + dashboard_override.is_authorized_commander("Author", "author", set()) + ) + self.assertTrue( + dashboard_override.is_authorized_commander("Approver", "author", {"approver"}) + ) + self.assertFalse( + dashboard_override.is_authorized_commander("outsider", "author", {"approver"}) + ) + + def test_pending_replies_for_unauthorized_and_unknown_commands(self) -> None: + raw = { + "issue_comments": [ + {"id": 1, "user": {"login": "author"}, "body": "/dashboard route:reviewers"}, + {"id": 2, "user": {"login": "outsider"}, "body": "/dashboard route:reviewers"}, + {"id": 3, "user": {"login": "reviewer"}, "body": "/dashboard frobnicate"}, + {"id": 4, "user": {"login": "author"}, "body": "/dashboard"}, + {"id": 5, "user": {"login": "reviewer"}, "body": "looks good to me"}, + {"id": 6, "user": {"login": "approver"}, "body": "/dashboard route:reviewers"}, + ] + } + + replies = dashboard_override.pending_command_replies(raw, "author", {"approver"}) + + self.assertEqual( + [ + {"comment_id": 2, "kind": "unauthorized", "user": "outsider", "subcommand": "route:reviewers"}, + {"comment_id": 3, "kind": "unknown_command", "user": "reviewer", "subcommand": "frobnicate"}, + {"comment_id": 4, "kind": "unknown_command", "user": "author", "subcommand": ""}, + ], + replies, + ) + + def test_already_replied_commands_are_not_repeated(self) -> None: + raw = { + "issue_comments": [ + {"id": 2, "user": {"login": "outsider"}, "body": "/dashboard route:reviewers"}, + { + "id": 9, + "user": {"login": "opentelemetry-pr-dashboard[bot]"}, + "body": dashboard_override.command_reply_marker(2) + "\n@outsider ...", + }, + ] + } + + self.assertEqual([], dashboard_override.pending_command_replies(raw, "author")) + + def test_forged_marker_from_non_app_user_does_not_suppress_reply(self) -> None: + raw = { + "issue_comments": [ + {"id": 2, "user": {"login": "outsider"}, "body": "/dashboard route:reviewers"}, + { + "id": 9, + "user": {"login": "outsider"}, + "body": dashboard_override.command_reply_marker(2) + "\nnothing to see here", + }, + ] + } + + replies = dashboard_override.pending_command_replies(raw, "author") + + self.assertEqual( + [{"comment_id": 2, "kind": "unauthorized", "user": "outsider", "subcommand": "route:reviewers"}], + replies, + ) + + def test_renders_command_replies(self) -> None: + unauthorized = dashboard_override.render_command_reply( + {"comment_id": 2, "kind": "unauthorized", "user": "outsider", "subcommand": "route:reviewers"} + ) + unknown = dashboard_override.render_command_reply( + {"comment_id": 3, "kind": "unknown_command", "user": "reviewer", "subcommand": "frobnicate"} + ) + routed = dashboard_override.render_command_reply( + {"comment_id": 4, "kind": "routed", "user": "author"} + ) + copilot_gated = dashboard_override.render_command_reply({ + "comment_id": 5, + "kind": "routed", + "route": "copilot", + "user": "author", + }) + + self.assertIn(dashboard_override.command_reply_marker(2), unauthorized) + self.assertIn( + "@outsider only the pull request author or a member of an approving " + "team can use `/dashboard route:reviewers`.", + unauthorized, + ) + self.assertIn(dashboard_override.command_reply_marker(3), unknown) + self.assertIn( + "`/dashboard frobnicate` is not a recognized dashboard command.", + unknown, + ) + self.assertIn(dashboard_override.command_reply_marker(4), routed) + self.assertIn(dashboard_override.override_ack_marker(4), routed) + self.assertIn("@author routed this pull request to reviewers.", routed) + self.assertIn(dashboard_override.command_reply_marker(5), copilot_gated) + self.assertIn( + "@author accepted the reviewer-routing override; the reviewer " + "handoff is waiting on Copilot.", + copilot_gated, + ) + + def test_renders_already_routed_replies_per_route(self) -> None: + cases = { + "approver": "already waiting on reviewers", + "maintainer": "already past review and waiting on maintainers", + "external": "waiting on an external dependency, not on you", + "copilot": "waiting on an automated Copilot review", + } + for route, phrase in cases.items(): + with self.subTest(route=route): + body = dashboard_override.render_command_reply( + {"comment_id": 7, "kind": "already_routed", "user": "author", "route": route} + ) + self.assertIn(dashboard_override.command_reply_marker(7), body) + self.assertIn(dashboard_override.override_ack_marker(7), body) + self.assertIn(f"@author this pull request is {phrase}, so", body) + self.assertIn("`/dashboard route:reviewers` had no effect", body) + + def test_appends_already_routed_reply_for_author_noop(self) -> None: + facts = { + "author": "author", + "dashboard_override_noop": True, + "dashboard_override_command_id": 12, + } + + dashboard_override.append_route_noop_reply({"issue_comments": []}, facts, "approver") + + self.assertEqual( + [{"comment_id": 12, "kind": "already_routed", "user": "author", "route": "approver"}], + facts["dashboard_command_replies"], + ) + + def test_no_already_routed_reply_when_command_applies(self) -> None: + facts = { + "author": "author", + "dashboard_override_noop": False, + "dashboard_override_command_id": 12, + } + + dashboard_override.append_route_noop_reply({"issue_comments": []}, facts, "author") + + self.assertNotIn("dashboard_command_replies", facts) + + def test_already_routed_reply_deduped_by_existing_marker(self) -> None: + facts = { + "author": "author", + "dashboard_override_noop": True, + "dashboard_override_command_id": 12, + } + raw = { + "issue_comments": [ + { + "user": {"login": "opentelemetry-pr-dashboard[bot]"}, + "body": dashboard_override.command_reply_marker(12) + "\n@author ...", + }, + ] + } + + dashboard_override.append_route_noop_reply(raw, facts, "approver") + + self.assertNotIn("dashboard_command_replies", facts) + + def test_forged_marker_does_not_dedupe_already_routed_reply(self) -> None: + facts = { + "author": "author", + "dashboard_override_noop": True, + "dashboard_override_command_id": 12, + } + raw = { + "issue_comments": [ + { + "user": {"login": "outsider"}, + "body": dashboard_override.command_reply_marker(12) + "\n@author ...", + }, + ] + } + + dashboard_override.append_route_noop_reply(raw, facts, "approver") + + self.assertEqual( + [{"comment_id": 12, "kind": "already_routed", "user": "author", "route": "approver"}], + facts["dashboard_command_replies"], + ) + + @patch.object(dashboard_override, "run_gh") + @patch.object(dashboard_override, "gh_api", return_value=[]) + @patch.object( + dashboard_override, + "load_dashboard_state_cache", + return_value={ + "prs": { + "5": { + "facts": { + "dashboard_command_replies": [ + {"comment_id": 2, "kind": "unauthorized", "user": "outsider", "subcommand": "route:reviewers"}, + ] + } + } + } + }, + ) + def test_delivers_pending_command_reply(self, _load_state, gh_api, run_gh) -> None: + errors = dashboard_override.deliver_dashboard_command_replies( + "open-telemetry/example" + ) + + self.assertEqual([], errors) + gh_api.assert_called_once_with( + "/repos/open-telemetry/example/issues/5/comments?per_page=100", + paginate=True, + ) + posted = run_gh.call_args.args[0] + self.assertEqual(posted[:5], ["gh", "api", "--method", "POST", "repos/open-telemetry/example/issues/5/comments"]) + self.assertIn(dashboard_override.command_reply_marker(2), posted[-1]) + + @patch.object(dashboard_override, "run_gh") + @patch.object( + dashboard_override, + "gh_api", + return_value=[ + { + "performed_via_github_app": {"slug": "opentelemetry-pr-dashboard"}, + "body": " already replied", + } + ], + ) + @patch.object( + dashboard_override, + "load_dashboard_state_cache", + return_value={ + "prs": { + "5": { + "facts": { + "dashboard_command_replies": [ + {"comment_id": 2, "kind": "unauthorized", "user": "outsider", "subcommand": "route:reviewers"}, + ] + } + } + } + }, + ) + def test_delivery_skips_already_replied_command(self, _load_state, _gh_api, run_gh) -> None: + errors = dashboard_override.deliver_dashboard_command_replies( + "open-telemetry/example" + ) + + self.assertEqual([], errors) + run_gh.assert_not_called() + + def test_command_stays_pending_until_app_acknowledges_it(self) -> None: + raw = { + "issue_comments": [ + {"id": 3, "user": {"login": "author"}, "body": "/dashboard route:reviewers"}, + ] + } + + first = dashboard_override.dashboard_override_facts(raw, "author", set()) + retry = dashboard_override.dashboard_override_facts(raw, "author", set()) + label_observed = dashboard_override.dashboard_override_facts( + raw, + "author", + {"dashboard:route-overridden"}, + ) + acknowledged_raw = { + "issue_comments": [ + *raw["issue_comments"], + { + "id": 4, + "user": {"login": "opentelemetry-pr-dashboard[bot]"}, + "body": dashboard_override.override_ack_marker(3), + }, + ] + } + acknowledged = dashboard_override.dashboard_override_facts( + acknowledged_raw, + "author", + set(), + ) + + self.assertTrue(first["dashboard_override_requested"]) + self.assertTrue(retry["dashboard_override_requested"]) + self.assertTrue(label_observed["dashboard_override_label_applied"]) + self.assertEqual(3, label_observed["dashboard_override_command_id"]) + self.assertFalse(label_observed["dashboard_override_requested"]) + self.assertEqual(0, acknowledged["dashboard_override_command_id"]) + self.assertFalse(acknowledged["dashboard_override_requested"]) + + def test_newer_command_reapplies_removed_override(self) -> None: + raw = { + "issue_comments": [ + {"id": 3, "user": {"login": "author"}, "body": "/dashboard route:reviewers"}, + { + "id": 4, + "user": {"login": "opentelemetry-pr-dashboard[bot]"}, + "body": dashboard_override.override_ack_marker(3), + }, + {"id": 5, "user": {"login": "author"}, "body": "/dashboard route:reviewers"}, + ] + } + + facts = dashboard_override.dashboard_override_facts( + raw, + "author", + set(), + ) + + self.assertTrue(facts["dashboard_override_requested"]) + self.assertEqual(5, facts["dashboard_override_command_id"]) + + def test_rebuilds_unacknowledged_already_routed_reply_across_refreshes(self) -> None: + raw = { + "issue_comments": [ + {"id": 5, "user": {"login": "author"}, "body": "/dashboard route:reviewers"}, + ] + } + + for _ in range(2): + facts = dashboard_override.dashboard_override_facts(raw, "author", set()) + route = dashboard_override.apply_dashboard_override(facts, "approver") + dashboard_override.append_route_noop_reply(raw, facts, route) + + self.assertEqual( + [{"comment_id": 5, "kind": "already_routed", "user": "author", "route": "approver"}], + facts["dashboard_command_replies"], + ) + + def test_acknowledged_command_does_not_replay_after_cache_eviction(self) -> None: + raw = { + "issue_comments": [ + {"id": 5, "user": {"login": "author"}, "body": "/dashboard route:reviewers"}, + { + "id": 9, + "user": {"login": "opentelemetry-pr-dashboard[bot]"}, + "body": dashboard_override.override_ack_marker(5) + "\n@author ...", + }, + ] + } + + facts = dashboard_override.dashboard_override_facts( + raw, + "author", + set(), + ) + + self.assertEqual(0, facts["dashboard_override_command_id"]) + self.assertFalse(facts["dashboard_override_requested"]) + self.assertEqual([], facts["dashboard_command_replies"]) + + def test_newest_acknowledgement_consumes_older_authorized_commands(self) -> None: + raw = { + "issue_comments": [ + {"id": 3, "user": {"login": "author"}, "body": "/dashboard route:reviewers"}, + {"id": 5, "user": {"login": "author"}, "body": "/dashboard route:reviewers"}, + { + "id": 9, + "user": {"login": "opentelemetry-pr-dashboard[bot]"}, + "body": dashboard_override.override_ack_marker(5), + }, + ] + } + + self.assertEqual( + (0, ""), + dashboard_override.latest_authorized_command(raw, "author", set()), + ) + + def test_fresh_command_with_label_present_is_a_noop(self) -> None: + facts = { + "dashboard_override_label_applied": True, + "dashboard_override_requested": False, + "dashboard_override_command_id": 5, + } + + route = dashboard_override.apply_dashboard_override(facts, "approver") + + self.assertEqual("approver", route) + self.assertTrue(facts["dashboard_override_noop"]) + self.assertTrue(facts["dashboard_override_release_requested"]) + + def test_fresh_command_gets_reply_when_label_already_overrides_author_route(self) -> None: + raw = { + "issue_comments": [ + {"id": 5, "user": {"login": "author"}, "body": "/dashboard route:reviewers"}, + ] + } + facts = dashboard_override.dashboard_override_facts( + raw, + "author", + {"dashboard:route-overridden"}, + ) + + route = dashboard_override.apply_dashboard_override(facts, "author") + dashboard_override.append_route_noop_reply(raw, facts, route) + + self.assertEqual("approver", route) + self.assertTrue(facts["dashboard_override_noop"]) + self.assertEqual( + [{"comment_id": 5, "kind": "already_routed", "user": "author", "route": "approver"}], + facts["dashboard_command_replies"], + ) + + def test_command_only_overrides_pre_review_routes(self) -> None: + for route, expected_route, expected_pending, expected_noop in ( + ("author", "approver", True, False), + ("external", "approver", True, False), + ("approver", "approver", False, True), + ("maintainer", "maintainer", False, True), + ("copilot", "copilot", False, True), + ): + with self.subTest(route=route): + facts = { + "dashboard_override_label_applied": False, + "dashboard_override_requested": True, + "dashboard_override_command_id": 5, + } + + actual = dashboard_override.apply_dashboard_override(facts, route) + + self.assertEqual(expected_route, actual) + self.assertEqual(expected_pending, facts["dashboard_override_requested"]) + self.assertEqual(expected_noop, facts["dashboard_override_noop"]) + + def test_label_yields_to_maintainer_route_and_releases(self) -> None: + facts = { + "dashboard_override_label_applied": True, + "dashboard_override_requested": False, + } + + route = dashboard_override.apply_dashboard_override(facts, "maintainer") + + self.assertEqual("maintainer", route) + self.assertFalse(facts["dashboard_override"]) + self.assertTrue(facts["dashboard_override_release_requested"]) + + def test_label_holds_external_route_at_reviewers(self) -> None: + facts = { + "dashboard_override_label_applied": True, + "dashboard_override_requested": False, + } + + route = dashboard_override.apply_dashboard_override(facts, "external") + + self.assertEqual("approver", route) + self.assertTrue(facts["dashboard_override"]) + self.assertFalse(facts["dashboard_override_release_requested"]) + + def test_releases_label_once_natural_route_reaches_reviewers(self) -> None: + facts = { + "dashboard_override_label_applied": True, + "dashboard_override_requested": False, + } + + route = dashboard_override.apply_dashboard_override(facts, "approver") + + self.assertEqual("approver", route) + self.assertTrue(facts["dashboard_override_release_requested"]) + + def test_does_not_release_label_while_override_holds_author_route(self) -> None: + facts = { + "dashboard_override_label_applied": True, + "dashboard_override_requested": False, + } + + route = dashboard_override.apply_dashboard_override(facts, "author") + + self.assertEqual("approver", route) + self.assertFalse(facts["dashboard_override_release_requested"]) + + @patch.object(dashboard_override, "run_gh") + @patch.object( + dashboard_override, + "load_dashboard_state_cache", + return_value={ + "prs": { + "9": {"facts": {"dashboard_override_release_requested": True}}, + } + }, + ) + def test_delivery_removes_released_override_label(self, _load_state, run_gh) -> None: + errors = dashboard_override.deliver_dashboard_override_requests( + "open-telemetry/example" + ) + + self.assertEqual([], errors) + self.assertEqual( + [ + call([ + "gh", "api", "--method", "DELETE", + "repos/open-telemetry/example/issues/9/labels/dashboard%3Aroute-overridden", + ]), + ], + run_gh.call_args_list, + ) + + @patch.object(dashboard_override, "run_gh") + @patch.object(dashboard_override, "load_dashboard_state_cache", return_value=None) + def test_does_not_create_label_without_dashboard_state(self, _load_state, run_gh) -> None: + errors = dashboard_override.deliver_dashboard_override_requests( + "open-telemetry/example" + ) + + self.assertEqual([], errors) + run_gh.assert_not_called() + + @patch.object(dashboard_override, "run_gh") + @patch.object( + dashboard_override, + "load_dashboard_state_cache", + return_value={"prs": {}}, + ) + def test_does_not_create_label_without_pending_command(self, _load_state, run_gh) -> None: + errors = dashboard_override.deliver_dashboard_override_requests( + "open-telemetry/example" + ) + + self.assertEqual([], errors) + run_gh.assert_not_called() + + @patch.object(dashboard_override, "run_gh") + @patch.object(dashboard_override, "gh_api", return_value=[]) + @patch.object( + dashboard_override, + "load_dashboard_state_cache", + return_value={ + "prs": { + "7": { + "route": "copilot", + "facts": { + "dashboard_override_requested": True, + "dashboard_override_command_id": 3, + "dashboard_override_command_user": "author", + } + }, + "8": {"facts": {"dashboard_override_requested": False}}, + } + }, + ) + def test_delivers_pending_override_label_and_acknowledges_command( + self, + _load_state, + gh_api, + run_gh, + ) -> None: + errors = dashboard_override.deliver_dashboard_override_requests( + "open-telemetry/example" + ) + + self.assertEqual([], errors) + gh_api.assert_called_once_with( + "/repos/open-telemetry/example/issues/7/comments?per_page=100", + paginate=True, + ) + self.assertEqual( + [ + call([ + "gh", "label", "create", "dashboard:route-overridden", + "--repo", "open-telemetry/example", + "--color", dashboard_override.DASHBOARD_OVERRIDE_LABEL_COLOR, + "--description", dashboard_override.DASHBOARD_OVERRIDE_LABEL_DESCRIPTION, + "--force", + ]), + call([ + "gh", "api", "--method", "POST", + "repos/open-telemetry/example/issues/7/labels", + "-f", "labels[]=dashboard:route-overridden", + ]), + call([ + "gh", "api", "--method", "POST", + "repos/open-telemetry/example/issues/7/comments", + "-f", "body=\n\n@author accepted the reviewer-routing override; the reviewer handoff is waiting on Copilot.\n", + ]), + ], + run_gh.call_args_list, + ) + + @patch.object(dashboard_override, "run_gh") + @patch.object(dashboard_override, "gh_api", return_value=[]) + @patch.object( + dashboard_override, + "load_dashboard_state_cache", + return_value={ + "prs": { + "7": { + "facts": { + "dashboard_override_requested": True, + "dashboard_override_command_id": 3, + "dashboard_override_command_user": "author", + } + }, + } + }, + ) + def test_retry_observes_acknowledgement_after_ambiguous_post_failure( + self, + _load_state, + gh_api, + run_gh, + ) -> None: + run_gh.side_effect = [None, None, RuntimeError("comment response lost")] + + errors = dashboard_override.deliver_dashboard_override_requests( + "open-telemetry/example" + ) + + self.assertEqual(["PR #7: comment response lost"], errors) + + run_gh.reset_mock() + run_gh.side_effect = None + gh_api.return_value = [{ + "performed_via_github_app": {"slug": "opentelemetry-pr-dashboard"}, + "body": dashboard_override.render_command_reply({ + "comment_id": 3, + "kind": "routed", + "user": "author", + }), + }] + + errors = dashboard_override.deliver_dashboard_override_requests( + "open-telemetry/example" + ) + + self.assertEqual([], errors) + self.assertEqual(2, run_gh.call_count) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/.github/scripts/pull-request-dashboard/test_delivery.py b/.github/scripts/pull-request-dashboard/test_delivery.py new file mode 100644 index 00000000000..c646b8d1ad9 --- /dev/null +++ b/.github/scripts/pull-request-dashboard/test_delivery.py @@ -0,0 +1,162 @@ +from __future__ import annotations + +from pathlib import Path +import unittest +from unittest.mock import ANY, Mock, call, patch + +import delivery + + +class DeliveryTest(unittest.TestCase): + @patch.object(delivery, "notify_slack_from_state", return_value=[]) + @patch.object(delivery, "deliver_copilot_review_requests", return_value=[]) + @patch.object(delivery, "deliver_prepared_author_nudges", return_value=[]) + @patch.object(delivery, "deliver_dashboard_command_replies", return_value=[]) + @patch.object(delivery, "deliver_dashboard_override_requests", return_value=[]) + @patch.object(delivery, "update_status_comments_from_state", return_value=[]) + @patch.object( + delivery, + "list_open_prs", + return_value=[ + {"number": 7, "isDraft": False, "title": "Seven"}, + {"number": 8, "isDraft": True, "title": "Eight"}, + ], + ) + def test_runs_all_repository_deliveries_in_order( + self, + _list_open, + status_comments, + dashboard_overrides, + dashboard_command_replies, + author_nudges, + copilot_reviews, + slack, + ) -> None: + order = Mock() + + def record(label: str) -> list[str]: + order(label) + return [] + + status_comments.side_effect = lambda *_args: record("status") + dashboard_overrides.side_effect = lambda *_args: record("override") + 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") + slack.side_effect = lambda *_args: record("slack") + errors = delivery.deliver_from_state( + "open-telemetry/example", + Path("author"), + Path("copilot"), + Path("slack"), + ) + + self.assertEqual([], errors) + _list_open.assert_called_once_with("open-telemetry/example") + self.assertEqual( + [call("override"), call("replies"), call("author"), call("status"), call("copilot"), call("slack")], + order.call_args_list, + ) + status_comments.assert_called_once_with( + "open-telemetry/example", + {7, 8}, + ) + slack.assert_called_once_with( + "open-telemetry/example", + ANY, + [ + {"number": 7, "isDraft": False, "title": "Seven"}, + {"number": 8, "isDraft": True, "title": "Eight"}, + ], + ANY, + ) + + @patch.object(delivery, "notify_slack_from_state", return_value=[]) + @patch.object(delivery, "deliver_copilot_review_requests", return_value=[]) + @patch.object(delivery, "deliver_prepared_author_nudges", return_value=[]) + @patch.object(delivery, "deliver_dashboard_command_replies", return_value=[]) + @patch.object(delivery, "deliver_dashboard_override_requests", return_value=[]) + @patch.object(delivery, "update_status_comments_from_state", side_effect=RuntimeError("boom")) + @patch.object( + delivery, + "list_open_prs", + return_value=[{"number": 7, "isDraft": False, "title": "Seven"}], + ) + def test_failure_does_not_block_later_deliveries( + self, + _list_open, + _status_comments, + dashboard_overrides, + dashboard_command_replies, + author_nudges, + copilot_reviews, + slack, + ) -> None: + errors = delivery.deliver_from_state( + "open-telemetry/example", + Path("author"), + Path("copilot"), + Path("slack"), + ) + + self.assertIn("status comments: boom", errors) + dashboard_overrides.assert_called_once() + dashboard_command_replies.assert_called_once() + author_nudges.assert_called_once() + copilot_reviews.assert_called_once() + slack.assert_called_once() + + def test_open_pr_list_failure_skips_dependent_stages(self) -> None: + with ( + patch.object(delivery, "list_open_prs", side_effect=RuntimeError("unavailable")), + patch.object(delivery, "deliver_dashboard_override_requests", return_value=[]) as overrides, + patch.object(delivery, "deliver_dashboard_command_replies", return_value=[]) as replies, + patch.object(delivery, "deliver_prepared_author_nudges", return_value=[]) as nudges, + patch.object(delivery, "update_status_comments_from_state", return_value=[]) as status, + patch.object(delivery, "deliver_copilot_review_requests", return_value=[]) as copilot, + patch.object(delivery, "notify_slack_from_state", return_value=[]) as slack, + ): + errors = delivery.deliver_from_state( + "open-telemetry/example", + Path("author"), + Path("copilot"), + Path("slack"), + ) + + self.assertEqual(["open pull requests: unavailable"], errors) + overrides.assert_called_once() + replies.assert_called_once() + nudges.assert_called_once() + copilot.assert_called_once() + status.assert_not_called() + slack.assert_not_called() + + @patch.object(delivery.sys, "stderr") + @patch.object(delivery, "deliver_from_state", return_value=["status comments: boom"]) + @patch.object(delivery.state_branch, "push_state_changes") + def test_reports_delivery_errors_after_state_push( + self, + push_state_changes, + _deliver_from_state, + _stderr, + ) -> None: + push_state_changes.side_effect = ( + lambda _state_dir, _message, update_state, **_kwargs: update_state() + ) + + with ( + patch.object(delivery, "author_nudge_state_path", return_value=Path("author")), + patch.object(delivery, "copilot_review_request_state_path", return_value=Path("copilot")), + patch.object(delivery, "notification_state_path", return_value=Path("slack")), + ): + status = delivery.deliver_with_state( + "open-telemetry/example", + "dashboard-state", + Path("state"), + ) + + self.assertEqual(1, status) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/.github/scripts/pull-request-dashboard/test_github_cli.py b/.github/scripts/pull-request-dashboard/test_github_cli.py index efe20db47b8..e6c5beee80b 100644 --- a/.github/scripts/pull-request-dashboard/test_github_cli.py +++ b/.github/scripts/pull-request-dashboard/test_github_cli.py @@ -1,7 +1,7 @@ from __future__ import annotations import unittest -from unittest.mock import patch +from unittest.mock import ANY, patch from github_cli import ( TransientGhError, @@ -10,15 +10,45 @@ fetch_pr_title_edits, gh_pr_check_rollup, gh_pr_checks, + gh_pr_view, gh_required_check_contexts, include_missing_required_checks, is_retryable_gh_error, - list_all_open_pr_numbers, list_open_prs, + request_copilot_review, ) class GithubCliTest(unittest.TestCase): + @patch("github_cli.run_gh_json") + def test_pr_view_fetches_body_for_routing_freshness(self, run_json) -> None: + run_json.return_value = {"mergeable": "MERGEABLE"} + + gh_pr_view("open-telemetry/example", 7) + + fields = run_json.call_args.args[0][-1] + self.assertIn("title", fields.split(",")) + self.assertIn("body", fields.split(",")) + + @patch("github_cli.gh_graphql") + def test_request_copilot_review_uses_request_reviews_mutation( + self, + graphql, + ) -> None: + request_copilot_review("PR_node_id") + + graphql.assert_called_once_with( + ANY, + { + "pullRequestId": "PR_node_id", + "botId": "BOT_kgDOCnlnWA", + }, + ) + mutation = graphql.call_args.args[0] + self.assertIn("requestReviews", mutation) + self.assertIn("botIds: [$botId]", mutation) + self.assertIn("union: true", mutation) + @patch("github_cli.gh_graphql") def test_fetch_pr_issue_comments_paginates(self, graphql) -> None: graphql.side_effect = [ @@ -194,22 +224,6 @@ def test_list_open_prs_uses_paginated_rest_api(self, gh_api) -> None: paginate=True, ) - @patch("github_cli.gh_api") - def test_list_all_open_pr_numbers_uses_paginated_rest_api(self, gh_api) -> None: - gh_api.return_value = [ - {"number": number} - for number in range(1, 502) - ] - - self.assertEqual( - set(range(1, 502)), - list_all_open_pr_numbers("open-telemetry/example"), - ) - gh_api.assert_called_once_with( - "/repos/open-telemetry/example/pulls?state=open&per_page=100", - paginate=True, - ) - @patch("github_cli.gh_graphql") def test_gh_pr_checks_preserves_reporting_app_identity(self, graphql) -> None: graphql.return_value = { @@ -559,6 +573,8 @@ def test_fetch_pr_review_data_normalizes_paginated_reviews_and_metadata(self, gr "nodes": [ { "fullDatabaseId": "4700712792", + "commit": {"oid": "reviewed-head-1"}, + "comments": {"totalCount": 1}, "url": "https://example.test/review/4700712792", "author": {"login": "reviewer-1"}, "state": "COMMENTED", @@ -586,6 +602,8 @@ def test_fetch_pr_review_data_normalizes_paginated_reviews_and_metadata(self, gr "nodes": [ { "fullDatabaseId": "5000000000", + "commit": {"oid": "reviewed-head-2"}, + "comments": {"totalCount": 0}, "url": "https://example.test/review/5000000000", "author": {"login": "reviewer-2"}, "state": "APPROVED", @@ -608,6 +626,8 @@ def test_fetch_pr_review_data_normalizes_paginated_reviews_and_metadata(self, gr "reviews": [ { "id": 4700712792, + "commit_id": "reviewed-head-1", + "finding_count": 1, "url": "https://example.test/review/4700712792", "user": {"login": "reviewer-1"}, "state": "COMMENTED", @@ -617,6 +637,8 @@ def test_fetch_pr_review_data_normalizes_paginated_reviews_and_metadata(self, gr }, { "id": 5000000000, + "commit_id": "reviewed-head-2", + "finding_count": 0, "url": "https://example.test/review/5000000000", "user": {"login": "reviewer-2"}, "state": "APPROVED", @@ -631,6 +653,9 @@ def test_fetch_pr_review_data_normalizes_paginated_reviews_and_metadata(self, gr }, }, ) + review_query = graphql.call_args_list[0].args[0] + self.assertIn("comments {", review_query) + self.assertIn("totalCount", review_query) self.assertEqual(graphql.call_args_list[1].args[1]["after"], "cursor-1") self.assertEqual(graphql.call_count, 2) diff --git a/.github/scripts/pull-request-dashboard/test_github_webhook.js b/.github/scripts/pull-request-dashboard/test_github_webhook.js index 207dbcbe350..8cbb47540c1 100644 --- a/.github/scripts/pull-request-dashboard/test_github_webhook.js +++ b/.github/scripts/pull-request-dashboard/test_github_webhook.js @@ -1,11 +1,24 @@ const assert = require("node:assert/strict"); const test = require("node:test"); -const { isDashboardSelfTriggeredCommentEvent } = require("./netlify/functions/github-webhook"); +const { + isAllowedAction, + isDashboardSelfTriggeredCommentEvent, +} = require("./netlify/functions/github-webhook"); const dashboardApp = { slug: "opentelemetry-pr-dashboard" }; const dashboardActor = { id: 1 }; +test("refreshes when the dashboard override label changes", () => { + assert.equal(isAllowedAction("pull_request", "labeled"), true); + assert.equal(isAllowedAction("pull_request", "unlabeled"), true); +}); + +test("refreshes when a review request is added or removed", () => { + assert.equal(isAllowedAction("pull_request", "review_requested"), true); + assert.equal(isAllowedAction("pull_request", "review_request_removed"), true); +}); + test("recognizes comments performed by the dashboard app", () => { assert.equal(isDashboardSelfTriggeredCommentEvent("issue_comment", { comment: { diff --git a/.github/scripts/pull-request-dashboard/test_notify_slack.py b/.github/scripts/pull-request-dashboard/test_notify_slack.py index 0f758de77e7..53901f7bdc4 100644 --- a/.github/scripts/pull-request-dashboard/test_notify_slack.py +++ b/.github/scripts/pull-request-dashboard/test_notify_slack.py @@ -1,20 +1,52 @@ from __future__ import annotations +from datetime import datetime, timezone import unittest from unittest.mock import patch +from notifications import next_notifications from notify_slack import notify_slack_from_state class NotifySlackTest(unittest.TestCase): + @patch("notifications.send_slack_notification") + def test_copilot_route_does_not_notify_reviewers(self, send_notification) -> None: + results = { + 7: { + "pr_number": 7, + "route": "copilot", + "facts": { + "reviewers": [{"login": "reviewer"}], + "waiting_since": "2026-07-20T01:00:00Z", + }, + }, + } + + with patch.dict( + "os.environ", + { + "SLACK_CHANNEL": "dashboard", + "SLACK_USER_MAP_JSON": '{"reviewer": "U123"}', + }, + clear=True, + ): + updated, errors = next_notifications( + "open-telemetry/example", + results, + {}, + datetime(2026, 7, 20, 2, tzinfo=timezone.utc), + ) + + self.assertEqual(updated, {}) + self.assertEqual(errors, []) + send_notification.assert_not_called() + @patch("notify_slack.save_notifications") @patch("notify_slack.load_notifications") - @patch("notify_slack.list_open_prs") @patch("notify_slack.load_dashboard_state_cache") def test_uncached_pr_does_not_pause_notifications_and_closed_state_is_pruned( self, load_dashboard_state_cache, - list_open_prs, load_notifications, save_notifications, ) -> None: @@ -27,7 +59,7 @@ def test_uncached_pr_does_not_pause_notifications_and_closed_state_is_pruned( }, } } - list_open_prs.return_value = [ + open_prs = [ {"number": 2, "isDraft": False, "title": "Open PR"}, {"number": 5, "isDraft": False, "title": "Not cached yet"}, {"number": 3, "isDraft": True, "title": "Draft PR"}, @@ -48,7 +80,12 @@ def test_uncached_pr_does_not_pause_notifications_and_closed_state_is_pruned( } with patch.dict("os.environ", {"SLACK_CHANNEL": "dashboard"}, clear=True): - errors = notify_slack_from_state("owner/repo", None, None, None) + errors = notify_slack_from_state( + "owner/repo", + None, + open_prs, + datetime(2026, 7, 20, 2, tzinfo=timezone.utc), + ) self.assertEqual(errors, []) save_notifications.assert_called_once_with( diff --git a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py index 3dac940859f..4539c5ddea0 100644 --- a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py @@ -26,6 +26,7 @@ def test_waiting_on_author_splits_review_feedback_links(self) -> None: "route": "author", "facts": { "author": "alice", + "author_nudge_episode_id": "abc123", "author_action_review_thread_urls": [ "https://github.com/open-telemetry/example/pull/1#discussion_r1", ], @@ -45,10 +46,74 @@ def test_waiting_on_author_splits_review_feedback_links(self) -> None: f"", body, ) + self.assertIn( + pr_status_comment.author_nudge_episode_marker("abc123"), + body, + ) self.assertNotIn("### Review feedback", body) self.assertIn(" - **Inline threads:** [1]", body) self.assertIn(" - **Top-level feedback:** [2]", body) self.assertIn(f" - _{pr_status_comment.AUTHOR_GUIDANCE}_", body) + self.assertIn( + "If you believe this pull request is incorrectly routed as waiting " + "on the author, comment `/dashboard route:reviewers` to route it from " + "waiting on the author to waiting on reviewers. If the last refreshed " + "time above predates your latest reply or push, the dashboard hasn't " + "processed it yet.", + body, + ) + + def test_recovers_episode_only_from_app_authored_status_comment(self) -> None: + marker = pr_status_comment.author_nudge_episode_marker("abc123") + comments = [ + { + "performed_via_github_app": None, + "body": f"{pr_status_comment.STATUS_MARKER}\n{marker}", + }, + { + "performed_via_github_app": { + "slug": pr_status_comment.DASHBOARD_APP_SLUG, + }, + "body": marker, + }, + { + "performed_via_github_app": { + "slug": pr_status_comment.DASHBOARD_APP_SLUG, + }, + "body": f"{pr_status_comment.STATUS_MARKER}\n{marker}", + }, + ] + + self.assertEqual( + "abc123", + pr_status_comment.status_author_nudge_episode_id(comments), + ) + + def test_recovers_episode_from_normalized_dashboard_bot_comment(self) -> None: + marker = pr_status_comment.author_nudge_episode_marker("abc123") + + self.assertEqual( + "abc123", + pr_status_comment.status_author_nudge_episode_id([{ + "user": {"login": "opentelemetry-pr-dashboard[bot]"}, + "body": f"{pr_status_comment.STATUS_MARKER}\n{marker}", + }]), + ) + + def test_does_not_recover_episode_from_other_normalized_authors(self) -> None: + marker = pr_status_comment.author_nudge_episode_marker("abc123") + comments = [ + { + "user": {"login": login}, + "body": f"{pr_status_comment.STATUS_MARKER}\n{marker}", + } + for login in ("alice", "another-app[bot]") + ] + + self.assertEqual( + "", + pr_status_comment.status_author_nudge_episode_id(comments), + ) @patch.object( pr_status_comment, @@ -371,10 +436,24 @@ def test_author_login_is_not_mentioned(self) -> None: self.assertIn("- **Waiting on:** Author", body) self.assertNotIn("@alice", body) + def test_external_route_advertises_reviewer_override(self) -> None: + body = pr_status_comment.render_status_comment( + self.pr(), + {"route": "external", "facts": {}}, + ) + + self.assertIn( + "waiting on an external dependency or decision, comment " + "`/dashboard route:reviewers` to route it from waiting on an " + "external dependency or decision to waiting on reviewers", + body, + ) + def test_routes_render_one_status_sentence(self) -> None: expected_summaries = { "approver": ("Reviewers", "Review the latest changes."), "maintainer": ("Maintainers", "Merge when ready."), + "copilot": ("Copilot", "Wait for the pending review to complete."), "external": ("An external dependency or decision", "Resolve it before work can continue."), "transient-failure": ("Pull request dashboard maintainers", "Determine the next action."), "unknown": ("Pull request dashboard maintainers", "Determine the next action."), @@ -499,62 +578,29 @@ def test_current_revision_drops_closed_prs_from_queue(self) -> None: pr_status_comment, "load_status_comment_rollout_state", return_value={ - "target_revision": 0, + "target_revision": pr_status_comment.STATUS_COMMENT_REVISION, "completed_revision": 0, - "pending_pr_numbers": [], + "pending_pr_numbers": [12, 34], }, ) - def test_rollout_drains_capped_batch( + def test_rollout_drains_queued_closed_pr( self, _load_rollout: object, _load_dashboard: object, publish_pr_status: Mock, save_rollout: Mock, ) -> None: - open_pr_numbers = set(range(1, 56)) - status = pr_status_comment.update_status_comments_from_state( "open-telemetry/example", - None, - open_pr_numbers, + {34}, ) self.assertEqual([], status) self.assertEqual( - list(range(1, 51)), + [12, 34], [call.args[1] for call in publish_pr_status.call_args_list], ) saved_state = save_rollout.call_args.args[0] - self.assertEqual([51, 52, 53, 54, 55], saved_state["pending_pr_numbers"]) - self.assertEqual(0, saved_state["completed_revision"]) - - @patch.object(pr_status_comment, "save_status_comment_rollout_state") - @patch.object(pr_status_comment, "publish_pr_status") - @patch.object(pr_status_comment, "load_dashboard_state_cache", return_value={"prs": {}}) - @patch.object( - pr_status_comment, - "load_status_comment_rollout_state", - return_value={ - "target_revision": pr_status_comment.STATUS_COMMENT_REVISION, - "completed_revision": 0, - "pending_pr_numbers": [12], - }, - ) - def test_targeted_refresh_completes_last_pending_comment( - self, - _load_rollout: object, - _load_dashboard: object, - _publish_pr_status: object, - save_rollout: Mock, - ) -> None: - status = pr_status_comment.update_status_comments_from_state( - "open-telemetry/example", - 12, - {12}, - ) - - self.assertEqual([], status) - saved_state = save_rollout.call_args.args[0] self.assertEqual([], saved_state["pending_pr_numbers"]) self.assertEqual( pr_status_comment.STATUS_COMMENT_REVISION, @@ -573,54 +619,28 @@ def test_targeted_refresh_completes_last_pending_comment( "pending_pr_numbers": [], }, ) - def test_targeted_refresh_initializes_revision_without_requeueing_itself( + def test_rollout_drains_capped_batch( self, _load_rollout: object, _load_dashboard: object, - _publish_pr_status: object, + publish_pr_status: Mock, save_rollout: Mock, ) -> None: - status = pr_status_comment.update_status_comments_from_state( - "open-telemetry/example", - 12, - {12, 34}, - ) - - self.assertEqual([], status) - saved_state = save_rollout.call_args.args[0] - self.assertEqual(pr_status_comment.STATUS_COMMENT_REVISION, saved_state["target_revision"]) - self.assertEqual([34], saved_state["pending_pr_numbers"]) - self.assertEqual(0, saved_state["completed_revision"]) + open_pr_numbers = set(range(1, 56)) - @patch.object(pr_status_comment, "save_status_comment_rollout_state") - @patch.object(pr_status_comment, "publish_pr_status") - @patch.object(pr_status_comment, "load_dashboard_state_cache", return_value={"prs": {}}) - @patch.object( - pr_status_comment, - "load_status_comment_rollout_state", - return_value={ - "target_revision": 0, - "completed_revision": 0, - "pending_pr_numbers": [], - }, - ) - def test_closed_targeted_pr_still_initializes_revision( - self, - _load_rollout: object, - _load_dashboard: object, - _publish_pr_status: object, - save_rollout: Mock, - ) -> None: status = pr_status_comment.update_status_comments_from_state( "open-telemetry/example", - 12, - {34}, + open_pr_numbers, ) self.assertEqual([], status) + self.assertEqual( + list(range(1, 51)), + [call.args[1] for call in publish_pr_status.call_args_list], + ) saved_state = save_rollout.call_args.args[0] - self.assertEqual(pr_status_comment.STATUS_COMMENT_REVISION, saved_state["target_revision"]) - self.assertEqual([34], saved_state["pending_pr_numbers"]) + self.assertEqual([51, 52, 53, 54, 55], saved_state["pending_pr_numbers"]) + self.assertEqual(0, saved_state["completed_revision"]) @patch.object(pr_status_comment, "save_status_comment_rollout_state") @patch.object( @@ -647,7 +667,6 @@ def test_failed_comment_write_retains_only_failed_pr_and_continues( ) -> None: errors = pr_status_comment.update_status_comments_from_state( "open-telemetry/example", - None, {12, 34}, ) diff --git a/.github/scripts/pull-request-dashboard/test_route_presentation.py b/.github/scripts/pull-request-dashboard/test_route_presentation.py index b9f2e54d6cc..4d17a796560 100644 --- a/.github/scripts/pull-request-dashboard/test_route_presentation.py +++ b/.github/scripts/pull-request-dashboard/test_route_presentation.py @@ -31,6 +31,12 @@ def test_author_status_does_not_mention_login(self) -> None: route_status_summary("author"), ) + def test_copilot_status_identifies_pending_review(self) -> None: + self.assertEqual( + ("Copilot", "Wait for the pending review to complete."), + route_status_summary("copilot"), + ) + def test_unrecognized_route_uses_unknown_presentation(self) -> None: self.assertEqual(route_label("unknown"), route_label("other")) self.assertEqual(route_status_summary("unknown"), route_status_summary("other")) diff --git a/.github/scripts/pull-request-dashboard/test_state.py b/.github/scripts/pull-request-dashboard/test_state.py index 3628ab69673..9e067fbc9d2 100644 --- a/.github/scripts/pull-request-dashboard/test_state.py +++ b/.github/scripts/pull-request-dashboard/test_state.py @@ -9,16 +9,22 @@ from unittest.mock import patch from state import ( + AUTHOR_NUDGE_STATE_VERSION, BACKFILL_STATE_VERSION, + COPILOT_REVIEW_REQUEST_STATE_VERSION, DASHBOARD_STATE_VERSION, NOTIFICATION_STATE_VERSION, STATUS_COMMENT_ROLLOUT_STATE_VERSION, + author_nudge_state_path, backfill_state_path, + copilot_review_request_state_path, dashboard_state_path, empty_state, enqueue_status_comment_update, load_accepted_dashboard_state, + load_author_nudges, load_backfill_state, + load_copilot_review_requests, load_dashboard_state_cache, load_state_file, load_status_comment_rollout_state, @@ -26,10 +32,14 @@ main, notification_state_path, save_state_file, + save_author_nudges, + save_copilot_review_requests, save_dashboard_state_cache, save_notifications, save_status_comment_rollout_state, stored_result, + union_merge_author_nudges, + union_merge_copilot_review_requests, update_dashboard_state_for_pr, ) @@ -153,6 +163,170 @@ def test_notification_state_version_is_independent(self) -> None: self.assertEqual(NOTIFICATION_STATE_VERSION, 3) self.assertEqual(DASHBOARD_STATE_VERSION, 5) self.assertEqual(STATUS_COMMENT_ROLLOUT_STATE_VERSION, 1) + self.assertEqual(AUTHOR_NUDGE_STATE_VERSION, 2) + self.assertEqual(COPILOT_REVIEW_REQUEST_STATE_VERSION, 3) + + def test_author_nudge_state_round_trip(self) -> None: + with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)): + save_author_nudges({ + "123": { + "waiting_since": "2026-07-10T00:00:00Z", + "nudged_at": "", + } + }) + + self.assertEqual( + load_author_nudges(), + { + "123": { + "waiting_since": "2026-07-10T00:00:00Z", + "nudged_at": "", + } + }, + ) + self.assertTrue(author_nudge_state_path().exists()) + + def test_copilot_review_request_state_round_trip(self) -> None: + with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)): + save_copilot_review_requests({ + "123": { + "head_sha": "current-head", + "observed_at": "2026-07-20T01:00:00Z", + "requested_at": "", + "routing_input_fingerprint": "accepted-fingerprint", + } + }) + + self.assertEqual( + load_copilot_review_requests(), + { + "123": { + "head_sha": "current-head", + "observed_at": "2026-07-20T01:00:00Z", + "requested_at": "", + "routing_input_fingerprint": "accepted-fingerprint", + } + }, + ) + self.assertTrue(copilot_review_request_state_path().exists()) + + def test_retry_snapshot_preserves_posted_author_nudge(self) -> None: + self.assertEqual( + { + "7": { + "waiting_since": "2026-07-10T02:00:00Z", + "nudged_at": "2026-07-20T02:00:00Z", + } + }, + union_merge_author_nudges( + {"7": {"waiting_since": "2026-07-10T02:00:00Z", "nudged_at": ""}}, + { + "7": { + "waiting_since": "2026-07-10T02:00:00Z", + "nudged_at": "2026-07-20T02:00:00Z", + } + }, + ), + ) + + def test_retry_snapshot_does_not_suppress_new_author_episode(self) -> None: + self.assertEqual( + {"7": {"waiting_since": "2026-07-20T02:00:00Z", "nudged_at": ""}}, + union_merge_author_nudges( + {"7": {"waiting_since": "2026-07-20T02:00:00Z", "nudged_at": ""}}, + { + "7": { + "waiting_since": "2026-07-10T02:00:00Z", + "nudged_at": "2026-07-17T02:00:00Z", + } + }, + ), + ) + + def test_retry_snapshot_preserves_same_head_copilot_request(self) -> None: + self.assertEqual( + { + "7": { + "head_sha": "current-head", + "observed_at": "2026-07-20T01:00:00Z", + "requested_at": "2026-07-20T02:00:00Z", + "routing_input_fingerprint": "accepted-fingerprint", + } + }, + union_merge_copilot_review_requests( + { + "7": { + "head_sha": "current-head", + "observed_at": "2026-07-20T01:00:00Z", + "requested_at": "", + "routing_input_fingerprint": "accepted-fingerprint", + } + }, + { + "7": { + "head_sha": "current-head", + "observed_at": "2026-07-20T01:00:00Z", + "requested_at": "2026-07-20T02:00:00Z", + "routing_input_fingerprint": "accepted-fingerprint", + } + }, + ), + ) + + def test_retry_snapshot_does_not_overwrite_new_copilot_head(self) -> None: + self.assertEqual( + { + "7": { + "head_sha": "new-head", + "observed_at": "2026-07-20T03:00:00Z", + "requested_at": "", + "routing_input_fingerprint": "new-fingerprint", + } + }, + union_merge_copilot_review_requests( + { + "7": { + "head_sha": "new-head", + "observed_at": "2026-07-20T03:00:00Z", + "requested_at": "", + "routing_input_fingerprint": "new-fingerprint", + } + }, + { + "7": { + "head_sha": "old-head", + "observed_at": "2026-07-20T01:00:00Z", + "requested_at": "2026-07-20T02:00:00Z", + "routing_input_fingerprint": "old-fingerprint", + } + }, + ), + ) + + def test_retry_snapshot_does_not_suppress_new_same_head_request(self) -> None: + pending = { + "7": { + "head_sha": "current-head", + "observed_at": "2026-07-20T03:00:00Z", + "requested_at": "", + "routing_input_fingerprint": "new-fingerprint", + } + } + + self.assertEqual( + pending, + union_merge_copilot_review_requests( + pending, + { + "7": { + "head_sha": "current-head", + "observed_at": "2026-07-20T01:00:00Z", + "requested_at": "2026-07-20T02:00:00Z", + "routing_input_fingerprint": "old-fingerprint", + } + }, + ), + ) def test_status_comment_rollout_state_round_trip(self) -> None: with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)): diff --git a/.github/scripts/pull-request-dashboard/test_top_level_actions.py b/.github/scripts/pull-request-dashboard/test_top_level_actions.py index c6ceea79900..abee639a1a1 100644 --- a/.github/scripts/pull-request-dashboard/test_top_level_actions.py +++ b/.github/scripts/pull-request-dashboard/test_top_level_actions.py @@ -183,6 +183,39 @@ def classify_feedback_domains( return review_classifications, top_level_classifications +class NormalizeEventsCommandTest(unittest.TestCase): + def _issue_comment_events(self, body: str) -> list[dict]: + events = normalize_events( + { + "commits": [], + "issue_comments": [ + { + "id": 1, + "user": {"login": "author"}, + "created_at": "2026-07-14T00:00:00Z", + "body": body, + } + ], + "review_comments": [], + "reviews": [], + }, + "author", + set(), + ) + return [e for e in events if e["kind"] == "issue-comment"] + + def test_command_only_comment_is_dropped(self) -> None: + self.assertEqual([], self._issue_comment_events("/dashboard route:reviewers")) + + def test_command_with_explanation_keeps_the_explanation(self) -> None: + events = self._issue_comment_events( + "/dashboard route:reviewers\n\nI addressed the feedback by doing X." + ) + + self.assertEqual(1, len(events)) + self.assertEqual("I addressed the feedback by doing X.", events[0]["body"]) + + class TopLevelActionLedgerTest(unittest.TestCase): def test_inline_prompt_treats_author_inability_as_completed_reply(self) -> None: discussion = review_thread_discussion("inline") @@ -957,12 +990,17 @@ def test_dashboard_refresh_reuses_stored_top_level_history(self, build_result) - } } }, + ["main"], ) self.assertEqual( build_result.call_args.kwargs["previous_top_level_history"], previous_state, ) + self.assertEqual( + build_result.call_args.kwargs["require_clean_copilot_review_branches"], + ["main"], + ) def test_top_level_decision_requires_matching_action_and_evidence(self) -> None: for action in ("reviewer", "approver"): diff --git a/.github/workflows/pull-request-dashboard-repo.yml b/.github/workflows/pull-request-dashboard-repo.yml index a193f03b5c8..687b24841eb 100644 --- a/.github/workflows/pull-request-dashboard-repo.yml +++ b/.github/workflows/pull-request-dashboard-repo.yml @@ -22,6 +22,10 @@ on: required: false default: "[]" type: string + require_clean_copilot_review_branches_json: + required: false + default: "[]" + type: string slack_channel: required: false default: "" @@ -123,6 +127,8 @@ jobs: REQUIRED_APPROVALS: ${{ inputs.required_approvals }} APPROVER_TEAMS_JSON: ${{ inputs.approver_teams_json }} NON_BLOCKING_CHECK_PATTERNS_JSON: ${{ inputs.non_blocking_check_patterns_json }} + REQUIRE_CLEAN_COPILOT_REVIEW_BRANCHES_JSON: ${{ inputs.require_clean_copilot_review_branches_json }} + PREPARE_AUTHOR_NUDGES: ${{ github.event_name == 'schedule' }} run: | set -euo pipefail state_branch="${DASHBOARD_STATE_BRANCH_PREFIX}/${REPO_NAME}" @@ -135,6 +141,13 @@ jobs: for pattern in "${non_blocking_check_patterns[@]}"; do dashboard_args+=(--non-blocking-check-pattern "$pattern") done + mapfile -t require_clean_copilot_review_branches < <(jq -r '.[]' <<< "$REQUIRE_CLEAN_COPILOT_REVIEW_BRANCHES_JSON") + for branch in "${require_clean_copilot_review_branches[@]}"; do + dashboard_args+=(--require-clean-copilot-review-branch "$branch") + done + if [[ "${PREPARE_AUTHOR_NUDGES:-false}" == "true" ]]; then + dashboard_args+=(--prepare-author-nudges) + fi if [[ -n "${TRIGGER_PR_NUMBER:-}" ]]; then dashboard_args+=(--pr-number "$TRIGGER_PR_NUMBER") fi @@ -163,14 +176,14 @@ jobs: GH_TOKEN: ${{ steps.dashboard-token.outputs.token }} run: python3 .github/scripts/pull-request-dashboard/report_rate_limits.py - update-pr-status-comment: + publish-dashboard: needs: update-dashboard if: >- always() && (needs.update-dashboard.result == 'success' || inputs.pr_number == '') && needs.update-dashboard.outputs.initial_backfill_complete == 'true' concurrency: - group: ${{ github.workflow }}-status-${{ inputs.repository }} + group: ${{ github.workflow }}-publish-${{ inputs.repository }} cancel-in-progress: false permissions: contents: write @@ -188,52 +201,14 @@ jobs: private-key: ${{ secrets.PR_DASHBOARD_PRIVATE_KEY }} owner: open-telemetry repositories: ${{ inputs.repository }} + permission-checks: read + permission-contents: read + permission-issues: write permission-pull-requests: write - - name: Update PR status comment - env: - GH_TOKEN: ${{ steps.dashboard-token.outputs.token }} - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - PR_NUMBER: ${{ inputs.pr_number }} - REPO_NAME: ${{ inputs.repository }} - run: | - set -euo pipefail - state_branch="${DASHBOARD_STATE_BRANCH_PREFIX}/${REPO_NAME}" - status_args=(--state-branch "$state_branch" --repo "$REPO_NAME") - if [[ -n "${PR_NUMBER:-}" ]]; then - status_args+=(--pr-number "$PR_NUMBER") - fi - python3 .github/scripts/pull-request-dashboard/pr_status_comment.py "${status_args[@]}" - - notify-slack: - needs: update-dashboard - if: >- - always() && - (needs.update-dashboard.result == 'success' || inputs.pr_number == '') && - needs.update-dashboard.outputs.initial_backfill_complete == 'true' && - (github.event_name == 'schedule' || inputs.pr_number != '') - concurrency: - group: ${{ github.workflow }}-notify-${{ inputs.repository }}-${{ inputs.pr_number || 'backfill' }} - cancel-in-progress: false - permissions: - contents: write - environment: protected - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - with: - persist-credentials: false - - - uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 - id: dashboard-token - with: - client-id: ${{ vars.PR_DASHBOARD_CLIENT_ID }} - private-key: ${{ secrets.PR_DASHBOARD_PRIVATE_KEY }} - owner: open-telemetry - repositories: ${{ inputs.repository }} - permission-pull-requests: read - - - name: Send Slack notifications + - name: Deliver dashboard updates + id: delivery + continue-on-error: true env: GH_TOKEN: ${{ steps.dashboard-token.outputs.token }} GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} @@ -241,46 +216,14 @@ jobs: SLACK_CHANNEL: ${{ inputs.slack_channel }} SLACK_USER_MAP_JSON: ${{ inputs.slack_user_mapping_json }} REPO_NAME: ${{ inputs.repository }} - TRIGGER_PR_NUMBER: ${{ inputs.pr_number }} run: | set -euo pipefail state_branch="${DASHBOARD_STATE_BRANCH_PREFIX}/${REPO_NAME}" - notify_args=(--state-branch "$state_branch" --repo "$REPO_NAME") - if [[ -n "${TRIGGER_PR_NUMBER:-}" ]]; then - notify_args+=(--pr-number "$TRIGGER_PR_NUMBER") - fi - - python3 .github/scripts/pull-request-dashboard/notify_slack.py "${notify_args[@]}" - - publish-dashboard: - needs: update-dashboard - if: >- - always() && - (needs.update-dashboard.result == 'success' || inputs.pr_number == '') && - needs.update-dashboard.outputs.initial_backfill_complete == 'true' - concurrency: - group: ${{ github.workflow }}-publish-${{ inputs.repository }} - cancel-in-progress: true - permissions: - contents: read - environment: protected - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - with: - persist-credentials: false - - - uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 - id: dashboard-token - with: - client-id: ${{ vars.PR_DASHBOARD_CLIENT_ID }} - private-key: ${{ secrets.PR_DASHBOARD_PRIVATE_KEY }} - owner: open-telemetry - repositories: ${{ inputs.repository }} - permission-issues: write - permission-pull-requests: read + delivery_args=(--state-branch "$state_branch" --repo "$REPO_NAME") + python3 .github/scripts/pull-request-dashboard/delivery.py "${delivery_args[@]}" - name: Publish dashboard issue + if: always() && steps.dashboard-token.outcome == 'success' env: GH_TOKEN: ${{ steps.dashboard-token.outputs.token }} REPO_NAME: ${{ inputs.repository }} @@ -298,3 +241,14 @@ jobs: publish_args+=(--large-repo) fi python3 .github/scripts/pull-request-dashboard/publish_dashboard.py "${publish_args[@]}" + + - name: Report GitHub App rate limits + if: always() && steps.dashboard-token.outcome == 'success' + continue-on-error: true + env: + GH_TOKEN: ${{ steps.dashboard-token.outputs.token }} + run: python3 .github/scripts/pull-request-dashboard/report_rate_limits.py + + - name: Check dashboard delivery + if: always() && steps.delivery.outcome == 'failure' + run: exit 1 diff --git a/.github/workflows/pull-request-dashboard.yml b/.github/workflows/pull-request-dashboard.yml index d84d501e5d0..c5dbef83e47 100644 --- a/.github/workflows/pull-request-dashboard.yml +++ b/.github/workflows/pull-request-dashboard.yml @@ -148,6 +148,7 @@ jobs: required_approvals: ${{ matrix.required_approvals || 1 }} approver_teams_json: ${{ toJSON(matrix.approver_teams || fromJSON('[]')) }} non_blocking_check_patterns_json: ${{ toJSON(matrix.non_blocking_check_patterns || fromJSON('[]')) }} + require_clean_copilot_review_branches_json: ${{ toJSON(matrix.require_clean_copilot_review_branches || fromJSON('[]')) }} slack_channel: ${{ matrix.slack_channel }} slack_user_mapping_json: ${{ toJSON(matrix.slack_user_mapping || fromJSON('{}')) }} large_repo: ${{ matrix.large_repo || false }} @@ -176,8 +177,7 @@ jobs: issues: write # needed to open/close the hourly failure tracking issue uses: ./.github/workflows/workflow-failure-issue.yml with: - # A superseded per-repo publish is cancelled (publish-dashboard uses - # cancel-in-progress: true), so count cancelled as success here to avoid - # opening noisy failure issues. Genuine failures still surface because - # matrix aggregation reports 'failure' in preference to 'cancelled'. + # GitHub replaces an older pending per-repo publisher even with + # cancel-in-progress disabled. Its replacement drains all durable work, + # while genuine matrix failures take precedence over cancellation. success: ${{ needs.resolve-targets.result == 'success' && (needs.run-repo-dashboard.result == 'success' || needs.run-repo-dashboard.result == 'cancelled') }} diff --git a/pull-request-dashboard/README.md b/pull-request-dashboard/README.md index 8b78439c1a2..5b76a9a666b 100644 --- a/pull-request-dashboard/README.md +++ b/pull-request-dashboard/README.md @@ -52,6 +52,7 @@ Open a pull request that adds your repository to [`.github/scripts/pull-request- "markdown-link-check / link-check", "codecov/*" ], + "require_clean_copilot_review_branches": ["main"], "labels_to_display": ["size/*", "breaking change"], "slack_channel": "#example-maintainers", "slack_user_mapping": { @@ -70,12 +71,20 @@ Fields: | `required_approvals` | no | Number of approvals required for an open PR to be marked ready to merge. 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 with no inline findings on the current head is required before routing a PR to reviewers or maintainers. The dashboard re-requests Copilot review when needed and does not duplicate a pending request. List only branches where automatic Copilot code review is enabled (typically `["main"]`); PRs targeting any other branch are never gated, so they cannot stall waiting for a review that never runs. 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, the *Draft pull requests* table, and the *Diagnostics* block) 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. | `labels_to_display` only controls which labels are shown. It does not filter pull requests or affect dashboard routing, notifications, or status comments. All matching labels are displayed in the order returned by GitHub; a label matching more than one configured pattern is shown once. +For each listed branch, `require_clean_copilot_review_branches` relies on automatic Copilot +code review for the initial review, so list only branches where automatic Copilot +code review is enabled. The dashboard requests later reviews using its GitHub App +installation token with pull-request write permission. Leave **Review new +pushes** disabled if the dashboard should request re-reviews only when a PR is +ready to return to reviewers or maintainers. + Ask a maintainer or admin to add the repository under [Repository access](https://github.com/organizations/open-telemetry/settings/installations/133550497). Once the PR is merged, the dashboard will pick up your repository on its next hourly backfill run. To run it sooner, see [Manual backfill run](#manual-backfill-run). The dashboard issue is discovered dynamically in your repository by the `dashboard` label and `Pull Request Dashboard` title; if it does not exist, the publish step creates the label and issue. @@ -184,10 +193,53 @@ for the tradeoffs behind this behavior. Targeted updates received before the first full dashboard run are ignored. +## Reviewer routing override + +When the dashboard says a pull request is waiting on its author or an external +dependency but the author believes it is ready for another review, the author +can comment `/dashboard route:reviewers`. The dashboard routes the pull request +to *Waiting on reviewers* and applies the `dashboard:route-overridden` label to +mark the override. Members of the repository's `approver_teams` can use the same +command. A `/dashboard route:reviewers` command from anyone else, or a command +on a pull request already at or past reviewers, has no routing effect. The +dashboard replies to a `/dashboard route:reviewers` from an unauthorized user +explaining that only the author or an approver can use it, replies to an author +or approver command on a pull request already at or past reviewers noting where +it is currently routed, and replies to any unrecognized `/dashboard` command. + +Removing the `dashboard:route-overridden` label restores automatic routing. The +dashboard also removes the label automatically once routing would place the pull +request with reviewers on its own, so a forgotten override does not linger. A +command that has already been applied is not replayed after label removal; the +author can post a new `/dashboard route:reviewers` command if another override +is needed later. + +## Author reminder + +The dashboard posts one reminder when a pull request remains in *Waiting on +authors* for one week. The reminder @-mentions the author, links to the +dashboard-managed status comment containing the remaining items, and notes that +addressing them (or replying with an update) automatically routes the pull +request back to reviewers. +Both the reminder and the live status comment advertise `/dashboard route:reviewers` +as an explicit handoff when the author believes the pull request is ready for +review. + +Leaving *Waiting on authors* resets the one-week clock. If the pull request +later returns to *Waiting on authors* and remains there for another week, the +dashboard posts another reminder. Reminders are delivered by hourly runs when +the pull request is next refreshed, so a due reminder in a large repository +may wait for a later round-robin run. + ## Configuration -The dashboard uses repository-scoped GitHub App access to read and update each -configured repository and to read approver team membership. +The dashboard separates calculation from delivery. The update job uses +repository-scoped, read-only GitHub App access to calculate routing and persist +pending work. A serialized publishing job holds pull request and issue write +access and durably delivers status comments, author reminders, Copilot +re-review requests, Slack notifications, and the dashboard issue. Successful +deliveries are recorded on the state branch before another publishing run can +start. Each repository can route Slack notifications to its own `slack_channel` and map GitHub logins to Slack user IDs via `slack_user_mapping`. Repositories