Skip to content

fix(ci): check PR titles without stripping a trailing issue reference #1819

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bokelley merged 1 commit into from
Nov 3, 2026
Merged

fix(ci): check PR titles without stripping a trailing issue reference #1819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 31 additions & 6 deletions scripts/check_main_policies.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
)
Expand Down Expand Up @@ -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)
Expand Down
67 changes: 65 additions & 2 deletions tests/test_main_release_policies.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Expand Down Expand Up @@ -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}
Expand All @@ -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}
Expand All @@ -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 = {
Expand Down
Loading