From 3c354f1facabb8e9f6f68e95e3c32baac54c94a4 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 16 Jul 2026 19:42:00 -0700 Subject: [PATCH 1/7] Add live PR dashboard status comments --- .../pull-request-dashboard/RATIONALE.md | 13 +- .../pull-request-dashboard/WEBHOOK_SETUP.md | 19 +- .../pull-request-dashboard/classification.py | 2 +- .../pull-request-dashboard/dashboard.py | 27 ++- .../pull-request-dashboard/github_cli.py | 2 + .../netlify/functions/github-webhook.js | 35 ++- .../pr_status_comment.py | 187 ++++++++++++++++ .../pull-request-dashboard/test_dashboard.py | 73 +++++++ .../test_github_webhook.js | 61 ++++++ .../test_pr_status_comment.py | 205 ++++++++++++++++++ .../workflows/pull-request-dashboard-repo.yml | 111 ++++------ .github/workflows/pull-request-dashboard.yml | 47 ++-- pull-request-dashboard/README.md | 27 ++- 13 files changed, 668 insertions(+), 141 deletions(-) create mode 100644 .github/scripts/pull-request-dashboard/pr_status_comment.py create mode 100644 .github/scripts/pull-request-dashboard/test_dashboard.py create mode 100644 .github/scripts/pull-request-dashboard/test_github_webhook.js create mode 100644 .github/scripts/pull-request-dashboard/test_pr_status_comment.py diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index dadd5b7144..3f20b88247 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -36,14 +36,13 @@ the implementation understandable and operationally cheap. - Coalescing is safe because each refresh loads current PR state from GitHub. Intermediate states can go unobserved, but the surviving run reflects the state that exists when it executes. -- Submitted reviews use the review id as a distinct concurrency identity so a - pending review-guidance event cannot be replaced by a generic PR refresh. - Manual runs are also separate because they can refresh large repositories - that webhook-driven runs intentionally skip. +- Submitted reviews can coalesce with generic PR refreshes because the live PR + status comment is rendered from current accepted dashboard state rather than + a review-specific event. Manual runs remain separate because they can refresh + large repositories that webhook-driven runs intentionally skip. - Concurrency bounds pending jobs per target; it does not debounce webhook - delivery or workflow dispatch. Different repositories, PRs, and submitted - review ids can still run independently, and every accepted webhook still - creates a workflow run. + delivery or workflow dispatch. Different repositories and PRs can still run + independently, and every accepted webhook still creates a workflow run. ## GitHub Actions Instead Of Netlify For Scheduled Backfills diff --git a/.github/scripts/pull-request-dashboard/WEBHOOK_SETUP.md b/.github/scripts/pull-request-dashboard/WEBHOOK_SETUP.md index 17b06c74d9..afc33dcf30 100644 --- a/.github/scripts/pull-request-dashboard/WEBHOOK_SETUP.md +++ b/.github/scripts/pull-request-dashboard/WEBHOOK_SETUP.md @@ -51,7 +51,7 @@ Repository permissions: - Contents: read-only - Issues: read and write - Metadata: read-only -- Pull requests: read and write +- Pull requests: read-only Organization permissions: @@ -63,14 +63,14 @@ Permission rationale: | ---------- | ------ | ---------------- | | Checks | Read | Required to subscribe to check-suite events and to read check data for dashboard rows. | | Contents | Read | Reads PR commits and repository metadata needed by pull/commit APIs. | -| Issues | Read and write | Finds/creates/updates the dashboard issue and reads existing PR conversation comments before posting review guidance. | +| Issues | Read and write | Finds/creates/updates the dashboard issue and creates or updates the dashboard-managed PR status comment. | | Metadata | Read | Required by GitHub for GitHub App repository access. | -| Pull requests | Read and write | Required to subscribe to PR review/comment/thread events, read PR details, reviews, review comments, commits, and GraphQL review threads, and post review-guidance comments on PRs. | +| Pull requests | Read | Required to subscribe to PR review/comment/thread events and read PR details, reviews, review comments, commits, and GraphQL review threads. | | Members | Read | Reads approver-team membership configured in `repositories.json`. | The dashboard does not create inline review comments, submit reviews, or resolve -review threads. It does create PR conversation comments for review guidance, -which requires `Pull requests: read and write`. +review threads. It creates and updates one PR conversation comment through the +issues API, which requires `Issues: read and write`. Subscribe to events: @@ -87,8 +87,8 @@ Event rationale: | ----- | ---------------- | | Check suite | Refreshes CI status when checks are requested, rerequested, or completed. | | Pull request | Refreshes dashboard rows when PR state, draft status, labels, assignees, branches, or metadata change. | -| Issue comment | Refreshes PR conversation state when PR issue comments are created, edited, or deleted. | -| Pull request review | Refreshes approval/change-request state and posts guidance for submitted reviews with review comments. | +| Issue comment | Refreshes PR conversation state when PR issue comments are created, edited, or deleted. Events for the dashboard App's own marked status comment are ignored. | +| Pull request review | Refreshes approval/change-request state and the live PR status comment. | | Pull request review comment | Refreshes inline review-comment discussion state. | | Pull request review thread | Refreshes when inline review threads are resolved or unresolved. | @@ -155,8 +155,7 @@ The webhook bridge should dispatch `pull-request-dashboard.yml` in "repository": "opentelemetry-java-instrumentation", "pr_number": "12345", "trigger_event": "pull_request_review_comment", - "trigger_action": "created", - "trigger_review_id": "67890" + "trigger_action": "created" } ``` @@ -164,6 +163,4 @@ Notes: - `repository` is the short repository name under `open-telemetry`. - Omit `pr_number` or set it to an empty string for a backfill. -- `trigger_review_id` is only required for `pull_request_review` `submitted` - events when review guidance may need to be posted. - The central workflow validates these inputs before using them. diff --git a/.github/scripts/pull-request-dashboard/classification.py b/.github/scripts/pull-request-dashboard/classification.py index 62aead6de8..40fa01aad6 100644 --- a/.github/scripts/pull-request-dashboard/classification.py +++ b/.github/scripts/pull-request-dashboard/classification.py @@ -224,7 +224,7 @@ def discussion_prompt_input(discussion: dict[str, Any]) -> dict[str, Any]: prompt_thread = { key: value for key, value in discussion.items() - if key != "comments" + if key not in ("comments", "discussion_url") } prompt_thread["comments"] = [ { diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index adf3ee0897..6b66c24e09 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -114,6 +114,9 @@ or PR creation time. waiting_age_basis str Which heuristic chose waiting_since. + author_action_discussion_urls list[str] Canonical links to unresolved + review discussions routed to + the author. reviewers list[dict] Reviewers to display (added by add_reviewers). Each entry is {"login": str, "approved": bool, @@ -557,8 +560,9 @@ def group_review_threads( # as live. if discussion.get("isResolved") or discussion.get("isOutdated"): continue + raw_comments = (discussion.get("comments") or {}).get("nodes") or [] comments = [] - for c in ((discussion.get("comments") or {}).get("nodes") or []): + for c in raw_comments: actor = reviewer_actor_login(c.get("author") or {}) comments.append(discussion_comment( c.get("updatedAt") or c.get("createdAt") or "", @@ -578,6 +582,7 @@ def group_review_threads( "path": discussion.get("path"), "line": discussion.get("line"), "resolved": False, + "discussion_url": raw_comments[0].get("url") if raw_comments else "", "comments": comments, }, comments, facts)) discussions.sort(key=lambda t: t["comments"][-1]["timestamp"]) @@ -900,6 +905,23 @@ def add_wait_age_facts( facts["waiting_age_basis"] = basis +def author_action_discussion_urls( + review_threads: list[dict[str, Any]], + pending_actions: dict[str, dict[str, Any]], +) -> list[str]: + by_id = discussions_by_id(review_threads) + urls: list[str] = [] + for discussion_id, entry in pending_actions.items(): + action = normalize_discussion_action(entry.get("action") or "") + if action != "author": + continue + thread = by_id.get(discussion_id) + url = (thread or {}).get("discussion_url") or "" + if url and url not in urls: + urls.append(url) + return urls + + # Discussion actions that count as open and unresolved. A reviewer who commented # in such a discussion is not yet satisfied, even if they have approved. # "none" means no follow-up is needed, so it does not block a clear check. @@ -1052,6 +1074,9 @@ def build_pr_result( } route = route_pr(facts, pending_actions, required_approvals) add_wait_age_facts(facts, route, pending_actions) + facts["author_action_discussion_urls"] = author_action_discussion_urls( + review_threads, pending_actions + ) add_reviewers( facts, events, review_threads, top_level_items, pending_actions ) diff --git a/.github/scripts/pull-request-dashboard/github_cli.py b/.github/scripts/pull-request-dashboard/github_cli.py index 410ce6643b..fb582a7c6d 100644 --- a/.github/scripts/pull-request-dashboard/github_cli.py +++ b/.github/scripts/pull-request-dashboard/github_cli.py @@ -341,6 +341,7 @@ def load_reviewer_set(org: str, approver_team_slugs: list[str]) -> set[str]: } nodes { id + url body createdAt updatedAt @@ -377,6 +378,7 @@ def load_reviewer_set(org: str, approver_team_slugs: list[str]) -> set[str]: } nodes { id + url body createdAt updatedAt diff --git a/.github/scripts/pull-request-dashboard/netlify/functions/github-webhook.js b/.github/scripts/pull-request-dashboard/netlify/functions/github-webhook.js index fcde8af051..114fa1a42f 100644 --- a/.github/scripts/pull-request-dashboard/netlify/functions/github-webhook.js +++ b/.github/scripts/pull-request-dashboard/netlify/functions/github-webhook.js @@ -6,6 +6,12 @@ const OWNER = "open-telemetry"; const WORKFLOW_REPOSITORY = "shared-workflows"; const WORKFLOW_ID = "pull-request-dashboard.yml"; const WORKFLOW_REF = "main"; +const DASHBOARD_APP_SLUG = "opentelemetry-pr-dashboard"; +const DASHBOARD_COMMENT_MARKERS = [ + "", + "", + "", +]; const ALLOWED_ACTIONS = { check_suite: new Set(["completed", "requested", "rerequested"]), @@ -35,6 +41,8 @@ exports.handler = async (event) => { } }; +exports.isDashboardSelfTriggeredCommentEvent = isDashboardSelfTriggeredCommentEvent; + async function handle(event) { if (event.httpMethod !== "POST") { return response(405, { error: "method not allowed" }); @@ -64,6 +72,9 @@ async function handle(event) { if (!ALLOWED_ACTIONS[eventName].has(action)) { return response(202, { status: "ignored", reason: `unsupported action: ${eventName}.${action || "missing"}` }); } + if (isDashboardSelfTriggeredCommentEvent(eventName, payload)) { + return response(202, { status: "ignored", reason: "dashboard-managed comment" }); + } const repository = readRepository(payload); if (!repository.fullName) { @@ -78,7 +89,6 @@ async function handle(event) { return response(202, { status: "ignored", reason: "no pull request number found" }); } - const triggerReviewId = extractTriggerReviewId(payload); const dispatcherJwt = createAppJwt({ appId: config.dispatcherAppId, privateKey: config.dispatcherPrivateKey }); const installationId = await findRepositoryInstallationId(dispatcherJwt, `${OWNER}/${WORKFLOW_REPOSITORY}`); const installationToken = await createInstallationToken(dispatcherJwt, installationId); @@ -87,7 +97,6 @@ async function handle(event) { pr_number: String(prNumber), trigger_event: eventName, trigger_action: action, - trigger_review_id: triggerReviewId ? String(triggerReviewId) : "", }); return response(202, { @@ -96,10 +105,26 @@ async function handle(event) { pr_number: prNumber, trigger_event: eventName, trigger_action: action, - trigger_review_id: triggerReviewId, }); } +function isDashboardSelfTriggeredCommentEvent(eventName, payload) { + if (eventName !== "issue_comment") { + return false; + } + const comment = payload.comment || {}; + const app = comment.performed_via_github_app || {}; + const body = comment.body || ""; + const commentAuthor = comment.user || {}; + const sender = payload.sender || {}; + return ( + app.slug === DASHBOARD_APP_SLUG && + DASHBOARD_COMMENT_MARKERS.some((marker) => body.includes(marker)) && + Boolean(sender.id) && + sender.id === commentAuthor.id + ); +} + function loadConfig() { const config = { dispatcherAppId: process.env.OTELBOT_SHARED_WORKFLOWS_APP_ID, @@ -131,10 +156,6 @@ function readRepository(payload) { }; } -function extractTriggerReviewId(payload) { - return payload.review && payload.review.id; -} - function readRawBody(event) { if (!event.body) { return Buffer.alloc(0); diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py new file mode 100644 index 0000000000..9bb13badc1 --- /dev/null +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -0,0 +1,187 @@ +#!/usr/bin/env python3 +"""Create or update the dashboard-managed status comment on a pull request.""" + +from __future__ import annotations + +import argparse +import sys +from typing import Any + +from github_cli import detect_repo, gh_api, normalize_repo, repo_state_key, run_gh +from state import load_dashboard_state_cache, set_state_dir +import state_branch + + +STATUS_MARKER = "" +AUTHOR_ACTION_DISCUSSION_LINK_LIMIT = 20 +AUTHOR_GUIDANCE = ( + "Please give every review discussion a clear outcome: link to the commit that addresses it, " + "explain why no change is needed, ask a follow-up question, or resolve the discussion." +) +DASHBOARD_APP_SLUG = "opentelemetry-pr-dashboard" +LEGACY_MARKERS = ( + "", + "", +) + + +def load_accepted_dashboard_state(repo: str, state_branch_name: str) -> dict[str, Any] | None: + if not state_branch.fetch_state_branch(state_branch_name, required=False): + return None + with state_branch.temporary_state_dir() as checkout_dir: + try: + state_branch.run([ + "git", "worktree", "add", "--quiet", "--detach", str(checkout_dir), + state_branch.remote_ref(state_branch_name), + ]) + set_state_dir(checkout_dir / repo_state_key(repo)) + return load_dashboard_state_cache() + finally: + state_branch.remove_existing_state_dir(checkout_dir) + + +def render_status_comment( + pr: dict[str, Any], + result: dict[str, Any] | None, +) -> str: + author = ((pr.get("user") or {}).get("login") or "").strip() or "the author" + state = (pr.get("state") or "").lower() + terminal = bool(pr.get("merged")) or state == "closed" + + if pr.get("merged"): + next_action = "No one" + waiting_on = "This pull request has been merged." + elif state == "closed": + next_action = "No one" + waiting_on = "This pull request has been closed." + elif pr.get("draft"): + next_action = f"@{author}" if author != "the author" else author + waiting_on = "The author to mark this pull request ready for review." + elif result is None: + next_action = "Dashboard maintainers" + waiting_on = "The dashboard to finish refreshing this pull request." + else: + facts = result.get("facts") or {} + route = result.get("route") or "unknown" + if route == "author": + effective_author = (facts.get("author") or author).strip() + next_action = f"@{effective_author}" if effective_author != "the author" else effective_author + waiting_on = "The author to address or respond to unresolved review discussions." + elif route == "approver": + next_action = "Reviewers" + waiting_on = "Reviewers to review the latest changes." + elif route == "maintainer": + next_action = "Maintainers" + waiting_on = "A maintainer to merge the pull request." + elif route == "external": + next_action = "External" + waiting_on = "An external dependency or decision." + else: + next_action = "Dashboard maintainers" + waiting_on = "The dashboard to determine the next action." + + lines = [ + STATUS_MARKER, + "## Pull request dashboard status", + "", + f"**Next action:** {next_action}", + "", + f"**Waiting on:** {waiting_on}", + ] + + if not terminal and result and result.get("route") == "author": + urls = (result.get("facts") or {}).get("author_action_discussion_urls") or [] + if urls: + lines.extend(["", "Unresolved discussions waiting on the author:"]) + displayed_urls = urls[:AUTHOR_ACTION_DISCUSSION_LINK_LIMIT] + lines.extend(f"- [Discussion {index}]({url})" for index, url in enumerate(displayed_urls, start=1)) + remaining_count = len(urls) - len(displayed_urls) + if remaining_count: + noun = "discussion" if remaining_count == 1 else "discussions" + lines.append(f"- {remaining_count} more unresolved {noun} not shown") + + if not terminal: + lines.extend(["", AUTHOR_GUIDANCE]) + lines.append("") + return "\n".join(lines) + + +def managed_status_comments(repo: str, pr_number: int) -> list[dict[str, Any]]: + comments = gh_api( + f"/repos/{repo}/issues/{pr_number}/comments?per_page=100", + paginate=True, + ) + markers = (STATUS_MARKER, *LEGACY_MARKERS) + return [ + comment + for comment in comments or [] + if (comment.get("performed_via_github_app") or {}).get("slug") == DASHBOARD_APP_SLUG + and any(marker in (comment.get("body") or "") for marker in markers) + ] + + +def upsert_status_comment(repo: str, pr_number: int, body: str) -> None: + comments = managed_status_comments(repo, pr_number) + if comments: + comment = comments[0] + comment_id = comment["id"] + if comment.get("body") == body: + print(f"PR #{pr_number} status comment is unchanged", file=sys.stderr) + else: + print(f"updating PR #{pr_number} status comment {comment_id}", file=sys.stderr) + run_gh([ + "gh", "api", "--method", "PATCH", + f"repos/{repo}/issues/comments/{comment_id}", + "-f", f"body={body}", + ]) + for duplicate in comments[1:]: + duplicate_id = duplicate["id"] + print(f"deleting duplicate PR #{pr_number} status comment {duplicate_id}", file=sys.stderr) + run_gh([ + "gh", "api", "--method", "DELETE", + f"repos/{repo}/issues/comments/{duplicate_id}", + ]) + return + + print(f"creating PR #{pr_number} status comment", file=sys.stderr) + run_gh([ + "gh", "api", "--method", "POST", + f"repos/{repo}/issues/{pr_number}/comments", + "-f", f"body={body}", + ]) + + +def publish_pr_status(repo: str, pr_number: int, dashboard_state: dict[str, Any]) -> None: + pr = gh_api(f"/repos/{repo}/pulls/{pr_number}") + result = (dashboard_state.get("prs") or {}).get(str(pr_number)) + upsert_status_comment(repo, pr_number, render_status_comment(pr, result)) + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--repo", help="target repository name, e.g. opentelemetry-java-instrumentation") + parser.add_argument("--state-branch", required=True, help="git branch used for workflow state") + parser.add_argument("--pr-number", type=int, help="pull request to update") + parser.add_argument( + "--check-state-exists", + action="store_true", + help='print "true" when accepted dashboard state exists, otherwise "false"', + ) + args = parser.parse_args() + + repo = normalize_repo(args.repo) if args.repo else detect_repo() + dashboard_state = load_accepted_dashboard_state(repo, args.state_branch) + if args.check_state_exists: + print("true" if dashboard_state is not None else "false") + return 0 + if args.pr_number is None: + parser.error("--pr-number is required unless --check-state-exists is set") + if dashboard_state is None: + print("dashboard result state not found; skipping PR status comment", file=sys.stderr) + return 0 + publish_pr_status(repo, args.pr_number, dashboard_state) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) \ No newline at end of file diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py new file mode 100644 index 0000000000..7e91fec7a4 --- /dev/null +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -0,0 +1,73 @@ +from __future__ import annotations + +import unittest + +from classification import discussion_prompt_input +from dashboard import author_action_discussion_urls, group_review_threads + + +class ReviewThreadDiscussionUrlTest(unittest.TestCase): + def test_group_review_threads_stores_first_comment_url_on_thread(self) -> None: + threads = group_review_threads( + { + "review_threads": [ + { + "id": "thread-1", + "isResolved": False, + "isOutdated": False, + "comments": { + "nodes": [ + { + "url": "https://example.test/discussion/1", + "body": "first", + "createdAt": "2026-07-14T02:00:00Z", + "author": {"login": "reviewer"}, + }, + { + "url": "https://example.test/discussion/2", + "body": "second", + "createdAt": "2026-07-14T01:00:00Z", + "author": {"login": "author"}, + }, + ], + }, + }, + ], + }, + "author", + {"reviewer"}, + {"conflicts": "no"}, + ) + + self.assertEqual("https://example.test/discussion/1", threads[0]["discussion_url"]) + self.assertEqual("second", threads[0]["comments"][0]["body"]) + self.assertNotIn("url", threads[0]["comments"][0]) + + def test_author_action_urls_use_thread_url_and_deduplicate(self) -> None: + threads = [ + {"discussion_id": "thread-1", "discussion_url": "https://example.test/discussion/1"}, + {"discussion_id": "thread-2", "discussion_url": "https://example.test/discussion/1"}, + ] + pending_actions = { + "thread-1": {"action": "author"}, + "thread-2": {"action": "author"}, + } + + self.assertEqual( + ["https://example.test/discussion/1"], + author_action_discussion_urls(threads, pending_actions), + ) + + def test_discussion_url_is_excluded_from_classifier_input(self) -> None: + prompt_input = discussion_prompt_input({ + "discussion_id": "thread-1", + "discussion_kind": "review-comment-thread", + "discussion_url": "https://example.test/discussion/1", + "comments": [], + }) + + self.assertNotIn("discussion_url", prompt_input) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/.github/scripts/pull-request-dashboard/test_github_webhook.js b/.github/scripts/pull-request-dashboard/test_github_webhook.js new file mode 100644 index 0000000000..7c67f45fc9 --- /dev/null +++ b/.github/scripts/pull-request-dashboard/test_github_webhook.js @@ -0,0 +1,61 @@ +const assert = require("node:assert/strict"); +const test = require("node:test"); + +const { isDashboardSelfTriggeredCommentEvent } = require("./netlify/functions/github-webhook"); + +const dashboardApp = { slug: "opentelemetry-pr-dashboard" }; +const dashboardActor = { id: 1 }; + +test("recognizes current and legacy dashboard-managed comments", () => { + for (const marker of [ + "", + "", + "", + ]) { + assert.equal(isDashboardSelfTriggeredCommentEvent("issue_comment", { + comment: { + body: `${marker} body`, + performed_via_github_app: dashboardApp, + user: dashboardActor, + }, + sender: dashboardActor, + }), true); + } +}); + +test("rejects spoofed markers and unrelated dashboard app comments", () => { + assert.equal(isDashboardSelfTriggeredCommentEvent("issue_comment", { + comment: { + body: " spoofed", + user: dashboardActor, + }, + sender: dashboardActor, + }), false); + assert.equal(isDashboardSelfTriggeredCommentEvent("issue_comment", { + comment: { + body: "ordinary comment", + performed_via_github_app: dashboardApp, + user: dashboardActor, + }, + sender: dashboardActor, + }), false); + assert.equal(isDashboardSelfTriggeredCommentEvent("pull_request", { + comment: { + body: " body", + performed_via_github_app: dashboardApp, + user: dashboardActor, + }, + sender: dashboardActor, + }), false); +}); + +test("allows dashboard-managed comment events triggered by another actor", () => { + assert.equal(isDashboardSelfTriggeredCommentEvent("issue_comment", { + comment: { + body: " body", + performed_via_github_app: dashboardApp, + user: dashboardActor, + }, + sender: { id: 2 }, + }), false); +}); \ No newline at end of file diff --git a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py new file mode 100644 index 0000000000..df96326f5f --- /dev/null +++ b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py @@ -0,0 +1,205 @@ +from __future__ import annotations + +from pathlib import Path +import unittest +from unittest.mock import patch + +import pr_status_comment + + +class LoadAcceptedDashboardStateTest(unittest.TestCase): + @patch.object(pr_status_comment.state_branch, "fetch_state_branch", return_value=True) + @patch.object(pr_status_comment.state_branch, "run") + @patch.object(pr_status_comment.state_branch, "remove_existing_state_dir") + @patch.object(pr_status_comment, "load_dashboard_state_cache", return_value={"version": 3, "prs": {}}) + def test_worktree_checkout_is_quiet( + self, + _load_state: object, + _remove_state_dir: object, + run: object, + _fetch_state_branch: object, + ) -> None: + checkout_dir = Path("checkout") + with patch.object(pr_status_comment.state_branch, "temporary_state_dir") as temporary_state_dir: + temporary_state_dir.return_value.__enter__.return_value = checkout_dir + + state = pr_status_comment.load_accepted_dashboard_state( + "open-telemetry/example", + "state-branch", + ) + + self.assertEqual({"version": 3, "prs": {}}, state) + run.assert_called_once_with([ + "git", "worktree", "add", "--quiet", "--detach", "checkout", + "refs/remotes/origin/state-branch", + ]) + + +class RenderStatusCommentTest(unittest.TestCase): + def pr(self, **overrides: object) -> dict[str, object]: + pr: dict[str, object] = { + "state": "open", + "draft": False, + "merged": False, + "user": {"login": "alice"}, + } + pr.update(overrides) + return pr + + def test_waiting_on_author_links_discussions(self) -> None: + body = pr_status_comment.render_status_comment( + self.pr(), + { + "route": "author", + "facts": { + "author": "alice", + "author_action_discussion_urls": [ + "https://github.com/open-telemetry/example/pull/1#discussion_r1", + ], + }, + }, + ) + + self.assertIn("**Next action:** @alice", body) + self.assertIn("[Discussion 1]", body) + self.assertIn("give every review discussion a clear outcome", body) + + def test_waiting_on_author_caps_discussion_links(self) -> None: + urls = [ + f"https://github.com/open-telemetry/example/pull/1#discussion_r{index}" + for index in range(pr_status_comment.AUTHOR_ACTION_DISCUSSION_LINK_LIMIT + 2) + ] + + body = pr_status_comment.render_status_comment( + self.pr(), + { + "route": "author", + "facts": { + "author": "alice", + "author_action_discussion_urls": urls, + }, + }, + ) + + self.assertEqual(pr_status_comment.AUTHOR_ACTION_DISCUSSION_LINK_LIMIT, body.count("- [Discussion ")) + self.assertIn("- 2 more unresolved discussions not shown", body) + self.assertNotIn(urls[-1], body) + + def test_draft_waits_on_author(self) -> None: + body = pr_status_comment.render_status_comment(self.pr(draft=True), None) + + self.assertIn("**Next action:** @alice", body) + self.assertIn("mark this pull request ready for review", body) + + def test_merged_pr_has_no_author_guidance(self) -> None: + body = pr_status_comment.render_status_comment( + self.pr(state="closed", merged=True), + None, + ) + + self.assertIn("**Next action:** No one", body) + self.assertIn("has been merged", body) + self.assertNotIn("give every review discussion a clear outcome", body) + + def test_terminal_pr_has_no_author_discussion_links(self) -> None: + result = { + "route": "author", + "facts": { + "author": "alice", + "author_action_discussion_urls": [ + "https://github.com/open-telemetry/example/pull/1#discussion_r1", + ], + }, + } + + for overrides in ({"state": "closed"}, {"state": "closed", "merged": True}): + with self.subTest(overrides=overrides): + body = pr_status_comment.render_status_comment(self.pr(**overrides), result) + + self.assertNotIn("Unresolved discussions waiting on the author", body) + self.assertNotIn("[Discussion 1]", body) + + def test_blank_login_falls_back_to_author(self) -> None: + body = pr_status_comment.render_status_comment( + self.pr(user={"login": " "}, draft=True), + None, + ) + + self.assertIn("**Next action:** the author", body) + self.assertNotIn("**Next action:** @", body) + + +class UpsertStatusCommentTest(unittest.TestCase): + def setUp(self) -> None: + self.commands: list[list[str]] = [] + self.run_gh_patch = patch.object( + pr_status_comment, + "run_gh", + side_effect=lambda command: self.commands.append(command) or "", + ) + self.run_gh_patch.start() + + def tearDown(self) -> None: + self.run_gh_patch.stop() + + @patch.object(pr_status_comment, "managed_status_comments", return_value=[]) + def test_creates_first_status_comment(self, _comments: object) -> None: + pr_status_comment.upsert_status_comment("open-telemetry/example", 1, "body") + + self.assertEqual("POST", self.commands[0][3]) + + @patch.object( + pr_status_comment, + "managed_status_comments", + return_value=[{"id": 7, "body": "body"}], + ) + def test_does_not_update_unchanged_comment(self, _comments: object) -> None: + pr_status_comment.upsert_status_comment("open-telemetry/example", 1, "body") + + self.assertEqual([], self.commands) + + @patch.object( + pr_status_comment, + "managed_status_comments", + return_value=[ + {"id": 7, "body": " old"}, + {"id": 8, "body": " duplicate"}, + ], + ) + def test_migrates_legacy_comment_and_deletes_duplicates(self, _comments: object) -> None: + pr_status_comment.upsert_status_comment("open-telemetry/example", 1, "body") + + self.assertEqual(["PATCH", "DELETE"], [command[3] for command in self.commands]) + + +class ManagedStatusCommentsTest(unittest.TestCase): + @patch.object( + pr_status_comment, + "gh_api", + return_value=[ + {"id": 1, "body": " spoofed"}, + { + "id": 2, + "body": " current", + "performed_via_github_app": {"slug": "opentelemetry-pr-dashboard"}, + }, + { + "id": 3, + "body": " legacy", + "performed_via_github_app": {"slug": "opentelemetry-pr-dashboard"}, + }, + { + "id": 4, + "body": " other app", + "performed_via_github_app": {"slug": "other-app"}, + }, + ], + ) + def test_requires_dashboard_app_identity_and_marker(self, _gh_api: object) -> None: + comments = pr_status_comment.managed_status_comments("open-telemetry/example", 1) + + self.assertEqual([2, 3], [comment["id"] for comment in comments]) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/.github/workflows/pull-request-dashboard-repo.yml b/.github/workflows/pull-request-dashboard-repo.yml index 18a3cfa7b5..e4fafe8d86 100644 --- a/.github/workflows/pull-request-dashboard-repo.yml +++ b/.github/workflows/pull-request-dashboard-repo.yml @@ -18,10 +18,6 @@ on: required: false default: "" type: string - trigger_review_id: - required: false - default: "" - type: string required_approvals: required: false default: 1 @@ -57,75 +53,6 @@ env: DASHBOARD_STATE_BRANCH_PREFIX: otelbot/pull-request-dashboard-state jobs: - post-review-guidance: - if: >- - inputs.pr_number != '' && - inputs.trigger_event == 'pull_request_review' && - inputs.trigger_action == 'submitted' && - inputs.trigger_review_id != '' - concurrency: - group: ${{ github.workflow }}-guidance-${{ inputs.repository }}-${{ inputs.trigger_review_id }} - cancel-in-progress: false - permissions: - contents: read - # The protected environment provides environment-level vars such as - # PR_DASHBOARD_CLIENT_ID. The GitHub docs suggest environment secrets should - # also be available to called reusable workflow jobs, but testing showed - # they resolve empty here. Passing the required secrets explicitly through - # workflow_call.secrets does work, so secrets are part of the reusable - # workflow contract above. - environment: protected - runs-on: cncf-ubuntu-2-8-x86 - steps: - - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 - with: - persist-credentials: false - - - uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 - id: dashboard-token - with: - client-id: ${{ vars.PR_DASHBOARD_CLIENT_ID }} - private-key: ${{ secrets.PR_DASHBOARD_PRIVATE_KEY }} - owner: open-telemetry - repositories: ${{ inputs.repository }} - permission-issues: write - permission-pull-requests: write - - - name: Post review guidance - env: - GH_TOKEN: ${{ steps.dashboard-token.outputs.token }} - PR_NUMBER: ${{ inputs.pr_number }} - REVIEW_ID: ${{ inputs.trigger_review_id }} - REPO: open-telemetry/${{ inputs.repository }} - MARKER: "" - LEGACY_MARKER: "" - run: | - set -euo pipefail - dashboard_url=$(python3 .github/scripts/pull-request-dashboard/publish_dashboard.py --repo "$REPO" --print-dashboard-url) - - if gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments" \ - --jq '.[] | select((.body | contains("'"${MARKER}"'")) or (.body | contains("'"${LEGACY_MARKER}"'"))) | .id' | grep -q .; then - echo "Guidance comment already posted on PR #${PR_NUMBER}; skipping." - exit 0 - fi - - review_comment_count=$(gh api --paginate "repos/${REPO}/pulls/${PR_NUMBER}/reviews/${REVIEW_ID}/comments" --jq '.[].id' | wc -l | tr -d ' ') - if [[ "$review_comment_count" == "0" ]]; then - echo "Review ${REVIEW_ID} has no review comments; skipping guidance comment." - exit 0 - fi - - body=$(cat <- diff --git a/.github/workflows/pull-request-dashboard.yml b/.github/workflows/pull-request-dashboard.yml index f458fa3590..2ff9fe5a59 100644 --- a/.github/workflows/pull-request-dashboard.yml +++ b/.github/workflows/pull-request-dashboard.yml @@ -22,10 +22,9 @@ on: required: false type: string trigger_review_id: - description: Pull request review id that requested the refresh. + description: Deprecated compatibility input; accepted and ignored during webhook rollout. required: false type: string - permissions: contents: read @@ -35,11 +34,12 @@ permissions: # # Dashboard state is loaded live, so only the latest pending webhook refresh is # needed. Manual runs remain separate because they can refresh large -# repositories that webhooks skip. Submitted reviews remain distinct because -# their review id is required to post review guidance. +# repositories that webhooks skip. Submitted reviews can coalesce with generic +# PR refreshes because the live status comment no longer depends on a specific +# review id. concurrency: group: >- - pull-request-dashboard-${{ inputs.repository || 'all-repositories' }}-${{ inputs.pr_number || 'backfill' }}-${{ inputs.trigger_event == 'pull_request_review' && inputs.trigger_action == 'submitted' && inputs.trigger_review_id || github.event_name == 'workflow_dispatch' && (inputs.trigger_event == '' || inputs.trigger_event == 'workflow_dispatch') && 'manual' || 'refresh' }} + pull-request-dashboard-${{ inputs.repository || 'all-repositories' }}-${{ inputs.pr_number || 'backfill' }}-${{ github.event_name == 'workflow_dispatch' && (inputs.trigger_event == '' || inputs.trigger_event == 'workflow_dispatch') && 'manual' || 'refresh' }} cancel-in-progress: false env: @@ -56,7 +56,6 @@ jobs: pr_number: ${{ steps.trigger.outputs.pr_number }} trigger_event: ${{ steps.trigger.outputs.event }} trigger_action: ${{ steps.trigger.outputs.action }} - trigger_review_id: ${{ steps.trigger.outputs.review_id }} dashboard_precondition_met: ${{ steps.dashboard-precondition.outputs.met }} steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 @@ -71,13 +70,11 @@ jobs: PR_FROM_INPUT: ${{ inputs.pr_number }} TRIGGER_EVENT_FROM_INPUT: ${{ inputs.trigger_event }} TRIGGER_ACTION_FROM_INPUT: ${{ inputs.trigger_action }} - TRIGGER_REVIEW_ID_FROM_INPUT: ${{ inputs.trigger_review_id }} run: | set -euo pipefail pr_number="" trigger_event="" trigger_action="" - trigger_review_id="" case "$EVENT_NAME" in schedule) @@ -87,7 +84,6 @@ jobs: pr_number="$PR_FROM_INPUT" trigger_event="${TRIGGER_EVENT_FROM_INPUT:-workflow_dispatch}" trigger_action="$TRIGGER_ACTION_FROM_INPUT" - trigger_review_id="$TRIGGER_REVIEW_ID_FROM_INPUT" ;; esac @@ -102,15 +98,10 @@ jobs: if [[ -n "$trigger_action" ]]; then [[ "$trigger_action" =~ ^[a-z_]{1,32}$ ]] || { echo "bad trigger action: $trigger_action"; exit 1; } fi - if [[ -n "$trigger_review_id" ]]; then - [[ "$trigger_review_id" =~ ^[1-9][0-9]{0,19}$ ]] || { echo "bad review id: $trigger_review_id"; exit 1; } - fi - { echo "pr_number=$pr_number" echo "event=$trigger_event" echo "action=$trigger_action" - echo "review_id=$trigger_review_id" } >> "$GITHUB_OUTPUT" - name: Resolve target repositories @@ -134,22 +125,11 @@ jobs: echo "repository=$(jq -r '.[0].name' <<< "$matrix")" >> "$GITHUB_OUTPUT" echo "$matrix" | jq . - - uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 - if: steps.trigger.outputs.pr_number != '' - id: dashboard-token - with: - client-id: ${{ vars.PR_DASHBOARD_CLIENT_ID }} - private-key: ${{ secrets.PR_DASHBOARD_PRIVATE_KEY }} - owner: open-telemetry - repositories: ${{ steps.targets.outputs.repository }} - permission-issues: read - - name: Check dashboard precondition id: dashboard-precondition env: - GH_TOKEN: ${{ steps.dashboard-token.outputs.token }} TRIGGER_PR_NUMBER: ${{ steps.trigger.outputs.pr_number }} - REPO: open-telemetry/${{ steps.targets.outputs.repository }} + REPO_NAME: ${{ steps.targets.outputs.repository }} run: | set -euo pipefail if [[ -z "$TRIGGER_PR_NUMBER" ]]; then @@ -157,17 +137,21 @@ jobs: exit 0 fi - dashboard_exists=$(python3 .github/scripts/pull-request-dashboard/publish_dashboard.py --repo "$REPO" --check-dashboard-exists) - if [[ "$dashboard_exists" == "true" ]]; then + state_branch="otelbot/pull-request-dashboard-state/${REPO_NAME}" + state_exists=$(python3 .github/scripts/pull-request-dashboard/pr_status_comment.py \ + --repo "$REPO_NAME" \ + --state-branch "$state_branch" \ + --check-state-exists) + if [[ "$state_exists" == "true" ]]; then echo "met=true" >> "$GITHUB_OUTPUT" exit 0 fi - if [[ "$dashboard_exists" == "false" ]]; then - echo "Dashboard issue does not exist yet; skipping targeted refresh." + if [[ "$state_exists" == "false" ]]; then + echo "Dashboard state does not exist yet; skipping targeted refresh." echo "met=false" >> "$GITHUB_OUTPUT" exit 0 fi - echo "unexpected dashboard existence result: $dashboard_exists" >&2 + echo "unexpected dashboard state result: $state_exists" >&2 exit 1 run-repo-dashboard: @@ -185,7 +169,6 @@ jobs: pr_number: ${{ needs.resolve-targets.outputs.pr_number }} trigger_event: ${{ needs.resolve-targets.outputs.trigger_event }} trigger_action: ${{ needs.resolve-targets.outputs.trigger_action }} - trigger_review_id: ${{ needs.resolve-targets.outputs.trigger_review_id }} required_approvals: ${{ matrix.required_approvals || 1 }} approver_teams_json: ${{ toJSON(matrix.approver_teams || fromJSON('[]')) }} slack_channel: ${{ matrix.slack_channel }} diff --git a/pull-request-dashboard/README.md b/pull-request-dashboard/README.md index 2d623b4e9e..2f6b283619 100644 --- a/pull-request-dashboard/README.md +++ b/pull-request-dashboard/README.md @@ -133,18 +133,27 @@ complete, such as when an unrelated commit matches the expected evidence kind. That handoff is deliberate: reviewers can see the activity and respond, while the dashboard avoids leaving an active author indefinitely marked as blocked. -## One-time PR guidance comment - -After the dashboard issue exists, the workflow adds one guidance comment to a -PR the first time a submitted review includes inline comments. A hidden marker -prevents repeat comments on later reviews or workflow runs. - -The guidance asks authors to give each review thread a clear outcome and points -reviewers toward the feedback mechanism with the lifecycle they need. Reviewers -should prefer inline comments for feedback requiring explicit resolution. See +## Live PR status comment + +After the first full dashboard run has populated repository state, each targeted +PR update creates or updates one dashboard-managed status comment on that PR. +The comment shows who has the next action and what the PR is waiting on. When the +author has the next action, it also links to relevant unresolved review +discussions when possible. Draft PRs show that they are waiting for the author +to mark them ready for review. + +A hidden marker lets the workflow update the comment in place. Existing one-time +guidance comments are upgraded rather than duplicated. The comment also asks +authors to give each review discussion a clear outcome, which keeps stale or +ambiguous threads from being routed to the wrong person. + +Reviewers should prefer inline comments for feedback requiring explicit +resolution. See [`RATIONALE.md`](../.github/scripts/pull-request-dashboard/RATIONALE.md#top-level-feedback) for the tradeoffs behind this behavior. +Targeted updates received before the first full dashboard run are ignored. + ## Configuration The target repository GitHub App is installed on each configured repository. From 1322baa9830addaa675af90f6b790520fceead68 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 16 Jul 2026 20:39:03 -0700 Subject: [PATCH 2/7] Address review comment from copilot-pull-request-reviewer: link top-level feedback --- .github/scripts/pull-request-dashboard/dashboard.py | 9 ++++++--- .github/scripts/pull-request-dashboard/github_cli.py | 2 ++ .github/scripts/pull-request-dashboard/test_dashboard.py | 8 +++++--- .../scripts/pull-request-dashboard/test_github_cli.py | 4 ++++ .../pull-request-dashboard/test_top_level_actions.py | 6 ++++++ 5 files changed, 23 insertions(+), 6 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index 6b66c24e09..a456be831e 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -341,6 +341,7 @@ def normalize_events(raw: dict[str, Any], author: str, reviewers: set[str]) -> l timestamp = c.get("updated_at") or c.get("created_at") or "" events.append({ "source_id": c.get("id"), + "discussion_url": c.get("html_url") or "", "kind": "issue-comment", "timestamp": timestamp, "created_timestamp": c.get("created_at") or timestamp, @@ -372,6 +373,7 @@ def normalize_events(raw: dict[str, Any], author: str, reviewers: set[str]) -> l state = r.get("state") or "" events.append({ "source_id": r.get("id"), + "discussion_url": r.get("url") or "", "kind": "review-state", "timestamp": r.get("submitted_at") or "", "updated_timestamp": r.get("updated_at") or r.get("submitted_at") or "", @@ -633,6 +635,7 @@ def derive_top_level_items( "discussion_kind": "top-level-feedback", "source_kind": source_kind, "source_id": event["source_id"], + "discussion_url": event.get("discussion_url") or "", "requester": comment["actor"], "review_state": state or None, "root_timestamp": root_timestamp, @@ -906,10 +909,10 @@ def add_wait_age_facts( def author_action_discussion_urls( - review_threads: list[dict[str, Any]], + discussions: list[dict[str, Any]], pending_actions: dict[str, dict[str, Any]], ) -> list[str]: - by_id = discussions_by_id(review_threads) + by_id = discussions_by_id(discussions) urls: list[str] = [] for discussion_id, entry in pending_actions.items(): action = normalize_discussion_action(entry.get("action") or "") @@ -1075,7 +1078,7 @@ def build_pr_result( route = route_pr(facts, pending_actions, required_approvals) add_wait_age_facts(facts, route, pending_actions) facts["author_action_discussion_urls"] = author_action_discussion_urls( - review_threads, pending_actions + review_threads + top_level_items, pending_actions ) add_reviewers( facts, events, review_threads, top_level_items, pending_actions diff --git a/.github/scripts/pull-request-dashboard/github_cli.py b/.github/scripts/pull-request-dashboard/github_cli.py index fb582a7c6d..8570701883 100644 --- a/.github/scripts/pull-request-dashboard/github_cli.py +++ b/.github/scripts/pull-request-dashboard/github_cli.py @@ -136,6 +136,7 @@ def gh_pr_view(repo: str, number: int) -> dict[str, Any]: } nodes { fullDatabaseId + url body state submittedAt @@ -231,6 +232,7 @@ def fetch_pr_review_data(owner: str, repo_name: str, number: int) -> dict[str, A continue reviews.append({ "id": database_id, + "url": review.get("url") or "", "user": review.get("author") or {}, "state": review.get("state") or "", "body": review.get("body") or "", diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index 7e91fec7a4..d2eee0e8a6 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -44,18 +44,20 @@ def test_group_review_threads_stores_first_comment_url_on_thread(self) -> None: self.assertNotIn("url", threads[0]["comments"][0]) def test_author_action_urls_use_thread_url_and_deduplicate(self) -> None: - threads = [ + discussions = [ {"discussion_id": "thread-1", "discussion_url": "https://example.test/discussion/1"}, {"discussion_id": "thread-2", "discussion_url": "https://example.test/discussion/1"}, + {"discussion_id": "top-level-1", "discussion_url": "https://example.test/discussion/2"}, ] pending_actions = { "thread-1": {"action": "author"}, "thread-2": {"action": "author"}, + "top-level-1": {"action": "author"}, } self.assertEqual( - ["https://example.test/discussion/1"], - author_action_discussion_urls(threads, pending_actions), + ["https://example.test/discussion/1", "https://example.test/discussion/2"], + author_action_discussion_urls(discussions, pending_actions), ) def test_discussion_url_is_excluded_from_classifier_input(self) -> None: diff --git a/.github/scripts/pull-request-dashboard/test_github_cli.py b/.github/scripts/pull-request-dashboard/test_github_cli.py index ad4c1ca3b2..1fec802337 100644 --- a/.github/scripts/pull-request-dashboard/test_github_cli.py +++ b/.github/scripts/pull-request-dashboard/test_github_cli.py @@ -20,6 +20,7 @@ def test_fetch_pr_review_data_normalizes_paginated_reviews_and_metadata(self, gr "nodes": [ { "fullDatabaseId": "4700712792", + "url": "https://example.test/review/4700712792", "author": {"login": "reviewer-1"}, "state": "COMMENTED", "body": "Please clarify this.", @@ -46,6 +47,7 @@ def test_fetch_pr_review_data_normalizes_paginated_reviews_and_metadata(self, gr "nodes": [ { "fullDatabaseId": "5000000000", + "url": "https://example.test/review/5000000000", "author": {"login": "reviewer-2"}, "state": "APPROVED", "body": "Looks good.", @@ -67,6 +69,7 @@ def test_fetch_pr_review_data_normalizes_paginated_reviews_and_metadata(self, gr "reviews": [ { "id": 4700712792, + "url": "https://example.test/review/4700712792", "user": {"login": "reviewer-1"}, "state": "COMMENTED", "body": "Please clarify this.", @@ -75,6 +78,7 @@ def test_fetch_pr_review_data_normalizes_paginated_reviews_and_metadata(self, gr }, { "id": 5000000000, + "url": "https://example.test/review/5000000000", "user": {"login": "reviewer-2"}, "state": "APPROVED", "body": "Looks good.", diff --git a/.github/scripts/pull-request-dashboard/test_top_level_actions.py b/.github/scripts/pull-request-dashboard/test_top_level_actions.py index 0905bd030d..527ad80b8d 100644 --- a/.github/scripts/pull-request-dashboard/test_top_level_actions.py +++ b/.github/scripts/pull-request-dashboard/test_top_level_actions.py @@ -456,6 +456,7 @@ def test_top_level_feedback_gets_stable_individual_items(self) -> None: "issue_comments": [ { "id": 101, + "html_url": "https://example.test/issue-comment/101", "created_at": ROOT_TIMESTAMP, "updated_at": ROOT_TIMESTAMP, "user": {"login": "reviewer"}, @@ -465,6 +466,7 @@ def test_top_level_feedback_gets_stable_individual_items(self) -> None: "reviews": [ { "id": 202, + "url": "https://example.test/review/202", "submitted_at": "2026-07-14T02:00:00Z", "updated_at": "2026-07-14T03:00:00Z", "user": {"login": "reviewer"}, @@ -490,6 +492,10 @@ def test_top_level_feedback_gets_stable_individual_items(self) -> None: [item["discussion_id"] for item in items], ["pr-issue-comment-101", "pr-review-202"], ) + self.assertEqual( + [item["discussion_url"] for item in items], + ["https://example.test/issue-comment/101", "https://example.test/review/202"], + ) self.assertEqual(items[1]["root_timestamp"], "2026-07-14T03:00:00Z") review_event = next(event for event in events if event["kind"] == "review-state") self.assertEqual(review_event["timestamp"], "2026-07-14T02:00:00Z") From dab488b3e2f63d499f3e7caa51f3767da8b60d60 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 16 Jul 2026 21:13:51 -0700 Subject: [PATCH 3/7] Address review comment from copilot-pull-request-reviewer: gate dashboard bootstrap --- .../pull-request-dashboard/RATIONALE.md | 12 ++++- .../pull-request-dashboard/dashboard.py | 21 +++++++- .../pull-request-dashboard/notify_slack.py | 4 -- .../pr_status_comment.py | 12 ++--- .../publish_dashboard.py | 50 ++++++++----------- .../scripts/pull-request-dashboard/state.py | 13 +++-- .../pull-request-dashboard/test_dashboard.py | 25 +++++++++- .../test_notify_slack.py | 40 +-------------- .../test_publish_dashboard.py | 23 +++++++++ .../pull-request-dashboard/test_state.py | 10 ++++ .../workflows/pull-request-dashboard-repo.yml | 25 +++++++++- .github/workflows/pull-request-dashboard.yml | 12 ++--- 12 files changed, 153 insertions(+), 94 deletions(-) create mode 100644 .github/scripts/pull-request-dashboard/test_publish_dashboard.py diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index 3f20b88247..88fc6f1d50 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -64,8 +64,11 @@ the implementation understandable and operationally cheap. do not contend on the same git ref during scheduled and webhook-driven runs. - Updates use `git push --force-with-lease`, so git refs provide the durable compare-and-swap boundary for concurrent same-repository runs. -- A missing repository state branch is bootstrapped by the next non-PR backfill; - targeted PR runs skip until initial dashboard state exists. +- A missing repository state branch is bootstrapped by non-PR backfills. The + dashboard state records when every open non-draft PR has been populated at + least once. Targeted PR runs, dashboard publishing, status comments, and + Slack notifications skip until that initial backfill is complete, so no + partial dashboard is exposed. - Targeted PR runs compute the triggered PR and merge that one PR slot with the latest accepted state on each state-branch compare-and-swap retry. @@ -87,6 +90,11 @@ the implementation understandable and operationally cheap. `backfill-state.json`. The cursor is the last successfully refreshed PR number, and the next run continues after it in sorted PR-number order, wrapping when needed. +- Initial-backfill completion is stored in dashboard state and becomes true in + the same accepted state commit that populates the final missing open + non-draft PR. Once set, it remains true. New PRs do not reset bootstrap; they + appear after their first targeted refresh or backfill, while existing + dashboard entries and Slack reminders continue normally. - A selected PR failure stops the run without advancing the cursor. This can temporarily block later PRs, but it keeps the scheduled workflow failure issue open and pointing at the blocked backfill until the failure is fixed. diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index a456be831e..656d46154e 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -177,7 +177,9 @@ prune_classification_cache, ) from state import ( + INITIAL_BACKFILL_COMPLETE_KEY, empty_state, + initial_backfill_complete, load_dashboard_state_cache, load_backfill_state, results_from_dashboard_state, @@ -1238,6 +1240,18 @@ def dashboard_state_pr_numbers(state: dict[str, Any]) -> set[int]: return numbers +def complete_initial_backfill_if_ready( + state: dict[str, Any], + open_pr_numbers: set[int], +) -> bool: + if initial_backfill_complete(state): + return False + if not open_pr_numbers.issubset(dashboard_state_pr_numbers(state)): + return False + state[INITIAL_BACKFILL_COMPLETE_KEY] = True + return True + + def backfill_cursor_pr_number(backfill_state: dict[str, Any]) -> int | None: cursor = backfill_state.get("cursor") or {} if not isinstance(cursor, dict): @@ -1535,6 +1549,7 @@ def update_dashboard_for_backfill(args: argparse.Namespace, state_dir: Path) -> if not selection.selected_prs: def save_current_dashboard_state() -> int: dashboard_state = load_dashboard_state_cache() or empty_state() + complete_initial_backfill_if_ready(dashboard_state, open_non_draft_pr_numbers) return save_dashboard_update_state(args, dashboard_state, False) return state_branch.push_state_changes( @@ -1566,10 +1581,14 @@ def update_selected_pr(pr_summary: dict[str, Any] = pr_summary) -> int: ) if not dashboard_state_unchanged and reject_failed_dashboard_result(calculation.trigger_pr_result): return 1 + initial_backfill_completed = complete_initial_backfill_if_ready( + calculation.dashboard_state, + open_non_draft_pr_numbers, + ) status = save_dashboard_update_state( args, calculation.dashboard_state, - dashboard_state_unchanged, + dashboard_state_unchanged and not initial_backfill_completed, ) if status != 0: return status diff --git a/.github/scripts/pull-request-dashboard/notify_slack.py b/.github/scripts/pull-request-dashboard/notify_slack.py index a2bb9b4ed3..aeed3fbb1d 100644 --- a/.github/scripts/pull-request-dashboard/notify_slack.py +++ b/.github/scripts/pull-request-dashboard/notify_slack.py @@ -60,10 +60,6 @@ def notify_slack_from_state( for number, result in results.items(): result["pr_title"] = current_prs.get(number, {}).get("title") or "" - if not open_pr_numbers.issubset(results): - print("dashboard backfill incomplete; skipping Slack notifications", file=sys.stderr) - return [] - saved_notifications = load_notifications() updated_notifications, delivery_errors = next_notifications( diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index 9bb13badc1..b51cb85e0e 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -8,7 +8,7 @@ from typing import Any from github_cli import detect_repo, gh_api, normalize_repo, repo_state_key, run_gh -from state import load_dashboard_state_cache, set_state_dir +from state import initial_backfill_complete, load_dashboard_state_cache, set_state_dir import state_branch @@ -163,19 +163,19 @@ def main() -> int: parser.add_argument("--state-branch", required=True, help="git branch used for workflow state") parser.add_argument("--pr-number", type=int, help="pull request to update") parser.add_argument( - "--check-state-exists", + "--check-initial-backfill-complete", action="store_true", - help='print "true" when accepted dashboard state exists, otherwise "false"', + help='print "true" when the initial dashboard backfill is complete, otherwise "false"', ) args = parser.parse_args() repo = normalize_repo(args.repo) if args.repo else detect_repo() dashboard_state = load_accepted_dashboard_state(repo, args.state_branch) - if args.check_state_exists: - print("true" if dashboard_state is not None else "false") + if args.check_initial_backfill_complete: + print("true" if initial_backfill_complete(dashboard_state) else "false") return 0 if args.pr_number is None: - parser.error("--pr-number is required unless --check-state-exists is set") + parser.error("--pr-number is required unless --check-initial-backfill-complete is set") if dashboard_state is None: print("dashboard result state not found; skipping PR status comment", file=sys.stderr) return 0 diff --git a/.github/scripts/pull-request-dashboard/publish_dashboard.py b/.github/scripts/pull-request-dashboard/publish_dashboard.py index 25cb48b336..e2b105b4fa 100644 --- a/.github/scripts/pull-request-dashboard/publish_dashboard.py +++ b/.github/scripts/pull-request-dashboard/publish_dashboard.py @@ -6,10 +6,16 @@ import argparse import sys from pathlib import Path +from typing import Any from github_cli import detect_repo, gh_graphql, list_open_prs, normalize_repo, repo_state_key, run_gh from render import render_pr_tables -from state import dashboard_markdown_path, load_dashboard_state_cache, results_from_dashboard_state, set_state_dir +from state import ( + dashboard_markdown_path, + load_dashboard_state_cache, + results_from_dashboard_state, + set_state_dir, +) import state_branch @@ -64,13 +70,6 @@ def find_dashboard_issue(repo: str) -> int | None: after = page_info["endCursor"] or "" -def dashboard_issue_url(repo: str) -> str: - number = find_dashboard_issue(repo) - if number is None: - raise RuntimeError(f"dashboard issue not found in {repo}") - return f"https://github.com/{repo}/issues/{number}" - - def ensure_dashboard_label(repo: str) -> None: label = run_gh([ "gh", @@ -139,6 +138,17 @@ def publish_dashboard(repo: str, dashboard_body: Path) -> None: ]) +def publishable_prs( + prs: list[dict[str, Any]], + results: dict[int, dict[str, Any]], +) -> list[dict[str, Any]]: + return [ + pr + for pr in prs + if pr.get("isDraft") or pr.get("number") in results + ] + + def render_dashboard_markdown(repo: str, large_repo: bool) -> Path: dashboard_state = load_dashboard_state_cache() if dashboard_state is None: @@ -148,7 +158,7 @@ def render_dashboard_markdown(repo: str, large_repo: bool) -> Path: open_non_draft_pr_numbers = {p["number"] for p in prs if not p.get("isDraft")} results = results_from_dashboard_state(dashboard_state, open_non_draft_pr_numbers) md = render_pr_tables( - prs, + publishable_prs(prs, results), results, max_rows_per_section=LARGE_REPO_MAX_ROWS_PER_SECTION if large_repo else None, skip_drafts=large_repo, @@ -172,18 +182,9 @@ def render_dashboard_markdown(repo: str, large_repo: bool) -> Path: def main() -> int: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--repo", help="target repository name, e.g. opentelemetry-java-instrumentation") - parser.add_argument( - "--print-dashboard-url", - action="store_true", - help="print the dashboard issue URL and exit", - ) - parser.add_argument( - "--check-dashboard-exists", - action="store_true", - help="print \"true\" if the dashboard issue exists, otherwise \"false\", and exit", - ) parser.add_argument( "--state-branch", + required=True, help="git branch used for workflow state", ) parser.add_argument( @@ -194,17 +195,6 @@ def main() -> int: args = parser.parse_args() repo = normalize_repo(args.repo) if args.repo else detect_repo() - if args.check_dashboard_exists: - print("true" if find_dashboard_issue(repo) is not None else "false") - return 0 - - if args.print_dashboard_url: - print(dashboard_issue_url(repo)) - return 0 - - if not args.state_branch: - parser.error("--state-branch is required unless --print-dashboard-url or --check-dashboard-exists is set") - with state_branch.temporary_state_dir() as state_dir: set_state_dir(state_dir / repo_state_key(repo)) state_branch.configure_git() diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index 39eb59a163..5c396eda6a 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -13,6 +13,7 @@ DASHBOARD_STATE_VERSION = 4 BACKFILL_STATE_VERSION = 3 NOTIFICATION_STATE_VERSION = 3 +INITIAL_BACKFILL_COMPLETE_KEY = "initial_backfill_complete" _state_dir: Path | None = None @@ -47,6 +48,10 @@ def empty_state() -> dict[str, Any]: return {"version": DASHBOARD_STATE_VERSION, "prs": {}} +def initial_backfill_complete(state: dict[str, Any] | None) -> bool: + return bool(state and state.get(INITIAL_BACKFILL_COMPLETE_KEY) is True) + + def empty_backfill_state() -> dict[str, Any]: return {"version": BACKFILL_STATE_VERSION, "cursor": {}} @@ -204,7 +209,7 @@ def update_dashboard_state_for_pr( prs.pop(key, None) else: prs[key] = stored_result(result) - return { - "version": DASHBOARD_STATE_VERSION, - "prs": prs, - } + updated = dict(state) + updated["version"] = DASHBOARD_STATE_VERSION + updated["prs"] = prs + return updated diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index d2eee0e8a6..6a344ae93b 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -3,7 +3,11 @@ import unittest from classification import discussion_prompt_input -from dashboard import author_action_discussion_urls, group_review_threads +from dashboard import ( + author_action_discussion_urls, + complete_initial_backfill_if_ready, + group_review_threads, +) class ReviewThreadDiscussionUrlTest(unittest.TestCase): @@ -71,5 +75,24 @@ def test_discussion_url_is_excluded_from_classifier_input(self) -> None: self.assertNotIn("discussion_url", prompt_input) +class InitialBackfillCompletionTest(unittest.TestCase): + def test_marks_complete_only_after_all_open_prs_are_cached(self) -> None: + state = {"prs": {"1": {}}} + + self.assertFalse(complete_initial_backfill_if_ready(state, {1, 2})) + self.assertNotIn("initial_backfill_complete", state) + + state["prs"]["2"] = {} + self.assertTrue(complete_initial_backfill_if_ready(state, {1, 2})) + self.assertTrue(state["initial_backfill_complete"]) + self.assertFalse(complete_initial_backfill_if_ready(state, {1, 2})) + + def test_empty_repository_completes_initial_backfill(self) -> None: + state = {"prs": {}} + + self.assertTrue(complete_initial_backfill_if_ready(state, set())) + self.assertTrue(state["initial_backfill_complete"]) + + if __name__ == "__main__": unittest.main() \ No newline at end of file diff --git a/.github/scripts/pull-request-dashboard/test_notify_slack.py b/.github/scripts/pull-request-dashboard/test_notify_slack.py index 4384f8e2e7..0f758de77e 100644 --- a/.github/scripts/pull-request-dashboard/test_notify_slack.py +++ b/.github/scripts/pull-request-dashboard/test_notify_slack.py @@ -11,44 +11,7 @@ class NotifySlackTest(unittest.TestCase): @patch("notify_slack.load_notifications") @patch("notify_slack.list_open_prs") @patch("notify_slack.load_dashboard_state_cache") - def test_partial_backfill_skips_notifications_without_changing_state( - self, - load_dashboard_state_cache, - list_open_prs, - load_notifications, - save_notifications, - ) -> None: - load_dashboard_state_cache.return_value = {"prs": {}} - list_open_prs.return_value = [ - {"number": 2, "isDraft": False, "title": "Open PR"}, - {"number": 3, "isDraft": True, "title": "Draft PR"}, - ] - load_notifications.return_value = { - "2": { - "last_notified_at": "2026-07-14T03:00:00Z", - "last_notification_kind": "initial", - }, - "3": { - "last_notified_at": "2026-07-14T03:00:00Z", - "last_notification_kind": "initial", - }, - "4": { - "last_notified_at": "2026-07-14T03:00:00Z", - "last_notification_kind": "initial", - }, - } - - errors = notify_slack_from_state("owner/repo", None, None, None) - - self.assertEqual(errors, []) - load_notifications.assert_not_called() - save_notifications.assert_not_called() - - @patch("notify_slack.save_notifications") - @patch("notify_slack.load_notifications") - @patch("notify_slack.list_open_prs") - @patch("notify_slack.load_dashboard_state_cache") - def test_complete_backfill_resumes_notifications_and_prunes_closed_state( + def test_uncached_pr_does_not_pause_notifications_and_closed_state_is_pruned( self, load_dashboard_state_cache, list_open_prs, @@ -66,6 +29,7 @@ def test_complete_backfill_resumes_notifications_and_prunes_closed_state( } list_open_prs.return_value = [ {"number": 2, "isDraft": False, "title": "Open PR"}, + {"number": 5, "isDraft": False, "title": "Not cached yet"}, {"number": 3, "isDraft": True, "title": "Draft PR"}, ] load_notifications.return_value = { diff --git a/.github/scripts/pull-request-dashboard/test_publish_dashboard.py b/.github/scripts/pull-request-dashboard/test_publish_dashboard.py new file mode 100644 index 0000000000..d2c562dbf0 --- /dev/null +++ b/.github/scripts/pull-request-dashboard/test_publish_dashboard.py @@ -0,0 +1,23 @@ +from __future__ import annotations + +import unittest + +from publish_dashboard import publishable_prs + + +class PublishablePrsTest(unittest.TestCase): + def test_omits_uncached_non_draft_prs_and_retains_drafts(self) -> None: + prs = [ + {"number": 1, "isDraft": False}, + {"number": 2, "isDraft": False}, + {"number": 3, "isDraft": True}, + ] + + self.assertEqual( + publishable_prs(prs, {1: {"route": "author"}}), + [prs[0], prs[2]], + ) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/.github/scripts/pull-request-dashboard/test_state.py b/.github/scripts/pull-request-dashboard/test_state.py index 8425fa4902..f8076c7dee 100644 --- a/.github/scripts/pull-request-dashboard/test_state.py +++ b/.github/scripts/pull-request-dashboard/test_state.py @@ -12,6 +12,7 @@ NOTIFICATION_STATE_VERSION, backfill_state_path, dashboard_state_path, + empty_state, load_backfill_state, load_dashboard_state_cache, load_state_file, @@ -20,6 +21,7 @@ save_state_file, save_notifications, stored_result, + update_dashboard_state_for_pr, ) @@ -93,6 +95,14 @@ def test_backfill_state_preserves_version_three_cursor(self) -> None: }, ) + def test_targeted_update_preserves_initial_backfill_marker(self) -> None: + state = empty_state() + state["initial_backfill_complete"] = True + + updated = update_dashboard_state_for_pr(state, 123, None) + + self.assertTrue(updated["initial_backfill_complete"]) + def test_notification_state_write_ignores_dashboard_version(self) -> None: with ( tempfile.TemporaryDirectory() as temp_dir, diff --git a/.github/workflows/pull-request-dashboard-repo.yml b/.github/workflows/pull-request-dashboard-repo.yml index e4fafe8d86..2462b22e69 100644 --- a/.github/workflows/pull-request-dashboard-repo.yml +++ b/.github/workflows/pull-request-dashboard-repo.yml @@ -61,6 +61,8 @@ jobs: contents: write environment: protected runs-on: cncf-ubuntu-2-8-x86 + outputs: + initial_backfill_complete: ${{ steps.bootstrap-status.outputs.complete }} steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: @@ -132,9 +134,25 @@ jobs: fi python3 .github/scripts/pull-request-dashboard/dashboard.py "${dashboard_args[@]}" + - name: Read dashboard bootstrap status + id: bootstrap-status + env: + REPO_NAME: ${{ inputs.repository }} + run: | + set -euo pipefail + state_branch="${DASHBOARD_STATE_BRANCH_PREFIX}/${REPO_NAME}" + complete=$(python3 .github/scripts/pull-request-dashboard/pr_status_comment.py \ + --state-branch "$state_branch" \ + --repo "$REPO_NAME" \ + --check-initial-backfill-complete) + echo "complete=$complete" >> "$GITHUB_OUTPUT" + update-pr-status-comment: needs: update-dashboard - if: needs.update-dashboard.result == 'success' && inputs.pr_number != '' + if: >- + needs.update-dashboard.result == 'success' && + needs.update-dashboard.outputs.initial_backfill_complete == 'true' && + inputs.pr_number != '' concurrency: group: ${{ github.workflow }}-status-${{ inputs.repository }}-${{ inputs.pr_number }} cancel-in-progress: false @@ -174,6 +192,7 @@ jobs: needs: update-dashboard if: >- needs.update-dashboard.result == 'success' && + needs.update-dashboard.outputs.initial_backfill_complete == 'true' && (inputs.trigger_event == 'schedule' || inputs.pr_number != '') concurrency: group: ${{ github.workflow }}-notify-${{ inputs.repository }}-${{ inputs.pr_number || 'backfill' }} @@ -217,7 +236,9 @@ jobs: publish-dashboard: needs: update-dashboard - if: needs.update-dashboard.result == 'success' + if: >- + needs.update-dashboard.result == 'success' && + needs.update-dashboard.outputs.initial_backfill_complete == 'true' concurrency: group: ${{ github.workflow }}-publish-${{ inputs.repository }} cancel-in-progress: true diff --git a/.github/workflows/pull-request-dashboard.yml b/.github/workflows/pull-request-dashboard.yml index 2ff9fe5a59..5c2fdcf49e 100644 --- a/.github/workflows/pull-request-dashboard.yml +++ b/.github/workflows/pull-request-dashboard.yml @@ -138,20 +138,20 @@ jobs: fi state_branch="otelbot/pull-request-dashboard-state/${REPO_NAME}" - state_exists=$(python3 .github/scripts/pull-request-dashboard/pr_status_comment.py \ + initial_backfill_complete=$(python3 .github/scripts/pull-request-dashboard/pr_status_comment.py \ --repo "$REPO_NAME" \ --state-branch "$state_branch" \ - --check-state-exists) - if [[ "$state_exists" == "true" ]]; then + --check-initial-backfill-complete) + if [[ "$initial_backfill_complete" == "true" ]]; then echo "met=true" >> "$GITHUB_OUTPUT" exit 0 fi - if [[ "$state_exists" == "false" ]]; then - echo "Dashboard state does not exist yet; skipping targeted refresh." + if [[ "$initial_backfill_complete" == "false" ]]; then + echo "Initial dashboard backfill is incomplete; skipping targeted refresh." echo "met=false" >> "$GITHUB_OUTPUT" exit 0 fi - echo "unexpected dashboard state result: $state_exists" >&2 + echo "unexpected initial backfill result: $initial_backfill_complete" >&2 exit 1 run-repo-dashboard: From 22591468c1a66166766182dca8f90f55bc3f9fcf Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Fri, 17 Jul 2026 06:48:32 -0700 Subject: [PATCH 4/7] Simplify pull request dashboard workflow state --- .../pull-request-dashboard/WEBHOOK_SETUP.md | 5 +-- .../pull-request-dashboard/dashboard.py | 17 ++++++++- .../netlify/functions/github-webhook.js | 9 ----- .../scripts/pull-request-dashboard/state.py | 34 ++++++++++++----- .../pull-request-dashboard/test_dashboard.py | 24 +++++++++++- .../test_github_webhook.js | 38 ++++++++----------- .../pull-request-dashboard/test_state.py | 32 ++++++++++++++-- .../workflows/pull-request-dashboard-repo.yml | 22 ++--------- .github/workflows/pull-request-dashboard.yml | 13 ------- 9 files changed, 112 insertions(+), 82 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/WEBHOOK_SETUP.md b/.github/scripts/pull-request-dashboard/WEBHOOK_SETUP.md index afc33dcf30..6255b0f043 100644 --- a/.github/scripts/pull-request-dashboard/WEBHOOK_SETUP.md +++ b/.github/scripts/pull-request-dashboard/WEBHOOK_SETUP.md @@ -87,7 +87,7 @@ Event rationale: | ----- | ---------------- | | Check suite | Refreshes CI status when checks are requested, rerequested, or completed. | | Pull request | Refreshes dashboard rows when PR state, draft status, labels, assignees, branches, or metadata change. | -| Issue comment | Refreshes PR conversation state when PR issue comments are created, edited, or deleted. Events for the dashboard App's own marked status comment are ignored. | +| Issue comment | Refreshes PR conversation state when PR issue comments are created, edited, or deleted. Events generated by the dashboard App changing its own comments are ignored. | | Pull request review | Refreshes approval/change-request state and the live PR status comment. | | Pull request review comment | Refreshes inline review-comment discussion state. | | Pull request review thread | Refreshes when inline review threads are resolved or unresolved. | @@ -154,8 +154,7 @@ The webhook bridge should dispatch `pull-request-dashboard.yml` in { "repository": "opentelemetry-java-instrumentation", "pr_number": "12345", - "trigger_event": "pull_request_review_comment", - "trigger_action": "created" + "trigger_event": "pull_request_review_comment" } ``` diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index 656d46154e..e92fde79a6 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -16,6 +16,7 @@ --approver-team TEAM [--approver-team TEAM] [--pr-number N] + [--github-output PATH] [--model NAME] Architecture overview @@ -1611,6 +1612,12 @@ def update_dashboard_via_state_branch(args: argparse.Namespace, state_dir: Path) return update_dashboard_for_pr_number(args, state_dir) +def write_initial_backfill_output(github_output: Path) -> None: + complete = initial_backfill_complete(load_dashboard_state_cache()) + with github_output.open("a", encoding="utf-8") as output: + output.write(f"initial_backfill_complete={'true' if complete else 'false'}\n") + + def main() -> int: parser = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) parser.add_argument( @@ -1633,13 +1640,21 @@ def main() -> int: help="minimum non-bot approvals needed before a PR can route to maintainers", ) parser.add_argument("--model", default=DEFAULT_MODEL, help=f"copilot model (default: {DEFAULT_MODEL})") + parser.add_argument( + "--github-output", + type=Path, + help="append initial_backfill_complete to this GitHub Actions output file", + ) args = parser.parse_args() if args.required_approvals < 1: parser.error("--required-approvals must be at least 1") with state_branch.temporary_state_dir() as state_dir: repo_key = repo_state_key(args.repo) if args.repo else repo_state_key(detect_repo()) set_state_dir(state_dir / repo_key) - return update_dashboard_via_state_branch(args, state_dir) + status = update_dashboard_via_state_branch(args, state_dir) + if status == 0 and args.github_output: + write_initial_backfill_output(args.github_output) + return status if __name__ == "__main__": diff --git a/.github/scripts/pull-request-dashboard/netlify/functions/github-webhook.js b/.github/scripts/pull-request-dashboard/netlify/functions/github-webhook.js index 114fa1a42f..13df9bdfd9 100644 --- a/.github/scripts/pull-request-dashboard/netlify/functions/github-webhook.js +++ b/.github/scripts/pull-request-dashboard/netlify/functions/github-webhook.js @@ -7,11 +7,6 @@ const WORKFLOW_REPOSITORY = "shared-workflows"; const WORKFLOW_ID = "pull-request-dashboard.yml"; const WORKFLOW_REF = "main"; const DASHBOARD_APP_SLUG = "opentelemetry-pr-dashboard"; -const DASHBOARD_COMMENT_MARKERS = [ - "", - "", - "", -]; const ALLOWED_ACTIONS = { check_suite: new Set(["completed", "requested", "rerequested"]), @@ -96,7 +91,6 @@ async function handle(event) { repository: repository.name, pr_number: String(prNumber), trigger_event: eventName, - trigger_action: action, }); return response(202, { @@ -104,7 +98,6 @@ async function handle(event) { repository: repository.fullName, pr_number: prNumber, trigger_event: eventName, - trigger_action: action, }); } @@ -114,12 +107,10 @@ function isDashboardSelfTriggeredCommentEvent(eventName, payload) { } const comment = payload.comment || {}; const app = comment.performed_via_github_app || {}; - const body = comment.body || ""; const commentAuthor = comment.user || {}; const sender = payload.sender || {}; return ( app.slug === DASHBOARD_APP_SLUG && - DASHBOARD_COMMENT_MARKERS.some((marker) => body.includes(marker)) && Boolean(sender.id) && sender.id === commentAuthor.id ); diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index 5c396eda6a..fdd895f9e8 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -45,7 +45,11 @@ def dashboard_markdown_path() -> Path: def empty_state() -> dict[str, Any]: - return {"version": DASHBOARD_STATE_VERSION, "prs": {}} + return { + "version": DASHBOARD_STATE_VERSION, + INITIAL_BACKFILL_COMPLETE_KEY: False, + "prs": {}, + } def initial_backfill_complete(state: dict[str, Any] | None) -> bool: @@ -109,14 +113,22 @@ def save_backfill_state(state: dict[str, Any]) -> None: def load_dashboard_state_cache() -> dict[str, Any] | None: state = load_state_file(dashboard_state_path(), DASHBOARD_STATE_VERSION) - if state is not None and not isinstance(state.get("prs"), dict): - state["prs"] = {} - return state + if state is None: + return None + prs = state.get("prs") + return { + "version": DASHBOARD_STATE_VERSION, + INITIAL_BACKFILL_COMPLETE_KEY: initial_backfill_complete(state), + "prs": prs if isinstance(prs, dict) else {}, + } def save_dashboard_state_cache(state: dict[str, Any]) -> None: - stored = dict(state) - stored.setdefault("prs", {}) + prs = state.get("prs") + stored = { + INITIAL_BACKFILL_COMPLETE_KEY: initial_backfill_complete(state), + "prs": prs if isinstance(prs, dict) else {}, + } save_state_file(dashboard_state_path(), stored, DASHBOARD_STATE_VERSION) @@ -194,6 +206,7 @@ def results_from_dashboard_state(state: dict[str, Any], open_pr_numbers: set[int def dashboard_state_from_results(results: dict[int, dict[str, Any]]) -> dict[str, Any]: return { "version": DASHBOARD_STATE_VERSION, + INITIAL_BACKFILL_COMPLETE_KEY: False, "prs": {str(number): stored_result(result) for number, result in sorted(results.items())}, } @@ -209,7 +222,8 @@ def update_dashboard_state_for_pr( prs.pop(key, None) else: prs[key] = stored_result(result) - updated = dict(state) - updated["version"] = DASHBOARD_STATE_VERSION - updated["prs"] = prs - return updated + return { + "version": DASHBOARD_STATE_VERSION, + INITIAL_BACKFILL_COMPLETE_KEY: initial_backfill_complete(state), + "prs": prs, + } diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index 6a344ae93b..eb278bb5f7 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -1,12 +1,16 @@ from __future__ import annotations +from pathlib import Path +import tempfile import unittest +from unittest.mock import patch from classification import discussion_prompt_input from dashboard import ( author_action_discussion_urls, complete_initial_backfill_if_ready, group_review_threads, + write_initial_backfill_output, ) @@ -77,10 +81,10 @@ def test_discussion_url_is_excluded_from_classifier_input(self) -> None: class InitialBackfillCompletionTest(unittest.TestCase): def test_marks_complete_only_after_all_open_prs_are_cached(self) -> None: - state = {"prs": {"1": {}}} + state = {"initial_backfill_complete": False, "prs": {"1": {}}} self.assertFalse(complete_initial_backfill_if_ready(state, {1, 2})) - self.assertNotIn("initial_backfill_complete", state) + self.assertFalse(state["initial_backfill_complete"]) state["prs"]["2"] = {} self.assertTrue(complete_initial_backfill_if_ready(state, {1, 2})) @@ -93,6 +97,22 @@ def test_empty_repository_completes_initial_backfill(self) -> None: self.assertTrue(complete_initial_backfill_if_ready(state, set())) self.assertTrue(state["initial_backfill_complete"]) + def test_writes_initial_backfill_status_to_github_output(self) -> None: + with tempfile.TemporaryDirectory() as temp_dir: + for name, state, expected in ( + ("incomplete", None, "false"), + ("complete", {"initial_backfill_complete": True, "prs": {}}, "true"), + ): + with self.subTest(name=name): + output_path = Path(temp_dir) / name + with patch("dashboard.load_dashboard_state_cache", return_value=state): + write_initial_backfill_output(output_path) + + self.assertEqual( + f"initial_backfill_complete={expected}\n", + output_path.read_text(encoding="utf-8"), + ) + if __name__ == "__main__": unittest.main() \ No newline at end of file diff --git a/.github/scripts/pull-request-dashboard/test_github_webhook.js b/.github/scripts/pull-request-dashboard/test_github_webhook.js index 7c67f45fc9..207dbcbe35 100644 --- a/.github/scripts/pull-request-dashboard/test_github_webhook.js +++ b/.github/scripts/pull-request-dashboard/test_github_webhook.js @@ -6,24 +6,18 @@ const { isDashboardSelfTriggeredCommentEvent } = require("./netlify/functions/gi const dashboardApp = { slug: "opentelemetry-pr-dashboard" }; const dashboardActor = { id: 1 }; -test("recognizes current and legacy dashboard-managed comments", () => { - for (const marker of [ - "", - "", - "", - ]) { - assert.equal(isDashboardSelfTriggeredCommentEvent("issue_comment", { - comment: { - body: `${marker} body`, - performed_via_github_app: dashboardApp, - user: dashboardActor, - }, - sender: dashboardActor, - }), true); - } +test("recognizes comments performed by the dashboard app", () => { + assert.equal(isDashboardSelfTriggeredCommentEvent("issue_comment", { + comment: { + body: "ordinary comment without a dashboard marker", + performed_via_github_app: dashboardApp, + user: dashboardActor, + }, + sender: dashboardActor, + }), true); }); -test("rejects spoofed markers and unrelated dashboard app comments", () => { +test("rejects comments not performed by the dashboard app", () => { assert.equal(isDashboardSelfTriggeredCommentEvent("issue_comment", { comment: { body: " spoofed", @@ -33,15 +27,16 @@ test("rejects spoofed markers and unrelated dashboard app comments", () => { }), false); assert.equal(isDashboardSelfTriggeredCommentEvent("issue_comment", { comment: { - body: "ordinary comment", - performed_via_github_app: dashboardApp, + performed_via_github_app: { slug: "other-app" }, user: dashboardActor, }, sender: dashboardActor, }), false); +}); + +test("does not filter non-comment events from the dashboard app", () => { assert.equal(isDashboardSelfTriggeredCommentEvent("pull_request", { comment: { - body: " body", performed_via_github_app: dashboardApp, user: dashboardActor, }, @@ -49,13 +44,12 @@ test("rejects spoofed markers and unrelated dashboard app comments", () => { }), false); }); -test("allows dashboard-managed comment events triggered by another actor", () => { +test("allows dashboard comment events triggered by another actor", () => { assert.equal(isDashboardSelfTriggeredCommentEvent("issue_comment", { comment: { - body: " body", performed_via_github_app: dashboardApp, user: dashboardActor, }, sender: { id: 2 }, }), false); -}); \ No newline at end of file +}); diff --git a/.github/scripts/pull-request-dashboard/test_state.py b/.github/scripts/pull-request-dashboard/test_state.py index f8076c7dee..b655660d72 100644 --- a/.github/scripts/pull-request-dashboard/test_state.py +++ b/.github/scripts/pull-request-dashboard/test_state.py @@ -19,6 +19,7 @@ load_notifications, notification_state_path, save_state_file, + save_dashboard_state_cache, save_notifications, stored_result, update_dashboard_state_for_pr, @@ -48,7 +49,7 @@ def test_versioned_state_helpers_preserve_arbitrary_payloads(self) -> None: def test_state_specific_loaders_own_payload_defaults(self) -> None: with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)): dashboard_state_path().write_text( - json.dumps({"version": DASHBOARD_STATE_VERSION}), + json.dumps({"version": DASHBOARD_STATE_VERSION, "unknown": "discard me"}), encoding="utf-8", ) notification_state_path().write_text( @@ -62,7 +63,11 @@ def test_state_specific_loaders_own_payload_defaults(self) -> None: self.assertEqual( load_dashboard_state_cache(), - {"version": DASHBOARD_STATE_VERSION, "prs": {}}, + { + "version": DASHBOARD_STATE_VERSION, + "initial_backfill_complete": False, + "prs": {}, + }, ) self.assertEqual(load_notifications(), {}) self.assertEqual( @@ -70,6 +75,19 @@ def test_state_specific_loaders_own_payload_defaults(self) -> None: {"version": BACKFILL_STATE_VERSION, "cursor": {}}, ) + def test_dashboard_state_save_writes_explicit_shape(self) -> None: + with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)): + save_dashboard_state_cache({"unknown": "discard me"}) + + self.assertEqual( + json.loads(dashboard_state_path().read_text(encoding="utf-8")), + { + "version": DASHBOARD_STATE_VERSION, + "initial_backfill_complete": False, + "prs": {}, + }, + ) + def test_notification_state_version_is_independent(self) -> None: self.assertEqual(BACKFILL_STATE_VERSION, 3) self.assertEqual(NOTIFICATION_STATE_VERSION, 3) @@ -98,10 +116,18 @@ def test_backfill_state_preserves_version_three_cursor(self) -> None: def test_targeted_update_preserves_initial_backfill_marker(self) -> None: state = empty_state() state["initial_backfill_complete"] = True + state["unknown"] = "discard me" updated = update_dashboard_state_for_pr(state, 123, None) - self.assertTrue(updated["initial_backfill_complete"]) + self.assertEqual( + updated, + { + "version": DASHBOARD_STATE_VERSION, + "initial_backfill_complete": True, + "prs": {}, + }, + ) def test_notification_state_write_ignores_dashboard_version(self) -> None: with ( diff --git a/.github/workflows/pull-request-dashboard-repo.yml b/.github/workflows/pull-request-dashboard-repo.yml index 2462b22e69..ae053f0f9b 100644 --- a/.github/workflows/pull-request-dashboard-repo.yml +++ b/.github/workflows/pull-request-dashboard-repo.yml @@ -14,10 +14,6 @@ on: required: false default: "" type: string - trigger_action: - required: false - default: "" - type: string required_approvals: required: false default: 1 @@ -62,7 +58,7 @@ jobs: environment: protected runs-on: cncf-ubuntu-2-8-x86 outputs: - initial_backfill_complete: ${{ steps.bootstrap-status.outputs.complete }} + initial_backfill_complete: ${{ steps.dashboard-update.outputs.initial_backfill_complete }} steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: @@ -112,6 +108,7 @@ jobs: echo "$GITHUB_WORKSPACE/.github/scripts/pull-request-dashboard/node_modules/.bin" >> "$GITHUB_PATH" - name: Generate and update dashboard state + id: dashboard-update env: TRIGGER_PR_NUMBER: ${{ inputs.pr_number }} GH_TOKEN: ${{ steps.dashboard-token.outputs.token }} @@ -124,7 +121,7 @@ jobs: run: | set -euo pipefail state_branch="${DASHBOARD_STATE_BRANCH_PREFIX}/${REPO_NAME}" - dashboard_args=(--state-branch "$state_branch" --repo "$REPO_NAME") + dashboard_args=(--state-branch "$state_branch" --repo "$REPO_NAME" --github-output "$GITHUB_OUTPUT") dashboard_args+=(--required-approvals "$REQUIRED_APPROVALS") for team in $(jq -r '.[]' <<< "$APPROVER_TEAMS_JSON"); do dashboard_args+=(--approver-team "$team") @@ -134,19 +131,6 @@ jobs: fi python3 .github/scripts/pull-request-dashboard/dashboard.py "${dashboard_args[@]}" - - name: Read dashboard bootstrap status - id: bootstrap-status - env: - REPO_NAME: ${{ inputs.repository }} - run: | - set -euo pipefail - state_branch="${DASHBOARD_STATE_BRANCH_PREFIX}/${REPO_NAME}" - complete=$(python3 .github/scripts/pull-request-dashboard/pr_status_comment.py \ - --state-branch "$state_branch" \ - --repo "$REPO_NAME" \ - --check-initial-backfill-complete) - echo "complete=$complete" >> "$GITHUB_OUTPUT" - update-pr-status-comment: needs: update-dashboard if: >- diff --git a/.github/workflows/pull-request-dashboard.yml b/.github/workflows/pull-request-dashboard.yml index 5c2fdcf49e..99716a2090 100644 --- a/.github/workflows/pull-request-dashboard.yml +++ b/.github/workflows/pull-request-dashboard.yml @@ -17,10 +17,6 @@ on: description: Event that requested the refresh. required: false type: string - trigger_action: - description: Event action that requested the refresh. - required: false - type: string trigger_review_id: description: Deprecated compatibility input; accepted and ignored during webhook rollout. required: false @@ -55,7 +51,6 @@ jobs: matrix: ${{ steps.targets.outputs.matrix }} pr_number: ${{ steps.trigger.outputs.pr_number }} trigger_event: ${{ steps.trigger.outputs.event }} - trigger_action: ${{ steps.trigger.outputs.action }} dashboard_precondition_met: ${{ steps.dashboard-precondition.outputs.met }} steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 @@ -69,12 +64,10 @@ jobs: TARGET_REPOSITORY_FROM_INPUT: ${{ inputs.repository }} PR_FROM_INPUT: ${{ inputs.pr_number }} TRIGGER_EVENT_FROM_INPUT: ${{ inputs.trigger_event }} - TRIGGER_ACTION_FROM_INPUT: ${{ inputs.trigger_action }} run: | set -euo pipefail pr_number="" trigger_event="" - trigger_action="" case "$EVENT_NAME" in schedule) @@ -83,7 +76,6 @@ jobs: workflow_dispatch) pr_number="$PR_FROM_INPUT" trigger_event="${TRIGGER_EVENT_FROM_INPUT:-workflow_dispatch}" - trigger_action="$TRIGGER_ACTION_FROM_INPUT" ;; esac @@ -95,13 +87,9 @@ jobs: [[ "$trigger_event" =~ ^(schedule|workflow_dispatch|check_suite|pull_request|issue_comment|pull_request_review|pull_request_review_comment|pull_request_review_thread)$ ]] \ || { echo "bad trigger event: $trigger_event"; exit 1; } fi - if [[ -n "$trigger_action" ]]; then - [[ "$trigger_action" =~ ^[a-z_]{1,32}$ ]] || { echo "bad trigger action: $trigger_action"; exit 1; } - fi { echo "pr_number=$pr_number" echo "event=$trigger_event" - echo "action=$trigger_action" } >> "$GITHUB_OUTPUT" - name: Resolve target repositories @@ -168,7 +156,6 @@ jobs: repository: ${{ matrix.name }} pr_number: ${{ needs.resolve-targets.outputs.pr_number }} trigger_event: ${{ needs.resolve-targets.outputs.trigger_event }} - trigger_action: ${{ needs.resolve-targets.outputs.trigger_action }} required_approvals: ${{ matrix.required_approvals || 1 }} approver_teams_json: ${{ toJSON(matrix.approver_teams || fromJSON('[]')) }} slack_channel: ${{ matrix.slack_channel }} From 168eafe7a3335a627eb8afa1fa83dedb9b021a16 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Fri, 17 Jul 2026 07:09:37 -0700 Subject: [PATCH 5/7] Simplify live PR status handling --- .../pr_status_comment.py | 67 ++++------------- .../publish_dashboard.py | 14 ++-- .../scripts/pull-request-dashboard/render.py | 14 +--- .../route_presentation.py | 39 ++++++++++ .../scripts/pull-request-dashboard/state.py | 33 +++++++++ .../pull-request-dashboard/state_branch.py | 16 +++++ .../test_pr_status_comment.py | 71 +++++++++---------- .../test_publish_dashboard.py | 33 ++++++++- .../test_route_presentation.py | 28 ++++++++ .../pull-request-dashboard/test_state.py | 56 +++++++++++++++ .../test_state_branch.py | 47 ++++++++++++ .github/workflows/pull-request-dashboard.yml | 9 ++- 12 files changed, 315 insertions(+), 112 deletions(-) create mode 100644 .github/scripts/pull-request-dashboard/route_presentation.py create mode 100644 .github/scripts/pull-request-dashboard/test_route_presentation.py create mode 100644 .github/scripts/pull-request-dashboard/test_state_branch.py diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index b51cb85e0e..8a1d1361d5 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -7,9 +7,9 @@ import sys from typing import Any -from github_cli import detect_repo, gh_api, normalize_repo, repo_state_key, run_gh -from state import initial_backfill_complete, load_dashboard_state_cache, set_state_dir -import state_branch +from github_cli import detect_repo, gh_api, normalize_repo, run_gh +from route_presentation import route_status +from state import load_accepted_dashboard_state STATUS_MARKER = "" @@ -25,19 +25,8 @@ ) -def load_accepted_dashboard_state(repo: str, state_branch_name: str) -> dict[str, Any] | None: - if not state_branch.fetch_state_branch(state_branch_name, required=False): - return None - with state_branch.temporary_state_dir() as checkout_dir: - try: - state_branch.run([ - "git", "worktree", "add", "--quiet", "--detach", str(checkout_dir), - state_branch.remote_ref(state_branch_name), - ]) - set_state_dir(checkout_dir / repo_state_key(repo)) - return load_dashboard_state_cache() - finally: - state_branch.remove_existing_state_dir(checkout_dir) +def author_mention(author: str) -> str: + return f"@{author}" if author != "the author" else author def render_status_comment( @@ -49,44 +38,24 @@ def render_status_comment( terminal = bool(pr.get("merged")) or state == "closed" if pr.get("merged"): - next_action = "No one" - waiting_on = "This pull request has been merged." + status = "This pull request has been merged." elif state == "closed": - next_action = "No one" - waiting_on = "This pull request has been closed." + status = "This pull request has been closed." elif pr.get("draft"): - next_action = f"@{author}" if author != "the author" else author - waiting_on = "The author to mark this pull request ready for review." + status = f"Waiting on {author_mention(author)} to mark this pull request ready for review." elif result is None: - next_action = "Dashboard maintainers" - waiting_on = "The dashboard to finish refreshing this pull request." + status = "Waiting for the dashboard to finish refreshing this pull request." else: facts = result.get("facts") or {} route = result.get("route") or "unknown" - if route == "author": - effective_author = (facts.get("author") or author).strip() - next_action = f"@{effective_author}" if effective_author != "the author" else effective_author - waiting_on = "The author to address or respond to unresolved review discussions." - elif route == "approver": - next_action = "Reviewers" - waiting_on = "Reviewers to review the latest changes." - elif route == "maintainer": - next_action = "Maintainers" - waiting_on = "A maintainer to merge the pull request." - elif route == "external": - next_action = "External" - waiting_on = "An external dependency or decision." - else: - next_action = "Dashboard maintainers" - waiting_on = "The dashboard to determine the next action." + effective_author = (facts.get("author") or author).strip() + status = route_status(route, author_mention(effective_author)) lines = [ STATUS_MARKER, "## Pull request dashboard status", "", - f"**Next action:** {next_action}", - "", - f"**Waiting on:** {waiting_on}", + f"**Status:** {status}", ] if not terminal and result and result.get("route") == "author": @@ -161,21 +130,11 @@ def main() -> int: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--repo", help="target repository name, e.g. opentelemetry-java-instrumentation") parser.add_argument("--state-branch", required=True, help="git branch used for workflow state") - parser.add_argument("--pr-number", type=int, help="pull request to update") - parser.add_argument( - "--check-initial-backfill-complete", - action="store_true", - help='print "true" when the initial dashboard backfill is complete, otherwise "false"', - ) + parser.add_argument("--pr-number", type=int, required=True, help="pull request to update") args = parser.parse_args() repo = normalize_repo(args.repo) if args.repo else detect_repo() dashboard_state = load_accepted_dashboard_state(repo, args.state_branch) - if args.check_initial_backfill_complete: - print("true" if initial_backfill_complete(dashboard_state) else "false") - return 0 - if args.pr_number is None: - parser.error("--pr-number is required unless --check-initial-backfill-complete is set") if dashboard_state is None: print("dashboard result state not found; skipping PR status comment", file=sys.stderr) return 0 diff --git a/.github/scripts/pull-request-dashboard/publish_dashboard.py b/.github/scripts/pull-request-dashboard/publish_dashboard.py index e2b105b4fa..55de00bc82 100644 --- a/.github/scripts/pull-request-dashboard/publish_dashboard.py +++ b/.github/scripts/pull-request-dashboard/publish_dashboard.py @@ -179,6 +179,14 @@ def render_dashboard_markdown(repo: str, large_repo: bool) -> Path: return output_path +def publish_accepted_dashboard(repo: str, state_branch_name: str, large_repo: bool) -> None: + with state_branch.accepted_state_dir(state_branch_name, required=True) as state_dir: + if state_dir is None: + raise RuntimeError(f"required state branch not found: {state_branch_name}") + set_state_dir(state_dir / repo_state_key(repo)) + publish_dashboard(repo, render_dashboard_markdown(repo, large_repo)) + + def main() -> int: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--repo", help="target repository name, e.g. opentelemetry-java-instrumentation") @@ -195,11 +203,7 @@ def main() -> int: args = parser.parse_args() repo = normalize_repo(args.repo) if args.repo else detect_repo() - with state_branch.temporary_state_dir() as state_dir: - set_state_dir(state_dir / repo_state_key(repo)) - state_branch.configure_git() - state_branch.checkout_state(state_dir, args.state_branch, require_existing=True) - publish_dashboard(repo, render_dashboard_markdown(repo, args.large_repo)) + publish_accepted_dashboard(repo, args.state_branch, args.large_repo) return 0 diff --git a/.github/scripts/pull-request-dashboard/render.py b/.github/scripts/pull-request-dashboard/render.py index 712a69c722..da8867c8fe 100644 --- a/.github/scripts/pull-request-dashboard/render.py +++ b/.github/scripts/pull-request-dashboard/render.py @@ -3,20 +3,10 @@ from datetime import datetime from typing import Any +from route_presentation import ROUTE_ORDER, route_label from utils import actor_login, activity_age, parse_ts, seconds_since -ROUTE_LABELS = { - "maintainer": "Waiting on maintainers", - "approver": "Waiting on reviewers", - "author": "Waiting on authors", - "external": "Waiting on external", - "transient-failure": "Transient GitHub failure retrieving PR data", - "unknown": "Unknown", -} -ROUTE_ORDER = ["maintainer", "approver", "author", "external", "transient-failure", "unknown"] - - def _md_escape(s: str) -> str: return ( (s or "") @@ -262,7 +252,7 @@ def row_sort_key(pr: dict[str, Any]) -> tuple[int, int]: continue rows.sort(key=row_sort_key, reverse=True) rows, truncated = _limit_rows(rows, max_rows_per_section) - out.append(f"## {ROUTE_LABELS.get(route, route)}") + out.append(f"## {route_label(route)}") out.append("") out.append("| PR | Author | Reviewers | CI | Conflicts | Age |") out.append("|---|---|---|:---:|:---:|:---:|") diff --git a/.github/scripts/pull-request-dashboard/route_presentation.py b/.github/scripts/pull-request-dashboard/route_presentation.py new file mode 100644 index 0000000000..9ddafff2cb --- /dev/null +++ b/.github/scripts/pull-request-dashboard/route_presentation.py @@ -0,0 +1,39 @@ +from __future__ import annotations + + +ROUTE_PRESENTATION = { + "maintainer": { + "dashboard_label": "Waiting on maintainers", + "status": "Waiting on maintainers to merge the pull request.", + }, + "approver": { + "dashboard_label": "Waiting on reviewers", + "status": "Waiting on reviewers to review the latest changes.", + }, + "author": { + "dashboard_label": "Waiting on authors", + "status": "Waiting on {author} to address or respond to unresolved review discussions.", + }, + "external": { + "dashboard_label": "Waiting on external", + "status": "Waiting on an external dependency or decision.", + }, + "transient-failure": { + "dashboard_label": "Transient GitHub failure retrieving PR data", + "status": "Waiting on dashboard maintainers to determine the next action.", + }, + "unknown": { + "dashboard_label": "Unknown", + "status": "Waiting on dashboard maintainers to determine the next action.", + }, +} +ROUTE_ORDER = list(ROUTE_PRESENTATION) + + +def route_label(route: str) -> str: + return ROUTE_PRESENTATION.get(route, ROUTE_PRESENTATION["unknown"])["dashboard_label"] + + +def route_status(route: str, author: str = "the author") -> str: + template = ROUTE_PRESENTATION.get(route, ROUTE_PRESENTATION["unknown"])["status"] + return template.format(author=author) \ No newline at end of file diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index fdd895f9e8..59a325d64d 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -1,10 +1,14 @@ from __future__ import annotations +import argparse import json import sys from pathlib import Path from typing import Any +from github_cli import detect_repo, normalize_repo, repo_state_key +import state_branch + DASHBOARD_MARKDOWN_FILE = "pull-request-dashboard.md" BACKFILL_STATE_FILE = "backfill-state.json" @@ -123,6 +127,19 @@ def load_dashboard_state_cache() -> dict[str, Any] | None: } +def load_accepted_dashboard_state( + repo: str, + state_branch_name: str, + *, + required: bool = False, +) -> dict[str, Any] | None: + with state_branch.accepted_state_dir(state_branch_name, required=required) as checkout_dir: + if checkout_dir is None: + return None + set_state_dir(checkout_dir / repo_state_key(repo)) + return load_dashboard_state_cache() + + def save_dashboard_state_cache(state: dict[str, Any]) -> None: prs = state.get("prs") stored = { @@ -227,3 +244,19 @@ def update_dashboard_state_for_pr( INITIAL_BACKFILL_COMPLETE_KEY: initial_backfill_complete(state), "prs": prs, } + + +def main() -> int: + parser = argparse.ArgumentParser(description="Read accepted PR dashboard state.") + parser.add_argument("--repo", help="target repository name, e.g. opentelemetry-java-instrumentation") + parser.add_argument("--state-branch", required=True, help="git branch used for workflow state") + args = parser.parse_args() + + repo = normalize_repo(args.repo) if args.repo else detect_repo() + dashboard_state = load_accepted_dashboard_state(repo, args.state_branch) + print("true" if initial_backfill_complete(dashboard_state) else "false") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/scripts/pull-request-dashboard/state_branch.py b/.github/scripts/pull-request-dashboard/state_branch.py index 3003f6640d..43efbd4ba3 100644 --- a/.github/scripts/pull-request-dashboard/state_branch.py +++ b/.github/scripts/pull-request-dashboard/state_branch.py @@ -25,6 +25,22 @@ def temporary_state_dir() -> Iterator[Path]: yield Path(temp_root) / "state" +@contextmanager +def accepted_state_dir(state_branch: str, required: bool) -> Iterator[Path | None]: + with temporary_state_dir() as checkout_dir: + if not fetch_state_branch(state_branch, required=required): + yield None + return + try: + run([ + "git", "worktree", "add", "--quiet", "--detach", str(checkout_dir), + remote_ref(state_branch), + ]) + yield checkout_dir + finally: + remove_existing_state_dir(checkout_dir) + + def run(cmd: list[str], check: bool = True, cwd: Path | None = None) -> subprocess.CompletedProcess[str]: return subprocess.run(cmd, check=check, cwd=cwd, text=True) diff --git a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py index df96326f5f..64955e37dd 100644 --- a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py @@ -1,40 +1,11 @@ from __future__ import annotations -from pathlib import Path import unittest from unittest.mock import patch import pr_status_comment -class LoadAcceptedDashboardStateTest(unittest.TestCase): - @patch.object(pr_status_comment.state_branch, "fetch_state_branch", return_value=True) - @patch.object(pr_status_comment.state_branch, "run") - @patch.object(pr_status_comment.state_branch, "remove_existing_state_dir") - @patch.object(pr_status_comment, "load_dashboard_state_cache", return_value={"version": 3, "prs": {}}) - def test_worktree_checkout_is_quiet( - self, - _load_state: object, - _remove_state_dir: object, - run: object, - _fetch_state_branch: object, - ) -> None: - checkout_dir = Path("checkout") - with patch.object(pr_status_comment.state_branch, "temporary_state_dir") as temporary_state_dir: - temporary_state_dir.return_value.__enter__.return_value = checkout_dir - - state = pr_status_comment.load_accepted_dashboard_state( - "open-telemetry/example", - "state-branch", - ) - - self.assertEqual({"version": 3, "prs": {}}, state) - run.assert_called_once_with([ - "git", "worktree", "add", "--quiet", "--detach", "checkout", - "refs/remotes/origin/state-branch", - ]) - - class RenderStatusCommentTest(unittest.TestCase): def pr(self, **overrides: object) -> dict[str, object]: pr: dict[str, object] = { @@ -60,7 +31,10 @@ def test_waiting_on_author_links_discussions(self) -> None: }, ) - self.assertIn("**Next action:** @alice", body) + self.assertIn( + "**Status:** Waiting on @alice to address or respond to unresolved review discussions.", + body, + ) self.assertIn("[Discussion 1]", body) self.assertIn("give every review discussion a clear outcome", body) @@ -88,8 +62,10 @@ def test_waiting_on_author_caps_discussion_links(self) -> None: def test_draft_waits_on_author(self) -> None: body = pr_status_comment.render_status_comment(self.pr(draft=True), None) - self.assertIn("**Next action:** @alice", body) - self.assertIn("mark this pull request ready for review", body) + self.assertIn( + "**Status:** Waiting on @alice to mark this pull request ready for review.", + body, + ) def test_merged_pr_has_no_author_guidance(self) -> None: body = pr_status_comment.render_status_comment( @@ -97,8 +73,7 @@ def test_merged_pr_has_no_author_guidance(self) -> None: None, ) - self.assertIn("**Next action:** No one", body) - self.assertIn("has been merged", body) + self.assertIn("**Status:** This pull request has been merged.", body) self.assertNotIn("give every review discussion a clear outcome", body) def test_terminal_pr_has_no_author_discussion_links(self) -> None: @@ -125,8 +100,32 @@ def test_blank_login_falls_back_to_author(self) -> None: None, ) - self.assertIn("**Next action:** the author", body) - self.assertNotIn("**Next action:** @", body) + self.assertIn( + "**Status:** Waiting on the author to mark this pull request ready for review.", + body, + ) + self.assertNotIn("**Status:** Waiting on @", body) + + def test_routes_render_one_status_sentence(self) -> None: + expected_statuses = { + "approver": "Waiting on reviewers to review the latest changes.", + "maintainer": "Waiting on maintainers to merge the pull request.", + "external": "Waiting on an external dependency or decision.", + "transient-failure": "Waiting on dashboard maintainers to determine the next action.", + "unknown": "Waiting on dashboard maintainers to determine the next action.", + } + + for route, expected in expected_statuses.items(): + with self.subTest(route=route): + body = pr_status_comment.render_status_comment( + self.pr(), + {"route": route, "facts": {}}, + ) + + self.assertIn(f"**Status:** {expected}", body) + self.assertEqual(1, body.count("**Status:**")) + self.assertNotIn("**Next action:**", body) + self.assertNotIn("**Waiting on:**", body) class UpsertStatusCommentTest(unittest.TestCase): diff --git a/.github/scripts/pull-request-dashboard/test_publish_dashboard.py b/.github/scripts/pull-request-dashboard/test_publish_dashboard.py index d2c562dbf0..612890e150 100644 --- a/.github/scripts/pull-request-dashboard/test_publish_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_publish_dashboard.py @@ -1,8 +1,11 @@ from __future__ import annotations +from contextlib import nullcontext +from pathlib import Path import unittest +from unittest.mock import patch -from publish_dashboard import publishable_prs +import publish_dashboard class PublishablePrsTest(unittest.TestCase): @@ -14,10 +17,36 @@ def test_omits_uncached_non_draft_prs_and_retains_drafts(self) -> None: ] self.assertEqual( - publishable_prs(prs, {1: {"route": "author"}}), + publish_dashboard.publishable_prs(prs, {1: {"route": "author"}}), [prs[0], prs[2]], ) + @patch.object(publish_dashboard, "publish_dashboard") + @patch.object(publish_dashboard, "render_dashboard_markdown", return_value=Path("dashboard.md")) + @patch.object(publish_dashboard, "set_state_dir") + def test_publishes_from_read_only_accepted_state( + self, + set_state_dir: object, + render_dashboard_markdown: object, + publish: object, + ) -> None: + checkout_dir = Path("checkout") + with patch.object( + publish_dashboard.state_branch, + "accepted_state_dir", + return_value=nullcontext(checkout_dir), + ) as accepted_state_dir: + publish_dashboard.publish_accepted_dashboard( + "open-telemetry/example", + "state-branch", + True, + ) + + accepted_state_dir.assert_called_once_with("state-branch", required=True) + set_state_dir.assert_called_once_with(checkout_dir / "example") + render_dashboard_markdown.assert_called_once_with("open-telemetry/example", True) + publish.assert_called_once_with("open-telemetry/example", Path("dashboard.md")) + if __name__ == "__main__": unittest.main() \ No newline at end of file diff --git a/.github/scripts/pull-request-dashboard/test_route_presentation.py b/.github/scripts/pull-request-dashboard/test_route_presentation.py new file mode 100644 index 0000000000..ec54f7f553 --- /dev/null +++ b/.github/scripts/pull-request-dashboard/test_route_presentation.py @@ -0,0 +1,28 @@ +from __future__ import annotations + +import unittest + +from route_presentation import ROUTE_ORDER, ROUTE_PRESENTATION, route_label, route_status + + +class RoutePresentationTest(unittest.TestCase): + def test_every_route_has_dashboard_and_status_presentation(self) -> None: + self.assertEqual(list(ROUTE_PRESENTATION), ROUTE_ORDER) + for route in ROUTE_ORDER: + with self.subTest(route=route): + self.assertTrue(route_label(route)) + self.assertTrue(route_status(route)) + + def test_author_status_includes_effective_author(self) -> None: + self.assertEqual( + "Waiting on @alice to address or respond to unresolved review discussions.", + route_status("author", "@alice"), + ) + + def test_unrecognized_route_uses_unknown_presentation(self) -> None: + self.assertEqual(route_label("unknown"), route_label("other")) + self.assertEqual(route_status("unknown"), route_status("other")) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/.github/scripts/pull-request-dashboard/test_state.py b/.github/scripts/pull-request-dashboard/test_state.py index b655660d72..d5c0922276 100644 --- a/.github/scripts/pull-request-dashboard/test_state.py +++ b/.github/scripts/pull-request-dashboard/test_state.py @@ -1,5 +1,7 @@ from __future__ import annotations +from contextlib import nullcontext, redirect_stdout +from io import StringIO import json from pathlib import Path import tempfile @@ -13,10 +15,12 @@ backfill_state_path, dashboard_state_path, empty_state, + load_accepted_dashboard_state, load_backfill_state, load_dashboard_state_cache, load_state_file, load_notifications, + main, notification_state_path, save_state_file, save_dashboard_state_cache, @@ -27,6 +31,58 @@ class StateTest(unittest.TestCase): + @patch( + "state.load_accepted_dashboard_state", + return_value={"initial_backfill_complete": True, "prs": {}}, + ) + def test_cli_prints_initial_backfill_readiness(self, load_state: object) -> None: + output = StringIO() + with ( + patch("sys.argv", [ + "state.py", + "--repo", "example", + "--state-branch", "state-branch", + ]), + redirect_stdout(output), + ): + status = main() + + self.assertEqual(0, status) + self.assertEqual("true\n", output.getvalue()) + load_state.assert_called_once_with("open-telemetry/example", "state-branch") + + def test_loads_accepted_dashboard_state_from_state_branch(self) -> None: + with tempfile.TemporaryDirectory() as temp_dir: + checkout_dir = Path(temp_dir) + state_path = checkout_dir / "example" / "dashboard-state.json" + state_path.parent.mkdir() + state_path.write_text( + json.dumps({ + "version": DASHBOARD_STATE_VERSION, + "initial_backfill_complete": True, + "prs": {"123": {"route": "author"}}, + }), + encoding="utf-8", + ) + with patch( + "state.state_branch.accepted_state_dir", + return_value=nullcontext(checkout_dir), + ) as accepted_state_dir: + dashboard_state = load_accepted_dashboard_state( + "open-telemetry/example", + "state-branch", + ) + + self.assertEqual( + dashboard_state, + { + "version": DASHBOARD_STATE_VERSION, + "initial_backfill_complete": True, + "prs": {"123": {"route": "author"}}, + }, + ) + accepted_state_dir.assert_called_once_with("state-branch", required=False) + def test_versioned_state_helpers_preserve_arbitrary_payloads(self) -> None: with tempfile.TemporaryDirectory() as temp_dir: path = Path(temp_dir) / "state.json" diff --git a/.github/scripts/pull-request-dashboard/test_state_branch.py b/.github/scripts/pull-request-dashboard/test_state_branch.py new file mode 100644 index 0000000000..4996f0076c --- /dev/null +++ b/.github/scripts/pull-request-dashboard/test_state_branch.py @@ -0,0 +1,47 @@ +from __future__ import annotations + +from pathlib import Path +import unittest +from unittest.mock import patch + +import state_branch + + +class AcceptedStateDirTest(unittest.TestCase): + @patch.object(state_branch, "fetch_state_branch", return_value=True) + @patch.object(state_branch, "run") + @patch.object(state_branch, "remove_existing_state_dir") + def test_checks_out_remote_state_quietly( + self, + remove_existing_state_dir: object, + run: object, + _fetch_state_branch: object, + ) -> None: + checkout_dir = Path("checkout") + with patch.object(state_branch, "temporary_state_dir") as temporary_state_dir: + temporary_state_dir.return_value.__enter__.return_value = checkout_dir + + with state_branch.accepted_state_dir("state-branch", required=True) as state_dir: + self.assertEqual(checkout_dir, state_dir) + + run.assert_called_once_with([ + "git", "worktree", "add", "--quiet", "--detach", "checkout", + "refs/remotes/origin/state-branch", + ]) + remove_existing_state_dir.assert_called_once_with(checkout_dir) + + @patch.object(state_branch, "fetch_state_branch", return_value=False) + @patch.object(state_branch, "run") + def test_returns_none_when_optional_state_is_missing( + self, + run: object, + _fetch_state_branch: object, + ) -> None: + with state_branch.accepted_state_dir("state-branch", required=False) as state_dir: + self.assertIsNone(state_dir) + + run.assert_not_called() + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/.github/workflows/pull-request-dashboard.yml b/.github/workflows/pull-request-dashboard.yml index 99716a2090..c14c7ce3cf 100644 --- a/.github/workflows/pull-request-dashboard.yml +++ b/.github/workflows/pull-request-dashboard.yml @@ -17,6 +17,10 @@ on: description: Event that requested the refresh. required: false type: string + trigger_action: + description: Deprecated compatibility input; accepted and ignored during webhook rollout. + required: false + type: string trigger_review_id: description: Deprecated compatibility input; accepted and ignored during webhook rollout. required: false @@ -126,10 +130,9 @@ jobs: fi state_branch="otelbot/pull-request-dashboard-state/${REPO_NAME}" - initial_backfill_complete=$(python3 .github/scripts/pull-request-dashboard/pr_status_comment.py \ + initial_backfill_complete=$(python3 .github/scripts/pull-request-dashboard/state.py \ --repo "$REPO_NAME" \ - --state-branch "$state_branch" \ - --check-initial-backfill-complete) + --state-branch "$state_branch") if [[ "$initial_backfill_complete" == "true" ]]; then echo "met=true" >> "$GITHUB_OUTPUT" exit 0 From 09b61a82e4866946b6880e77aa07d8b4c645fee4 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Fri, 17 Jul 2026 07:37:43 -0700 Subject: [PATCH 6/7] Simplify dashboard dispatch compatibility --- .../pull-request-dashboard/WEBHOOK_SETUP.md | 26 +++++++++++++++++++ .../pr_status_comment.py | 2 ++ .../workflows/pull-request-dashboard-repo.yml | 6 +---- .github/workflows/pull-request-dashboard.yml | 24 ++++------------- 4 files changed, 34 insertions(+), 24 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/WEBHOOK_SETUP.md b/.github/scripts/pull-request-dashboard/WEBHOOK_SETUP.md index 6255b0f043..606b77d241 100644 --- a/.github/scripts/pull-request-dashboard/WEBHOOK_SETUP.md +++ b/.github/scripts/pull-request-dashboard/WEBHOOK_SETUP.md @@ -163,3 +163,29 @@ Notes: - `repository` is the short repository name under `open-telemetry`. - Omit `pr_number` or set it to an empty string for a backfill. - The central workflow validates these inputs before using them. + +## 6. Post-rollout compatibility cleanup + +The central workflow temporarily accepts the deprecated `trigger_action` and +`trigger_review_id` inputs so it remains compatible with the previously +deployed webhook bridge. The status-comment publisher also recognizes the +legacy `review-guidance` and `copilot-review-guidance` markers so it can upgrade +existing comments in place. + +Remove those compatibility paths after completing this checklist: + +1. Confirm the **Deploy pull request dashboard webhook** workflow completed + successfully on `main` for the commit containing the current dispatch + contract. +2. Remove the deprecated `trigger_action` and `trigger_review_id` inputs from + `pull-request-dashboard.yml`. +3. Dispatch one targeted dashboard refresh for each open PR with a + dashboard-App-authored legacy guidance comment. Hourly backfills do not + update PR status comments, so they do not perform this migration. +4. Verify that no open PR in a configured repository has a + dashboard-App-authored comment containing either legacy marker. +5. Remove `LEGACY_MARKERS` from `pr_status_comment.py`, leaving only + `STATUS_MARKER` as the managed-comment marker. + +Closed PR comments do not need migration because they will not be updated +again. diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index 8a1d1361d5..26217c2b54 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -19,6 +19,8 @@ "explain why no change is needed, ask a follow-up question, or resolve the discussion." ) DASHBOARD_APP_SLUG = "opentelemetry-pr-dashboard" +# Remove after migrating open PRs as described by the post-rollout +# compatibility cleanup in WEBHOOK_SETUP.md. LEGACY_MARKERS = ( "", "", diff --git a/.github/workflows/pull-request-dashboard-repo.yml b/.github/workflows/pull-request-dashboard-repo.yml index ae053f0f9b..fe51aa13f1 100644 --- a/.github/workflows/pull-request-dashboard-repo.yml +++ b/.github/workflows/pull-request-dashboard-repo.yml @@ -10,10 +10,6 @@ on: required: false default: "" type: string - trigger_event: - required: false - default: "" - type: string required_approvals: required: false default: 1 @@ -177,7 +173,7 @@ jobs: if: >- needs.update-dashboard.result == 'success' && needs.update-dashboard.outputs.initial_backfill_complete == 'true' && - (inputs.trigger_event == 'schedule' || inputs.pr_number != '') + (github.event_name == 'schedule' || inputs.pr_number != '') concurrency: group: ${{ github.workflow }}-notify-${{ inputs.repository }}-${{ inputs.pr_number || 'backfill' }} cancel-in-progress: false diff --git a/.github/workflows/pull-request-dashboard.yml b/.github/workflows/pull-request-dashboard.yml index c14c7ce3cf..3666b32ee2 100644 --- a/.github/workflows/pull-request-dashboard.yml +++ b/.github/workflows/pull-request-dashboard.yml @@ -17,6 +17,8 @@ on: description: Event that requested the refresh. required: false type: string + # Remove these two inputs after the post-rollout compatibility cleanup in + # WEBHOOK_SETUP.md. trigger_action: description: Deprecated compatibility input; accepted and ignored during webhook rollout. required: false @@ -54,7 +56,6 @@ jobs: outputs: matrix: ${{ steps.targets.outputs.matrix }} pr_number: ${{ steps.trigger.outputs.pr_number }} - trigger_event: ${{ steps.trigger.outputs.event }} dashboard_precondition_met: ${{ steps.dashboard-precondition.outputs.met }} steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 @@ -64,24 +65,13 @@ jobs: - name: Resolve trigger inputs id: trigger env: - EVENT_NAME: ${{ github.event_name }} TARGET_REPOSITORY_FROM_INPUT: ${{ inputs.repository }} PR_FROM_INPUT: ${{ inputs.pr_number }} TRIGGER_EVENT_FROM_INPUT: ${{ inputs.trigger_event }} run: | set -euo pipefail - pr_number="" - trigger_event="" - - case "$EVENT_NAME" in - schedule) - trigger_event="$EVENT_NAME" - ;; - workflow_dispatch) - pr_number="$PR_FROM_INPUT" - trigger_event="${TRIGGER_EVENT_FROM_INPUT:-workflow_dispatch}" - ;; - esac + pr_number="$PR_FROM_INPUT" + trigger_event="$TRIGGER_EVENT_FROM_INPUT" if [[ -n "$pr_number" ]]; then [[ "$pr_number" =~ ^[1-9][0-9]{0,6}$ ]] || { echo "bad PR number: $pr_number"; exit 1; } @@ -91,10 +81,7 @@ jobs: [[ "$trigger_event" =~ ^(schedule|workflow_dispatch|check_suite|pull_request|issue_comment|pull_request_review|pull_request_review_comment|pull_request_review_thread)$ ]] \ || { echo "bad trigger event: $trigger_event"; exit 1; } fi - { - echo "pr_number=$pr_number" - echo "event=$trigger_event" - } >> "$GITHUB_OUTPUT" + echo "pr_number=$pr_number" >> "$GITHUB_OUTPUT" - name: Resolve target repositories id: targets @@ -158,7 +145,6 @@ jobs: with: repository: ${{ matrix.name }} pr_number: ${{ needs.resolve-targets.outputs.pr_number }} - trigger_event: ${{ needs.resolve-targets.outputs.trigger_event }} required_approvals: ${{ matrix.required_approvals || 1 }} approver_teams_json: ${{ toJSON(matrix.approver_teams || fromJSON('[]')) }} slack_channel: ${{ matrix.slack_channel }} From 0bacd7ca3ca6fce9a5ac2103008b2875dc27913d Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Fri, 17 Jul 2026 08:41:53 -0700 Subject: [PATCH 7/7] Align PR status feedback terminology --- .../pull-request-dashboard/dashboard.py | 17 ++++-- .../pr_status_comment.py | 43 +++++++++++---- .../scripts/pull-request-dashboard/render.py | 2 +- .../route_presentation.py | 2 +- .../test_pr_status_comment.py | 53 ++++++++++++------- .../pull-request-dashboard/test_render.py | 2 +- .../test_route_presentation.py | 2 +- pull-request-dashboard/README.md | 10 ++-- 8 files changed, 89 insertions(+), 42 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index e92fde79a6..52abb159cf 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -115,9 +115,13 @@ or PR creation time. waiting_age_basis str Which heuristic chose waiting_since. - author_action_discussion_urls list[str] Canonical links to unresolved - review discussions routed to - the author. + author_action_review_thread_urls + list[str] Canonical links to unresolved + inline review threads routed + to the author. + author_action_top_level_feedback_urls + list[str] Canonical links to top-level + feedback routed to the author. reviewers list[dict] Reviewers to display (added by add_reviewers). Each entry is {"login": str, "approved": bool, @@ -1080,8 +1084,11 @@ def build_pr_result( } route = route_pr(facts, pending_actions, required_approvals) add_wait_age_facts(facts, route, pending_actions) - facts["author_action_discussion_urls"] = author_action_discussion_urls( - review_threads + top_level_items, pending_actions + facts["author_action_review_thread_urls"] = author_action_discussion_urls( + review_threads, pending_actions + ) + facts["author_action_top_level_feedback_urls"] = author_action_discussion_urls( + top_level_items, pending_actions ) add_reviewers( facts, events, review_threads, top_level_items, pending_actions diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index 26217c2b54..19a5f2211e 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -13,10 +13,10 @@ STATUS_MARKER = "" -AUTHOR_ACTION_DISCUSSION_LINK_LIMIT = 20 +AUTHOR_ACTION_FEEDBACK_LINK_LIMIT = 20 AUTHOR_GUIDANCE = ( - "Please give every review discussion a clear outcome: link to the commit that addresses it, " - "explain why no change is needed, ask a follow-up question, or resolve the discussion." + "Please give each review feedback item a clear outcome: link to the commit that addresses it, " + "explain why no change is needed, or ask a follow-up question." ) DASHBOARD_APP_SLUG = "opentelemetry-pr-dashboard" # Remove after migrating open PRs as described by the post-rollout @@ -61,15 +61,38 @@ def render_status_comment( ] if not terminal and result and result.get("route") == "author": - urls = (result.get("facts") or {}).get("author_action_discussion_urls") or [] - if urls: - lines.extend(["", "Unresolved discussions waiting on the author:"]) - displayed_urls = urls[:AUTHOR_ACTION_DISCUSSION_LINK_LIMIT] - lines.extend(f"- [Discussion {index}]({url})" for index, url in enumerate(displayed_urls, start=1)) + facts = result.get("facts") or {} + feedback_sections = ( + ( + "Unresolved inline review threads waiting on the author:", + "Thread", + "inline review thread", + "inline review threads", + facts.get("author_action_review_thread_urls") or [], + ), + ( + "Top-level feedback waiting on the author:", + "Feedback", + "top-level feedback item", + "top-level feedback items", + facts.get("author_action_top_level_feedback_urls") or [], + ), + ) + remaining_limit = AUTHOR_ACTION_FEEDBACK_LINK_LIMIT + for heading, label, singular, plural, urls in feedback_sections: + if not urls: + continue + lines.extend(["", heading]) + displayed_urls = urls[:remaining_limit] + lines.extend( + f"- [{label} {index}]({url})" + for index, url in enumerate(displayed_urls, start=1) + ) remaining_count = len(urls) - len(displayed_urls) if remaining_count: - noun = "discussion" if remaining_count == 1 else "discussions" - lines.append(f"- {remaining_count} more unresolved {noun} not shown") + noun = singular if remaining_count == 1 else plural + lines.append(f"- {remaining_count} more {noun} not shown") + remaining_limit -= len(displayed_urls) if not terminal: lines.extend(["", AUTHOR_GUIDANCE]) diff --git a/.github/scripts/pull-request-dashboard/render.py b/.github/scripts/pull-request-dashboard/render.py index da8867c8fe..984aeaba56 100644 --- a/.github/scripts/pull-request-dashboard/render.py +++ b/.github/scripts/pull-request-dashboard/render.py @@ -220,7 +220,7 @@ def render_pr_tables( ) reviewers_note = ( "Reviewers column: ✅ approved · ✔️ approved (non-code-owner) · " - "💬 open discussion · 📌 author action pending · 🔴 changes requested." + "💬 open review thread · 📌 author action pending · 🔴 changes requested." ) out: list[str] = [ "> [!NOTE]", diff --git a/.github/scripts/pull-request-dashboard/route_presentation.py b/.github/scripts/pull-request-dashboard/route_presentation.py index 9ddafff2cb..79b8b4e84b 100644 --- a/.github/scripts/pull-request-dashboard/route_presentation.py +++ b/.github/scripts/pull-request-dashboard/route_presentation.py @@ -12,7 +12,7 @@ }, "author": { "dashboard_label": "Waiting on authors", - "status": "Waiting on {author} to address or respond to unresolved review discussions.", + "status": "Waiting on {author} to address or respond to review feedback.", }, "external": { "dashboard_label": "Waiting on external", diff --git a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py index 64955e37dd..a5971f19c7 100644 --- a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py @@ -17,31 +17,41 @@ def pr(self, **overrides: object) -> dict[str, object]: pr.update(overrides) return pr - def test_waiting_on_author_links_discussions(self) -> None: + def test_waiting_on_author_splits_review_feedback_links(self) -> None: body = pr_status_comment.render_status_comment( self.pr(), { "route": "author", "facts": { "author": "alice", - "author_action_discussion_urls": [ + "author_action_review_thread_urls": [ "https://github.com/open-telemetry/example/pull/1#discussion_r1", ], + "author_action_top_level_feedback_urls": [ + "https://github.com/open-telemetry/example/pull/1#pullrequestreview-2", + ], }, }, ) self.assertIn( - "**Status:** Waiting on @alice to address or respond to unresolved review discussions.", + "**Status:** Waiting on @alice to address or respond to review feedback.", body, ) - self.assertIn("[Discussion 1]", body) - self.assertIn("give every review discussion a clear outcome", body) - - def test_waiting_on_author_caps_discussion_links(self) -> None: - urls = [ + self.assertIn("Unresolved inline review threads waiting on the author:", body) + self.assertIn("[Thread 1]", body) + self.assertIn("Top-level feedback waiting on the author:", body) + self.assertIn("[Feedback 1]", body) + self.assertIn("give each review feedback item a clear outcome", body) + + def test_waiting_on_author_caps_feedback_links_across_sections(self) -> None: + review_thread_urls = [ f"https://github.com/open-telemetry/example/pull/1#discussion_r{index}" - for index in range(pr_status_comment.AUTHOR_ACTION_DISCUSSION_LINK_LIMIT + 2) + for index in range(pr_status_comment.AUTHOR_ACTION_FEEDBACK_LINK_LIMIT - 1) + ] + top_level_feedback_urls = [ + f"https://github.com/open-telemetry/example/pull/1#pullrequestreview-{index}" + for index in range(3) ] body = pr_status_comment.render_status_comment( @@ -50,14 +60,16 @@ def test_waiting_on_author_caps_discussion_links(self) -> None: "route": "author", "facts": { "author": "alice", - "author_action_discussion_urls": urls, + "author_action_review_thread_urls": review_thread_urls, + "author_action_top_level_feedback_urls": top_level_feedback_urls, }, }, ) - self.assertEqual(pr_status_comment.AUTHOR_ACTION_DISCUSSION_LINK_LIMIT, body.count("- [Discussion ")) - self.assertIn("- 2 more unresolved discussions not shown", body) - self.assertNotIn(urls[-1], body) + self.assertEqual(len(review_thread_urls), body.count("- [Thread ")) + self.assertEqual(1, body.count("- [Feedback ")) + self.assertIn("- 2 more top-level feedback items not shown", body) + self.assertNotIn(top_level_feedback_urls[-1], body) def test_draft_waits_on_author(self) -> None: body = pr_status_comment.render_status_comment(self.pr(draft=True), None) @@ -74,16 +86,19 @@ def test_merged_pr_has_no_author_guidance(self) -> None: ) self.assertIn("**Status:** This pull request has been merged.", body) - self.assertNotIn("give every review discussion a clear outcome", body) + self.assertNotIn("give each review feedback item a clear outcome", body) - def test_terminal_pr_has_no_author_discussion_links(self) -> None: + def test_terminal_pr_has_no_author_feedback_links(self) -> None: result = { "route": "author", "facts": { "author": "alice", - "author_action_discussion_urls": [ + "author_action_review_thread_urls": [ "https://github.com/open-telemetry/example/pull/1#discussion_r1", ], + "author_action_top_level_feedback_urls": [ + "https://github.com/open-telemetry/example/pull/1#pullrequestreview-2", + ], }, } @@ -91,8 +106,10 @@ def test_terminal_pr_has_no_author_discussion_links(self) -> None: with self.subTest(overrides=overrides): body = pr_status_comment.render_status_comment(self.pr(**overrides), result) - self.assertNotIn("Unresolved discussions waiting on the author", body) - self.assertNotIn("[Discussion 1]", body) + self.assertNotIn("Unresolved inline review threads waiting on the author", body) + self.assertNotIn("Top-level feedback waiting on the author", body) + self.assertNotIn("[Thread 1]", body) + self.assertNotIn("[Feedback 1]", body) def test_blank_login_falls_back_to_author(self) -> None: body = pr_status_comment.render_status_comment( diff --git a/.github/scripts/pull-request-dashboard/test_render.py b/.github/scripts/pull-request-dashboard/test_render.py index 2015763847..ed8a9f1ba6 100644 --- a/.github/scripts/pull-request-dashboard/test_render.py +++ b/.github/scripts/pull-request-dashboard/test_render.py @@ -54,7 +54,7 @@ def test_reviewer_legend_includes_top_level_feedback(self) -> None: markdown = render_pr_tables([], {}) self.assertIn( - "💬 open discussion · 📌 author action pending · 🔴 changes requested.", + "💬 open review thread · 📌 author action pending · 🔴 changes requested.", markdown, ) diff --git a/.github/scripts/pull-request-dashboard/test_route_presentation.py b/.github/scripts/pull-request-dashboard/test_route_presentation.py index ec54f7f553..e70a8fc9ff 100644 --- a/.github/scripts/pull-request-dashboard/test_route_presentation.py +++ b/.github/scripts/pull-request-dashboard/test_route_presentation.py @@ -15,7 +15,7 @@ def test_every_route_has_dashboard_and_status_presentation(self) -> None: def test_author_status_includes_effective_author(self) -> None: self.assertEqual( - "Waiting on @alice to address or respond to unresolved review discussions.", + "Waiting on @alice to address or respond to review feedback.", route_status("author", "@alice"), ) diff --git a/pull-request-dashboard/README.md b/pull-request-dashboard/README.md index 2f6b283619..387fad83ad 100644 --- a/pull-request-dashboard/README.md +++ b/pull-request-dashboard/README.md @@ -138,14 +138,14 @@ the dashboard avoids leaving an active author indefinitely marked as blocked. After the first full dashboard run has populated repository state, each targeted PR update creates or updates one dashboard-managed status comment on that PR. The comment shows who has the next action and what the PR is waiting on. When the -author has the next action, it also links to relevant unresolved review -discussions when possible. Draft PRs show that they are waiting for the author -to mark them ready for review. +author has the next action, it also links separately to unresolved inline review +threads and top-level feedback when possible. Draft PRs show that they are +waiting for the author to mark them ready for review. A hidden marker lets the workflow update the comment in place. Existing one-time guidance comments are upgraded rather than duplicated. The comment also asks -authors to give each review discussion a clear outcome, which keeps stale or -ambiguous threads from being routed to the wrong person. +authors to give each review feedback item a clear outcome, which keeps stale or +ambiguous feedback from being routed to the wrong person. Reviewers should prefer inline comments for feedback requiring explicit resolution. See