diff --git a/.github/scripts/pull-request-dashboard/author_nudge.py b/.github/scripts/pull-request-dashboard/author_nudge.py index ccc6990e2fa..dbb980343bb 100644 --- a/.github/scripts/pull-request-dashboard/author_nudge.py +++ b/.github/scripts/pull-request-dashboard/author_nudge.py @@ -3,16 +3,18 @@ from __future__ import annotations -from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone import hashlib import json from pathlib import Path +import re import sys from typing import Any from github_cli import ( fetch_pr_routing_raw, gh_api, + gh_graphql, run_gh, ) from dashboard_override import author_override_guidance @@ -30,13 +32,80 @@ NUDGE_AFTER = timedelta(weeks=1) +LEGACY_NUDGE_RECOVERY_WINDOW = timedelta(days=1) NUDGE_MARKER_PREFIX = "" +) +LEGACY_EPISODE_PREFIX = "legacy-nudge:" +COMMENT_MINIMIZATION_STATE_QUERY = """ +query($id: ID!) { + node(id: $id) { + ... on IssueComment { + isMinimized + minimizedReason + } + } +} +""" +MINIMIZE_COMMENT_MUTATION = """ +mutation($id: ID!) { + minimizeComment(input: {subjectId: $id, classifier: OUTDATED}) { + minimizedComment { + isMinimized + } + } +} +""" +UNMINIMIZE_COMMENT_MUTATION = """ +mutation($id: ID!) { + unminimizeComment(input: {subjectId: $id}) { + unminimizedComment { + isMinimized + } + } +} +""" def nudge_marker(episode_id: str) -> str: return f"{NUDGE_MARKER_PREFIX}{episode_id} -->" +def completed_nudge_marker(episode_id: str) -> str: + return f"{COMPLETED_NUDGE_MARKER_PREFIX}{episode_id} -->" + + +def legacy_episode_id(nudged_at: str) -> str: + return f"{LEGACY_EPISODE_PREFIX}{nudged_at}" + + +def display_time(value: datetime) -> str: + return value.astimezone(timezone.utc).strftime("%Y-%m-%d %H:%M UTC") + + +def completion_only(entry: dict[str, Any]) -> dict[str, Any] | None: + completions = list(entry.get("completions") or []) + return {"completions": completions} if completions else None + + +def queue_completion( + completions: list[dict[str, Any]], + episode_id: str, + completed_at: datetime, + kind: str, +) -> None: + if not any(item.get("episode_id") == episode_id for item in completions): + completions.append({ + "episode_id": episode_id, + "completed_at": format_ts(completed_at), + "kind": kind, + }) + + def routing_inputs(raw: dict[str, Any]) -> dict[str, Any]: dashboard_login = f"{DASHBOARD_APP_SLUG}[bot]" pr = raw.get("pr") or {} @@ -119,17 +188,76 @@ def plan_nudge( or result.get("route") in ("transient-failure", "unknown") ): return False, entry or None + if result is None: + if nudged_at: + completions = list(entry.get("completions") or []) + episode_id = ( + entry.get("episode_id") + or legacy_episode_id(nudged_at) + ) + queue_completion( + completions, + episode_id, + now, + "routing_changed", + ) + return False, {"completions": completions} + return False, completion_only(entry) + facts = (result or {}).get("facts") or {} + current_episode_id = str(facts.get("author_nudge_episode_id") or "") if not waiting_on_author(result): - return False, None + route = result.get("route") or "" + if route in ("approver", "maintainer"): + completion_kind = "left_author" + elif route == "author" and facts.get("route_held_for_gates"): + completion_kind = "routing_changed" + else: + return False, completion_only(entry) + if nudged_at: + episode_id = ( + entry.get("episode_id") + or legacy_episode_id(nudged_at) + ) + completions = list(entry.get("completions") or []) + queue_completion(completions, episode_id, now, completion_kind) + return False, {"completions": completions} + return False, completion_only(entry) + completions = list(entry.get("completions") or []) + previous_episode_id = entry.get("episode_id") or "" + if current_episode_id and previous_episode_id and ( + current_episode_id != previous_episode_id + ): + if nudged_at: + queue_completion( + completions, + previous_episode_id, + now, + "routing_changed", + ) + entry = { + "waiting_since": format_ts(now), + "nudged_at": "", + "episode_id": current_episode_id, + } + if completions: + entry["completions"] = completions + return False, entry + if current_episode_id: + entry["episode_id"] = current_episode_id if nudged_at: return False, entry waiting_since = parse_ts(entry.get("waiting_since") or "") if waiting_since is None: - return False, { + entry = { "waiting_since": format_ts(now), "nudged_at": "", } + if current_episode_id: + entry["episode_id"] = current_episode_id + if completions: + entry["completions"] = completions + return False, entry return now - waiting_since >= NUDGE_AFTER, entry @@ -155,24 +283,130 @@ def existing_nudge_comment( ) -def render_nudge(author: str, status_url: str, episode_id: str) -> str: +def recover_legacy_nudge_episode_id( + repo: str, + pr_number: int, + legacy_id: str, +) -> str: + if not legacy_id.startswith(LEGACY_EPISODE_PREFIX): + return "" + expected_created_at = parse_ts(legacy_id.removeprefix(LEGACY_EPISODE_PREFIX)) + if expected_created_at is None: + return "" + comments = gh_api( + f"/repos/{repo}/issues/{pr_number}/comments?per_page=100", + paginate=True, + ) + best_match: tuple[timedelta, str] | None = None + for comment in comments or []: + if (comment.get("performed_via_github_app") or {}).get("slug") != ( + DASHBOARD_APP_SLUG + ): + continue + created_at = parse_ts(comment.get("created_at") or "") + match = NUDGE_MARKER_RE.search(comment.get("body") or "") + if created_at is None or match is None: + continue + distance = abs(created_at - expected_created_at) + if distance > LEGACY_NUDGE_RECOVERY_WINDOW: + continue + candidate = (distance, match.group(1)) + if best_match is None or candidate[0] < best_match[0]: + best_match = candidate + return best_match[1] if best_match else "" + + +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 is " "waiting on you.", "", 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.", + f"[dashboard status comment]({status_url}) for the full list and current " + "routing; that comment is kept current. " + "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( + "Use this command only while the live dashboard status still says the " + "pull request is waiting on the author." + ), "", - author_override_guidance(), + "_This reminder is a snapshot; the linked dashboard status is the current " + "source of truth._", "", ]) +def render_completed_nudge( + original_body: str, + status_url: str, + episode_id: str, + completed_at: datetime, + kind: str = "left_author", +) -> str: + if kind == "left_author": + note = ( + f"_Outdated as of {display_time(completed_at)}: this pull request is " + "no longer waiting on you. See the " + f"[dashboard status comment]({status_url}) for its current routing._" + ) + else: + note = ( + f"_Outdated as of {display_time(completed_at)}: this reminder no " + "longer reflects the current dashboard state. Check the " + f"[dashboard status comment]({status_url}) to see whether action is " + "needed._" + ) + return "\n\n".join([ + original_body.rstrip(), + completed_nudge_marker(episode_id), + note, + ]) + "\n" + + +def comment_minimization_reason(node_id: str) -> str: + data = gh_graphql(COMMENT_MINIMIZATION_STATE_QUERY, {"id": node_id}) + node = (data.get("data") or {}).get("node") + if not isinstance(node, dict) or "isMinimized" not in node: + raise RuntimeError("author nudge minimization state not found") + if not node["isMinimized"]: + return "" + reason = node.get("minimizedReason") + if not isinstance(reason, str) or not reason: + raise RuntimeError("author nudge minimization reason not found") + return reason.upper().replace("-", "_") + + +def unminimize_comment(node_id: str) -> None: + data = gh_graphql(UNMINIMIZE_COMMENT_MUTATION, {"id": node_id}) + unminimized = ( + ((data.get("data") or {}).get("unminimizeComment") or {}) + .get("unminimizedComment") + or {} + ) + if unminimized.get("isMinimized") is not False: + raise RuntimeError("author nudge was not unminimized") + + +def minimize_comment(node_id: str) -> None: + data = gh_graphql(MINIMIZE_COMMENT_MUTATION, {"id": node_id}) + minimized = ( + ((data.get("data") or {}).get("minimizeComment") or {}) + .get("minimizedComment") + or {} + ) + if not minimized.get("isMinimized"): + raise RuntimeError("author nudge was not marked outdated") + + def ensure_nudge( repo: str, pr_number: int, @@ -205,11 +439,59 @@ def ensure_nudge( 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)}", + "-f", + f"body={render_nudge(author, status_comments[0]['html_url'], episode_id)}", ]) return format_ts(now) +def ensure_nudge_completed( + repo: str, + pr_number: int, + episode_id: str, + dashboard_state: dict[str, Any], + completed_at: datetime, + kind: str = "left_author", +) -> None: + comment = existing_nudge_comment(repo, pr_number, episode_id) + if comment is None: + return + original_body = comment.get("body") or "" + comment_id = comment.get("id") + node_id = comment.get("node_id") or "" + if not comment_id: + raise RuntimeError(f"author nudge comment id not found for PR #{pr_number}") + if not node_id: + raise RuntimeError(f"author nudge comment node id not found for PR #{pr_number}") + + if completed_nudge_marker(episode_id) not in original_body: + status_comments = managed_status_comments(repo, pr_number) + if not status_comments: + 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}") + body = render_completed_nudge( + original_body, + status_comments[0]["html_url"], + episode_id, + completed_at, + kind, + ) + run_gh([ + "gh", "api", "--method", "PATCH", + f"repos/{repo}/issues/comments/{comment_id}", + "-f", + f"body={body}", + ]) + + minimized_reason = comment_minimization_reason(node_id) + if minimized_reason != "OUTDATED": + if minimized_reason: + unminimize_comment(node_id) + minimize_comment(node_id) + + def record_author_nudge_observation( pr_number: int, result: dict[str, Any] | None, @@ -225,12 +507,15 @@ def record_author_nudge_observation( head_sha = facts.get("head_sha") or "" routing_fingerprint = facts.get("routing_input_fingerprint") or "" if head_sha and routing_fingerprint: + episode_id = str(facts.get("author_nudge_episode_id") or "") entry = { **entry, "pending_at": format_ts(now), "head_sha": head_sha, "routing_input_fingerprint": routing_fingerprint, } + if episode_id: + entry["episode_id"] = episode_id if entry is None: updated.pop(key, None) else: @@ -251,10 +536,67 @@ def deliver_prepared_author_nudges( 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) + completions = list((entry or {}).get("completions") or []) + remaining_completions: list[dict[str, Any]] = [] + for completion in completions: + episode_id = completion.get("episode_id") or "" + if episode_id.startswith(LEGACY_EPISODE_PREFIX): + recovered_episode_id = recover_legacy_nudge_episode_id( + repo, + pr_number, + episode_id, + ) + if recovered_episode_id: + completion = { + **completion, + "episode_id": recovered_episode_id, + } + episode_id = recovered_episode_id + else: + print( + f"PR #{pr_number}: legacy author nudge comment not found; " + "discarding completion", + file=sys.stderr, + ) + continue + completed_at = parse_ts(completion.get("completed_at") or "") + kind = completion.get("kind") or "left_author" + if ( + not episode_id + or completed_at is None + or kind not in ("left_author", "routing_changed") + ): + errors.append(f"PR #{pr_number}: invalid pending nudge completion") + remaining_completions.append(completion) + continue + try: + ensure_nudge_completed( + repo, + pr_number, + episode_id, + dashboard_state, + completed_at, + kind, + ) + except Exception as e: + errors.append(f"PR #{pr_number}: {e}") + remaining_completions.append(completion) + entry = dict(entry or {}) + if remaining_completions: + entry["completions"] = remaining_completions + else: + entry.pop("completions", None) + if not entry.get("waiting_since") and not entry.get("pending_at"): + if entry: + updated[key] = entry + else: + updated.pop(key, None) + continue + if not (entry or {}).get("pending_at"): + updated[key] = entry + continue if not waiting_on_author(result): _due, reset_entry = plan_nudge(result, entry, now) if reset_entry is None: @@ -271,7 +613,11 @@ def deliver_prepared_author_nudges( expected_routing_fingerprint = entry.get("routing_input_fingerprint") or "" current_head = pr.get("headRefOid") or "" if pr.get("state") != "OPEN" or pr.get("isDraft"): - updated.pop(key, None) + completion_entry = completion_only(entry) + if completion_entry is None: + updated.pop(key, None) + else: + updated[key] = completion_entry continue if ( not expected_head @@ -301,9 +647,19 @@ def deliver_prepared_author_nudges( errors.append(f"PR #{pr_number}: {e}") continue if nudged_at: + episode_id = str( + ((result or {}).get("facts") or {}).get("author_nudge_episode_id") + or entry.get("episode_id") + or "" + ) updated[key] = { "waiting_since": entry.get("waiting_since") or "", "nudged_at": nudged_at, } + if episode_id: + updated[key]["episode_id"] = episode_id + completions = list(entry.get("completions") or []) + if completions: + updated[key]["completions"] = completions save_author_nudges(updated) return errors diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index 5bf0df43c95..5c8a1d67b49 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -29,7 +29,7 @@ # notification-state.json: pending and delivered Slack notification records. NOTIFICATION_STATE_VERSION = 3 # author-nudge-state.json: waiting episodes and delivered author reminders. -AUTHOR_NUDGE_STATE_VERSION = 2 +AUTHOR_NUDGE_STATE_VERSION = 3 # copilot-review-request-state.json: pending and delivered review requests. COPILOT_REVIEW_REQUEST_STATE_VERSION = 4 # status-comment-rollout-state.json: target/completed renderer revisions and queue. @@ -120,6 +120,8 @@ def current_delivery_versions() -> dict[str, int]: def load_state_file( path: Path, current_version: int, + *, + compatible_versions: tuple[int, ...] = (), ) -> dict[str, Any] | None: if not path.exists(): return None @@ -133,7 +135,7 @@ def load_state_file( return None if not isinstance(data, dict): return None - if data.get("version") != current_version: + if data.get("version") not in (current_version, *compatible_versions): print( f"state version changed; regenerating {path}", file=sys.stderr, @@ -321,7 +323,11 @@ def save_notifications(notifications: dict[str, Any]) -> None: def load_author_nudge_state_file(path: Path) -> dict[str, Any]: - state = load_state_file(path, AUTHOR_NUDGE_STATE_VERSION) + state = load_state_file( + path, + AUTHOR_NUDGE_STATE_VERSION, + compatible_versions=(2,), + ) if state is None or not isinstance(state.get("prs"), dict): return {} return state["prs"] @@ -335,12 +341,40 @@ def union_merge_author_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 "" + baseline_entry = dict(baseline_nudges.get(key) or {}) + baseline_waiting_since = baseline_entry.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, } + completions = list(baseline_entry.get("completions") or []) + if completions: + merged[key]["completions"] = completions + episode_id = ( + (retry_entry or {}).get("episode_id") + or baseline_entry.get("episode_id") + or "" + ) + if episode_id: + merged[key]["episode_id"] = episode_id + elif nudged_at: + episode_id = ( + (retry_entry or {}).get("episode_id") + or f"legacy-nudge:{nudged_at}" + ) + completions = list(baseline_entry.get("completions") or []) + if not any( + completion.get("episode_id") == episode_id + for completion in completions + ): + completions.append({ + "episode_id": episode_id, + "completed_at": nudged_at, + "kind": "routing_changed", + }) + baseline_entry["completions"] = completions + merged[key] = baseline_entry return merged diff --git a/.github/scripts/pull-request-dashboard/test_author_nudge.py b/.github/scripts/pull-request-dashboard/test_author_nudge.py index 7fcaa616ffb..254ce398b3b 100644 --- a/.github/scripts/pull-request-dashboard/test_author_nudge.py +++ b/.github/scripts/pull-request-dashboard/test_author_nudge.py @@ -160,6 +160,12 @@ def test_nudge_advertises_dashboard_override_command(self) -> None: "episode-1", ) + self.assertIn("just a friendly reminder", body) + self.assertIn( + "_This reminder is a snapshot; the linked dashboard status is the " + "current source of truth._", + body, + ) self.assertIn( "comment `/dashboard route:reviewers` to route it from waiting on the " "author to waiting on reviewers", @@ -172,7 +178,11 @@ def test_first_author_route_observation_starts_clock(self) -> None: self.assertFalse(due) self.assertEqual( entry, - {"waiting_since": "2026-07-17T00:00:00+00:00", "nudged_at": ""}, + { + "waiting_since": "2026-07-17T00:00:00+00:00", + "nudged_at": "", + "episode_id": "episode-1", + }, ) def test_nudge_is_due_after_one_week(self) -> None: @@ -203,6 +213,102 @@ def test_leaving_author_route_resets_unnudged_clock(self) -> None: self.assertFalse(due) self.assertIsNone(entry) + def test_leaving_author_route_prepares_posted_nudge_for_completion(self) -> None: + due, entry = author_nudge.plan_nudge( + author_result("approver"), + { + "waiting_since": "2026-07-10T00:00:00+00:00", + "nudged_at": "2026-07-17T00:00:00+00:00", + "episode_id": "episode-1", + }, + NOW, + ) + + self.assertFalse(due) + self.assertEqual( + entry, + { + "completions": [{ + "episode_id": "episode-1", + "completed_at": "2026-07-17T00:00:00+00:00", + "kind": "left_author", + }], + }, + ) + + def test_legacy_posted_nudge_queues_marker_recovery(self) -> None: + due, entry = author_nudge.plan_nudge( + author_result("approver"), + { + "waiting_since": "2026-07-10T00:00:00+00:00", + "nudged_at": "2026-07-17T00:00:00+00:00", + }, + NOW, + ) + + self.assertFalse(due) + self.assertEqual( + { + "completions": [{ + "episode_id": ( + "legacy-nudge:2026-07-17T00:00:00+00:00" + ), + "completed_at": "2026-07-17T00:00:00+00:00", + "kind": "left_author", + }], + }, + entry, + ) + + def test_removed_pr_does_not_claim_author_wait_ended(self) -> None: + previous = { + "waiting_since": "2026-07-10T00:00:00+00:00", + "nudged_at": "2026-07-17T00:00:00+00:00", + "episode_id": "episode-1", + } + + self.assertEqual( + ( + False, + { + "completions": [{ + "episode_id": "episode-1", + "completed_at": "2026-07-17T00:00:00+00:00", + "kind": "routing_changed", + }], + }, + ), + author_nudge.plan_nudge(None, previous, NOW), + ) + + def test_gate_hold_closes_posted_reminder_without_claiming_handoff(self) -> None: + previous = { + "waiting_since": "2026-07-10T00:00:00+00:00", + "nudged_at": "2026-07-17T00:00:00+00:00", + "episode_id": "episode-1", + } + held = author_result() + held["facts"]["route_held_for_gates"] = True + + due, entry = author_nudge.plan_nudge(held, previous, NOW) + + self.assertFalse(due) + self.assertEqual( + { + "completions": [{ + "episode_id": "episode-1", + "completed_at": "2026-07-17T00:00:00+00:00", + "kind": "routing_changed", + }], + }, + entry, + ) + + self.assertEqual( + (False, entry), + author_nudge.plan_nudge(author_result("approver"), entry, NOW), + ) + def test_gate_held_route_resets_clock_and_does_not_nudge(self) -> None: held = author_result() held["facts"]["route_held_for_gates"] = True @@ -220,17 +326,57 @@ 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", + "episode_id": "previous-episode", } due, entry = author_nudge.plan_nudge(author_result("approver"), previous, NOW) self.assertFalse(due) - self.assertIsNone(entry) + completion = { + "episode_id": "previous-episode", + "completed_at": "2026-07-17T00:00:00+00:00", + "kind": "left_author", + } + self.assertEqual({"completions": [completion]}, 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": ""}, + { + "waiting_since": "2026-07-17T00:00:00+00:00", + "nudged_at": "", + "episode_id": "episode-1", + "completions": [completion], + }, + ) + + def test_new_episode_id_closes_posted_reminder_before_resetting_clock(self) -> None: + next_episode = author_result() + next_episode["facts"]["author_nudge_episode_id"] = "episode-2" + + due, entry = author_nudge.plan_nudge( + next_episode, + { + "waiting_since": "2026-07-01T00:00:00+00:00", + "nudged_at": "2026-07-10T00:00:00+00:00", + "episode_id": "episode-1", + }, + NOW, + ) + + self.assertFalse(due) + self.assertEqual( + { + "waiting_since": "2026-07-17T00:00:00+00:00", + "nudged_at": "", + "episode_id": "episode-2", + "completions": [{ + "episode_id": "episode-1", + "completed_at": "2026-07-17T00:00:00+00:00", + "kind": "routing_changed", + }], + }, + entry, ) def test_failed_refresh_preserves_clock(self) -> None: @@ -258,7 +404,13 @@ def test_observation_starts_clock( self.assertEqual( save_nudges.call_args.args[0], - {"2": {"waiting_since": "2026-07-17T00:00:00+00:00", "nudged_at": ""}}, + { + "2": { + "waiting_since": "2026-07-17T00:00:00+00:00", + "nudged_at": "", + "episode_id": "episode-1", + } + }, ) @patch.object(author_nudge, "save_author_nudges") @@ -308,6 +460,7 @@ def test_due_accepted_observation_records_pending_nudge( "pending_at": "2026-07-17T00:00:00+00:00", "head_sha": "current-head", "routing_input_fingerprint": "current-fingerprint", + "episode_id": "episode-1", } }, ) @@ -365,9 +518,272 @@ def test_delivery_records_posted_nudge( "1": { "waiting_since": "2026-07-01T00:00:00+00:00", "nudged_at": "2026-07-17T00:00:00+00:00", + "episode_id": "episode-1", }, }) + @patch.object(author_nudge, "ensure_nudge_completed") + @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": "2026-07-10T00:00:00+00:00", + "episode_id": "episode-1", + "completions": [{ + "episode_id": "previous-episode", + "completed_at": "2026-07-17T00:00:00+00:00", + }], + } + }, + ) + @patch.object( + author_nudge, + "load_dashboard_state_cache", + return_value={"prs": {"1": author_result("approver")}}, + ) + def test_delivery_completes_posted_nudge( + self, + dashboard_state, + _load_nudges, + save_nudges, + ensure_completed, + ) -> None: + errors = author_nudge.deliver_prepared_author_nudges( + "open-telemetry/example", + NOW, + ) + + self.assertEqual([], errors) + ensure_completed.assert_called_once_with( + "open-telemetry/example", + 1, + "previous-episode", + dashboard_state.return_value, + NOW, + "left_author", + ) + save_nudges.assert_called_once_with({ + "1": { + "waiting_since": "2026-07-01T00:00:00+00:00", + "nudged_at": "2026-07-10T00:00:00+00:00", + "episode_id": "episode-1", + } + }) + + @patch.object( + author_nudge, + "recover_legacy_nudge_episode_id", + return_value="recovered-episode", + ) + @patch.object(author_nudge, "ensure_nudge_completed") + @patch.object(author_nudge, "save_author_nudges") + @patch.object( + author_nudge, + "load_author_nudges", + return_value={ + "1": { + "completions": [{ + "episode_id": ( + "legacy-nudge:2026-07-10T00:00:00+00:00" + ), + "completed_at": "2026-07-17T00:00:00+00:00", + "kind": "left_author", + }], + }, + }, + ) + @patch.object( + author_nudge, + "load_dashboard_state_cache", + return_value={"prs": {"1": author_result("approver")}}, + ) + def test_delivery_recovers_legacy_posted_nudge_episode( + self, + dashboard_state, + _load_nudges, + _save_nudges, + ensure_completed, + recover_episode, + ) -> None: + errors = author_nudge.deliver_prepared_author_nudges( + "open-telemetry/example", + NOW, + ) + + self.assertEqual([], errors) + recover_episode.assert_called_once_with( + "open-telemetry/example", + 1, + "legacy-nudge:2026-07-10T00:00:00+00:00", + ) + ensure_completed.assert_called_once_with( + "open-telemetry/example", + 1, + "recovered-episode", + dashboard_state.return_value, + NOW, + "left_author", + ) + + @patch.object( + author_nudge, + "recover_legacy_nudge_episode_id", + return_value="", + ) + @patch.object(author_nudge, "ensure_nudge_completed") + @patch.object(author_nudge, "save_author_nudges") + @patch.object( + author_nudge, + "load_author_nudges", + return_value={ + "1": { + "completions": [{ + "episode_id": ( + "legacy-nudge:2026-07-10T00:00:00.403635+00:00" + ), + "completed_at": "2026-07-17T00:00:00+00:00", + "kind": "left_author", + }], + }, + }, + ) + @patch.object( + author_nudge, + "load_dashboard_state_cache", + return_value={"prs": {"1": author_result("approver")}}, + ) + def test_missing_legacy_comment_discards_completion( + self, + _dashboard_state, + _load_nudges, + save_nudges, + ensure_completed, + _recover_episode, + ) -> None: + with patch("sys.stderr") as stderr: + errors = author_nudge.deliver_prepared_author_nudges( + "open-telemetry/example", + NOW, + ) + + self.assertEqual([], errors) + stderr.write.assert_any_call( + "PR #1: legacy author nudge comment not found; discarding completion" + ) + ensure_completed.assert_not_called() + save_nudges.assert_called_once_with({}) + + def test_failed_completion_survives_closed_pr_delivery(self) -> None: + completion = { + "episode_id": "previous-episode", + "completed_at": "2026-07-17T00:00:00+00:00", + } + pending = { + "1": { + "waiting_since": "2026-07-10T00:00:00+00:00", + "nudged_at": "", + "episode_id": "episode-1", + "pending_at": "2026-07-17T00:00:00+00:00", + "head_sha": "current-head", + "routing_input_fingerprint": "current-fingerprint", + "completions": [completion], + } + } + with ( + 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, + "ensure_nudge_completed", + side_effect=RuntimeError("retry"), + ), + patch.object( + author_nudge, + "fetch_current_pr_routing_state", + return_value=({ + "state": "CLOSED", + "isDraft": False, + "headRefOid": "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(["PR #1: retry"], errors) + ensure_nudge.assert_not_called() + save_nudges.assert_called_once_with({"1": {"completions": [completion]}}) + + def test_failed_completion_survives_successful_new_nudge(self) -> None: + completion = { + "episode_id": "previous-episode", + "completed_at": "2026-07-17T00:00:00+00:00", + } + pending = { + "1": { + "waiting_since": "2026-07-10T00:00:00+00:00", + "nudged_at": "", + "episode_id": "episode-1", + "pending_at": "2026-07-17T00:00:00+00:00", + "head_sha": "current-head", + "routing_input_fingerprint": "current-fingerprint", + "completions": [completion], + } + } + with ( + 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, + "ensure_nudge_completed", + side_effect=RuntimeError("retry"), + ), + patch.object( + author_nudge, + "fetch_current_pr_routing_state", + return_value=({ + "state": "OPEN", + "isDraft": False, + "headRefOid": "current-head", + }, "current-fingerprint"), + ), + patch.object( + author_nudge, + "ensure_nudge", + return_value="2026-07-17T00:00:00+00:00", + ), + ): + errors = author_nudge.deliver_prepared_author_nudges( + "open-telemetry/example", + NOW, + ) + + self.assertEqual(["PR #1: retry"], errors) + save_nudges.assert_called_once_with({ + "1": { + "waiting_since": "2026-07-10T00:00:00+00:00", + "nudged_at": "2026-07-17T00:00:00+00:00", + "episode_id": "episode-1", + "completions": [completion], + } + }) + @patch.object(author_nudge, "ensure_nudge") @patch.object(author_nudge, "save_author_nudges") @patch.object( @@ -518,12 +934,388 @@ def test_rendered_nudge_mentions_author_and_links_status(self) -> None: ) self.assertIn("@alice", body) + self.assertIn( + "just a friendly reminder that this pull request is waiting on you", + body, + ) + self.assertNotIn("had been waiting on you for a week", body) self.assertIn("[dashboard status comment](https://example.test/status)", body) self.assertIn( author_nudge.nudge_marker("episode-1"), body, ) + def test_rendered_completed_nudge_appends_handoff_note(self) -> None: + original = "\n".join([ + author_nudge.nudge_marker("episode-1"), + "Original friendly reminder.", + "/dashboard route:reviewers", + ]) + body = author_nudge.render_completed_nudge( + original, + "https://example.test/status", + "episode-1", + NOW, + ) + + self.assertTrue(body.startswith(original)) + self.assertIn( + "_Outdated as of 2026-07-17 00:00 UTC: this pull request is no " + "longer waiting on you.", + body, + ) + self.assertIn("[dashboard status comment](https://example.test/status)", body) + self.assertIn("/dashboard route:reviewers", body) + self.assertIn(author_nudge.completed_nudge_marker("episode-1"), body) + self.assertEqual( + 1, + body.count(author_nudge.completed_nudge_marker("episode-1")), + ) + + def test_rendered_gate_completion_does_not_claim_author_handoff(self) -> None: + body = author_nudge.render_completed_nudge( + "Original friendly reminder.", + "https://example.test/status", + "episode-1", + NOW, + "routing_changed", + ) + + self.assertIn("this reminder no longer reflects the current dashboard state", body) + self.assertIn("to see whether action is needed", body) + self.assertNotIn("no longer waiting on you", body) + + @patch.object(author_nudge, "minimize_comment") + @patch.object(author_nudge, "comment_minimization_reason", return_value="") + @patch.object(author_nudge, "run_gh") + @patch.object(author_nudge, "publish_pr_status") + @patch.object( + author_nudge, + "managed_status_comments", + return_value=[{"html_url": "https://example.test/status"}], + ) + @patch.object( + author_nudge, + "existing_nudge_comment", + return_value={ + "id": 17, + "node_id": "IC_17", + "created_at": "2026-07-10T00:00:00Z", + "body": "\n".join([ + author_nudge.nudge_marker("episode-1"), + "Original friendly reminder.", + ]), + }, + ) + def test_completion_appends_note_then_marks_comment_outdated( + self, + _existing_nudge, + _status_comments, + publish_status, + run_gh, + minimization_reason, + minimize_comment, + ) -> None: + dashboard_state = {"prs": {"1": author_result("approver")}} + + author_nudge.ensure_nudge_completed( + "open-telemetry/example", + 1, + "episode-1", + dashboard_state, + NOW, + ) + + publish_status.assert_not_called() + command = run_gh.call_args.args[0] + self.assertEqual(command[2:4], ["--method", "PATCH"]) + self.assertIn("repos/open-telemetry/example/issues/comments/17", command) + self.assertIn("Original friendly reminder.", command[-1]) + self.assertIn( + "this pull request is no longer waiting on you", + command[-1], + ) + self.assertIn(author_nudge.completed_nudge_marker("episode-1"), command[-1]) + minimization_reason.assert_called_once_with("IC_17") + minimize_comment.assert_called_once_with("IC_17") + + @patch.object( + author_nudge, + "minimize_comment", + side_effect=RuntimeError("minimize failed"), + ) + @patch.object(author_nudge, "comment_minimization_reason", return_value="") + @patch.object(author_nudge, "run_gh") + @patch.object(author_nudge, "publish_pr_status") + @patch.object( + author_nudge, + "managed_status_comments", + return_value=[{"html_url": "https://example.test/status"}], + ) + @patch.object( + author_nudge, + "existing_nudge_comment", + return_value={ + "id": 17, + "node_id": "IC_17", + "body": "\n".join([ + author_nudge.nudge_marker("episode-1"), + "Original friendly reminder.", + ]), + }, + ) + def test_failed_minimization_happens_after_completion_note_is_patched( + self, + _existing_nudge, + _status_comments, + _publish_status, + run_gh, + _is_minimized, + minimize_comment, + ) -> None: + with self.assertRaisesRegex(RuntimeError, "minimize failed"): + author_nudge.ensure_nudge_completed( + "open-telemetry/example", + 1, + "episode-1", + {"prs": {}}, + NOW, + ) + + self.assertIn( + author_nudge.completed_nudge_marker("episode-1"), + run_gh.call_args.args[0][-1], + ) + minimize_comment.assert_called_once_with("IC_17") + + @patch.object(author_nudge, "minimize_comment") + @patch.object(author_nudge, "comment_minimization_reason", return_value="") + @patch.object(author_nudge, "run_gh") + @patch.object(author_nudge, "publish_pr_status") + @patch.object( + author_nudge, + "existing_nudge_comment", + return_value={ + "id": 17, + "node_id": "IC_17", + "created_at": "2026-07-10T00:00:00Z", + "body": "\n".join([ + author_nudge.nudge_marker("episode-1"), + author_nudge.completed_nudge_marker("episode-1"), + ]), + }, + ) + def test_completion_marker_prevents_duplicate_edit( + self, + _existing_nudge, + publish_status, + run_gh, + minimization_reason, + minimize_comment, + ) -> None: + author_nudge.ensure_nudge_completed( + "open-telemetry/example", + 1, + "episode-1", + {"prs": {}}, + NOW, + ) + + publish_status.assert_not_called() + run_gh.assert_not_called() + minimization_reason.assert_called_once_with("IC_17") + minimize_comment.assert_called_once_with("IC_17") + + @patch.object(author_nudge, "minimize_comment") + @patch.object( + author_nudge, + "comment_minimization_reason", + return_value="OUTDATED", + ) + @patch.object(author_nudge, "run_gh") + @patch.object(author_nudge, "publish_pr_status") + @patch.object( + author_nudge, + "existing_nudge_comment", + return_value={ + "id": 17, + "node_id": "IC_17", + "body": "\n".join([ + author_nudge.nudge_marker("episode-1"), + author_nudge.completed_nudge_marker("episode-1"), + ]), + }, + ) + def test_completed_and_minimized_comment_is_unchanged( + self, + _existing_nudge, + publish_status, + run_gh, + minimization_reason, + minimize_comment, + ) -> None: + author_nudge.ensure_nudge_completed( + "open-telemetry/example", + 1, + "episode-1", + {"prs": {}}, + NOW, + ) + + publish_status.assert_not_called() + run_gh.assert_not_called() + minimization_reason.assert_called_once_with("IC_17") + minimize_comment.assert_not_called() + + @patch.object(author_nudge, "minimize_comment") + @patch.object(author_nudge, "unminimize_comment") + @patch.object( + author_nudge, + "comment_minimization_reason", + return_value="SPAM", + ) + @patch.object(author_nudge, "run_gh") + @patch.object(author_nudge, "publish_pr_status") + @patch.object( + author_nudge, + "existing_nudge_comment", + return_value={ + "id": 17, + "node_id": "IC_17", + "body": "\n".join([ + author_nudge.nudge_marker("episode-1"), + author_nudge.completed_nudge_marker("episode-1"), + ]), + }, + ) + def test_completed_comment_with_other_classifier_is_reclassified( + self, + _existing_nudge, + publish_status, + run_gh, + minimization_reason, + unminimize_comment, + minimize_comment, + ) -> None: + author_nudge.ensure_nudge_completed( + "open-telemetry/example", + 1, + "episode-1", + {"prs": {}}, + NOW, + ) + + publish_status.assert_not_called() + run_gh.assert_not_called() + minimization_reason.assert_called_once_with("IC_17") + unminimize_comment.assert_called_once_with("IC_17") + minimize_comment.assert_called_once_with("IC_17") + + @patch.object( + author_nudge, + "gh_graphql", + return_value={ + "data": { + "node": { + "isMinimized": True, + "minimizedReason": "outdated", + }, + }, + }, + ) + def test_comment_minimization_reason_uses_node_id(self, graphql) -> None: + self.assertEqual( + "OUTDATED", + author_nudge.comment_minimization_reason("IC_17"), + ) + + query, variables = graphql.call_args.args + self.assertIn("isMinimized", query) + self.assertIn("minimizedReason", query) + self.assertEqual({"id": "IC_17"}, variables) + + @patch.object( + author_nudge, + "gh_graphql", + return_value={ + "data": { + "unminimizeComment": { + "unminimizedComment": {"isMinimized": False}, + }, + }, + }, + ) + def test_unminimize_comment_uses_node_id(self, graphql) -> None: + author_nudge.unminimize_comment("IC_17") + + query, variables = graphql.call_args.args + self.assertIn("unminimizeComment", query) + self.assertEqual({"id": "IC_17"}, variables) + + @patch.object( + author_nudge, + "gh_graphql", + return_value={ + "data": { + "minimizeComment": { + "minimizedComment": {"isMinimized": True}, + }, + }, + }, + ) + def test_minimize_comment_classifies_comment_as_outdated(self, graphql) -> None: + author_nudge.minimize_comment("IC_17") + + query, variables = graphql.call_args.args + self.assertIn("classifier: OUTDATED", query) + self.assertEqual({"id": "IC_17"}, variables) + + @patch.object( + author_nudge, + "gh_api", + return_value=[ + { + "performed_via_github_app": { + "slug": "opentelemetry-pr-dashboard", + }, + "created_at": "2026-07-17T00:00:02Z", + "body": author_nudge.nudge_marker("recovered-episode"), + }, + ], + ) + def test_recovers_legacy_episode_from_posted_comment(self, _gh_api) -> None: + self.assertEqual( + "recovered-episode", + author_nudge.recover_legacy_nudge_episode_id( + "open-telemetry/example", + 1, + "legacy-nudge:2026-07-17T00:00:00.403635+00:00", + ), + ) + + @patch.object( + author_nudge, + "gh_api", + return_value=[ + { + "performed_via_github_app": { + "slug": "opentelemetry-pr-dashboard", + }, + "created_at": "2026-07-17T12:00:00Z", + "body": author_nudge.nudge_marker("recovered-episode"), + }, + ], + ) + def test_recovers_legacy_episode_from_long_delivery_run(self, _gh_api) -> None: + self.assertEqual( + "recovered-episode", + author_nudge.recover_legacy_nudge_episode_id( + "open-telemetry/example", + 1, + "legacy-nudge:2026-07-17T00:00:00+00:00", + ), + ) + @patch.object( author_nudge, "gh_api", diff --git a/.github/scripts/pull-request-dashboard/test_state.py b/.github/scripts/pull-request-dashboard/test_state.py index 20021070a1b..3d87ca02f5f 100644 --- a/.github/scripts/pull-request-dashboard/test_state.py +++ b/.github/scripts/pull-request-dashboard/test_state.py @@ -166,7 +166,7 @@ def test_notification_state_version_is_independent(self) -> None: self.assertEqual(NOTIFICATION_STATE_VERSION, 3) self.assertEqual(DASHBOARD_STATE_VERSION, 7) self.assertEqual(STATUS_COMMENT_ROLLOUT_STATE_VERSION, 1) - self.assertEqual(AUTHOR_NUDGE_STATE_VERSION, 2) + self.assertEqual(AUTHOR_NUDGE_STATE_VERSION, 3) self.assertEqual(COPILOT_REVIEW_REQUEST_STATE_VERSION, 4) def test_author_nudge_state_round_trip(self) -> None: @@ -189,6 +189,33 @@ def test_author_nudge_state_round_trip(self) -> None: ) self.assertTrue(author_nudge_state_path().exists()) + def test_author_nudge_state_loads_version_two_for_migration(self) -> None: + with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)): + author_nudge_state_path().write_text( + json.dumps({ + "version": 2, + "prs": { + "123": { + "waiting_since": "2026-07-10T00:00:00Z", + "nudged_at": "2026-07-17T00:00:00Z", + "episode_id": "episode-1", + }, + }, + }), + encoding="utf-8", + ) + + self.assertEqual( + { + "123": { + "waiting_since": "2026-07-10T00:00:00Z", + "nudged_at": "2026-07-17T00:00:00Z", + "episode_id": "episode-1", + }, + }, + load_author_nudges(), + ) + 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({ @@ -219,28 +246,115 @@ def test_retry_snapshot_preserves_posted_author_nudge(self) -> None: "7": { "waiting_since": "2026-07-10T02:00:00Z", "nudged_at": "2026-07-20T02:00:00Z", + "episode_id": "episode-1", } }, union_merge_author_nudges( - {"7": {"waiting_since": "2026-07-10T02:00:00Z", "nudged_at": ""}}, + { + "7": { + "waiting_since": "2026-07-10T02:00:00Z", + "nudged_at": "", + "pending_at": "2026-07-20T01:00:00Z", + "head_sha": "head", + "routing_input_fingerprint": "fingerprint", + }, + }, { "7": { "waiting_since": "2026-07-10T02:00:00Z", "nudged_at": "2026-07-20T02:00:00Z", + "episode_id": "episode-1", } }, ), ) - def test_retry_snapshot_does_not_suppress_new_author_episode(self) -> None: + def test_retry_snapshot_preserves_pending_nudge_completion(self) -> None: self.assertEqual( - {"7": {"waiting_since": "2026-07-20T02:00:00Z", "nudged_at": ""}}, + { + "7": { + "waiting_since": "2026-07-10T02:00:00Z", + "nudged_at": "2026-07-20T02:00:00Z", + "episode_id": "episode-1", + "completions": [{ + "episode_id": "previous-episode", + "completed_at": "2026-07-21T02:00:00Z", + }], + } + }, + union_merge_author_nudges( + { + "7": { + "waiting_since": "2026-07-10T02:00:00Z", + "nudged_at": "2026-07-20T02:00:00Z", + "episode_id": "episode-1", + "completions": [{ + "episode_id": "previous-episode", + "completed_at": "2026-07-21T02:00:00Z", + }], + } + }, + { + "7": { + "waiting_since": "2026-07-10T02:00:00Z", + "nudged_at": "2026-07-20T02:00:00Z", + } + }, + ), + ) + + def test_retry_snapshot_completes_posted_nudge_without_suppressing_new_episode( + self, + ) -> None: + self.assertEqual( + { + "7": { + "waiting_since": "2026-07-20T02:00:00Z", + "nudged_at": "", + "episode_id": "episode-2", + "completions": [{ + "episode_id": "episode-1", + "completed_at": "2026-07-17T02:00:00Z", + "kind": "routing_changed", + }], + } + }, + union_merge_author_nudges( + { + "7": { + "waiting_since": "2026-07-20T02:00:00Z", + "nudged_at": "", + "episode_id": "episode-2", + } + }, + { + "7": { + "waiting_since": "2026-07-10T02:00:00Z", + "nudged_at": "2026-07-17T02:00:00Z", + "episode_id": "episode-1", + } + }, + ), + ) + + def test_retry_snapshot_completes_posted_nudge_removed_from_baseline(self) -> None: + self.assertEqual( + { + "7": { + "completions": [{ + "episode_id": "episode-1", + "completed_at": "2026-07-17T02:00:00Z", + "kind": "routing_changed", + }], + } + }, 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", + "episode_id": "episode-1", } }, ), diff --git a/pull-request-dashboard/README.md b/pull-request-dashboard/README.md index b5221f3d597..538ca291f98 100644 --- a/pull-request-dashboard/README.md +++ b/pull-request-dashboard/README.md @@ -205,19 +205,27 @@ which routes the pull request back to the author on the next refresh. ## 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 +authors* for one week. The friendly 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. +request back to reviewers. An italic footer calls the reminder a snapshot and +identifies the linked status comment as the live source of truth. +Both an active 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. + +When the dashboard routes the pull request to approvers or maintainers, it +appends an italic note saying that the pull request is no longer waiting on the +author, then marks the comment **Outdated** so GitHub collapses it. The original +reminder remains available when the comment is expanded. If a temporary gate +hold, episode reset, or removal from the dashboard ends the author-waiting +episode, the appended note says only that the reminder no longer reflects the +current dashboard state. Both notes link to the live status without naming the +next route, which may change again. Leaving *Waiting on authors* also 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