From 0c8b222df457f69346fdc871ef8d43cea474a83b Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Wed, 22 Jul 2026 15:06:58 -0700 Subject: [PATCH 1/4] Guard dashboard delivery by explicit revision --- .../pull-request-dashboard/delivery.py | 14 +++++ .../scripts/pull-request-dashboard/state.py | 54 +++++++++++++++++++ .../pull-request-dashboard/test_delivery.py | 41 ++++++++++++++ .../pull-request-dashboard/test_state.py | 28 ++++++++++ .../workflows/pull-request-dashboard-repo.yml | 11 +++- 5 files changed, 146 insertions(+), 2 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/delivery.py b/.github/scripts/pull-request-dashboard/delivery.py index 131f0ad8bd0..80359302290 100644 --- a/.github/scripts/pull-request-dashboard/delivery.py +++ b/.github/scripts/pull-request-dashboard/delivery.py @@ -21,6 +21,7 @@ from pr_status_comment import update_status_comments_from_state from state import ( author_nudge_state_path, + claim_delivery_revision, copilot_review_request_state_path, notification_state_path, set_state_dir, @@ -112,14 +113,22 @@ def deliver_with_state( repo: str, state_branch_name: str, state_dir: Path, + github_output: Path | None = None, ) -> int: repo_key = repo_state_key(repo) author_retry = runner_temp_path("prior-author-nudge-state.json") copilot_retry = runner_temp_path("prior-copilot-review-request-state.json") notification_retry = runner_temp_path("prior-notification-state.json") errors: list[str] = [] + active_revision = False def deliver() -> int: + nonlocal active_revision + active_revision = claim_delivery_revision() + if not active_revision: + errors.clear() + print("a newer dashboard delivery revision is active; skipping", file=sys.stderr) + return 0 errors[:] = deliver_from_state( repo, author_retry, @@ -142,6 +151,9 @@ def deliver() -> int: ) if status != 0: return status + if github_output is not None: + with github_output.open("a", encoding="utf-8") as output: + output.write(f"active={'true' if active_revision else 'false'}\n") if not errors: return 0 print("Dashboard delivery failed:", file=sys.stderr) @@ -153,6 +165,7 @@ def main() -> int: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--repo", help="target repository name") parser.add_argument("--state-branch", required=True, help="git branch used for workflow state") + parser.add_argument("--github-output", type=Path, help="append the active revision result") args = parser.parse_args() repo = normalize_repo(args.repo) if args.repo else detect_repo() with state_branch.temporary_state_dir() as state_dir: @@ -161,6 +174,7 @@ def main() -> int: repo, args.state_branch, state_dir, + args.github_output, ) diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index 64db5c46017..1fc92c8e998 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -15,6 +15,7 @@ AUTHOR_NUDGE_STATE_FILE = "author-nudge-state.json" COPILOT_REVIEW_REQUEST_STATE_FILE = "copilot-review-request-state.json" STATUS_COMMENT_ROLLOUT_STATE_FILE = "status-comment-rollout-state.json" +DELIVERY_REVISION_STATE_FILE = "delivery-revision-state.json" # State files are disposable workflow caches, not durable user data. Bump only # the version for the state shape whose meaning changed. DASHBOARD_STATE_VERSION = 5 @@ -23,6 +24,10 @@ AUTHOR_NUDGE_STATE_VERSION = 2 COPILOT_REVIEW_REQUEST_STATE_VERSION = 3 STATUS_COMMENT_ROLLOUT_STATE_VERSION = 1 +DELIVERY_REVISION_STATE_VERSION = 1 +# Bump when older queued workers must not deliver against newer dashboard state +# or behavior. Unlike the state-shape versions above, this coordinates rollout. +DELIVERY_REVISION = 1 INITIAL_BACKFILL_COMPLETE_KEY = "initial_backfill_complete" _state_dir: Path | None = None @@ -62,6 +67,10 @@ def status_comment_rollout_state_path() -> Path: return state_dir() / STATUS_COMMENT_ROLLOUT_STATE_FILE +def delivery_revision_state_path() -> Path: + return state_dir() / DELIVERY_REVISION_STATE_FILE + + def dashboard_markdown_path() -> Path: return state_dir() / DASHBOARD_MARKDOWN_FILE @@ -91,6 +100,13 @@ def empty_status_comment_rollout_state() -> dict[str, Any]: } +def empty_delivery_revision_state() -> dict[str, Any]: + return { + "version": DELIVERY_REVISION_STATE_VERSION, + "active_revision": 0, + } + + def load_state_file( path: Path, current_version: int, @@ -183,6 +199,44 @@ def save_status_comment_rollout_state(state: dict[str, Any]) -> None: ) +def load_delivery_revision_state() -> dict[str, Any] | None: + path = delivery_revision_state_path() + if not path.exists(): + return empty_delivery_revision_state() + state = load_state_file(path, DELIVERY_REVISION_STATE_VERSION) + if state is None: + return None + try: + active_revision = max(int(state.get("active_revision") or 0), 0) + except (TypeError, ValueError): + return None + return { + "version": DELIVERY_REVISION_STATE_VERSION, + "active_revision": active_revision, + } + + +def save_delivery_revision_state(state: dict[str, Any]) -> None: + save_state_file( + delivery_revision_state_path(), + {"active_revision": int(state.get("active_revision") or 0)}, + DELIVERY_REVISION_STATE_VERSION, + ) + + +def claim_delivery_revision() -> bool: + state = load_delivery_revision_state() + if state is None: + print("delivery revision state is unreadable; skipping delivery", file=sys.stderr) + return False + active_revision = state["active_revision"] + if active_revision > DELIVERY_REVISION: + return False + if active_revision < DELIVERY_REVISION: + save_delivery_revision_state({"active_revision": DELIVERY_REVISION}) + return True + + def enqueue_status_comment_update(pr_number: int) -> None: state = load_status_comment_rollout_state() pending = set(state["pending_pr_numbers"]) diff --git a/.github/scripts/pull-request-dashboard/test_delivery.py b/.github/scripts/pull-request-dashboard/test_delivery.py index c646b8d1ad9..add3d55391b 100644 --- a/.github/scripts/pull-request-dashboard/test_delivery.py +++ b/.github/scripts/pull-request-dashboard/test_delivery.py @@ -1,6 +1,7 @@ from __future__ import annotations from pathlib import Path +import tempfile import unittest from unittest.mock import ANY, Mock, call, patch @@ -133,10 +134,12 @@ def test_open_pr_list_failure_skips_dependent_stages(self) -> None: @patch.object(delivery.sys, "stderr") @patch.object(delivery, "deliver_from_state", return_value=["status comments: boom"]) + @patch.object(delivery, "claim_delivery_revision", return_value=True) @patch.object(delivery.state_branch, "push_state_changes") def test_reports_delivery_errors_after_state_push( self, push_state_changes, + _claim_delivery_revision, _deliver_from_state, _stderr, ) -> None: @@ -145,17 +148,55 @@ def test_reports_delivery_errors_after_state_push( ) with ( + tempfile.TemporaryDirectory() as temp_dir, patch.object(delivery, "author_nudge_state_path", return_value=Path("author")), patch.object(delivery, "copilot_review_request_state_path", return_value=Path("copilot")), patch.object(delivery, "notification_state_path", return_value=Path("slack")), ): + github_output = Path(temp_dir) / "github-output" status = delivery.deliver_with_state( "open-telemetry/example", "dashboard-state", Path("state"), + github_output, ) + github_output_text = github_output.read_text(encoding="utf-8") self.assertEqual(1, status) + self.assertEqual("active=true\n", github_output_text) + + @patch.object(delivery, "deliver_from_state") + @patch.object(delivery, "claim_delivery_revision", return_value=False) + @patch.object(delivery.state_branch, "push_state_changes") + def test_stale_revision_skips_delivery_and_reports_inactive( + self, + push_state_changes, + claim_delivery_revision, + deliver_from_state, + ) -> None: + push_state_changes.side_effect = ( + lambda _state_dir, _message, update_state, **_kwargs: update_state() + ) + + with ( + tempfile.TemporaryDirectory() as temp_dir, + patch.object(delivery, "author_nudge_state_path", return_value=Path("author")), + patch.object(delivery, "copilot_review_request_state_path", return_value=Path("copilot")), + patch.object(delivery, "notification_state_path", return_value=Path("slack")), + ): + github_output = Path(temp_dir) / "github-output" + status = delivery.deliver_with_state( + "open-telemetry/example", + "dashboard-state", + Path("state"), + github_output, + ) + github_output_text = github_output.read_text(encoding="utf-8") + + self.assertEqual(0, status) + claim_delivery_revision.assert_called_once_with() + deliver_from_state.assert_not_called() + self.assertEqual("active=false\n", github_output_text) if __name__ == "__main__": diff --git a/.github/scripts/pull-request-dashboard/test_state.py b/.github/scripts/pull-request-dashboard/test_state.py index 9e067fbc9d2..9f2c9b9fb8f 100644 --- a/.github/scripts/pull-request-dashboard/test_state.py +++ b/.github/scripts/pull-request-dashboard/test_state.py @@ -13,11 +13,13 @@ BACKFILL_STATE_VERSION, COPILOT_REVIEW_REQUEST_STATE_VERSION, DASHBOARD_STATE_VERSION, + DELIVERY_REVISION_STATE_VERSION, NOTIFICATION_STATE_VERSION, STATUS_COMMENT_ROLLOUT_STATE_VERSION, author_nudge_state_path, backfill_state_path, copilot_review_request_state_path, + claim_delivery_revision, dashboard_state_path, empty_state, enqueue_status_comment_update, @@ -26,6 +28,7 @@ load_backfill_state, load_copilot_review_requests, load_dashboard_state_cache, + load_delivery_revision_state, load_state_file, load_status_comment_rollout_state, load_notifications, @@ -357,6 +360,31 @@ def test_enqueue_status_comment_update_is_deduplicated(self) -> None: load_status_comment_rollout_state()["pending_pr_numbers"], ) + def test_delivery_revision_only_advances(self) -> None: + with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)): + with patch("state.DELIVERY_REVISION", 1): + self.assertTrue(claim_delivery_revision()) + self.assertTrue(claim_delivery_revision()) + with patch("state.DELIVERY_REVISION", 2): + self.assertTrue(claim_delivery_revision()) + with patch("state.DELIVERY_REVISION", 1): + self.assertFalse(claim_delivery_revision()) + + self.assertEqual( + { + "version": DELIVERY_REVISION_STATE_VERSION, + "active_revision": 2, + }, + load_delivery_revision_state(), + ) + + def test_delivery_revision_state_fails_closed(self) -> None: + with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)): + delivery_state = Path(temp_dir) / "delivery-revision-state.json" + delivery_state.write_text("not json", encoding="utf-8") + + self.assertFalse(claim_delivery_revision()) + def test_backfill_state_preserves_version_three_cursor(self) -> None: with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)): backfill_state_path().write_text( diff --git a/.github/workflows/pull-request-dashboard-repo.yml b/.github/workflows/pull-request-dashboard-repo.yml index 687b24841eb..23d42570bb9 100644 --- a/.github/workflows/pull-request-dashboard-repo.yml +++ b/.github/workflows/pull-request-dashboard-repo.yml @@ -219,11 +219,18 @@ jobs: run: | set -euo pipefail state_branch="${DASHBOARD_STATE_BRANCH_PREFIX}/${REPO_NAME}" - delivery_args=(--state-branch "$state_branch" --repo "$REPO_NAME") + delivery_args=( + --state-branch "$state_branch" + --repo "$REPO_NAME" + --github-output "$GITHUB_OUTPUT" + ) python3 .github/scripts/pull-request-dashboard/delivery.py "${delivery_args[@]}" - name: Publish dashboard issue - if: always() && steps.dashboard-token.outcome == 'success' + if: >- + always() && + steps.dashboard-token.outcome == 'success' && + steps.delivery.outputs.active == 'true' env: GH_TOKEN: ${{ steps.dashboard-token.outputs.token }} REPO_NAME: ${{ inputs.repository }} From 78ffcf1fb9b8c902021fb4384b8ed7e8cba0c081 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Wed, 22 Jul 2026 15:10:14 -0700 Subject: [PATCH 2/4] Document dashboard version contracts --- .../pr_status_comment.py | 6 ++++-- .../scripts/pull-request-dashboard/state.py | 20 +++++++++++++++---- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index 62b1dd6ff96..0a3eec52867 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -30,8 +30,10 @@ _AUTHOR_NUDGE_EPISODE_MARKER_RE = re.compile( r"" ) -# Increment whenever render_status_comment changes in a way existing comments -# need to adopt. Hourly runs durably roll the revision out to all open PRs. +# Renderer rollout revision, not a state schema version. Increment whenever +# render_status_comment changes in a way existing comments need to adopt; +# hourly runs durably roll it out to all open PRs. Also bump DELIVERY_REVISION +# in state.py so queued workers cannot restore older content. STATUS_COMMENT_REVISION = 13 STATUS_COMMENT_ROLLOUT_BATCH_SIZE = 50 AUTHOR_ACTION_FEEDBACK_LINK_LIMIT = 20 diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index 1fc92c8e998..020764f7588 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -16,17 +16,29 @@ COPILOT_REVIEW_REQUEST_STATE_FILE = "copilot-review-request-state.json" STATUS_COMMENT_ROLLOUT_STATE_FILE = "status-comment-rollout-state.json" DELIVERY_REVISION_STATE_FILE = "delivery-revision-state.json" -# State files are disposable workflow caches, not durable user data. Bump only -# the version for the state shape whose meaning changed. + +# Each state version describes one JSON file's schema, not rollout order. Bump +# only the version whose stored shape or meaning changed. Schema mismatches for +# the ordinary state files below regenerate that disposable workflow cache. +# dashboard-state.json: accepted PR routing results and backfill readiness. DASHBOARD_STATE_VERSION = 5 +# backfill-state.json: round-robin cursor used by full dashboard refreshes. BACKFILL_STATE_VERSION = 3 +# notification-state.json: pending and delivered Slack notification records. NOTIFICATION_STATE_VERSION = 3 +# author-nudge-state.json: waiting episodes and delivered author reminders. AUTHOR_NUDGE_STATE_VERSION = 2 +# copilot-review-request-state.json: pending and delivered review requests. COPILOT_REVIEW_REQUEST_STATE_VERSION = 3 +# status-comment-rollout-state.json: target/completed renderer revisions and queue. STATUS_COMMENT_ROLLOUT_STATE_VERSION = 1 +# delivery-revision-state.json: active delivery revision. This safety state is +# not regenerated on a mismatch. If its schema changes, add an explicit +# migration from older versions; incompatible workers fail closed. DELIVERY_REVISION_STATE_VERSION = 1 -# Bump when older queued workers must not deliver against newer dashboard state -# or behavior. Unlike the state-shape versions above, this coordinates rollout. +# Bump alongside any state version or delivery behavior change that makes older +# queued workers unsafe. Higher revisions activate, equal revisions resume, and +# lower revisions skip all delivery side effects. DELIVERY_REVISION = 1 INITIAL_BACKFILL_COMPLETE_KEY = "initial_backfill_complete" _state_dir: Path | None = None From 247a8b91463ca8145e75c8f0bd907e4e6dc19e55 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Wed, 22 Jul 2026 15:18:53 -0700 Subject: [PATCH 3/4] Use state versions to guard dashboard delivery --- .../pull-request-dashboard/delivery.py | 16 ++-- .../pr_status_comment.py | 6 +- .../scripts/pull-request-dashboard/state.py | 90 ++++++++++--------- .../pull-request-dashboard/test_delivery.py | 12 +-- .../pull-request-dashboard/test_state.py | 48 +++++----- 5 files changed, 90 insertions(+), 82 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/delivery.py b/.github/scripts/pull-request-dashboard/delivery.py index 80359302290..e0d215a67b3 100644 --- a/.github/scripts/pull-request-dashboard/delivery.py +++ b/.github/scripts/pull-request-dashboard/delivery.py @@ -21,7 +21,7 @@ from pr_status_comment import update_status_comments_from_state from state import ( author_nudge_state_path, - claim_delivery_revision, + claim_delivery_versions, copilot_review_request_state_path, notification_state_path, set_state_dir, @@ -120,14 +120,14 @@ def deliver_with_state( copilot_retry = runner_temp_path("prior-copilot-review-request-state.json") notification_retry = runner_temp_path("prior-notification-state.json") errors: list[str] = [] - active_revision = False + active_versions = False def deliver() -> int: - nonlocal active_revision - active_revision = claim_delivery_revision() - if not active_revision: + nonlocal active_versions + active_versions = claim_delivery_versions() + if not active_versions: errors.clear() - print("a newer dashboard delivery revision is active; skipping", file=sys.stderr) + print("newer dashboard delivery versions are active; skipping", file=sys.stderr) return 0 errors[:] = deliver_from_state( repo, @@ -153,7 +153,7 @@ def deliver() -> int: return status if github_output is not None: with github_output.open("a", encoding="utf-8") as output: - output.write(f"active={'true' if active_revision else 'false'}\n") + output.write(f"active={'true' if active_versions else 'false'}\n") if not errors: return 0 print("Dashboard delivery failed:", file=sys.stderr) @@ -165,7 +165,7 @@ def main() -> int: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--repo", help="target repository name") parser.add_argument("--state-branch", required=True, help="git branch used for workflow state") - parser.add_argument("--github-output", type=Path, help="append the active revision result") + parser.add_argument("--github-output", type=Path, help="append the active versions result") args = parser.parse_args() repo = normalize_repo(args.repo) if args.repo else detect_repo() with state_branch.temporary_state_dir() as state_dir: diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index 0a3eec52867..03fb7687ba6 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -15,6 +15,7 @@ from dashboard_override import PRE_REVIEW_ROUTES, author_override_guidance from route_presentation import route_status_summary from state import ( + STATUS_COMMENT_REVISION, load_dashboard_state_cache, load_status_comment_rollout_state, save_status_comment_rollout_state, @@ -30,11 +31,6 @@ _AUTHOR_NUDGE_EPISODE_MARKER_RE = re.compile( r"" ) -# Renderer rollout revision, not a state schema version. Increment whenever -# render_status_comment changes in a way existing comments need to adopt; -# hourly runs durably roll it out to all open PRs. Also bump DELIVERY_REVISION -# in state.py so queued workers cannot restore older content. -STATUS_COMMENT_REVISION = 13 STATUS_COMMENT_ROLLOUT_BATCH_SIZE = 50 AUTHOR_ACTION_FEEDBACK_LINK_LIMIT = 20 NON_BLOCKING_CHECK_FAILURE_LIMIT = 20 diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index 020764f7588..a30b799679e 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -15,11 +15,13 @@ AUTHOR_NUDGE_STATE_FILE = "author-nudge-state.json" COPILOT_REVIEW_REQUEST_STATE_FILE = "copilot-review-request-state.json" STATUS_COMMENT_ROLLOUT_STATE_FILE = "status-comment-rollout-state.json" -DELIVERY_REVISION_STATE_FILE = "delivery-revision-state.json" +DELIVERY_VERSIONS_FILE = "delivery-versions.json" -# Each state version describes one JSON file's schema, not rollout order. Bump -# only the version whose stored shape or meaning changed. Schema mismatches for -# the ordinary state files below regenerate that disposable workflow cache. +# These monotonic versions jointly order delivery compatibility. Increment the +# relevant version whenever its stored shape, meaning, or delivered behavior +# changes. A worker lower in any component skips delivery; after claiming the +# 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 = 5 # backfill-state.json: round-robin cursor used by full dashboard refreshes. @@ -32,14 +34,9 @@ COPILOT_REVIEW_REQUEST_STATE_VERSION = 3 # status-comment-rollout-state.json: target/completed renderer revisions and queue. STATUS_COMMENT_ROLLOUT_STATE_VERSION = 1 -# delivery-revision-state.json: active delivery revision. This safety state is -# not regenerated on a mismatch. If its schema changes, add an explicit -# migration from older versions; incompatible workers fail closed. -DELIVERY_REVISION_STATE_VERSION = 1 -# Bump alongside any state version or delivery behavior change that makes older -# queued workers unsafe. Higher revisions activate, equal revisions resume, and -# lower revisions skip all delivery side effects. -DELIVERY_REVISION = 1 +# 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 = 13 INITIAL_BACKFILL_COMPLETE_KEY = "initial_backfill_complete" _state_dir: Path | None = None @@ -79,8 +76,8 @@ def status_comment_rollout_state_path() -> Path: return state_dir() / STATUS_COMMENT_ROLLOUT_STATE_FILE -def delivery_revision_state_path() -> Path: - return state_dir() / DELIVERY_REVISION_STATE_FILE +def delivery_versions_path() -> Path: + return state_dir() / DELIVERY_VERSIONS_FILE def dashboard_markdown_path() -> Path: @@ -112,10 +109,11 @@ def empty_status_comment_rollout_state() -> dict[str, Any]: } -def empty_delivery_revision_state() -> dict[str, Any]: +def current_delivery_versions() -> dict[str, int]: return { - "version": DELIVERY_REVISION_STATE_VERSION, - "active_revision": 0, + name: value + for name, value in globals().items() + if name.endswith(("_STATE_VERSION", "_REVISION")) } @@ -211,41 +209,49 @@ def save_status_comment_rollout_state(state: dict[str, Any]) -> None: ) -def load_delivery_revision_state() -> dict[str, Any] | None: - path = delivery_revision_state_path() +def load_delivery_versions() -> dict[str, int] | None: + path = delivery_versions_path() if not path.exists(): - return empty_delivery_revision_state() - state = load_state_file(path, DELIVERY_REVISION_STATE_VERSION) - if state is None: - return None + return {} try: - active_revision = max(int(state.get("active_revision") or 0), 0) - except (TypeError, ValueError): + data = json.loads(path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError) as e: + print(f"warning: unreadable delivery versions {path}: {e!r}", file=sys.stderr) return None - return { - "version": DELIVERY_REVISION_STATE_VERSION, - "active_revision": active_revision, - } + if not isinstance(data, dict): + return None + if any( + not isinstance(value, int) or isinstance(value, bool) or value < 0 + for value in data.values() + ): + return None + return data -def save_delivery_revision_state(state: dict[str, Any]) -> None: - save_state_file( - delivery_revision_state_path(), - {"active_revision": int(state.get("active_revision") or 0)}, - DELIVERY_REVISION_STATE_VERSION, +def save_delivery_versions(versions: dict[str, int]) -> None: + path = delivery_versions_path() + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text( + json.dumps(versions, sort_keys=True, indent=2), + encoding="utf-8", ) -def claim_delivery_revision() -> bool: - state = load_delivery_revision_state() - if state is None: - print("delivery revision state is unreadable; skipping delivery", file=sys.stderr) +def claim_delivery_versions() -> bool: + active_versions = load_delivery_versions() + if active_versions is None: + print("delivery versions are unreadable; skipping delivery", file=sys.stderr) + return False + current_versions = current_delivery_versions() + if active_versions.keys() - current_versions.keys(): return False - active_revision = state["active_revision"] - if active_revision > DELIVERY_REVISION: + if any( + active_versions.get(name, 0) > version + for name, version in current_versions.items() + ): return False - if active_revision < DELIVERY_REVISION: - save_delivery_revision_state({"active_revision": DELIVERY_REVISION}) + if active_versions != current_versions: + save_delivery_versions(current_versions) return True diff --git a/.github/scripts/pull-request-dashboard/test_delivery.py b/.github/scripts/pull-request-dashboard/test_delivery.py index add3d55391b..672e9dc4b8a 100644 --- a/.github/scripts/pull-request-dashboard/test_delivery.py +++ b/.github/scripts/pull-request-dashboard/test_delivery.py @@ -134,12 +134,12 @@ def test_open_pr_list_failure_skips_dependent_stages(self) -> None: @patch.object(delivery.sys, "stderr") @patch.object(delivery, "deliver_from_state", return_value=["status comments: boom"]) - @patch.object(delivery, "claim_delivery_revision", return_value=True) + @patch.object(delivery, "claim_delivery_versions", return_value=True) @patch.object(delivery.state_branch, "push_state_changes") def test_reports_delivery_errors_after_state_push( self, push_state_changes, - _claim_delivery_revision, + _claim_delivery_versions, _deliver_from_state, _stderr, ) -> None: @@ -166,12 +166,12 @@ def test_reports_delivery_errors_after_state_push( self.assertEqual("active=true\n", github_output_text) @patch.object(delivery, "deliver_from_state") - @patch.object(delivery, "claim_delivery_revision", return_value=False) + @patch.object(delivery, "claim_delivery_versions", return_value=False) @patch.object(delivery.state_branch, "push_state_changes") - def test_stale_revision_skips_delivery_and_reports_inactive( + def test_stale_versions_skip_delivery_and_report_inactive( self, push_state_changes, - claim_delivery_revision, + claim_delivery_versions, deliver_from_state, ) -> None: push_state_changes.side_effect = ( @@ -194,7 +194,7 @@ def test_stale_revision_skips_delivery_and_reports_inactive( github_output_text = github_output.read_text(encoding="utf-8") self.assertEqual(0, status) - claim_delivery_revision.assert_called_once_with() + claim_delivery_versions.assert_called_once_with() deliver_from_state.assert_not_called() self.assertEqual("active=false\n", github_output_text) diff --git a/.github/scripts/pull-request-dashboard/test_state.py b/.github/scripts/pull-request-dashboard/test_state.py index 9f2c9b9fb8f..650a3330820 100644 --- a/.github/scripts/pull-request-dashboard/test_state.py +++ b/.github/scripts/pull-request-dashboard/test_state.py @@ -13,13 +13,13 @@ BACKFILL_STATE_VERSION, COPILOT_REVIEW_REQUEST_STATE_VERSION, DASHBOARD_STATE_VERSION, - DELIVERY_REVISION_STATE_VERSION, NOTIFICATION_STATE_VERSION, STATUS_COMMENT_ROLLOUT_STATE_VERSION, author_nudge_state_path, backfill_state_path, copilot_review_request_state_path, - claim_delivery_revision, + claim_delivery_versions, + current_delivery_versions, dashboard_state_path, empty_state, enqueue_status_comment_update, @@ -28,7 +28,7 @@ load_backfill_state, load_copilot_review_requests, load_dashboard_state_cache, - load_delivery_revision_state, + load_delivery_versions, load_state_file, load_status_comment_rollout_state, load_notifications, @@ -360,30 +360,36 @@ def test_enqueue_status_comment_update_is_deduplicated(self) -> None: load_status_comment_rollout_state()["pending_pr_numbers"], ) - def test_delivery_revision_only_advances(self) -> None: + def test_each_delivery_version_rejects_older_workers(self) -> None: + baseline = current_delivery_versions() + for name in baseline: + with self.subTest(name=name), tempfile.TemporaryDirectory() as temp_dir: + with patch("state._state_dir", Path(temp_dir)): + self.assertTrue(claim_delivery_versions()) + newer = dict(baseline) + newer[name] += 1 + with patch("state.current_delivery_versions", return_value=newer): + self.assertTrue(claim_delivery_versions()) + self.assertFalse(claim_delivery_versions()) + self.assertEqual(newer, load_delivery_versions()) + + def test_new_delivery_version_rejects_older_workers(self) -> None: with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)): - with patch("state.DELIVERY_REVISION", 1): - self.assertTrue(claim_delivery_revision()) - self.assertTrue(claim_delivery_revision()) - with patch("state.DELIVERY_REVISION", 2): - self.assertTrue(claim_delivery_revision()) - with patch("state.DELIVERY_REVISION", 1): - self.assertFalse(claim_delivery_revision()) + self.assertTrue(claim_delivery_versions()) + with patch("state.FUTURE_STATE_VERSION", 1, create=True): + newer = current_delivery_versions() + self.assertEqual(1, newer["FUTURE_STATE_VERSION"]) + self.assertTrue(claim_delivery_versions()) - self.assertEqual( - { - "version": DELIVERY_REVISION_STATE_VERSION, - "active_revision": 2, - }, - load_delivery_revision_state(), - ) + self.assertFalse(claim_delivery_versions()) + self.assertEqual(newer, load_delivery_versions()) - def test_delivery_revision_state_fails_closed(self) -> None: + def test_delivery_versions_fail_closed(self) -> None: with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)): - delivery_state = Path(temp_dir) / "delivery-revision-state.json" + delivery_state = Path(temp_dir) / "delivery-versions.json" delivery_state.write_text("not json", encoding="utf-8") - self.assertFalse(claim_delivery_revision()) + self.assertFalse(claim_delivery_versions()) def test_backfill_state_preserves_version_three_cursor(self) -> None: with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)): From c2833e1cd86f9cef2adddd7554915a128471d54e Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Wed, 22 Jul 2026 15:23:21 -0700 Subject: [PATCH 4/4] Address review comment from copilot-pull-request-reviewer: validate malformed delivery versions --- .../pull-request-dashboard/test_state.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/test_state.py b/.github/scripts/pull-request-dashboard/test_state.py index 650a3330820..209ba8e2f68 100644 --- a/.github/scripts/pull-request-dashboard/test_state.py +++ b/.github/scripts/pull-request-dashboard/test_state.py @@ -385,11 +385,21 @@ def test_new_delivery_version_rejects_older_workers(self) -> None: self.assertEqual(newer, load_delivery_versions()) def test_delivery_versions_fail_closed(self) -> None: - with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)): - delivery_state = Path(temp_dir) / "delivery-versions.json" - delivery_state.write_text("not json", encoding="utf-8") + malformed_versions = [ + "not json", + json.dumps([]), + json.dumps({"DASHBOARD_STATE_VERSION": None}), + json.dumps({"DASHBOARD_STATE_VERSION": False}), + json.dumps({"DASHBOARD_STATE_VERSION": -1}), + json.dumps({"DASHBOARD_STATE_VERSION": 1.5}), + ] + for contents in malformed_versions: + with self.subTest(contents=contents), tempfile.TemporaryDirectory() as temp_dir: + with patch("state._state_dir", Path(temp_dir)): + delivery_state = Path(temp_dir) / "delivery-versions.json" + delivery_state.write_text(contents, encoding="utf-8") - self.assertFalse(claim_delivery_versions()) + self.assertFalse(claim_delivery_versions()) def test_backfill_state_preserves_version_three_cursor(self) -> None: with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)):