From 73da8b8056158f1eb05aa6151b2dc9fae03ca0b0 Mon Sep 17 00:00:00 2001 From: zhanghui <23442919+gloryfromca@users.noreply.github.com> Date: Thu, 15 Oct 2026 17:41:59 +0800 Subject: [PATCH] fix(ci): reserve the room a squash merge takes The PR title check measured the title an author types. What lands on main is the squash subject GitHub builds from it, " (#<number>)", which is longer. v1.4.2 went out that way: #496's title was a legal 66 chars and the commit on main is 73, over the 72 this policy enforces -- and `commit messages` then failed on every open PR for a commit none of them wrote. Reserve the suffix before comparing. CI now passes PR_NUMBER so the reserve is exact; a local run without it falls back to the width of a four-digit number. 5 of the 29 PRs open today would still land an over-length subject, so this turns them red before the merge rather than after it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --- .github/workflows/commits.yml | 3 ++ scripts/check_pr_title.py | 37 +++++++++++-- .../unit/test_scripts/test_check_pr_title.py | 54 ++++++++++++++++++- 3 files changed, 89 insertions(+), 5 deletions(-) diff --git a/.github/workflows/commits.yml b/.github/workflows/commits.yml index 936ce8f9b..5be54dab8 100644 --- a/.github/workflows/commits.yml +++ b/.github/workflows/commits.yml @@ -23,6 +23,9 @@ jobs: - name: Validate Conventional Commit PR title env: PR_TITLE: ${{ github.event.pull_request.title }} + # The squash subject is "<title> (#<number>)", so the title budget + # depends on how long the number is. + PR_NUMBER: ${{ github.event.pull_request.number }} run: make check-pr-title messages: diff --git a/scripts/check_pr_title.py b/scripts/check_pr_title.py index 5935b75c8..5b5977d6e 100644 --- a/scripts/check_pr_title.py +++ b/scripts/check_pr_title.py @@ -28,14 +28,43 @@ def _load_commit_policy() -> ModuleType: MAX_TITLE_LENGTH = _POLICY.MAX_TITLE_LENGTH TITLE_RE = _POLICY.TITLE_RE +# Room for the longest suffix a squash merge is likely to append, used when the +# PR number is not known (a local ``make check-pr-title``). CI passes the real +# number and gets an exact budget. +FALLBACK_SUFFIX_LENGTH = len(" (#0000)") -def validate_title(title: str) -> list[str]: + +def squash_suffix(pr_number: str | int | None) -> str | None: + """What a squash merge appends to the title, if the PR number is known. + + This repository merges by squash, and GitHub builds that commit's subject + as ``<PR title> (#<number>)``. The subject that lands on ``main`` is + therefore longer than the title the author typed -- which is the only + thing this check used to measure. v1.4.2 shipped a 73-char subject that + way, from a title that was a perfectly legal 66, and every open PR then + failed ``commit messages`` for a commit none of them wrote. + """ + if pr_number is None or not str(pr_number).strip(): + return None + return f" (#{str(pr_number).strip()})" + + +def validate_title(title: str, *, pr_number: str | int | None = None) -> list[str]: title = title.strip() if not title: return ["missing PR title"] - if len(title) > MAX_TITLE_LENGTH: - return [f"PR title is {len(title)} chars; max is {MAX_TITLE_LENGTH}: {title}"] + suffix = squash_suffix(pr_number) + reserved = len(suffix) if suffix is not None else FALLBACK_SUFFIX_LENGTH + budget = MAX_TITLE_LENGTH - reserved + + if len(title) > budget: + appended = f'"{suffix}"' if suffix is not None else f"{reserved} chars" + return [ + f"PR title is {len(title)} chars; max is {budget}: {title}\n" + f" a squash merge appends {appended}, and the commit subject it " + f"produces may not exceed {MAX_TITLE_LENGTH}" + ] if not TITLE_RE.match(title): allowed = ", ".join(ALLOWED_TYPES) @@ -56,7 +85,7 @@ def _title_from_args_or_env(argv: list[str]) -> str: def main(argv: list[str] | None = None) -> int: title = _title_from_args_or_env(sys.argv[1:] if argv is None else argv) - failures = validate_title(title) + failures = validate_title(title, pr_number=os.getenv("PR_NUMBER")) if failures: print("Pull request title check failed:") print("\n".join(failures)) diff --git a/tests/unit/test_scripts/test_check_pr_title.py b/tests/unit/test_scripts/test_check_pr_title.py index 852f2a595..c11e3841d 100644 --- a/tests/unit/test_scripts/test_check_pr_title.py +++ b/tests/unit/test_scripts/test_check_pr_title.py @@ -46,7 +46,46 @@ def test_long_pr_title_is_blocked() -> None: failures = checker.validate_title(title) assert len(failures) == 1 - assert "max is 72" in failures[0] + assert "max is 64" in failures[0] + + +def test_title_that_only_fits_before_the_squash_suffix_is_blocked() -> None: + """The v1.4.2 regression: a legal title that squashes into an illegal one. + + ``fix(lancedb): cap both caches, give queries their own embed budget`` is + 66 characters and passed this check. GitHub appended `` (#496)`` on squash + and put a 73-character subject on ``main``, which then failed + ``commit messages`` on every open PR until each one rebased past it. + """ + checker = _load_checker() + title = "fix(lancedb): cap both caches, give queries their own embed budget" + assert len(title) == 66 + + assert checker.validate_title(title, pr_number=None) != [] + + failures = checker.validate_title(title, pr_number=496) + + assert len(failures) == 1 + assert "max is 65" in failures[0] + assert '" (#496)"' in failures[0] + + +def test_title_that_still_fits_with_the_suffix_is_allowed() -> None: + checker = _load_checker() + title = "fix(lancedb): cap both caches, give queries their own budget" + assert len(title) + len(" (#496)") <= checker.MAX_TITLE_LENGTH + + assert checker.validate_title(title, pr_number=496) == [] + + +def test_longer_pr_numbers_cost_more_budget() -> None: + """The reserve is the real suffix, not a guess.""" + checker = _load_checker() + title = "fix(lancedb): cap both caches, give queries their own embe budget" + assert len(title) == 65 + + assert checker.validate_title(title, pr_number=496) == [] + assert checker.validate_title(title, pr_number=4096) != [] def test_main_reads_pr_title_environment(monkeypatch) -> None: @@ -54,3 +93,16 @@ def test_main_reads_pr_title_environment(monkeypatch) -> None: monkeypatch.setenv("PR_TITLE", "docs(readme): polish launch highlights") assert checker.main([]) == 0 + + +def test_main_reads_pr_number_environment(monkeypatch) -> None: + """CI passes the number; without it the check falls back to a reserve.""" + checker = _load_checker() + monkeypatch.setenv( + "PR_TITLE", "fix(lancedb): cap both caches, give queries their own budget" + ) + monkeypatch.setenv("PR_NUMBER", "496") + assert checker.main([]) == 0 + + monkeypatch.setenv("PR_NUMBER", "496000000") + assert checker.main([]) == 1