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 = {