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 " (#<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