GRM-51: fix: enforce conventional commit check in automated PR review
This commit is contained in:
@@ -301,6 +301,50 @@ def check_test_coverage(files: list[dict[str, Any]], result: ReviewResult) -> No
|
||||
result.add_summary("- Tests: OK")
|
||||
|
||||
|
||||
def check_commit_conventions(client: GiteaClient, pr_number: str, result: ReviewResult) -> None:
|
||||
"""Check that PR commits follow conventional commit format.
|
||||
|
||||
Verifies that at least one commit on the PR branch matches the
|
||||
conventional commit pattern (type: description). Merge commits
|
||||
and revert commits are exempt.
|
||||
"""
|
||||
try:
|
||||
commits = client.get_pr_commits(pr_number)
|
||||
except APIError as e:
|
||||
result.add_summary(f"- Commit conventions: ERROR — could not fetch commits: {e.message}")
|
||||
return
|
||||
|
||||
if not commits:
|
||||
result.add_summary("- Commit conventions: OK (no commits to check)")
|
||||
return
|
||||
|
||||
from gitea_runner_manager.config import CONVENTIONAL_RE
|
||||
|
||||
has_conventional = False
|
||||
non_conventional: list[str] = []
|
||||
|
||||
for commit in commits:
|
||||
commit_info = commit.get("commit", {})
|
||||
message = str(commit_info.get("message", "") if isinstance(commit_info, dict) else "").split("\n")[0]
|
||||
# Skip merge commits and revert commits
|
||||
if message.startswith(("Merge", "Revert")):
|
||||
continue
|
||||
if CONVENTIONAL_RE.match(message):
|
||||
has_conventional = True
|
||||
else:
|
||||
non_conventional.append(message[:60])
|
||||
|
||||
if has_conventional:
|
||||
result.add_summary("- Commit conventions: OK")
|
||||
elif non_conventional:
|
||||
result.add_summary(
|
||||
f"- Commit conventions: WARNING — no conventional commit found. "
|
||||
f"Non-conventional commits: {', '.join(non_conventional[:3])}"
|
||||
)
|
||||
else:
|
||||
result.add_summary("- Commit conventions: OK (all commits are merges/reverts)")
|
||||
|
||||
|
||||
def run_review(client: GiteaClient, pr_number: str) -> ReviewResult:
|
||||
"""Run all review checks and return the result."""
|
||||
result = ReviewResult()
|
||||
@@ -322,6 +366,7 @@ def run_review(client: GiteaClient, pr_number: str) -> ReviewResult:
|
||||
check_function_length(files, result)
|
||||
check_documentation(files, result)
|
||||
check_test_coverage(files, result)
|
||||
check_commit_conventions(client, pr_number, result)
|
||||
|
||||
return result
|
||||
|
||||
|
||||
@@ -10,6 +10,7 @@ from scripts.ci.pr_review import (
|
||||
build_review_body,
|
||||
check_architecture_compliance,
|
||||
check_best_practices,
|
||||
check_commit_conventions,
|
||||
check_documentation,
|
||||
check_function_length,
|
||||
check_security,
|
||||
@@ -392,6 +393,7 @@ class TestRunReview:
|
||||
"patch": "@@ -10,3 +10,4 @@\n def foo():\n pass\n+ print('hello')\n",
|
||||
}
|
||||
]
|
||||
mock_client.get_pr_commits.return_value = [{"commit": {"message": "fix: resolve print issue"}}]
|
||||
result = run_review(mock_client, "42")
|
||||
assert result.has_issues
|
||||
|
||||
@@ -402,6 +404,68 @@ class TestRunReview:
|
||||
assert any("ERROR" in s for s in result.summary)
|
||||
|
||||
|
||||
class TestCheckCommitConventions:
|
||||
def test_conventional_commit_found(self) -> None:
|
||||
"""Should report OK when at least one commit is conventional."""
|
||||
client = MagicMock()
|
||||
client.get_pr_commits.return_value = [
|
||||
{"commit": {"message": "fix: resolve bug\n\nDetails"}},
|
||||
{"commit": {"message": "wip: testing"}},
|
||||
]
|
||||
result = ReviewResult()
|
||||
check_commit_conventions(client, "42", result)
|
||||
assert any("OK" in s for s in result.summary)
|
||||
|
||||
def test_no_conventional_commit(self) -> None:
|
||||
"""Should warn when no commits are conventional."""
|
||||
client = MagicMock()
|
||||
client.get_pr_commits.return_value = [
|
||||
{"commit": {"message": "updated stuff"}},
|
||||
{"commit": {"message": "wip: testing"}},
|
||||
]
|
||||
result = ReviewResult()
|
||||
check_commit_conventions(client, "42", result)
|
||||
assert any("WARNING" in s for s in result.summary)
|
||||
|
||||
def test_merge_commits_excluded(self) -> None:
|
||||
"""Merge commits should be excluded from the check."""
|
||||
client = MagicMock()
|
||||
client.get_pr_commits.return_value = [
|
||||
{"commit": {"message": "Merge branch 'feature' into master"}},
|
||||
{"commit": {"message": "fix: resolve bug"}},
|
||||
]
|
||||
result = ReviewResult()
|
||||
check_commit_conventions(client, "42", result)
|
||||
assert any("OK" in s for s in result.summary)
|
||||
|
||||
def test_all_merges_and_reverts(self) -> None:
|
||||
"""Should report OK when all commits are merges/reverts."""
|
||||
client = MagicMock()
|
||||
client.get_pr_commits.return_value = [
|
||||
{"commit": {"message": "Merge branch 'feature' into master"}},
|
||||
{"commit": {"message": "Revert: bad commit"}},
|
||||
]
|
||||
result = ReviewResult()
|
||||
check_commit_conventions(client, "42", result)
|
||||
assert any("merges/reverts" in s for s in result.summary)
|
||||
|
||||
def test_no_commits(self) -> None:
|
||||
"""Should report OK when there are no commits."""
|
||||
client = MagicMock()
|
||||
client.get_pr_commits.return_value = []
|
||||
result = ReviewResult()
|
||||
check_commit_conventions(client, "42", result)
|
||||
assert any("no commits" in s for s in result.summary)
|
||||
|
||||
def test_api_error(self) -> None:
|
||||
"""Should report ERROR when API call fails."""
|
||||
client = MagicMock()
|
||||
client.get_pr_commits.side_effect = APIError(500, "server error")
|
||||
result = ReviewResult()
|
||||
check_commit_conventions(client, "42", result)
|
||||
assert any("ERROR" in s for s in result.summary)
|
||||
|
||||
|
||||
class TestPostReview:
|
||||
def test_post_review_with_issues(self) -> None:
|
||||
client = MagicMock()
|
||||
|
||||
Reference in New Issue
Block a user