From 4e79ee5afc088374ba5f16d93df07fbb0cc3b152 Mon Sep 17 00:00:00 2001 From: David Phillips Date: Thu, 30 Jul 2026 21:04:01 -0700 Subject: [PATCH 1/2] Improve commit checker tests Table-driven cases obscure which inputs are accepted and what each failure reports. Use focused assertion helpers so the policy examples read directly. --- check-commit-messages/test_check.py | 252 +++++++++++++++------------- 1 file changed, 138 insertions(+), 114 deletions(-) diff --git a/check-commit-messages/test_check.py b/check-commit-messages/test_check.py index 9910f49..f8f60dd 100755 --- a/check-commit-messages/test_check.py +++ b/check-commit-messages/test_check.py @@ -7,7 +7,6 @@ from unittest.mock import patch from check import ( - PROHIBITED_ATTRIBUTION_MARKERS, SCISSORS_LINE_SUFFIX, check_commit_message_file, get_attribution_violations, @@ -20,124 +19,97 @@ class TestCommitMessages(unittest.TestCase): def test_subject_length(self) -> None: - cases = [ - ("60 characters", "x" * 60, []), - ("61 characters", "x" * 61, [61]), - ] - - for name, subject, expected_lengths in cases: - with self.subTest(name=name): - violations = get_subject_violations( - "abc123", - f"{subject}\n\nThis line is wrapped.", - ) - self.assertEqual( - [violation.length for violation in violations], - expected_lengths, - ) + self.assertSubjectAccepted("X" * 60) + self.assertSubjectLengthViolation("X" * 61, expected_length=61) def test_description_wrapping(self) -> None: - cases = [ - ( - "wrapped description", - "This line is wrapped.\n" - "This line is also wrapped before it gets too wide.", - [], - ), - ("79 characters", "x" * 79, []), - ( - "80 characters", - "This line is exactly eighty characters long and it should " - "fail as written here.!", - [3], - ), - ( - "long URL", - "See https://example.com/path/that/is/long/enough/to/exceed/" - "the/wrap/limit for context.", - [], - ), - ( - "unwrapped text around a URL", - "This surrounding prose is still far too long and should not " - "be hidden behind a long URL. https://example.com/long/url", - [3], - ), - ( - "long unwrappable token", - "Use com.example.really.long.package.name.with.enough.parts." - "to.exceed.the.wrap.limit.without.spaces.", - [], - ), - ( - "code block", - "```\n" - "This line is ordinary prose in a code block but should not " - "be checked by wrapping rules.\n" - "```", - [], - ), - ( - "block quote", - "> This quoted body line can exceed seventy-nine characters " - "because wrapping it would alter quoted text.", - [], - ), - ( - "trailer", - "Signed-off-by: Example Person With A Very Long Name " - "", - [], - ), - ] - - for name, body, expected_lines in cases: - with self.subTest(name=name): - self.assertEqual( - description_violating_lines(commit_message(body)), - expected_lines, - ) + self.assertDescriptionAccepted( + "This line is wrapped.\n" + "This line is also wrapped before it gets too wide." + ) + self.assertDescriptionAccepted("x" * 79) + self.assertDescriptionWrappingViolation( + "This line is exactly eighty characters long and it should " + "fail as written here.!" + ) + self.assertDescriptionAccepted( + "See https://example.com/path/that/is/long/enough/to/exceed/" + "the/wrap/limit for context." + ) + self.assertDescriptionWrappingViolation( + "This surrounding prose is still far too long and should not " + "be hidden behind a long URL. https://example.com/long/url" + ) + self.assertDescriptionAccepted( + "Use com.example.really.long.package.name.with.enough.parts." + "to.exceed.the.wrap.limit.without.spaces." + ) + self.assertDescriptionAccepted( + "```\n" + "This line is ordinary prose in a code block but should not " + "be checked by wrapping rules.\n" + "```" + ) + self.assertDescriptionAccepted( + "> This quoted body line can exceed seventy-nine characters " + "because wrapping it would alter quoted text." + ) + self.assertDescriptionAccepted( + "Signed-off-by: Example Person With A Very Long Name " + "" + ) def test_rejects_prohibited_attribution_markers(self) -> None: - for marker in PROHIBITED_ATTRIBUTION_MARKERS: - with self.subTest(marker=marker): - self.assertEqual( - attribution_violating_lines( - commit_message( - f"Co-authored-by: {marker} " - ) - ), - [3], - ) + self.assertAttributionViolation( + "Co-authored-by: Aider " + ) + self.assertAttributionViolation( + "Co-authored-by: Claude Opus 4.6 " + ) + self.assertAttributionViolation( + "Co-authored-by: Cline " + ) + self.assertAttributionViolation( + "Co-authored-by: OpenAI Codex " + ) + self.assertAttributionViolation( + "Co-authored-by: GitHub Copilot " + ) + self.assertAttributionViolation( + "Co-authored-by: Cursor Agent " + ) + self.assertAttributionViolation( + "Co-authored-by: Devin AI " + ) + self.assertAttributionViolation( + "Co-authored-by: gemini-code-assist[bot] " + "<176961590+gemini-code-assist[bot]@users.noreply.github.com>" + ) + self.assertAttributionViolation( + "Co-authored-by: ChatGPT " + ) + self.assertAttributionViolation( + "Co-authored-by: Windsurf Cascade " + ) def test_rejects_assisted_by_case_insensitively(self) -> None: - self.assertEqual( - attribution_violating_lines( - commit_message( - "assisted-BY: Internal CoDeX helper " - ) - ), - [3], + self.assertAttributionViolation( + "assisted-BY: OpenAI CoDeX " ) def test_accepts_human_attributions(self) -> None: - message = commit_message( + self.assertAttributionAccepted( "Assisted-by: Alex Example \n" "Co-authored-by: Taylor Example " ) - self.assertEqual(attribution_violating_lines(message), []) def test_accepts_examples_that_are_not_attribution_trailers(self) -> None: - messages = [ - commit_message( - "```\nCo-authored-by: ChatGPT \n```" - ), - commit_message("> Co-authored-by: ChatGPT "), - ] - - for message in messages: - with self.subTest(message=message): - self.assertEqual(attribution_violating_lines(message), []) + self.assertAttributionAccepted( + "```\nCo-authored-by: ChatGPT \n```" + ) + self.assertAttributionAccepted( + "> Co-authored-by: ChatGPT " + ) def test_checks_commit_message_file(self) -> None: with patch("check.get_comment_prefix", return_value="#"): @@ -149,7 +121,7 @@ def test_checks_commit_message_file(self) -> None: f"{'x' * 61}\n\n" "This line is exactly eighty characters long and it should " "fail as written here.!\n" - "Co-authored-by: Codex \n" + "Co-authored-by: OpenAI Codex \n" "# This comment is intentionally long enough to fail if " "commit template comments are validated.\n" "# ------------------------ >8 ------------------------\n" @@ -209,19 +181,71 @@ def test_truncates_scissors_line_with_configured_comment_prefix(self) -> None: "Add useful check\n\n", ) + def assertSubjectAccepted(self, subject: str) -> None: + self.assertEqual( + get_subject_violations( + "abc123", + f"{subject}\n\nThis line is wrapped.", + ), + [], + ) -def commit_message(body: str) -> str: - return f"Add useful check\n\n{body}" + def assertSubjectLengthViolation( + self, + subject: str, + expected_length: int, + ) -> None: + violations = get_subject_violations( + "abc123", + f"{subject}\n\nThis line is wrapped.", + ) + self.assertEqual( + [(violation.subject, violation.length) for violation in violations], + [(subject, expected_length)], + ) + def assertDescriptionAccepted(self, body: str) -> None: + self.assertEqual( + get_description_violations("abc123", commit_message(body)), + [], + ) -def description_violating_lines(message: str) -> list[int]: - violations = get_description_violations("abc123", message) - return [violation.line_number for violation in violations] + def assertDescriptionWrappingViolation(self, body: str) -> None: + violations = get_description_violations("abc123", commit_message(body)) + self.assertEqual( + [ + ( + violation.line_number, + violation.length, + violation.line, + ) + for violation in violations + ], + [(3, len(body), body)], + ) + def assertAttributionAccepted(self, body: str) -> None: + self.assertEqual( + get_attribution_violations("abc123", commit_message(body)), + [], + ) -def attribution_violating_lines(message: str) -> list[int]: - violations = get_attribution_violations("abc123", message) - return [violation.line_number for violation in violations] + def assertAttributionViolation(self, body: str) -> None: + violations = get_attribution_violations("abc123", commit_message(body)) + self.assertEqual( + [ + ( + violation.line_number, + violation.line, + ) + for violation in violations + ], + [(3, body)], + ) + + +def commit_message(body: str) -> str: + return f"Add useful check\n\n{body}" if __name__ == "__main__": From 98d057842e6a0899ec562bdaf11ef33b085eeb12 Mon Sep 17 00:00:00 2001 From: David Phillips Date: Thu, 30 Jul 2026 19:22:09 -0700 Subject: [PATCH 2/2] Check commit subject style The shared checker accepts malformed and common past-tense subjects, so local and CI checks do not enforce common Airlift commit style. Reject those forms with specific guidance. --- check-commit-messages/README.md | 5 +- check-commit-messages/action.yml | 2 +- check-commit-messages/check.py | 87 ++++++++++++++--- check-commit-messages/test_check.py | 145 ++++++++++++++++++++++++++++ 4 files changed, 225 insertions(+), 14 deletions(-) diff --git a/check-commit-messages/README.md b/check-commit-messages/README.md index bcc281c..d6ab01d 100644 --- a/check-commit-messages/README.md +++ b/check-commit-messages/README.md @@ -4,7 +4,10 @@ Checks every non-merge commit in a pull request. ## Policy -- Commit titles should be at most 50 characters and must not exceed 60. +- Commit titles must not start with a lowercase letter or end with a period. + They should be at most 50 characters and must not exceed 60. +- Commit titles should use imperative mood. Common past-tense leading words are + rejected with the corresponding imperative form. - Commit descriptions should wrap at 72 characters. Ordinary text must not exceed 79 characters. - `Assisted-by` and `Co-authored-by` trailers must not credit common AI models diff --git a/check-commit-messages/action.yml b/check-commit-messages/action.yml index a1cd2dd..3306854 100644 --- a/check-commit-messages/action.yml +++ b/check-commit-messages/action.yml @@ -1,5 +1,5 @@ name: Check commit messages -description: Check commit title length, description wrapping, and attribution trailers +description: Check commit title style and length, description wrapping, and attribution trailers inputs: base_ref: diff --git a/check-commit-messages/check.py b/check-commit-messages/check.py index dcec416..7775021 100755 --- a/check-commit-messages/check.py +++ b/check-commit-messages/check.py @@ -25,6 +25,28 @@ r"^(?:Assisted-by|Co-authored-by):\s*\S.*$", re.IGNORECASE, ) +PAST_TENSE_SUBJECT_STARTS = { + "added": "Add", + "bumped": "Bump", + "changed": "Change", + "converted": "Convert", + "created": "Create", + "disabled": "Disable", + "documented": "Document", + "enabled": "Enable", + "fixed": "Fix", + "implemented": "Implement", + "improved": "Improve", + "migrated": "Migrate", + "moved": "Move", + "refactored": "Refactor", + "removed": "Remove", + "renamed": "Rename", + "replaced": "Replace", + "reverted": "Revert", + "updated": "Update", + "upgraded": "Upgrade", +} PROHIBITED_ATTRIBUTION_MARKERS = ( "aider", "claude", @@ -46,6 +68,9 @@ class CommitSubjectViolation: commit: str subject: str length: int + starts_with_lowercase: bool + ends_with_period: bool + suggested_imperative: str | None @dataclass(frozen=True) @@ -103,14 +128,31 @@ def get_subject_violations( commit: str, message: str ) -> list[CommitSubjectViolation]: lines = message.splitlines() - if not lines or len(lines[0]) <= MAX_SUBJECT_LENGTH: + if not lines: + return [] + + subject = lines[0] + starts_with_lowercase = subject[:1].islower() + ends_with_period = subject.endswith(".") + words = subject.split(maxsplit=1) + first_word = words[0].rstrip(".,:;").casefold() if words else "" + suggested_imperative = PAST_TENSE_SUBJECT_STARTS.get(first_word) + if ( + len(subject) <= MAX_SUBJECT_LENGTH + and not starts_with_lowercase + and not ends_with_period + and suggested_imperative is None + ): return [] return [ CommitSubjectViolation( commit=commit, - subject=lines[0], - length=len(lines[0]), + subject=subject, + length=len(subject), + starts_with_lowercase=starts_with_lowercase, + ends_with_period=ends_with_period, + suggested_imperative=suggested_imperative, ) ] @@ -330,6 +372,15 @@ def strip_commit_comments(message: str) -> str: def print_subject_violations( violations: list[CommitSubjectViolation], ) -> None: + print( + "Commit subjects must not start with a lowercase letter or end with " + "a period.", + file=sys.stderr, + ) + print( + "Common past-tense leading verbs must use their imperative form.", + file=sys.stderr, + ) print( f"Commit subjects should be at most {RECOMMENDED_SUBJECT_LENGTH} " f"characters; this check fails subjects over {MAX_SUBJECT_LENGTH} " @@ -340,10 +391,20 @@ def print_subject_violations( for violation in violations: print_commit_header(violation.commit, violation.subject) - print( - f" subject: {violation.length} characters", - file=sys.stderr, - ) + if violation.starts_with_lowercase: + print(" subject: starts with a lowercase letter", file=sys.stderr) + if violation.ends_with_period: + print(" subject: ends with a period", file=sys.stderr) + if violation.suggested_imperative is not None: + print( + f" subject: use imperative '{violation.suggested_imperative}'", + file=sys.stderr, + ) + if violation.length > MAX_SUBJECT_LENGTH: + print( + f" subject: {violation.length} characters", + file=sys.stderr, + ) print(file=sys.stderr) @@ -407,7 +468,7 @@ def format_commit_reference(commit: str) -> str: def parse_args() -> argparse.Namespace: parser = argparse.ArgumentParser( description=( - "Check commit subject length, description wrapping, " + "Check commit subject style and length, description wrapping, " "and attribution trailers." ) ) @@ -453,14 +514,16 @@ def main() -> int: if args.message_file is not None: print( - "Checked commit message; subject and description meet length limits " - "and no prohibited attributions were found." + "Checked commit message; subject meets style and length " + "requirements, description meets length limits, and no prohibited " + "attributions were found." ) else: noun = "message" if commit_count == 1 else "messages" print( - f"Checked {commit_count} commit {noun}; subjects and descriptions " - "meet length limits and no prohibited attributions were found." + f"Checked {commit_count} commit {noun}; subjects meet style and " + "length requirements, descriptions meet length limits, and no " + "prohibited attributions were found." ) return 0 diff --git a/check-commit-messages/test_check.py b/check-commit-messages/test_check.py index f8f60dd..b350476 100755 --- a/check-commit-messages/test_check.py +++ b/check-commit-messages/test_check.py @@ -22,6 +22,110 @@ def test_subject_length(self) -> None: self.assertSubjectAccepted("X" * 60) self.assertSubjectLengthViolation("X" * 61, expected_length=61) + def test_subject_style(self) -> None: + self.assertSubjectAccepted("Add useful check") + self.assertSubjectAccepted("123 useful checks") + self.assertSubjectStyleViolation( + "add useful check", + starts_with_lowercase=True, + ) + self.assertSubjectStyleViolation( + "Add useful check.", + ends_with_period=True, + ) + self.assertSubjectStyleViolation( + "add useful check.", + starts_with_lowercase=True, + ends_with_period=True, + ) + + def test_rejects_common_past_tense_subject_starts(self) -> None: + self.assertSubjectCorrection( + "Added commit hook", + "Add commit hook", + ) + self.assertSubjectCorrection( + "Bumped checkout version", + "Bump checkout version", + ) + self.assertSubjectCorrection( + "Changed error message", + "Change error message", + ) + self.assertSubjectCorrection( + "Converted workflow to Python", + "Convert workflow to Python", + ) + self.assertSubjectCorrection( + "Created release action", + "Create release action", + ) + self.assertSubjectCorrection( + "Disabled stale checks", + "Disable stale checks", + ) + self.assertSubjectCorrection( + "Documented local setup", + "Document local setup", + ) + self.assertSubjectCorrection( + "Enabled grouped updates", + "Enable grouped updates", + ) + self.assertSubjectCorrection( + "Fixed commit parsing", + "Fix commit parsing", + ) + self.assertSubjectCorrection( + "Implemented subject validation", + "Implement subject validation", + ) + self.assertSubjectCorrection( + "Improved error output", + "Improve error output", + ) + self.assertSubjectCorrection( + "Migrated action inputs", + "Migrate action inputs", + ) + self.assertSubjectCorrection( + "Moved shared helper", + "Move shared helper", + ) + self.assertSubjectCorrection( + "Refactored checker output", + "Refactor checker output", + ) + self.assertSubjectCorrection( + "Removed old hook", + "Remove old hook", + ) + self.assertSubjectCorrection( + "Renamed workflow job", + "Rename workflow job", + ) + self.assertSubjectCorrection( + "Replaced local checker", + "Replace local checker", + ) + self.assertSubjectCorrection( + "Reverted cache change", + "Revert cache change", + ) + self.assertSubjectCorrection( + "Updated dependency pin", + "Update dependency pin", + ) + self.assertSubjectCorrection( + "Upgraded Python version", + "Upgrade Python version", + ) + + def test_accepts_other_subject_starts(self) -> None: + self.assertSubjectAccepted("Fixes #123") + self.assertSubjectAccepted("Adding support") + self.assertSubjectAccepted("Fixed-point arithmetic") + def test_description_wrapping(self) -> None: self.assertDescriptionAccepted( "This line is wrapped.\n" @@ -204,6 +308,47 @@ def assertSubjectLengthViolation( [(subject, expected_length)], ) + def assertSubjectStyleViolation( + self, + subject: str, + *, + starts_with_lowercase: bool = False, + ends_with_period: bool = False, + ) -> None: + violations = get_subject_violations("abc123", subject) + self.assertEqual( + [ + ( + violation.subject, + violation.starts_with_lowercase, + violation.ends_with_period, + violation.suggested_imperative, + ) + for violation in violations + ], + [(subject, starts_with_lowercase, ends_with_period, None)], + ) + + def assertSubjectCorrection( + self, + invalid_subject: str, + valid_subject: str, + ) -> None: + violations = get_subject_violations("abc123", invalid_subject) + self.assertEqual( + [ + ( + violation.subject, + violation.starts_with_lowercase, + violation.ends_with_period, + violation.suggested_imperative, + ) + for violation in violations + ], + [(invalid_subject, False, False, valid_subject.split(maxsplit=1)[0])], + ) + self.assertSubjectAccepted(valid_subject) + def assertDescriptionAccepted(self, body: str) -> None: self.assertEqual( get_description_violations("abc123", commit_message(body)),