From 587ceda79d58effe906f6ad55a29e78ffbc469b6 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 2 Nov 2026 22:32:56 +0000 Subject: [PATCH] fix(ci): check PR titles without stripping a trailing issue reference The conventional-commit policy stripped a trailing "(#N)" from every message, including PR titles. #1810's title ended in "(#1780)", passed as if that were the PR number, and integrated as "... (#1780) (#1810)", which the main check rejects. Main CI failed at the 9.5.0 release commit, so Release publish refused to publish 9.5.0. PR titles are now checked as written, since the squash appends the PR number only at integration. 272fced5 keeps its subject through an exact-commit-and-subject exception, like the historical breaking-footer ones. Every main commit since the policy floor validates. Co-Authored-By: Claude Opus 5.5 --- scripts/check_main_policies.py | 37 +++++++++++++--- tests/test_main_release_policies.py | 67 ++++++++++++++++++++++++++++- 2 files changed, 96 insertions(+), 8 deletions(-) diff --git a/scripts/check_main_policies.py b/scripts/check_main_policies.py index 2e78f4d4f..6cfbc131b 100644 --- a/scripts/check_main_policies.py +++ b/scripts/check_main_policies.py @@ -106,6 +106,18 @@ def pages(self, suffix: str, key: str | None = None) -> list[Any]: ), } +# #1810's PR title ended in an issue reference, "(#1780)". The PR check +# stripped it as if it were the PR number, and the squash subject then carried +# a second reference, so the integrated description keeps "(#1780)". PR titles +# are now checked without that stripping; only this exact commit and subject +# keep the reference. +HISTORICAL_DESCRIPTION_SUBJECTS = { + "272fced52d57632dedf32f2c175cf294100bff63": ( + "feat(decisioning): let a raised AdcpError reject a request with every error" + " (#1780) (#1810)" + ), +} + # Rebase-merged PRs record only their last commit as merge_commit_sha, so the # earlier commits they put on main match no PR by merge commit. These two were # rebase-merged so a storage-only commit would not release adcp; each maps to @@ -131,15 +143,28 @@ def main_commits(target: str) -> list[str]: return commits -def validate_message(message: str, *, commit_sha: str | None = None) -> None: +def validate_message( + message: str, *, commit_sha: str | None = None, integrated: bool = True +) -> None: subject, _, body = message.partition("\n") match = CONVENTIONAL.fullmatch(subject) require(match is not None, "commit subject is not a conventional commit") assert match is not None - # GitHub integration subjects may include the PR number. Parentheses/quotes in - # the actual description remain forbidden by the repository's parser rule. - description = re.sub(r" \(#[1-9][0-9]*\)$", "", match["description"]) - require(description and not any(c in description for c in '()"'), "unsafe release description") + # A squash integration appends the PR number to the subject. A PR title has + # none yet, so a trailing "(#N)" there is part of the description. + # Parentheses/quotes in the actual description remain forbidden by the + # repository's parser rule. + description = match["description"] + if integrated: + description = re.sub(r" \(#[1-9][0-9]*\)$", "", description) + historical_description_exception = ( + commit_sha is not None and HISTORICAL_DESCRIPTION_SUBJECTS.get(commit_sha) == subject + ) + require( + description + and (historical_description_exception or not any(c in description for c in '()"')), + "unsafe release description", + ) historical_footer_exception = ( commit_sha is not None and HISTORICAL_BREAKING_SUBJECTS.get(commit_sha) == subject ) @@ -199,7 +224,7 @@ def conventional( api = GitHub(os.environ["GH_TOKEN"]) current = api.repo(f"pulls/{pr['number']}") require(current["head"]["sha"] == pr["head"]["sha"], "PR head changed; run current CI") - validate_message(current["title"] + "\n\n" + (current["body"] or "")) + validate_message(current["title"] + "\n\n" + (current["body"] or ""), integrated=False) head, base = pr["head"]["sha"], pr["base"]["sha"] require(SHA.fullmatch(head) and SHA.fullmatch(base), "invalid PR source identities") fork = git("merge-base", base, head) diff --git a/tests/test_main_release_policies.py b/tests/test_main_release_policies.py index 4fa64cc8f..65c950ab0 100644 --- a/tests/test_main_release_policies.py +++ b/tests/test_main_release_policies.py @@ -19,6 +19,8 @@ LEDGER_SHA = "d" * 40 BREAKING_SUBJECT = "fix(reporting)!: scope configuration generations by account (#1174)" +# A PR title carries no PR number; the squash subject appends it. +BREAKING_PR_TITLE = BREAKING_SUBJECT.removesuffix(" (#1174)") BREAKING_FOOTER = ( "BREAKING CHANGE: generation_key returns ReportingConfigurationGenerationKey " "instead of a two-tuple. Use its named fields." @@ -107,7 +109,7 @@ def test_conventional_rerun_reads_the_corrected_pr_body(history: list[str]) -> N "number": 1174, "head": {"sha": history[-1]}, "base": {"sha": history[0]}, - "title": BREAKING_SUBJECT, + "title": BREAKING_PR_TITLE, "body": "Missing the breaking footer in the original webhook.", } api.data["pulls/1174"] = {**pr, "body": BREAKING_FOOTER} @@ -124,7 +126,7 @@ def test_conventional_rerun_rejects_a_newly_invalid_pr_message(history: list[str "number": 1174, "head": {"sha": history[-1]}, "base": {"sha": history[0]}, - "title": BREAKING_SUBJECT, + "title": BREAKING_PR_TITLE, "body": BREAKING_FOOTER, } api.data["pulls/1174"] = {**pr, "body": None} @@ -133,6 +135,67 @@ def test_conventional_rerun_rejects_a_newly_invalid_pr_message(history: list[str policies.conventional("pull_request", history[-1], {"pull_request": pr}, api=api) +def test_conventional_pr_title_keeps_a_trailing_issue_reference(history: list[str]) -> None: + # A PR title has no PR number yet; the squash appends one. A title ending + # in "(#1780)" would integrate as "... (#1780) (#1810)", which main rejects. + api = PolicyAPI(history) + pr = { + "number": 1810, + "head": {"sha": history[-1]}, + "base": {"sha": history[0]}, + "title": "feat(decisioning): reject a request with every error (#1780)", + "body": "", + } + api.data["pulls/1810"] = pr + + with pytest.raises(ReleaseRejectedError, match="unsafe release description"): + policies.conventional("pull_request", history[-1], {"pull_request": pr}, api=api) + + +@pytest.mark.parametrize( + ("message", "integrated", "accepted"), + [ + ("fix: handle the empty case (#1810)", True, True), + ("fix: handle the empty case (#1810)", False, False), + ("fix: handle the empty case (#1780) (#1810)", True, False), + ("fix: handle the empty case", False, True), + ], +) +def test_pr_number_is_stripped_only_from_integrated_subjects( + message: str, integrated: bool, accepted: bool +) -> None: + if accepted: + policies.validate_message(message, integrated=integrated) + else: + with pytest.raises(ReleaseRejectedError, match="unsafe release description"): + policies.validate_message(message, integrated=integrated) + + +HISTORICAL_DESCRIPTION_SHA = "272fced52d57632dedf32f2c175cf294100bff63" +HISTORICAL_DESCRIPTION_SUBJECT = ( + "feat(decisioning): let a raised AdcpError reject a request with every error (#1780) (#1810)" +) + + +@pytest.mark.parametrize( + ("commit_sha", "subject", "accepted"), + [ + (HISTORICAL_DESCRIPTION_SHA, HISTORICAL_DESCRIPTION_SUBJECT, True), + ("a" * 40, HISTORICAL_DESCRIPTION_SUBJECT, False), + (None, HISTORICAL_DESCRIPTION_SUBJECT, False), + (HISTORICAL_DESCRIPTION_SHA, "fix: bad (description) (#1810)", False), + ], +) +def test_historical_description_exception_requires_exact_commit_and_subject( + commit_sha: str | None, subject: str, accepted: bool +) -> None: + if accepted: + policies.validate_message(subject, commit_sha=commit_sha) + else: + with pytest.raises(ReleaseRejectedError, match="unsafe release description"): + policies.validate_message(subject, commit_sha=commit_sha) + + def test_conventional_rerun_cannot_validate_a_different_pr_head(history: list[str]) -> None: api = PolicyAPI(history) pr = {