diff --git a/.github/scripts/pull-request-dashboard/CONTEXT.md b/.github/scripts/pull-request-dashboard/CONTEXT.md index 1fd1b2e867de..80521d0c215f 100644 --- a/.github/scripts/pull-request-dashboard/CONTEXT.md +++ b/.github/scripts/pull-request-dashboard/CONTEXT.md @@ -30,7 +30,7 @@ diagnostics keep typed classification results and freeze only the source discussion records. `state.py` owns the JSON boundary. Its dashboard facts, stored-result, and state -codecs translate the immutable contracts to the version 15 +codecs translate the immutable contracts to the version 16 `dashboard-state.json` shape. Malformed pull request entries are discarded individually, so one bad entry does not prevent valid entries from loading. diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index 757f524c0a70..5e8acb69536b 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -280,7 +280,9 @@ the implementation understandable and operationally cheap. CI failure, including when review feedback also needs author action. Repository-configured `non_blocking_check_patterns` identify failed optional checks in a note alongside this action, without changing required-check facts - or routing. + or routing. An optional check in `ACTION_REQUIRED` is not included in that + failure-only note: it does not block merge and has neither failed nor been + cancelled. - A merge conflict does not decide who should act. Discussion, CI, and approval routing still identify the owner, while the conflict remains visible as a separate merge blocker. This lets maintainers handle routine conflicts, such @@ -301,6 +303,16 @@ the implementation understandable and operationally cheap. author is never held, because a failing check or new author-owned discussion is evidence the gates cannot undo. Unavailable check results hold the handoff for the same reason a pending one does, and resolve on a later run. +- An `ACTION_REQUIRED` check from the GitHub Actions app with an attached + workflow run is the exception to that hold. That metadata identifies a + workflow approval: a final, reported result that still blocks merge because + someone with repository write access must unblock the workflow. It remains + unsettled and appears as ๐Ÿ” in the CI column, but it does not hold routing + with the author. The PR routes to reviewers while approvals are outstanding, + then to maintainers once it has enough approvals. `ACTION_REQUIRED` from + another app, or without a workflow run, has unknown ownership and routes + conservatively as a failure. A genuine or unknown-owner required-check + failure alongside a workflow approval also routes to the author. - A held PR is presented as waiting on its author rather than on the robot it is waiting for, so a separate route would add a section that nobody is expected to act on. What it waits for is named in the columns instead: the CI diff --git a/.github/scripts/pull-request-dashboard/copilot_review.py b/.github/scripts/pull-request-dashboard/copilot_review.py index 57056b40f4ed..ea3285d404ef 100644 --- a/.github/scripts/pull-request-dashboard/copilot_review.py +++ b/.github/scripts/pull-request-dashboard/copilot_review.py @@ -222,7 +222,7 @@ def stale_request_reason( failing = [ check for check in checks - if check.bucket in ("fail", "cancel") + if check.bucket in ("fail", "cancel", "action_required") ] if failing: return f"required checks are failing: {named_checks(failing)}" diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index ebbf3a605824..93b3a0fe3490 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -116,6 +116,11 @@ fetched. ci_failing_since str (iso) Earliest completion time among current required failures. + ci_maintainer_action_required_count + int Required checks that need a + maintainer-owned permission + action; absent when checks + could not be fetched. ci_pending_count int Merge-blocking checks only; absent when checks could not be fetched, and excludes required @@ -193,9 +198,9 @@ Copilot review are still outstanding. required_checks_settled bool Every required check has - reported on the current head, - so the computed route is not - provisional. + reported a final result, and + none is waiting for + maintainer action. route_held_since str (iso) When the gates first kept this PR off its reviewers on this head. Cleared once every gate diff --git a/.github/scripts/pull-request-dashboard/dashboard_contracts.py b/.github/scripts/pull-request-dashboard/dashboard_contracts.py index 7fe5174ec253..8a58111fdb29 100644 --- a/.github/scripts/pull-request-dashboard/dashboard_contracts.py +++ b/.github/scripts/pull-request-dashboard/dashboard_contracts.py @@ -102,6 +102,7 @@ class DashboardFacts: last_approver_activity_at: str = "" ci_failing_count: int | None = None ci_failing_since: str | None = None + ci_maintainer_action_required_count: int | None = None ci_pending_count: int | None = None non_blocking_check_failures: tuple[str, ...] = () copilot_first_review_missing_since: str | None = None diff --git a/.github/scripts/pull-request-dashboard/github_cli.py b/.github/scripts/pull-request-dashboard/github_cli.py index bd2e95561632..f69b3cdfda28 100644 --- a/.github/scripts/pull-request-dashboard/github_cli.py +++ b/.github/scripts/pull-request-dashboard/github_cli.py @@ -14,6 +14,7 @@ DEFAULT_OWNER = "open-telemetry" COPILOT_REVIEWER_BOT_ID = "BOT_kgDOCnlnWA" CODE_SCANNING_APP_ID = 57789 # github-advanced-security +GITHUB_ACTIONS_APP_ID = 15368 REQUEST_COPILOT_REVIEW_MUTATION = """ @@ -362,12 +363,24 @@ def fetch_pr_reviews(owner: str, repo_name: str, number: int) -> list[dict[str, """ -def check_bucket(state: str) -> str: +def check_bucket( + state: str, + *, + integration_id: int | None = None, + workflow_run_id: int | None = None, +) -> str: if state == "SUCCESS": return "pass" if state in ("SKIPPED", "NEUTRAL"): return "skipping" - if state in ("ERROR", "FAILURE", "TIMED_OUT", "ACTION_REQUIRED", "STARTUP_FAILURE"): + if state == "ACTION_REQUIRED": + if ( + integration_id == GITHUB_ACTIONS_APP_ID + and workflow_run_id is not None + ): + return "maintainer_action_required" + return "action_required" + if state in ("ERROR", "FAILURE", "TIMED_OUT", "STARTUP_FAILURE"): return "fail" if state == "CANCELLED": return "cancel" @@ -380,6 +393,8 @@ def normalize_check(node: dict[str, Any]) -> dict[str, Any]: app = suite.get("app") or {} workflow_run = suite.get("workflowRun") or {} workflow = workflow_run.get("workflow") or {} + integration_id = None if is_status else app.get("databaseId") + workflow_run_id = None if is_status else workflow_run.get("databaseId") state = ( node.get("state") if is_status @@ -388,15 +403,19 @@ def normalize_check(node: dict[str, Any]) -> dict[str, Any]: return { "name": (node.get("context") if is_status else node.get("name")) or "", "state": state, - "bucket": check_bucket(state), + "bucket": check_bucket( + state, + integration_id=integration_id, + workflow_run_id=workflow_run_id, + ), "workflow": workflow.get("name") or "", - "workflow_run_id": workflow_run.get("databaseId"), + "workflow_run_id": workflow_run_id, "description": node.get("description") or "", "link": (node.get("targetUrl") if is_status else node.get("detailsUrl")) or "", "started_at": (node.get("createdAt") if is_status else node.get("startedAt")) or "", "completed_at": (node.get("createdAt") if is_status else node.get("completedAt")) or "", "check_run_id": None if is_status else check_run_id(node["url"]), - "integration_id": None if is_status else app.get("databaseId"), + "integration_id": integration_id, "status_context": is_status, } @@ -488,7 +507,8 @@ def gh_pr_check_rollup( "non_blocking_failures": [ check for check, is_required in checks - if not is_required and check.get("bucket") in ("fail", "cancel") + if not is_required + and check.get("bucket") in ("fail", "cancel") ], "code_scanning": [ check diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index 90f598f401e1..8b77ed8ef827 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -216,6 +216,18 @@ def author_body( return [fallback_next_step] +def workflow_action_required_summary(count: int) -> str: + if count == 1: + return ( + "1 required check needs action from someone with write access " + "to this repository." + ) + return ( + f"{count} required checks need action from someone with write access " + "to this repository." + ) + + def is_terminal_pr(pr: dict[str, Any]) -> bool: return bool(pr.get("merged")) or (pr.get("state") or "").lower() == "closed" @@ -235,6 +247,9 @@ def render_status_comment( top_level_feedback_urls = facts.author_action_top_level_feedback_urls feedback_count = len(review_thread_urls) + len(top_level_feedback_urls) failing_count = facts.ci_failing_count or 0 + maintainer_action_required_count = ( + facts.ci_maintainer_action_required_count or 0 + ) non_blocking_check_failures = facts.non_blocking_check_failures override_route = "" @@ -289,7 +304,14 @@ def render_status_comment( body = ( ["Resolve merge conflicts, then merge when ready."] if conflicted and route is DashboardRoute.MAINTAINER - else [next_step] + else ( + ["Approve or otherwise unblock the required workflow checks."] + if ( + route is DashboardRoute.MAINTAINER + and maintainer_action_required_count + ) + else [next_step] + ) ) abandoned_gates = ( abandoned_gate_note(facts) @@ -317,6 +339,16 @@ def render_status_comment( body.extend(["", f"**{label}:** {names}"]) if conflicted and route is not DashboardRoute.MAINTAINER: body.extend(["", "**Also blocked by:** Merge conflicts."]) + if maintainer_action_required_count: + body.extend([ + "", + ( + "**Workflow action required:** " + + workflow_action_required_summary( + maintainer_action_required_count + ) + ), + ]) lines = [ STATUS_MARKER, diff --git a/.github/scripts/pull-request-dashboard/pull_request_evaluation.py b/.github/scripts/pull-request-dashboard/pull_request_evaluation.py index 2d0cce546746..604fe04beb68 100644 --- a/.github/scripts/pull-request-dashboard/pull_request_evaluation.py +++ b/.github/scripts/pull-request-dashboard/pull_request_evaluation.py @@ -194,13 +194,18 @@ def _compute_facts( failing = [ check for check in checks or () - if check.bucket in ("fail", "cancel") + if check.bucket in ("fail", "cancel", "action_required") ] pending = [ check for check in checks or () if check.bucket == "pending" ] + maintainer_action_required = [ + check + for check in checks or () + if check.bucket == "maintainer_action_required" + ] failing_timestamps = [parse_ts(check.completed_at) for check in failing] failing_timestamps = [ts for ts in failing_timestamps if ts is not None] created_ts = parse_ts(pr.created_at) @@ -280,6 +285,11 @@ def _compute_facts( if failing_timestamps else None ), + ci_maintainer_action_required_count=( + len(maintainer_action_required) + if checks is not None + else None + ), ci_pending_count=len(pending) if checks is not None else None, non_blocking_check_failures=non_blocking_check_failures, ) diff --git a/.github/scripts/pull-request-dashboard/render.py b/.github/scripts/pull-request-dashboard/render.py index 69d13ea4cba6..3c4f3f92556c 100644 --- a/.github/scripts/pull-request-dashboard/render.py +++ b/.github/scripts/pull-request-dashboard/render.py @@ -91,10 +91,24 @@ def render_draft_pr_section( def ci_cell(facts: DashboardFacts) -> str: - if facts.ci_failing_count is None and facts.ci_pending_count is None: + if ( + facts.ci_failing_count is None + and facts.ci_maintainer_action_required_count is None + and facts.ci_pending_count is None + ): return "?" - if (facts.ci_failing_count or 0) > 0: + failing = (facts.ci_failing_count or 0) > 0 + write_access_required = ( + facts.ci_maintainer_action_required_count or 0 + ) > 0 + if failing and write_access_required: + return "โŒ ๐Ÿ”" + if failing: return "โŒ" + if (facts.ci_pending_count or 0) > 0 and write_access_required: + return "โณ ๐Ÿ”" + if write_access_required: + return "๐Ÿ”" if (facts.ci_pending_count or 0) > 0: return "โณ" return "โœ…" @@ -237,11 +251,17 @@ def render_pr_tables( "โณ review pending ยท ๐Ÿ’ฌ open review thread ยท ๐Ÿ“Œ top-level feedback needs author action ยท " "๐Ÿ”ด changes requested." ) + ci_note = ( + "CI column: โœ… passing ยท โณ running ยท โŒ failing ยท " + "๐Ÿ” workflow action required." + ) out: list[str] = [ "> [!NOTE]", f"> {grouping_note}", ">", f"> {reviewers_note}", + ">", + f"> {ci_note}", "", ] diff --git a/.github/scripts/pull-request-dashboard/route_presentation.py b/.github/scripts/pull-request-dashboard/route_presentation.py index 824b196c166a..9b964d56f8c6 100644 --- a/.github/scripts/pull-request-dashboard/route_presentation.py +++ b/.github/scripts/pull-request-dashboard/route_presentation.py @@ -1,6 +1,7 @@ from __future__ import annotations from dashboard_contracts import DashboardFacts, DashboardRoute +from utils import required_checks_unreported ROUTE_PRESENTATION = { @@ -73,7 +74,7 @@ def unreported_gate_phrase(facts: DashboardFacts) -> str: # findings holds it but has reported, so naming it would send the reader # after a gate that arrived. gates = [] - if not facts.required_checks_settled: + if required_checks_unreported(facts): gates.append("the required status checks") if facts.copilot_review_unreported: gates.append("the Copilot review") @@ -87,7 +88,7 @@ def held_gate_phrase(facts: DashboardFacts) -> str: # review, which has already arrived. A held route always has one of these, # so the phrase is never empty while the pull request is held. gates = [] - if not facts.required_checks_settled: + if required_checks_unreported(facts): gates.append("the required status checks") if facts.copilot_review_unreported: gates.append("the Copilot review") diff --git a/.github/scripts/pull-request-dashboard/routing_decision.py b/.github/scripts/pull-request-dashboard/routing_decision.py index 79ade178ea34..a9d4ce21db00 100644 --- a/.github/scripts/pull-request-dashboard/routing_decision.py +++ b/.github/scripts/pull-request-dashboard/routing_decision.py @@ -15,7 +15,13 @@ set_copilot_review_request_needed, ) from dashboard_contracts import DashboardFacts, DashboardRoute -from utils import format_ts, parse_ts, required_checks_settled, utc_now +from utils import ( + format_ts, + parse_ts, + required_checks_settled, + required_checks_unreported, + utc_now, +) @dataclass(frozen=True) @@ -166,11 +172,12 @@ def _hold_route_until_gates_settle( ), required_checks_settled=required_checks_settled(facts), ) + checks_unreported = required_checks_unreported(facts) gates_outstanding = gates_enabled and ( - not facts.required_checks_settled or facts.copilot_review_outstanding + checks_unreported or facts.copilot_review_outstanding ) unreported_gates = gates_enabled and ( - not facts.required_checks_settled or facts.copilot_review_unreported + checks_unreported or facts.copilot_review_unreported ) would_hold = _route_progress(route) > _route_progress(effective_previous_route) facts = _set_gate_hold_clock( diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index bc7a3329a699..4d9499c3deeb 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -36,7 +36,7 @@ # current vector, ordinary state loaders may regenerate mismatched disposable # caches. Every constant ending in _STATE_VERSION or _REVISION is included. # dashboard-state.json: accepted PR routing results and backfill readiness. -DASHBOARD_STATE_VERSION = 15 +DASHBOARD_STATE_VERSION = 16 # backfill-state.json: round-robin cursor used by full dashboard refreshes. BACKFILL_STATE_VERSION = 3 # notification-state.json: pending and delivered Slack notification records. @@ -49,7 +49,7 @@ STATUS_COMMENT_ROLLOUT_STATE_VERSION = 2 # Rendered status-comment behavior. Increment when existing comments need to # adopt a change; hourly runs durably roll it out to all open PRs. -STATUS_COMMENT_REVISION = 16 +STATUS_COMMENT_REVISION = 19 INITIAL_BACKFILL_COMPLETE_KEY = "initial_backfill_complete" _state_dir: Path | None = None @@ -560,6 +560,10 @@ def decode_dashboard_facts(value: Any) -> DashboardFacts: value.get("ci_failing_since"), "facts.ci_failing_since", ), + ci_maintainer_action_required_count=_optional_integer( + value.get("ci_maintainer_action_required_count"), + "facts.ci_maintainer_action_required_count", + ), ci_pending_count=_optional_integer( value.get("ci_pending_count"), "facts.ci_pending_count", @@ -682,6 +686,10 @@ def encode_dashboard_facts(facts: DashboardFacts) -> dict[str, Any]: stored["ci_failing_count"] = facts.ci_failing_count if facts.ci_failing_since is not None: stored["ci_failing_since"] = facts.ci_failing_since + if facts.ci_maintainer_action_required_count is not None: + stored["ci_maintainer_action_required_count"] = ( + facts.ci_maintainer_action_required_count + ) if facts.ci_pending_count is not None: stored["ci_pending_count"] = facts.ci_pending_count if facts.non_blocking_check_failures: @@ -819,7 +827,6 @@ def load_dashboard_state_cache() -> DashboardState | None: state = load_state_file( dashboard_state_path(), DASHBOARD_STATE_VERSION, - compatible_versions=(11, 12, 13), ) if state is None: return None diff --git a/.github/scripts/pull-request-dashboard/test_copilot_review.py b/.github/scripts/pull-request-dashboard/test_copilot_review.py index 3874c187d30e..50551df5719e 100644 --- a/.github/scripts/pull-request-dashboard/test_copilot_review.py +++ b/.github/scripts/pull-request-dashboard/test_copilot_review.py @@ -986,6 +986,23 @@ def test_pending_required_checks_do_not_make_a_request_stale(self) -> None: ), ) + def test_request_recorded_while_pending_is_stale_after_action_required( + self, + ) -> None: + # The unchanged fingerprint models a request recorded while this check + # was pending because check results are not part of that fingerprint. + self.assertEqual( + "required checks are failing: build", + self.reason( + raw={ + "checks": [ + {"name": "build", "bucket": "action_required"}, + {"name": "lint", "bucket": "pass"}, + ], + }, + ), + ) + def test_summarizes_long_lists_of_failing_checks(self) -> None: self.assertEqual( "required checks are failing: a, b, c and 2 more", diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index 857b7d9810e4..dc959f4e7939 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -80,6 +80,7 @@ evaluate_pull_request, ) from pull_request_activity import PullRequestActivity +from render import render_pr_tables from reviewer_state import ReviewerInput, prepare_reviewers from routing_decision import resolve_routing @@ -992,12 +993,18 @@ def test_normal_routing_flows_through_evaluation( @patch("routing_decision.utc_now") @patch("pull_request_evaluation.fetch_pull_request_source") - def test_running_required_check_keeps_integrated_route_held( + def test_running_check_and_workflow_approval_keep_integrated_route_held( self, fetch_raw: Mock, utc_now: Mock ) -> None: utc_now.return_value = datetime(2026, 8, 16, 12, 0, tzinfo=timezone.utc) fetch_raw.return_value = self.raw_pr( - checks=[{"name": "required", "bucket": "pending"}] + checks=[ + {"name": "required", "bucket": "pending"}, + { + "name": "workflow approval", + "bucket": "maintainer_action_required", + }, + ] ) classifier = FakeClassificationOperation() @@ -1022,6 +1029,22 @@ def test_running_required_check_keeps_integrated_route_held( self.assertEqual( "2026-08-16T12:00:00+00:00", result.facts.route_held_since ) + self.assertEqual(1, result.facts.ci_pending_count) + self.assertEqual(1, result.facts.ci_maintainer_action_required_count) + markdown = render_pr_tables( + [{ + "number": 7, + "title": "Pull request", + "author": {"login": "author"}, + "isDraft": False, + }], + (stored_dashboard_result( + 7, + route=result.route, + facts=result.facts, + ),), + ) + self.assertIn("| โณ ๐Ÿ” |", markdown) self.assertEqual(len(classifier.requests), 1) @patch("pull_request_evaluation.fetch_pull_request_source") @@ -1819,15 +1842,21 @@ def test_non_blocking_check_failures_use_deterministic_casefold_tiebreaker(self) def test_required_check_buckets_control_ci_facts(self) -> None: cases = ( - ("TIMED_OUT", "fail", 1, 0), - ("ACTION_REQUIRED", "fail", 1, 0), - ("STARTUP_FAILURE", "fail", 1, 0), - ("CANCELLED", "cancel", 1, 0), - ("IN_PROGRESS", "pending", 0, 1), - ("SKIPPED", "skipping", 0, 0), - ("SUCCESS", "pass", 0, 0), - ) - for state, bucket, failing, pending in cases: + ("TIMED_OUT", "fail", 1, 0, 0), + ( + "ACTION_REQUIRED", + "action_required", + 1, + 0, + 0, + ), + ("STARTUP_FAILURE", "fail", 1, 0, 0), + ("CANCELLED", "cancel", 1, 0, 0), + ("IN_PROGRESS", "pending", 0, 0, 1), + ("SKIPPED", "skipping", 0, 0, 0), + ("SUCCESS", "pass", 0, 0, 0), + ) + for state, bucket, failing, maintainer_action, pending in cases: with self.subTest(state=state, bucket=bucket): facts = evaluation_facts( { @@ -1849,12 +1878,65 @@ def test_required_check_buckets_control_ci_facts(self) -> None: ) self.assertEqual(failing, facts.ci_failing_count) + self.assertEqual( + maintainer_action, + facts.ci_maintainer_action_required_count, + ) self.assertEqual(pending, facts.ci_pending_count) self.assertEqual( ("workflow-notification",), facts.non_blocking_check_failures, ) + def test_workflow_approval_is_distinct_from_generic_action_required( + self, + ) -> None: + facts = evaluation_facts( + { + "pr": { + "createdAt": "2026-07-14T01:00:00Z", + "author": {"login": "author"}, + "mergeStateStatus": "CLEAN", + "mergeable": "MERGEABLE", + }, + "checks": [ + {"state": "ACTION_REQUIRED", "bucket": "action_required"}, + { + "state": "ACTION_REQUIRED", + "bucket": "maintainer_action_required", + }, + ], + }, + "author", + [], + ) + + self.assertEqual(1, facts.ci_failing_count) + self.assertEqual(1, facts.ci_maintainer_action_required_count) + + @patch("pull_request_evaluation.fetch_pull_request_source") + def test_generic_action_required_routes_to_author( + self, + fetch_source: Mock, + ) -> None: + fetch_source.return_value = pull_request_source( + checks=(check_source( + state="ACTION_REQUIRED", + bucket="action_required", + ),), + ) + + result = evaluate_pr({"number": 7}) + + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual(DashboardRoute.AUTHOR, result.route) + self.assertEqual(1, result.facts.ci_failing_count) + self.assertEqual( + 0, + result.facts.ci_maintainer_action_required_count, + ) + def test_override_command_does_not_clear_required_check_failures(self) -> None: facts = evaluation_facts( { @@ -1887,6 +1969,48 @@ def test_override_command_does_not_clear_required_check_failures(self) -> None: self.assertEqual(3, facts.ci_failing_count) self.assertEqual("2026-07-17T01:00:00+00:00", facts.ci_failing_since) + @patch("pull_request_evaluation.fetch_pull_request_source") + def test_permission_owned_blockers_route_like_audited_pull_requests( + self, + fetch_source: Mock, + ) -> None: + cases = ( + (4998, "opentelemetry-python-contrib", (), DashboardRoute.APPROVER), + ( + 3706, + "opentelemetry-js-contrib", + (review_source(state="APPROVED", body=""),), + DashboardRoute.MAINTAINER, + ), + ) + for number, repository, reviews, expected_route in cases: + with self.subTest(number=number, repository=repository): + fetch_source.return_value = pull_request_source( + pull_request=pull_request_metadata( + number=number, + title=f"{repository} workflow approval", + ), + reviews=reviews, + checks=(check_source( + name="workflow approval", + state="ACTION_REQUIRED", + bucket="maintainer_action_required", + ),), + ) + + result = evaluate_pr({"number": number}) + + self.assertIsInstance(result, EvaluationSuccess) + assert isinstance(result, EvaluationSuccess) + self.assertEqual(expected_route, result.route) + self.assertEqual(0, result.facts.ci_failing_count) + self.assertEqual( + 1, + result.facts.ci_maintainer_action_required_count, + ) + self.assertFalse(result.facts.required_checks_settled) + self.assertFalse(result.facts.route_held_for_gates) + class ActivityFactsIntegrationTest(unittest.TestCase): def test_formats_activity_clocks_and_clamps_overall_activity_to_creation( diff --git a/.github/scripts/pull-request-dashboard/test_github_cli.py b/.github/scripts/pull-request-dashboard/test_github_cli.py index 402c711b636d..a23e5a27a14a 100644 --- a/.github/scripts/pull-request-dashboard/test_github_cli.py +++ b/.github/scripts/pull-request-dashboard/test_github_cli.py @@ -5,6 +5,7 @@ from github_cli import ( TransientGhError, + check_bucket, code_scanning_tools, fetch_pr_issue_comments, fetch_pr_reviews, @@ -18,6 +19,7 @@ is_retryable_gh_error, list_open_prs, merge_code_scanning_checks, + normalize_check, request_copilot_review, required_check_contexts, required_code_scanning_checks, @@ -130,6 +132,72 @@ def test_follows_pagination(self) -> None: class GithubCliTest(unittest.TestCase): + def test_generic_action_required_keeps_unknown_ownership(self) -> None: + self.assertEqual( + "action_required", + check_bucket("ACTION_REQUIRED"), + ) + self.assertEqual("fail", check_bucket("FAILURE")) + self.assertEqual("pending", check_bucket("IN_PROGRESS")) + + def test_actions_workflow_approval_has_a_maintainer_owned_bucket( + self, + ) -> None: + check = normalize_check({ + "__typename": "CheckRun", + "name": "build", + "status": "COMPLETED", + "conclusion": "ACTION_REQUIRED", + "url": "https://github.com/open-telemetry/example/runs/1", + "checkSuite": { + "app": {"databaseId": 15368}, + "workflowRun": { + "databaseId": 101, + "workflow": {"name": "CI"}, + }, + }, + }) + + self.assertEqual("maintainer_action_required", check["bucket"]) + + def test_actions_action_required_without_a_workflow_run_is_generic( + self, + ) -> None: + check = normalize_check({ + "__typename": "CheckRun", + "name": "custom check", + "status": "COMPLETED", + "conclusion": "ACTION_REQUIRED", + "url": "https://github.com/open-telemetry/example/runs/1", + "checkSuite": {"app": {"databaseId": 15368}}, + }) + + self.assertEqual("action_required", check["bucket"]) + + @patch("github_cli.gh_graphql") + def test_optional_action_required_check_remains_non_blocking( + self, + graphql, + ) -> None: + graphql.return_value = _rollup_page([{ + "__typename": "CheckRun", + "name": "optional-deploy", + "status": "COMPLETED", + "conclusion": "ACTION_REQUIRED", + "url": "https://github.com/open-telemetry/example/runs/1", + "isRequired": False, + }]) + + rollup = gh_pr_check_rollup( + "open-telemetry/example", + "PR_id", + ["optional-*"], + ) + + assert rollup is not None + self.assertEqual([], rollup["required"]) + self.assertEqual([], rollup["non_blocking_failures"]) + @patch("github_cli.run_gh_json") def test_pr_view_fetches_body_for_routing_freshness(self, run_json) -> None: run_json.return_value = {"mergeable": "MERGEABLE"} 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 9a251567f793..a965c15f8169 100644 --- a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py @@ -359,6 +359,100 @@ def test_waiting_on_author_names_required_ci_failure(self) -> None: self.assertNotIn("### Review feedback", body) self.assertNotIn(pr_status_comment.RESPONSE_EXAMPLES, body) + def test_generic_action_required_renders_as_an_author_owned_failure( + self, + ) -> None: + body = pr_status_comment.render_status_comment( + self.pr(), + status_result( + DashboardRoute.AUTHOR, + ci_failing_count=1, + ci_maintainer_action_required_count=0, + ci_pending_count=0, + ), + ) + + self.assertIn("Investigate required status check failures.", body) + self.assertNotIn("Workflow action required", body) + + def test_waiting_on_reviewers_names_maintainer_owned_blocker(self) -> None: + body = pr_status_comment.render_status_comment( + self.pr(), + status_result( + DashboardRoute.APPROVER, + ci_failing_count=0, + ci_maintainer_action_required_count=1, + ci_pending_count=0, + ), + ) + + self.assertIn("**Waiting on reviewers** ยท refreshed ", body) + self.assertIn("Review the latest changes.", body) + self.assertIn( + "**Workflow action required:** 1 required check needs action from someone " + "with write access to this repository.", + body, + ) + self.assertNotIn("status check is failing", body) + + def test_pending_check_and_workflow_action_name_both_blockers(self) -> None: + body = pr_status_comment.render_status_comment( + self.pr(), + status_result( + DashboardRoute.AUTHOR, + ci_failing_count=0, + ci_maintainer_action_required_count=1, + ci_pending_count=1, + required_checks_settled=False, + route_held_for_gates=True, + ), + ) + + self.assertIn( + "Wait for the required status checks to report; this pull request " + "moves to reviewers once the results are clean.", + body, + ) + self.assertIn( + "**Workflow action required:** 1 required check needs action from someone " + "with write access to this repository.", + body, + ) + + def test_waiting_on_maintainers_leads_with_permission_action(self) -> None: + body = pr_status_comment.render_status_comment( + self.pr(), + status_result( + DashboardRoute.MAINTAINER, + approval_count=1, + ci_failing_count=0, + ci_maintainer_action_required_count=2, + ci_pending_count=0, + ), + ) + + self.assertIn("Approve or otherwise unblock the required workflow checks.", body) + self.assertIn( + "**Workflow action required:** 2 required checks need action from someone " + "with write access to this repository.", + body, + ) + self.assertNotIn("Merge when ready.", body) + + def test_mixed_failure_and_permission_action_names_both_blockers(self) -> None: + body = pr_status_comment.render_status_comment( + self.pr(), + status_result( + DashboardRoute.AUTHOR, + ci_failing_count=1, + ci_maintainer_action_required_count=1, + ci_pending_count=0, + ), + ) + + self.assertIn("Investigate required status check failures.", body) + self.assertIn("**Workflow action required:**", body) + def test_waiting_on_author_names_merge_conflicts(self) -> None: body = pr_status_comment.render_status_comment( self.pr(), diff --git a/.github/scripts/pull-request-dashboard/test_pull_request_source.py b/.github/scripts/pull-request-dashboard/test_pull_request_source.py index 5051d0253574..c874fb6ccc77 100644 --- a/.github/scripts/pull-request-dashboard/test_pull_request_source.py +++ b/.github/scripts/pull-request-dashboard/test_pull_request_source.py @@ -136,7 +136,13 @@ def test_normalizes_mixed_gh_rest_and_graphql_shapes(self) -> None: "state": "SUCCESS", "bucket": "pass", "integration_id": 1, - } + }, + { + "name": "workflow approval", + "state": "ACTION_REQUIRED", + "bucket": "maintainer_action_required", + "integration_id": 2, + }, ], "non_blocking_check_failures": [ { @@ -179,6 +185,10 @@ def test_normalizes_mixed_gh_rest_and_graphql_shapes(self) -> None: .user_logins, ) self.assertEqual("pass", source.checks[0].bucket) + self.assertEqual( + "maintainer_action_required", + source.checks[1].bucket, + ) self.assertEqual("optional", source.non_blocking_failures[0].name) def test_normalizes_bot_and_human_actor_cases(self) -> None: diff --git a/.github/scripts/pull-request-dashboard/test_render.py b/.github/scripts/pull-request-dashboard/test_render.py index 44704ff59d7f..430c1afd25e0 100644 --- a/.github/scripts/pull-request-dashboard/test_render.py +++ b/.github/scripts/pull-request-dashboard/test_render.py @@ -8,6 +8,7 @@ stored_dashboard_result, ) from render import ( + ci_cell, render_draft_pr_section, render_pr_tables, reviewers_cell_text, @@ -15,6 +16,45 @@ class RenderTest(unittest.TestCase): + def test_permission_owned_check_action_has_a_distinct_ci_icon(self) -> None: + self.assertEqual( + "๐Ÿ”", + ci_cell(dashboard_facts( + ci_failing_count=0, + ci_maintainer_action_required_count=1, + ci_pending_count=0, + )), + ) + + def test_pending_check_and_permission_action_show_both_icons(self) -> None: + self.assertEqual( + "โณ ๐Ÿ”", + ci_cell(dashboard_facts( + ci_failing_count=0, + ci_maintainer_action_required_count=1, + ci_pending_count=1, + )), + ) + + def test_mixed_failure_and_permission_action_shows_both_icons(self) -> None: + self.assertEqual( + "โŒ ๐Ÿ”", + ci_cell(dashboard_facts( + ci_failing_count=1, + ci_maintainer_action_required_count=1, + ci_pending_count=0, + )), + ) + + def test_ci_legend_explains_write_access_icon(self) -> None: + markdown = render_pr_tables([], ()) + + self.assertIn( + "CI column: โœ… passing ยท โณ running ยท โŒ failing ยท " + "๐Ÿ” workflow action required.", + markdown, + ) + def test_reviewer_legend_includes_top_level_feedback(self) -> None: markdown = render_pr_tables([], ()) diff --git a/.github/scripts/pull-request-dashboard/test_routing_decision.py b/.github/scripts/pull-request-dashboard/test_routing_decision.py index ef97e1188a91..08e60408f3e6 100644 --- a/.github/scripts/pull-request-dashboard/test_routing_decision.py +++ b/.github/scripts/pull-request-dashboard/test_routing_decision.py @@ -288,6 +288,53 @@ def test_required_check_failure_routes_human_authored_pr_to_author(self) -> None self.assertEqual("2026-07-17T01:00:00+00:00", outcome.facts.waiting_since) self.assertEqual("ci_failure", outcome.facts.waiting_age_basis) + def test_only_maintainer_owned_check_actions_route_by_approval_count( + self, + ) -> None: + for approval_count, expected in ( + (0, DashboardRoute.APPROVER), + (1, DashboardRoute.MAINTAINER), + ): + with self.subTest(approval_count=approval_count): + outcome = self.resolve({ + "approval_count": approval_count, + "ci_failing_count": 0, + "ci_maintainer_action_required_count": 1, + "ci_pending_count": 0, + "is_maintenance_bot": False, + }) + + self.assertEqual(expected, outcome.route) + self.assertFalse(outcome.facts.required_checks_settled) + self.assertFalse(outcome.facts.route_held_for_gates) + + def test_genuine_failure_wins_over_maintainer_owned_check_action( + self, + ) -> None: + outcome = self.resolve({ + "approval_count": 1, + "ci_failing_count": 1, + "ci_maintainer_action_required_count": 1, + "ci_pending_count": 0, + "is_maintenance_bot": False, + }) + + self.assertEqual(DashboardRoute.AUTHOR, outcome.route) + + def test_pending_checks_still_hold_maintainer_owned_action_route( + self, + ) -> None: + outcome = self.resolve({ + "approval_count": 0, + "ci_failing_count": 0, + "ci_maintainer_action_required_count": 1, + "ci_pending_count": 1, + "is_maintenance_bot": False, + }) + + self.assertEqual(DashboardRoute.AUTHOR, outcome.route) + self.assertTrue(outcome.facts.route_held_for_gates) + def test_reviewer_handoff_is_bound_to_the_current_head(self) -> None: self.assertTrue( reviewer_handoff_active( diff --git a/.github/scripts/pull-request-dashboard/test_state.py b/.github/scripts/pull-request-dashboard/test_state.py index dc16cade63d5..a6aff7d3a663 100644 --- a/.github/scripts/pull-request-dashboard/test_state.py +++ b/.github/scripts/pull-request-dashboard/test_state.py @@ -231,6 +231,7 @@ def test_dashboard_facts_codec_round_trip(self) -> None: last_approver_activity_at="2026-08-16T10:00:00Z", ci_failing_count=1, ci_failing_since="2026-08-16T09:00:00Z", + ci_maintainer_action_required_count=2, ci_pending_count=2, non_blocking_check_failures=("CodeQL",), copilot_first_review_missing_since="2026-08-16T08:30:00Z", @@ -296,7 +297,7 @@ def test_legacy_facts_infer_whether_the_author_can_act(self) -> None: ).route, ) - def test_version_thirteen_state_infers_author_capability(self) -> None: + def test_legacy_state_infers_author_capability(self) -> None: persisted = { "version": 13, "initial_backfill_complete": True, @@ -330,21 +331,10 @@ def test_version_thirteen_state_infers_author_capability(self) -> None: }, }, } - with ( - tempfile.TemporaryDirectory() as temp_dir, - patch("state._state_dir", Path(temp_dir)), - ): - dashboard_state_path().write_text( - json.dumps(persisted), - encoding="utf-8", - ) - - warnings = StringIO() - with redirect_stderr(warnings): - decoded = load_dashboard_state_cache() + warnings = StringIO() + with redirect_stderr(warnings): + decoded = decode_dashboard_state(persisted) - self.assertIsNotNone(decoded) - assert decoded is not None self.assertEqual(frozenset({124, 125, 126}), decoded.pr_numbers) decoded_by_number = { result.pr_number: result @@ -466,6 +456,7 @@ def test_dashboard_facts_accepts_null_optional_fields(self) -> None: decode_dashboard_facts({ "ci_failing_count": None, "ci_failing_since": None, + "ci_maintainer_action_required_count": None, "ci_pending_count": None, "copilot_first_review_missing_since": None, "route_held_since": None, @@ -541,7 +532,7 @@ def test_malformed_persisted_results_are_rejected_individually(self) -> None: "warning: ignoring malformed dashboard result" )) - def test_version_eleven_dashboard_state_migrates_to_current_shape(self) -> None: + def test_legacy_dashboard_state_payload_reencodes_to_current_shape(self) -> None: persisted = { "version": 11, "initial_backfill_complete": True, @@ -650,11 +641,27 @@ def test_version_eleven_dashboard_state_migrates_to_current_shape(self) -> None: def test_notification_state_version_is_independent(self) -> None: self.assertEqual(BACKFILL_STATE_VERSION, 3) self.assertEqual(NOTIFICATION_STATE_VERSION, 3) - self.assertEqual(DASHBOARD_STATE_VERSION, 15) + self.assertEqual(DASHBOARD_STATE_VERSION, 16) self.assertEqual(STATUS_COMMENT_ROLLOUT_STATE_VERSION, 2) self.assertEqual(AUTHOR_NUDGE_STATE_VERSION, 3) self.assertEqual(COPILOT_REVIEW_REQUEST_STATE_VERSION, 6) + def test_version_fifteen_dashboard_state_is_regenerated(self) -> None: + with ( + tempfile.TemporaryDirectory() as temp_dir, + patch("state._state_dir", Path(temp_dir)), + ): + dashboard_state_path().write_text( + json.dumps({ + "version": 15, + "initial_backfill_complete": True, + "prs": {}, + }), + encoding="utf-8", + ) + + self.assertIsNone(load_dashboard_state_cache()) + def test_author_nudge_state_round_trip(self) -> None: with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)): save_author_nudges({ diff --git a/.github/scripts/pull-request-dashboard/utils.py b/.github/scripts/pull-request-dashboard/utils.py index 3dc7a730a79d..4171ecd95a71 100644 --- a/.github/scripts/pull-request-dashboard/utils.py +++ b/.github/scripts/pull-request-dashboard/utils.py @@ -98,11 +98,23 @@ def is_copilot_reviewer_login(login: str) -> bool: def required_checks_settled(facts: DashboardFacts) -> bool: + if facts.ci_pending_count is None: + return False + return not ( + facts.ci_pending_count + or facts.ci_maintainer_action_required_count + ) + + +def required_checks_unreported(facts: DashboardFacts) -> bool: # A route computed while checks are still running is provisional because a # failure becomes visible only after the check completes. - if facts.ci_pending_count is None: + if facts.required_checks_settled: return False - return not facts.ci_pending_count + return ( + facts.ci_pending_count is None + or facts.ci_pending_count > 0 + ) def format_ts(ts: datetime | None) -> str: