From 653aef3bac20c293aa5ed1254f7bc28d331e6ffd Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Thu, 25 Jun 2026 04:21:44 -0400 Subject: [PATCH] Reject Conventional Commits style commit messages (#68633) * Reject Conventional Commits style commit messages Airflow does not use Conventional Commits, but feat:/fix:/chore: style subjects have crept into commits more and more often in recent months. Add a commit-msg prek hook that rejects them locally, and a CI step that checks every commit of a pull request, so contributors get plain-prose subjects focused on user impact instead. * Apply suggestions from code review Co-authored-by: Ash Berlin-Taylor * Check PR title for Conventional Commits and fix applied-suggestion typos Under "squash and merge" the PR title becomes the commit subject on the target branch, so CI now validates the PR title (read from an env var to avoid shell injection) instead of the individual commit subjects, which are often "fixup"/"apply review" and do not survive the squash. Also fix a missing comma that concatenated the "bugfix"/"build" types and a stray non-breaking space that broke test collection. * Fix yamllint line-length in the PR-title check step --------- Co-authored-by: Ash Berlin-Taylor --- .github/workflows/ci-image-checks.yml | 12 ++ .pre-commit-config.yaml | 9 ++ AGENTS.md | 9 ++ .../check_no_conventional_commit_message.py | 128 ++++++++++++++++++ ...st_check_no_conventional_commit_message.py | 108 +++++++++++++++ 5 files changed, 266 insertions(+) create mode 100755 scripts/ci/prek/check_no_conventional_commit_message.py create mode 100644 scripts/tests/ci/prek/test_check_no_conventional_commit_message.py diff --git a/.github/workflows/ci-image-checks.yml b/.github/workflows/ci-image-checks.yml index d678fc2c096..a7f8c058183 100644 --- a/.github/workflows/ci-image-checks.yml +++ b/.github/workflows/ci-image-checks.yml @@ -160,6 +160,18 @@ jobs: - name: "Show prek log on failure" run: cat ~/.cache/prek/prek.log || true if: failure() + - name: "Check PR title is not Conventional Commits" + # With "squash and merge" the PR title becomes the commit subject that lands on the + # target branch (individual commit subjects are often "fixup"/"apply review" and do + # not survive the squash), so the title is what we must validate. Read it from an env + # var rather than interpolating it into the script to avoid shell injection from a + # crafted PR title. + if: github.event_name == 'pull_request' + env: + PR_TITLE: ${{ github.event.pull_request.title }} + run: | + printf '%s\n' "$PR_TITLE" \ + | ./scripts/ci/prek/check_no_conventional_commit_message.py --from-stdin build-docs: timeout-minutes: 150 diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index f3461ed417c..14b57af8f8a 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -16,6 +16,7 @@ # under the License. --- default_stages: [pre-commit, pre-push] +default_install_hook_types: [pre-commit, commit-msg] minimum_prek_version: '0.3.4' default_language_version: python: python3 @@ -1093,6 +1094,14 @@ repos: files: changelog\.(rst|txt)$ entry: ./scripts/ci/prek/changelog_duplicates.py pass_filenames: true + - id: check-no-conventional-commit-message + name: Check commit message is not Conventional Commits style + description: "Reject feat:/fix:/chore: style commit subjects - Airflow uses plain prose" + language: python + entry: ./scripts/ci/prek/check_no_conventional_commit_message.py + stages: [commit-msg] + always_run: true + pass_filenames: true - id: check-changelog-format name: Check changelog format language: python diff --git a/AGENTS.md b/AGENTS.md index 6934db81083..0199cf2483a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -168,6 +168,15 @@ Write commit messages focused on user impact, not implementation details. - **Good:** `Fix airflow dags test command failure without serialized Dags` - **Good:** `UI: Fix Grid view not refreshing after task actions` - **Bad:** `Initialize Dag bundles in CLI get_dag function` +- **Bad:** `fix(cli): dags test failure` — Airflow does not use Conventional Commits + (`feat:`, `fix:`, `chore:` …). Write the subject as plain prose. A `commit-msg` + prek hook (`check-no-conventional-commit-message`) rejects these, and CI checks + every commit of the PR. + +**Always run `prek install` before committing any code.** It installs the +`commit-msg` hook (in addition to `pre-commit`) so the Conventional Commits guard +runs locally; a clone that ran `prek install` before this hook existed must re-run +it to pick up the new hook type. Use the **imperative mood** and a plain message — do **not** use Conventional Commits prefixes (`fix:`, `feat:`, `chore:`, `docs:`, `refactor:`, …). apache/airflow does not follow that diff --git a/scripts/ci/prek/check_no_conventional_commit_message.py b/scripts/ci/prek/check_no_conventional_commit_message.py new file mode 100755 index 00000000000..2463288f812 --- /dev/null +++ b/scripts/ci/prek/check_no_conventional_commit_message.py @@ -0,0 +1,128 @@ +#!/usr/bin/env python +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Reject Conventional Commits style commit subjects. + +Airflow does not use Conventional Commits (``feat:``, ``fix:``, ``chore: ...``). +Commit messages should describe user impact in plain prose, e.g. +``Fix airflow dags test command failure without serialized Dags`` rather than +``fix(cli): dags test failure``. + +As a ``commit-msg`` stage hook it receives the path to the file holding the commit +message being created and fails if the subject line looks like a Conventional +Commit. + +With ``--from-stdin`` it instead reads subjects (one per line) from standard input +and fails if any of them looks like a Conventional Commit. This is how CI validates +the pull request title, which is the subject that lands on the target branch under +"squash and merge". +""" + +from __future__ import annotations + +import argparse +import re +import sys +from pathlib import Path + +# Standard Conventional Commits types (https://www.conventionalcommits.org). +CONVENTIONAL_TYPES = ( + "bugfix", + "build", + "chore", + "ci", + "docs", + "feat", + "fix", + "perf", + "refactor", + "revert", + "style", + "test", +) + +# Matches "type: ...", "type(scope): ..." and "type!: ..." (optionally combined), +# case-insensitively so "Fix:", "FEAT(api)!:" etc. are caught too. Airflow prose +# prefixes such as "UI:" or "API:" are not Conventional Commit types and are not +# matched. +CONVENTIONAL_COMMIT_REGEX = re.compile( + rf"^(?:{'|'.join(CONVENTIONAL_TYPES)})(?:\([^)]*\))?!?[:/]", + re.IGNORECASE, +) + + +def get_subject_line(commit_msg_file: str) -> str | None: + """Return the first non-empty, non-comment line of the commit message, if any.""" + for raw_line in Path(commit_msg_file).read_text(encoding="utf-8").splitlines(): + line = raw_line.strip() + if not line or line.startswith("#"): + continue + return line + return None + + +def is_conventional_commit(subject: str) -> bool: + return bool(CONVENTIONAL_COMMIT_REGEX.match(subject)) + + +def report(subject: str) -> None: + print( + "ERROR: Conventional Commits style commit message detected:\n\n" + f" {subject}\n\n" + "Airflow does not use Conventional Commits (feat:, fix:, chore: ...).\n" + "Write the subject as plain prose focused on user impact, for example:\n\n" + " Fix airflow dags test command failure without serialized Dags\n" + " UI: Fix Grid view not refreshing after task actions\n\n" + "See contributing-docs/05_pull_requests.rst for commit message guidance." + ) + + +def iter_subjects(args: argparse.Namespace) -> list[str]: + if args.from_stdin: + # CI passes commit subjects, one per line, on stdin. + return [line.strip() for line in sys.stdin.read().splitlines() if line.strip()] + # commit-msg hooks pass the path(s) to the commit message file(s). + subjects = [] + for commit_msg_file in args.files: + subject = get_subject_line(commit_msg_file) + if subject is not None: + subjects.append(subject) + return subjects + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--from-stdin", + action="store_true", + help="Read commit subjects (one per line) from stdin instead of from commit message files.", + ) + parser.add_argument("files", nargs="*", help="Commit message file(s) passed by the commit-msg hook.") + args = parser.parse_args(argv) + + failed = False + for subject in iter_subjects(args): + if is_conventional_commit(subject): + report(subject) + failed = True + return 1 if failed else 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/tests/ci/prek/test_check_no_conventional_commit_message.py b/scripts/tests/ci/prek/test_check_no_conventional_commit_message.py new file mode 100644 index 00000000000..463c8b26679 --- /dev/null +++ b/scripts/tests/ci/prek/test_check_no_conventional_commit_message.py @@ -0,0 +1,108 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import io + +import pytest +from ci.prek import check_no_conventional_commit_message as hook + + +class TestIsConventionalCommit: + @pytest.mark.parametrize( + "subject", + [ + "feat: add new operator", + "fix: bug in scheduler", + "fix(cli): dags test failure", + "chore: bump deps", + "docs: update readme", + "refactor!: drop old api", + "feat(scope)!: breaking change", + "FEAT(api)!: shouting type", + "Fix: capitalized type", + "ci: tweak workflow", + "test: add coverage", + "revert: undo change", + "build: tweak wheel", + "perf: speed up parsing", + "style: reformat", + "fix/issue-66592: something", + "Bugfix/make edge resilient to something", + ], + ) + def test_conventional_subjects_are_detected(self, subject): + assert hook.is_conventional_commit(subject) is True + + @pytest.mark.parametrize( + "subject", + [ + "Fix airflow dags test command failure without serialized Dags", + "UI: Fix Grid view not refreshing after task actions", + "API: add new endpoint", + "Add conventional commit", + 'Revert "Add something broken"', + "Merge branch 'main' into feature", + "Fixup test flakiness", + # A non-Conventional-Commit type word before a colon must not match. + "Scheduler: improve logging", + # Looks close but no colon after the type. + "fix the broken thing", + ], + ) + def test_plain_prose_subjects_are_allowed(self, subject): + assert hook.is_conventional_commit(subject) is False + + +class TestGetSubjectLine: + def test_first_non_comment_line_returned(self, tmp_path): + path = tmp_path / "COMMIT_EDITMSG" + path.write_text("\n# a comment\nfeat: do thing\n# trailing comment\n") + assert hook.get_subject_line(str(path)) == "feat: do thing" + + def test_only_comments_and_blank_lines_returns_none(self, tmp_path): + path = tmp_path / "COMMIT_EDITMSG" + path.write_text("\n# only a comment\n\n") + assert hook.get_subject_line(str(path)) is None + + +class TestMain: + def test_commit_msg_file_with_conventional_subject_fails(self, tmp_path, capsys): + path = tmp_path / "COMMIT_EDITMSG" + path.write_text("feat: shiny new thing\n") + assert hook.main([str(path)]) == 1 + assert "Conventional Commits" in capsys.readouterr().out + + def test_commit_msg_file_with_plain_subject_passes(self, tmp_path): + path = tmp_path / "COMMIT_EDITMSG" + path.write_text("Fix a real bug in the scheduler\n") + assert hook.main([str(path)]) == 0 + + def test_empty_commit_message_file_passes(self, tmp_path): + path = tmp_path / "COMMIT_EDITMSG" + path.write_text("\n# nothing here\n") + assert hook.main([str(path)]) == 0 + + def test_stdin_mode_flags_any_conventional_subject(self, monkeypatch, capsys): + monkeypatch.setattr("sys.stdin", io.StringIO("Fix real bug\nfeat: shiny thing\nUI: tweak grid\n")) + assert hook.main(["--from-stdin"]) == 1 + out = capsys.readouterr().out + assert "feat: shiny thing" in out + + def test_stdin_mode_all_clean_passes(self, monkeypatch): + monkeypatch.setattr("sys.stdin", io.StringIO("Fix real bug\nUI: tweak grid\nAPI: add endpoint\n")) + assert hook.main(["--from-stdin"]) == 0