From e4ef729276c16144bf259c464ec76e0d03e0eb6f Mon Sep 17 00:00:00 2001 From: bymyself Date: Thu, 3 Sep 2026 20:56:29 +0000 Subject: [PATCH 1/2] fix(linear-ticket): ignore post-close signal runs --- scripts/linear-ticket/tests/test_validate.py | 22 +++++++++++- scripts/linear-ticket/validate.py | 35 ++++++++++++++------ 2 files changed, 46 insertions(+), 11 deletions(-) diff --git a/scripts/linear-ticket/tests/test_validate.py b/scripts/linear-ticket/tests/test_validate.py index f4678917..52db212f 100644 --- a/scripts/linear-ticket/tests/test_validate.py +++ b/scripts/linear-ticket/tests/test_validate.py @@ -15,14 +15,22 @@ class FakeGitHub: def __init__(self, protected): self.protected = protected self.current_base = "release/next" + self.pr_state = "open" self.statuses = [] self.deleted_comments = [] def get(self, path, *, paginate=False): + if path.endswith("/commits/abc123/pulls"): + return [{ + "number": 17, + "state": self.pr_state, + "head": {"sha": "abc123"}, + "base": {"repo": {"full_name": self.repo}}, + }] if path.endswith("/pulls/17"): return { "number": 17, - "state": "open", + "state": self.pr_state, "html_url": "https://github.com/Comfy-Org/example/pull/17", "head": {"sha": "abc123", "ref": "feature/be-123"}, "base": {"ref": "release/next"}, @@ -97,6 +105,18 @@ def test_unknown_protection_state_fails_closed_without_querying_linear(self): self.assertEqual(validator.run(event()), 1) self.assertEqual(github.statuses, []) + def test_signal_that_finishes_after_pr_merge_is_a_noop(self): + github = FakeGitHub(protected=True) + github.pr_state = "closed" + validator = self.validator(github) + validator._query_attachments = lambda _url: self.fail("Linear must not be queried") + stale_event = event() + stale_event["workflow_run"]["pull_requests"] = [] + + self.assertEqual(validator.run(stale_event), 0) + self.assertEqual(github.statuses, []) + self.assertEqual(github.deleted_comments, []) + def test_retargeted_pr_does_not_publish_stale_terminal_status(self): github = FakeGitHub(protected=False) github.current_base = "main" diff --git a/scripts/linear-ticket/validate.py b/scripts/linear-ticket/validate.py index 105be819..2935da5d 100644 --- a/scripts/linear-ticket/validate.py +++ b/scripts/linear-ticket/validate.py @@ -372,7 +372,11 @@ def run(self, event: dict) -> int: "signal workflow must run on pull_request; refusing to validate.") return 1 - self.pr_number = self._resolve_pr(event, head_sha) + self.pr_number, completed = self._resolve_pr(event, head_sha) + if completed: + log(f"The PR at {head_sha} closed before its signal run completed โ€” nothing to " + "validate.") + return 0 if self.pr_number is None: return 1 # error already reported @@ -433,31 +437,42 @@ def run(self, event: dict) -> int: return self._diagnose_and_fail(nodes, infra_error, branch, title, body) - def _resolve_pr(self, event: dict, head_sha: str) -> int | None: - """Exactly one open PR. Same-repo runs carry workflow_run.pull_requests; fork runs do - not, so fall back to the commit->PR association (GitHub-owned data either way).""" + def _resolve_pr(self, event: dict, head_sha: str) -> tuple[int | None, bool]: + """Resolve one open PR, or identify a signal whose exact-head PR already closed. + + Same-repo runs normally carry ``workflow_run.pull_requests``. GitHub can empty that + list when a fast merge or close beats the signal run, and fork runs omit it, so fall + back to the commit->PR association (GitHub-owned data either way). The boolean return + is true only for an unambiguous completed exact-head PR; callers may safely no-op it. + """ wr = event.get("workflow_run") or {} candidates = [pr.get("number") for pr in (wr.get("pull_requests") or []) if pr.get("number")] if not candidates: assoc = self.gh.get(f"/repos/{self.gh.repo}/commits/{head_sha}/pulls") or [] candidates = [ pr.get("number") for pr in assoc - if pr.get("state") == "open" - and (pr.get("base") or {}).get("repo", {}).get("full_name") == self.gh.repo + if (pr.get("base") or {}).get("repo", {}).get("full_name") == self.gh.repo ] open_prs: list[int] = [] + completed_prs: list[int] = [] for number in dict.fromkeys(candidates): # de-dup, preserve order data = self.gh.get(f"/repos/{self.gh.repo}/pulls/{number}") if data and data.get("state") == "open": open_prs.append(number) + elif data and (data.get("head") or {}).get("sha") == head_sha: + completed_prs.append(number) + + if len(open_prs) == 1: + return open_prs[0], False + if not open_prs and len(completed_prs) == 1: + return None, True if len(open_prs) != 1: error(f"Expected exactly one open PR associated with {head_sha}, found " - f"{len(open_prs)} (event={wr.get('event')}). Refusing to publish an " - "ambiguous result.") - return None - return open_prs[0] + f"{len(open_prs)} (and {len(completed_prs)} completed exact-head PRs; " + f"event={wr.get('event')}). Refusing to publish an ambiguous result.") + return None, False def _query_attachments(self, html_url: str): """attachmentsForURL(this PR) with bounded retry for the async-link race (design ยง5 From 4c79d883c5bccc01efed77ba01104c215bcf753c Mon Sep 17 00:00:00 2001 From: bymyself Date: Fri, 4 Sep 2026 02:42:48 +0000 Subject: [PATCH 2/2] fix(linear-ticket): fail closed resolving completed PRs Addresses https://github.com/Comfy-Org/github-workflows/pull/260#discussion_r3930120926 and the associated exact-head review round. --- scripts/linear-ticket/tests/test_validate.py | 55 +++++++++++++++++++- scripts/linear-ticket/validate.py | 34 ++++++++---- 2 files changed, 77 insertions(+), 12 deletions(-) diff --git a/scripts/linear-ticket/tests/test_validate.py b/scripts/linear-ticket/tests/test_validate.py index 52db212f..cdaa3922 100644 --- a/scripts/linear-ticket/tests/test_validate.py +++ b/scripts/linear-ticket/tests/test_validate.py @@ -16,23 +16,31 @@ def __init__(self, protected): self.protected = protected self.current_base = "release/next" self.pr_state = "open" + self.pr_merged = False + self.pr_head = "abc123" + self.association_paginated = None + self.fail_pull_numbers = set() self.statuses = [] self.deleted_comments = [] def get(self, path, *, paginate=False): if path.endswith("/commits/abc123/pulls"): + self.association_paginated = paginate return [{ "number": 17, "state": self.pr_state, - "head": {"sha": "abc123"}, + "head": {"sha": self.pr_head}, "base": {"repo": {"full_name": self.repo}}, }] if path.endswith("/pulls/17"): + if 17 in self.fail_pull_numbers: + return None return { "number": 17, "state": self.pr_state, + "merged": self.pr_merged, "html_url": "https://github.com/Comfy-Org/example/pull/17", - "head": {"sha": "abc123", "ref": "feature/be-123"}, + "head": {"sha": self.pr_head, "ref": "feature/be-123"}, "base": {"ref": "release/next"}, "title": "Change something", "body": "", @@ -108,6 +116,7 @@ def test_unknown_protection_state_fails_closed_without_querying_linear(self): def test_signal_that_finishes_after_pr_merge_is_a_noop(self): github = FakeGitHub(protected=True) github.pr_state = "closed" + github.pr_merged = True validator = self.validator(github) validator._query_attachments = lambda _url: self.fail("Linear must not be queried") stale_event = event() @@ -117,6 +126,48 @@ def test_signal_that_finishes_after_pr_merge_is_a_noop(self): self.assertEqual(github.statuses, []) self.assertEqual(github.deleted_comments, []) + def test_closed_unmerged_pr_fails_closed(self): + github = FakeGitHub(protected=True) + github.pr_state = "closed" + validator = self.validator(github) + stale_event = event() + stale_event["workflow_run"]["pull_requests"] = [] + + self.assertEqual(validator.run(stale_event), 1) + self.assertEqual(github.statuses, []) + + def test_merged_pr_with_a_newer_head_is_a_noop(self): + github = FakeGitHub(protected=True) + github.pr_state = "closed" + github.pr_merged = True + github.pr_head = "newer-head" + validator = self.validator(github) + validator._query_attachments = lambda _url: self.fail("Linear must not be queried") + stale_event = event() + stale_event["workflow_run"]["pull_requests"] = [] + + self.assertEqual(validator.run(stale_event), 0) + self.assertEqual(github.statuses, []) + + def test_pr_fetch_failure_does_not_become_a_completed_noop(self): + github = FakeGitHub(protected=True) + github.fail_pull_numbers.add(17) + validator = self.validator(github) + stale_event = event() + stale_event["workflow_run"]["pull_requests"] = [] + + self.assertEqual(validator.run(stale_event), 1) + self.assertEqual(github.statuses, []) + + def test_commit_associations_are_paginated(self): + github = FakeGitHub(protected=True) + validator = self.validator(github) + stale_event = event() + stale_event["workflow_run"]["pull_requests"] = [] + + self.assertEqual(validator._resolve_pr(stale_event, "abc123"), (17, False)) + self.assertTrue(github.association_paginated) + def test_retargeted_pr_does_not_publish_stale_terminal_status(self): github = FakeGitHub(protected=False) github.current_base = "main" diff --git a/scripts/linear-ticket/validate.py b/scripts/linear-ticket/validate.py index 2935da5d..9aaf6134 100644 --- a/scripts/linear-ticket/validate.py +++ b/scripts/linear-ticket/validate.py @@ -374,7 +374,7 @@ def run(self, event: dict) -> int: self.pr_number, completed = self._resolve_pr(event, head_sha) if completed: - log(f"The PR at {head_sha} closed before its signal run completed โ€” nothing to " + log(f"The PR at {head_sha} merged before its signal run completed โ€” nothing to " "validate.") return 0 if self.pr_number is None: @@ -448,30 +448,44 @@ def _resolve_pr(self, event: dict, head_sha: str) -> tuple[int | None, bool]: wr = event.get("workflow_run") or {} candidates = [pr.get("number") for pr in (wr.get("pull_requests") or []) if pr.get("number")] if not candidates: - assoc = self.gh.get(f"/repos/{self.gh.repo}/commits/{head_sha}/pulls") or [] + assoc = self.gh.get( + f"/repos/{self.gh.repo}/commits/{head_sha}/pulls", paginate=True) + if assoc is None: + error(f"Could not fetch PRs associated with {head_sha}; failing closed.") + return None, False candidates = [ pr.get("number") for pr in assoc - if (pr.get("base") or {}).get("repo", {}).get("full_name") == self.gh.repo + if ((pr.get("base") or {}).get("repo") or {}).get("full_name") == self.gh.repo ] open_prs: list[int] = [] completed_prs: list[int] = [] + other_prs: list[int] = [] + unreadable_prs: list[int] = [] for number in dict.fromkeys(candidates): # de-dup, preserve order data = self.gh.get(f"/repos/{self.gh.repo}/pulls/{number}") - if data and data.get("state") == "open": + if data is None: + unreadable_prs.append(number) + elif data.get("state") == "open": open_prs.append(number) - elif data and (data.get("head") or {}).get("sha") == head_sha: + elif data.get("merged") is True or data.get("merged_at"): completed_prs.append(number) + else: + other_prs.append(number) + + if unreadable_prs: + error(f"Could not fetch associated PR(s) {unreadable_prs}; failing closed.") + return None, False if len(open_prs) == 1: return open_prs[0], False - if not open_prs and len(completed_prs) == 1: + if not open_prs and len(completed_prs) == 1 and not other_prs: return None, True - if len(open_prs) != 1: - error(f"Expected exactly one open PR associated with {head_sha}, found " - f"{len(open_prs)} (and {len(completed_prs)} completed exact-head PRs; " - f"event={wr.get('event')}). Refusing to publish an ambiguous result.") + error(f"Expected exactly one open PR associated with {head_sha}, found " + f"{len(open_prs)} (and {len(completed_prs)} merged, " + f"{len(other_prs)} non-merged closed/unknown; event={wr.get('event')}). " + "Refusing to publish an ambiguous result.") return None, False def _query_attachments(self, html_url: str):