diff --git a/examples/.github-webhook-server.yaml b/examples/.github-webhook-server.yaml index aa52a875..288b36ed 100644 --- a/examples/.github-webhook-server.yaml +++ b/examples/.github-webhook-server.yaml @@ -93,8 +93,27 @@ can-be-merged-required-labels: - "tests-passed" - "security-reviewed" -# Conventional commit configuration -conventional-title: "feat,fix,docs,style,refactor,test,chore" +# Conventional Commits validation +# Enforces Conventional Commits v1.0.0 specification for PR titles +# Format: [optional scope]: +# +# Standard types (recommended): +# - feat: New features (triggers MINOR version bump in semver) +# - fix: Bug fixes (triggers PATCH version bump in semver) +# - build, chore, ci, docs, style, refactor, perf, test, revert +# +# Custom types: You can define your own types! The spec allows any noun. +# Examples: my-title, hotfix, release, custom +# +# Valid PR title examples: +# - feat: add user authentication +# - fix(parser): handle edge case in XML parsing +# - feat!: breaking API change to authentication +# - my-title: custom type example +# - hotfix(api): resolve production issue +# +# Resources: https://www.conventionalcommits.org/en/v1.0.0/ +conventional-title: "feat,fix,build,chore,ci,docs,style,refactor,perf,test,revert" # Minimum LGTM count required minimum-lgtm: 2 diff --git a/examples/config.yaml b/examples/config.yaml index b0406728..d134708d 100644 --- a/examples/config.yaml +++ b/examples/config.yaml @@ -118,7 +118,27 @@ repositories: - my-label1 - my-label2 - conventional-title: "ci,docs,feat,fix,refactor,test,release" # Check PR title start with any of these words + : + # Conventional Commits validation + # Enforces Conventional Commits v1.0.0 specification for PR titles + # Format: [optional scope]: + # + # Standard types (recommended): + # - feat: New features (triggers MINOR version bump in semver) + # - fix: Bug fixes (triggers PATCH version bump in semver) + # - build, chore, ci, docs, style, refactor, perf, test, revert + # + # Custom types: You can define your own types! The spec allows any noun. + # Examples: my-title, hotfix, release, custom + # + # Valid PR title examples: + # - feat: add user authentication + # - fix(parser): handle edge case in XML parsing + # - feat!: breaking API change to authentication + # - my-title: custom type example + # - hotfix(api): resolve production issue + # + # Resources: https://www.conventionalcommits.org/en/v1.0.0/ + conventional-title: "feat,fix,build,chore,ci,docs,style,refactor,perf,test,revert" branch-protection: strict: True require_code_owner_reviews: True diff --git a/webhook_server/config/schema.yaml b/webhook_server/config/schema.yaml index 51f9631b..77324641 100644 --- a/webhook_server/config/schema.yaml +++ b/webhook_server/config/schema.yaml @@ -254,7 +254,34 @@ properties: description: Required labels for PR to be marked as can-be-merged conventional-title: type: string - description: Comma-separated list of conventional commit prefixes + description: | + Comma-separated list of allowed commit types for Conventional Commits validation. + + Enforces Conventional Commits v1.0.0 specification: [optional scope]: + + Standard types (recommended): + - feat: New features (MINOR in semver) + - fix: Bug fixes (PATCH in semver) + - build, chore, ci, docs, style, refactor, perf, test, revert + + Custom types: The specification allows custom types (any noun). + Examples: my-title, hotfix, release, custom + + Format requirements: + - Type from configured list + - Optional scope in parentheses: (scope) + - Optional breaking change indicator: ! + - Mandatory colon + space: ": " + - Non-empty description + + Valid examples: + - feat: add authentication + - fix(api): handle edge case + - feat!: breaking change + - my-title: custom type + + Resources: https://www.conventionalcommits.org/en/v1.0.0/ + example: "feat,fix,build,chore,ci,docs,style,refactor,perf,test,revert" minimum-lgtm: type: integer description: Minimum number of LGTM required before approving PR diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 95dd48aa..1f89e739 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -602,9 +602,13 @@ async def run_conventional_title_check(self, pull_request: PullRequest) -> None: ) output: dict[str, str] = { - "title": "Conventional Title", - "summary": "", - "text": "", + "title": "✅ Conventional Title", + "summary": "PR title follows Conventional Commits format", + "text": ( + f"**Format:** `[optional scope]: `\n\n" + f"**Your title:** `{pull_request.title}`\n\n" + f"This title complies with the Conventional Commits v1.0.0 specification." + ), } if await self.check_run_handler.is_check_run_in_progress(check_run=CONVENTIONAL_TITLE_STR): @@ -615,11 +619,11 @@ async def run_conventional_title_check(self, pull_request: PullRequest) -> None: f"Setting conventional title check status to in-progress", ) await self.check_run_handler.set_conventional_title_in_progress() - allowed_names = self.github_webhook.conventional_title.split(",") + allowed_names = [name.strip() for name in self.github_webhook.conventional_title.split(",") if name.strip()] title = pull_request.title self.logger.debug(f"{self.log_prefix} Conventional title check for title: {title}, allowed: {allowed_names}") - if any([re.search(rf"{_name}(.*):", title) for _name in allowed_names]): + if any([re.match(rf"^{re.escape(_name)}(\([^)]+\))?!?: .+", title) for _name in allowed_names]): self.logger.step( # type: ignore[attr-defined] f"{self.log_prefix} {format_task_fields('runner', 'ci_check', 'completed')} " f"Conventional title check completed successfully", @@ -631,8 +635,41 @@ async def run_conventional_title_check(self, pull_request: PullRequest) -> None: f"{format_task_fields('runner', 'ci_check', 'failed')} " f"Conventional title check failed" ) - output["summary"] = "Failed" - output["text"] = f"Pull request title must starts with allowed title: {': ,'.join(allowed_names)}" + output["title"] = "❌ Conventional Title" + output["summary"] = "Conventional Commit Format Violation" + output["text"] = f"""## Conventional Commits Validation Failed + +**Your PR title:** +> {title} + +**Required format:** +``` +[optional scope]: +``` + +**Configured types for this repository:** +{", ".join(f"`{t}`" for t in allowed_names)} + +**Valid examples:** +- `feat: add user authentication` +- `fix(parser): handle edge case in URL parsing` +- `feat!: breaking change in API response` +- `refactor(core)!: major architectural change` +- `docs: update installation guide` + +**Format rules:** +- Type must be one of the configured types +- Optional scope in parentheses: `(scope)` +- Optional breaking change indicator: `!` +- **Mandatory**: colon followed by space `: ` +- **Mandatory**: non-empty description after the space + +**Note:** The Conventional Commits specification allows custom types beyond the standard recommendations. +Your team can configure additional types in the repository settings. + +**Resources:** +- [Conventional Commits v1.0.0 Specification](https://www.conventionalcommits.org/en/v1.0.0/) +""" await self.check_run_handler.set_conventional_title_failure(output=output) async def is_branch_exists(self, branch: str) -> Branch: diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index fe0134bc..84b27234 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -27,7 +27,7 @@ def mock_github_webhook(self) -> Mock: mock_webhook.pre_commit = True mock_webhook.build_and_push_container = True mock_webhook.pypi = {"token": "dummy"} - mock_webhook.conventional_title = "feat,fix,docs" + mock_webhook.conventional_title = "feat,fix,docs,style,refactor,perf,test,build,ci,chore,revert" mock_webhook.container_repository_username = "test-user" mock_webhook.container_repository_password = "test-pass" # pragma: allowlist secret mock_webhook.slack_webhook_url = "https://hooks.slack.com/test" @@ -389,10 +389,109 @@ async def test_run_install_python_module_failure( mock_set_failure.assert_called_once() @pytest.mark.asyncio - async def test_run_conventional_title_check_success( - self, runner_handler: RunnerHandler, mock_pull_request: Mock + @pytest.mark.parametrize( + "title,should_pass,reason", + [ + # Valid: Basic format + ("feat: add authentication", True, "basic feat format"), + ("fix: resolve parsing error", True, "basic fix format"), + ("docs: update README", True, "basic docs format"), + ("style: fix formatting", True, "basic style format"), + ("refactor: improve code structure", True, "basic refactor format"), + ("perf: optimize database queries", True, "basic perf format"), + ("test: add unit tests", True, "basic test format"), + ("build: update dependencies", True, "basic build format"), + ("ci: configure GitHub Actions", True, "basic ci format"), + ("chore: update .gitignore", True, "basic chore format"), + ("revert: revert previous commit", True, "basic revert format"), + # Valid: With scope + ("feat(api): add new endpoint", True, "feat with scope"), + ("fix(parser): handle edge case", True, "fix with scope"), + ("docs(readme): update installation steps", True, "docs with scope"), + ("style(css): improve button styling", True, "style with scope"), + ("refactor(auth): simplify token handling", True, "refactor with scope"), + ("perf(db): optimize query performance", True, "perf with scope"), + ("test(unit): add parser tests", True, "test with scope"), + ("build(deps): upgrade packages", True, "build with scope"), + ("ci(actions): update workflow", True, "ci with scope"), + ("chore(config): update settings", True, "chore with scope"), + # Valid: With breaking change indicator + ("feat!: breaking API change", True, "feat with breaking change"), + ("fix!: breaking bug fix", True, "fix with breaking change"), + ("refactor!: major refactor", True, "refactor with breaking change"), + ("feat(api)!: breaking API change", True, "feat with scope and breaking change"), + ("fix(core)!: breaking bug fix", True, "fix with scope and breaking change"), + # Valid: Multi-word descriptions + ("feat: add user authentication with OAuth2", True, "feat with multi-word description"), + ( + "fix(parser): handle edge case in URL parsing with special characters", + True, + "fix with scope and long description", + ), + ("docs: update installation guide for Windows users", True, "docs with multi-word description"), + # Valid: Spec examples + ("docs: correct spelling of CHANGELOG", True, "conventional commits spec example 1"), + ("feat(lang): add Polish language", True, "conventional commits spec example 2"), + ("fix: prevent racing of requests", True, "conventional commits spec example 3"), + # Valid: Special characters in description + ("feat: add support for UTF-8 characters 日本語", True, "feat with UTF-8 characters"), + ("fix: handle URLs with query params ?foo=bar&baz=qux", True, "fix with special characters"), + ("docs: update guide with symbols @#$%", True, "docs with symbols"), + # Valid: Numbers in scope + ("feat(v2): add version 2 API", True, "feat with version number in scope"), + ("fix(CVE-2023-1234): security patch", True, "fix with CVE number in scope"), + ("docs(python3.12): update compatibility notes", True, "docs with version in scope"), + # Invalid: Missing space after colon + ("feat:no space", False, "missing space after colon"), + ("fix(api):missing space", False, "missing space after colon with scope"), + ("docs:test", False, "missing space after colon for docs"), + # Invalid: Empty description + ("feat:", False, "empty description"), + ("feat: ", False, "empty description with space"), + ("fix(scope): ", False, "empty description with scope"), + ("fix(scope):", False, "empty description with scope no space"), + # Invalid: Wrong type + ("Feature: add authentication", False, "wrong type capitalized"), + ("FEAT: add auth", False, "wrong type uppercase"), + ("bugfix: fix issue", False, "wrong type bugfix instead of fix"), + ("feature: add new feature", False, "wrong type feature instead of feat"), + ("documentation: update docs", False, "wrong type documentation instead of docs"), + # Invalid: Missing colon + ("feat add auth", False, "missing colon"), + ("fix parser error", False, "missing colon for fix"), + ("docs update README", False, "missing colon for docs"), + # Invalid: Invalid characters before colon + ("feat hello: test", False, "invalid characters before colon"), + ("fix test test: broken", False, "invalid characters before colon"), + # Invalid: Malformed scope + ("feat(: broken scope", False, "malformed scope - missing closing paren"), + ("feat): broken scope", False, "malformed scope - missing opening paren"), + ("feat(): empty scope", False, "malformed scope - empty scope"), + ("feat(api)(auth): multiple scopes", False, "malformed scope - multiple scopes not allowed"), + # Invalid: No description after type + ("feat", False, "no colon or description"), + ("fix(api)", False, "no colon or description with scope"), + ("docs!", False, "no colon or description with breaking change indicator"), + # Edge cases: Numbers and special characters + ("fix: handle error #123", True, "fix with issue number"), + ("feat: add support for v1.0.0", True, "feat with version number"), + ("chore: update deps (security)", True, "chore with parentheses in description"), + ], + ) + async def test_conventional_title_validation( + self, runner_handler: RunnerHandler, mock_pull_request: Mock, title: str, should_pass: bool, reason: str ) -> None: - """Test run_conventional_title_check with valid title.""" + """Test Conventional Commits v1.0.0 title validation. + + Tests comprehensive validation covering: + - Valid formats (basic, with scope, with breaking change indicator) + - Multi-word descriptions + - Special characters and UTF-8 + - Invalid formats (missing space, empty description, wrong type, malformed scope) + - Edge cases (numbers, symbols, etc.) + """ + mock_pull_request.title = title + with patch.object( runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=False) ): @@ -402,29 +501,105 @@ async def test_run_conventional_title_check_success( with patch.object( runner_handler.check_run_handler, "set_conventional_title_success" ) as mock_set_success: + with patch.object( + runner_handler.check_run_handler, "set_conventional_title_failure" + ) as mock_set_failure: + await runner_handler.run_conventional_title_check(mock_pull_request) + + mock_set_progress.assert_called_once() + + if should_pass: + assert mock_set_success.call_count == 1, ( + f"Expected '{title}' to pass validation ({reason}), but it failed" + ) + assert mock_set_failure.call_count == 0, ( + f"Expected '{title}' to pass validation ({reason}), but failure was called" + ) + else: + assert mock_set_failure.call_count == 1, ( + f"Expected '{title}' to fail validation ({reason}), but it passed" + ) + assert mock_set_success.call_count == 0, ( + f"Expected '{title}' to fail validation ({reason}), but success was called" + ) + + @pytest.mark.asyncio + async def test_run_conventional_title_check_disabled( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test run_conventional_title_check when conventional_title is not configured.""" + runner_handler.github_webhook.conventional_title = "" + + with patch.object(runner_handler.check_run_handler, "set_conventional_title_in_progress") as mock_set_progress: + with patch.object(runner_handler.check_run_handler, "set_conventional_title_success") as mock_set_success: + with patch.object( + runner_handler.check_run_handler, "set_conventional_title_failure" + ) as mock_set_failure: await runner_handler.run_conventional_title_check(mock_pull_request) - mock_set_progress.assert_called_once() - mock_set_success.assert_called_once() + + # Should return early without doing anything + mock_set_progress.assert_not_called() + mock_set_success.assert_not_called() + mock_set_failure.assert_not_called() @pytest.mark.asyncio - async def test_run_conventional_title_check_failure( + async def test_run_conventional_title_check_custom_types( self, runner_handler: RunnerHandler, mock_pull_request: Mock ) -> None: - """Test run_conventional_title_check with invalid title.""" - mock_pull_request.title = "Invalid title" + """Test run_conventional_title_check with custom type configuration.""" + runner_handler.github_webhook.conventional_title = "my-title,hotfix,custom" + + # Valid custom types + valid_titles = [ + "my-title: custom type example", + "hotfix: critical production fix", + "custom: special handling", + "my-title(api): custom type with scope", + "hotfix!: breaking hotfix", + ] + + for title in valid_titles: + mock_pull_request.title = title + + with patch.object( + runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=False) + ): + with patch.object( + runner_handler.check_run_handler, "set_conventional_title_in_progress" + ) as mock_set_progress: + with patch.object( + runner_handler.check_run_handler, "set_conventional_title_success" + ) as mock_set_success: + with patch.object( + runner_handler.check_run_handler, "set_conventional_title_failure" + ) as mock_set_failure: + await runner_handler.run_conventional_title_check(mock_pull_request) + + mock_set_progress.assert_called_once() + mock_set_success.assert_called_once() + mock_set_failure.assert_not_called() + + @pytest.mark.asyncio + async def test_run_conventional_title_check_in_progress( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test run_conventional_title_check when check is already in progress.""" + mock_pull_request.title = "feat: test feature" with patch.object( - runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=False) + runner_handler.check_run_handler, "is_check_run_in_progress", new=AsyncMock(return_value=True) ): with patch.object( runner_handler.check_run_handler, "set_conventional_title_in_progress" ) as mock_set_progress: with patch.object( - runner_handler.check_run_handler, "set_conventional_title_failure" - ) as mock_set_failure: + runner_handler.check_run_handler, "set_conventional_title_success" + ) as mock_set_success: await runner_handler.run_conventional_title_check(mock_pull_request) + + # Should still proceed with the check mock_set_progress.assert_called_once() - mock_set_failure.assert_called_once() + mock_set_success.assert_called_once() @pytest.mark.asyncio async def test_is_branch_exists(self, runner_handler: RunnerHandler) -> None: