From ffe3cbb27c6d318b2b2e93393d1f73107f34f75e Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sat, 8 Aug 2026 15:17:13 -0700 Subject: [PATCH 1/4] Collapse completed author reminders Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f7e532a5-ed16-440b-9de4-ad18e5f20c51 --- .../pull-request-dashboard/author_nudge.py | 354 ++++++++- .../scripts/pull-request-dashboard/state.py | 23 +- .../test_author_nudge.py | 699 +++++++++++++++++- .../pull-request-dashboard/test_state.py | 75 +- pull-request-dashboard/README.md | 30 +- 5 files changed, 1147 insertions(+), 34 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/author_nudge.py b/.github/scripts/pull-request-dashboard/author_nudge.py index ccc6990e2fa..7fa8b3c7f6c 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,70 @@ NUDGE_AFTER = timedelta(weeks=1) +LEGACY_NUDGE_RECOVERY_WINDOW = timedelta(minutes=10) NUDGE_MARKER_PREFIX = "" +) +LEGACY_EPISODE_PREFIX = "legacy-nudge:" +COMMENT_MINIMIZATION_STATE_QUERY = """ +query($id: ID!) { + node(id: $id) { + ... on IssueComment { + isMinimized + } + } +} +""" +MINIMIZE_COMMENT_MUTATION = """ +mutation($id: ID!) { + minimizeComment(input: {subjectId: $id, classifier: OUTDATED}) { + minimizedComment { + 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 +178,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 +273,114 @@ 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(), + author_override_guidance( + "Use this command only while the live dashboard status still says the " + "pull request is waiting on the author." + ), + "", + "_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_is_minimized(node_id: str) -> bool: + 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") + return bool(node["isMinimized"]) + + +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 +413,56 @@ 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}", + ]) + + if not comment_is_minimized(node_id): + minimize_comment(node_id) + + def record_author_nudge_observation( pr_number: int, result: dict[str, Any] | None, @@ -225,12 +478,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 +507,66 @@ 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: + errors.append( + f"PR #{pr_number}: legacy author nudge comment not found" + ) + remaining_completions.append(completion) + 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 +583,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 +617,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..761a99df272 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"] @@ -337,10 +343,21 @@ def union_merge_author_nudges( 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: + baseline_entry = baseline_nudges.get(key) or {} 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 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..a876e2785b8 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,271 @@ 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_failed_legacy_recovery_remains_queued( + 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( + ["PR #1: legacy author nudge comment not found"], + errors, + ) + ensure_completed.assert_not_called() + save_nudges.assert_called_once_with(load_nudges.return_value) + + 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 +933,288 @@ 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_is_minimized", return_value=False) + @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, + is_minimized, + 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]) + is_minimized.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_is_minimized", return_value=False) + @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_is_minimized", return_value=False) + @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, + is_minimized, + 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() + is_minimized.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_is_minimized", return_value=True) + @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, + is_minimized, + 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() + is_minimized.assert_called_once_with("IC_17") + minimize_comment.assert_not_called() + + @patch.object( + author_nudge, + "gh_graphql", + return_value={"data": {"node": {"isMinimized": True}}}, + ) + def test_comment_minimization_state_uses_node_id(self, graphql) -> None: + self.assertTrue(author_nudge.comment_is_minimized("IC_17")) + + query, variables = graphql.call_args.args + self.assertIn("isMinimized", 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", diff --git a/.github/scripts/pull-request-dashboard/test_state.py b/.github/scripts/pull-request-dashboard/test_state.py index 20021070a1b..1594c5b8966 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,10 +246,54 @@ 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": "", + "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_preserves_pending_nudge_completion(self) -> None: + self.assertEqual( + { + "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": ""}}, + { + "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", 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 From e5ebf8180adfdc69c8371fd7b4fb84cc85e19713 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 9 Aug 2026 08:38:38 -0700 Subject: [PATCH 2/4] Address Copilot review comment: preserve CAS retry completions Copilot comment: On a CAS retry, this drops an already-posted reminder when a concurrent refresh has removed the baseline entry after the PR left the author route: `baseline_waiting_since` is empty, so the retry snapshot is ignored even though its GitHub comment was already created. With no accepted nudge entry left, no completion is queued and that reminder remains active indefinitely. Reconcile this case by retaining the retry episode as a pending completion without overwriting any newer baseline episode. Analysis: A successful comment post can be captured only in the retry snapshot while the accepted baseline concurrently advances to a different or absent author episode. Merge the posted retry episode into the baseline as a routing-changed completion, preserving all current baseline fields and deduplicating by episode ID. Upsides: Every posted reminder is eventually collapsed, and newer accepted author episodes remain authoritative across CAS retries. Downsides: The completion timestamp uses the reminder post time because the exact concurrent routing-change time is unavailable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e40eb183-885e-4b33-b29c-a50f4f1d74f9 --- .../scripts/pull-request-dashboard/state.py | 21 +++++++- .../pull-request-dashboard/test_state.py | 49 +++++++++++++++++-- 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index 761a99df272..5c8a1d67b49 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -341,9 +341,9 @@ 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: - baseline_entry = baseline_nudges.get(key) or {} merged[key] = { "waiting_since": waiting_since, "nudged_at": nudged_at, @@ -358,6 +358,23 @@ def union_merge_author_nudges( ) 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_state.py b/.github/scripts/pull-request-dashboard/test_state.py index 1594c5b8966..3d87ca02f5f 100644 --- a/.github/scripts/pull-request-dashboard/test_state.py +++ b/.github/scripts/pull-request-dashboard/test_state.py @@ -303,15 +303,58 @@ def test_retry_snapshot_preserves_pending_nudge_completion(self) -> None: ), ) - def test_retry_snapshot_does_not_suppress_new_author_episode(self) -> None: + 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": ""}}, + { + "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": ""}}, + { + "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-10T02:00:00Z", + "nudged_at": "2026-07-17T02:00:00Z", + "episode_id": "episode-1", } }, ), From 648c81ae99a0efccaaf28df4b66d2448b64fa540 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 9 Aug 2026 08:40:14 -0700 Subject: [PATCH 3/4] Address Copilot review comment: enforce Outdated classifier Copilot comment: `isMinimized` does not identify the classifier. If this reminder was already minimized as SPAM, ABUSE, or another reason, this branch skips the mutation and never applies the required **Outdated** classification. Query the minimization reason and skip only when it is already `OUTDATED`; otherwise explicitly reclassify it. Analysis: A minimized comment can carry a classifier other than Outdated. Query and normalize the minimization reason, leave comments already classified Outdated unchanged, and unminimize then minimize comments carrying another classifier so GitHub applies Outdated deterministically. Upsides: Completed reminders consistently use the documented Outdated classification while preserving idempotency for comments already classified correctly. Downsides: Reclassification requires one additional GraphQL mutation for comments minimized under another classifier. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e40eb183-885e-4b33-b29c-a50f4f1d74f9 --- .../pull-request-dashboard/author_nudge.py | 35 +++++- .../test_author_nudge.py | 103 +++++++++++++++--- 2 files changed, 122 insertions(+), 16 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/author_nudge.py b/.github/scripts/pull-request-dashboard/author_nudge.py index 7fa8b3c7f6c..33cd12b4a40 100644 --- a/.github/scripts/pull-request-dashboard/author_nudge.py +++ b/.github/scripts/pull-request-dashboard/author_nudge.py @@ -46,6 +46,7 @@ node(id: $id) { ... on IssueComment { isMinimized + minimizedReason } } } @@ -59,6 +60,15 @@ } } """ +UNMINIMIZE_COMMENT_MUTATION = """ +mutation($id: ID!) { + unminimizeComment(input: {subjectId: $id}) { + unminimizedComment { + isMinimized + } + } +} +""" def nudge_marker(episode_id: str) -> str: @@ -362,12 +372,28 @@ def render_completed_nudge( ]) + "\n" -def comment_is_minimized(node_id: str) -> bool: +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") - return bool(node["isMinimized"]) + 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: @@ -459,7 +485,10 @@ def ensure_nudge_completed( f"body={body}", ]) - if not comment_is_minimized(node_id): + minimized_reason = comment_minimization_reason(node_id) + if minimized_reason != "OUTDATED": + if minimized_reason: + unminimize_comment(node_id) minimize_comment(node_id) diff --git a/.github/scripts/pull-request-dashboard/test_author_nudge.py b/.github/scripts/pull-request-dashboard/test_author_nudge.py index a876e2785b8..a55d6ea9cb7 100644 --- a/.github/scripts/pull-request-dashboard/test_author_nudge.py +++ b/.github/scripts/pull-request-dashboard/test_author_nudge.py @@ -985,7 +985,7 @@ def test_rendered_gate_completion_does_not_claim_author_handoff(self) -> None: self.assertNotIn("no longer waiting on you", body) @patch.object(author_nudge, "minimize_comment") - @patch.object(author_nudge, "comment_is_minimized", return_value=False) + @patch.object(author_nudge, "comment_minimization_reason", return_value="") @patch.object(author_nudge, "run_gh") @patch.object(author_nudge, "publish_pr_status") @patch.object( @@ -1012,7 +1012,7 @@ def test_completion_appends_note_then_marks_comment_outdated( _status_comments, publish_status, run_gh, - is_minimized, + minimization_reason, minimize_comment, ) -> None: dashboard_state = {"prs": {"1": author_result("approver")}} @@ -1035,7 +1035,7 @@ def test_completion_appends_note_then_marks_comment_outdated( command[-1], ) self.assertIn(author_nudge.completed_nudge_marker("episode-1"), command[-1]) - is_minimized.assert_called_once_with("IC_17") + minimization_reason.assert_called_once_with("IC_17") minimize_comment.assert_called_once_with("IC_17") @patch.object( @@ -1043,7 +1043,7 @@ def test_completion_appends_note_then_marks_comment_outdated( "minimize_comment", side_effect=RuntimeError("minimize failed"), ) - @patch.object(author_nudge, "comment_is_minimized", return_value=False) + @patch.object(author_nudge, "comment_minimization_reason", return_value="") @patch.object(author_nudge, "run_gh") @patch.object(author_nudge, "publish_pr_status") @patch.object( @@ -1088,7 +1088,7 @@ def test_failed_minimization_happens_after_completion_note_is_patched( minimize_comment.assert_called_once_with("IC_17") @patch.object(author_nudge, "minimize_comment") - @patch.object(author_nudge, "comment_is_minimized", return_value=False) + @patch.object(author_nudge, "comment_minimization_reason", return_value="") @patch.object(author_nudge, "run_gh") @patch.object(author_nudge, "publish_pr_status") @patch.object( @@ -1109,7 +1109,7 @@ def test_completion_marker_prevents_duplicate_edit( _existing_nudge, publish_status, run_gh, - is_minimized, + minimization_reason, minimize_comment, ) -> None: author_nudge.ensure_nudge_completed( @@ -1122,11 +1122,15 @@ def test_completion_marker_prevents_duplicate_edit( publish_status.assert_not_called() run_gh.assert_not_called() - is_minimized.assert_called_once_with("IC_17") + 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_is_minimized", return_value=True) + @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( @@ -1146,7 +1150,7 @@ def test_completed_and_minimized_comment_is_unchanged( _existing_nudge, publish_status, run_gh, - is_minimized, + minimization_reason, minimize_comment, ) -> None: author_nudge.ensure_nudge_completed( @@ -1159,19 +1163,92 @@ def test_completed_and_minimized_comment_is_unchanged( publish_status.assert_not_called() run_gh.assert_not_called() - is_minimized.assert_called_once_with("IC_17") + 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}}}, + return_value={ + "data": { + "node": { + "isMinimized": True, + "minimizedReason": "outdated", + }, + }, + }, ) - def test_comment_minimization_state_uses_node_id(self, graphql) -> None: - self.assertTrue(author_nudge.comment_is_minimized("IC_17")) + 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( From 9c44d853b3d70a6876647a9bdc8d7a1bce2bd226 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 9 Aug 2026 08:45:48 -0700 Subject: [PATCH 4/4] Address Copilot review comments: harden legacy recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot comment: An exhaustive lookup that finds no legacy comment is terminal—for example, the author may have deleted the reminder—and the desired state already has no stale comment to collapse. Keeping this completion and returning an error makes every future hourly delivery fail forever; API failures already raise and are retried without saving state. Log the missing comment and discard this completion instead of re-queuing it. Copilot comment: `nudged_at` in legacy v2 state was recorded from the delivery run's shared `now`, not from the comment API's actual `created_at` (see `ensure_nudge` line 445). A delivery that reaches this PR more than ten minutes after starting therefore cannot recover an existing reminder and leaves it uncollapsed. Since a second reminder cannot be posted until a new week-long episode, a wider sub-week window (for example, one day) remains unambiguous while covering long runs. ``` LEGACY_NUDGE_RECOVERY_WINDOW = timedelta(minutes=10) ``` Analysis: Legacy timestamps represent the delivery run start rather than exact comment creation. Expand recovery to one day, which remains below the one-week reminder interval, and treat an exhaustive no-match as terminal while allowing API failures to continue propagating for retry. Upsides: Long delivery runs can recover existing reminders, while deleted or otherwise absent comments no longer poison every hourly delivery. Downsides: Legacy matching accepts a wider timestamp range, though the one-reminder-per-week invariant keeps candidates unambiguous. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e40eb183-885e-4b33-b29c-a50f4f1d74f9 --- .../pull-request-dashboard/author_nudge.py | 9 ++-- .../test_author_nudge.py | 44 ++++++++++++++----- 2 files changed, 39 insertions(+), 14 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/author_nudge.py b/.github/scripts/pull-request-dashboard/author_nudge.py index 33cd12b4a40..dbb980343bb 100644 --- a/.github/scripts/pull-request-dashboard/author_nudge.py +++ b/.github/scripts/pull-request-dashboard/author_nudge.py @@ -32,7 +32,7 @@ NUDGE_AFTER = timedelta(weeks=1) -LEGACY_NUDGE_RECOVERY_WINDOW = timedelta(minutes=10) +LEGACY_NUDGE_RECOVERY_WINDOW = timedelta(days=1) NUDGE_MARKER_PREFIX = "