fix(welcome-msg): conditional display of Container Operations and Pre-commit sections

webhook_server/libs/handlers/pull_request_handler.py

@@ -252,7 +252,7 @@ def _prepare_welcome_comment(self) -> str:
 * **Reviewer Assignment**: Reviewers are automatically assigned based on the OWNERS file in the repository root
 * **Size Labeling**: PR size labels (XS, S, M, L, XL, XXL) are automatically applied based on changes
 {issue_creation_note}
-* **Pre-commit Checks**: [pre-commit](https://pre-commit.ci/) runs automatically if `.pre-commit-config.yaml` exists
+{self._prepare_pre_commit_welcome_line}\
 * **Branch Labeling**: Branch-specific labels are applied to track the target branch
 * **Auto-verification**: Auto-verified users have their PRs automatically marked as verified
 
@@ -277,11 +277,7 @@ def _prepare_welcome_comment(self) -> str:
 
 #### Testing & Validation
 {self._prepare_retest_welcome_comment}
-
-#### Container Operations
-* `/build-and-push-container` - Build and push container image (tagged with PR number)
-  * Supports additional build arguments: `/build-and-push-container --build-arg KEY=value`
-
+{self._prepare_container_operations_welcome_section}\
 #### Cherry-pick Operations
 * `/cherry-pick <branch>` - Schedule cherry-pick to target branch when PR is merged
   * Multiple branches: `/cherry-pick branch1 branch2 branch3`
@@ -366,6 +362,26 @@ def _prepare_retest_welcome_comment(self) -> str:
 
         return " * No retest actions are configured for this repository" if not retest_msg else retest_msg
 
+    @property
+    def _prepare_pre_commit_welcome_line(self) -> str:
+        if self.github_webhook.pre_commit:
+            return (
+                "* **Pre-commit Checks**: [pre-commit](https://pre-commit.ci/) runs automatically "
+                "if `.pre-commit-config.yaml` exists\n"
+            )
+        return ""
+
+    @property
+    def _prepare_container_operations_welcome_section(self) -> str:
+        if self.github_webhook.build_and_push_container:
+            return """
+#### Container Operations
+* `/build-and-push-container` - Build and push container image (tagged with PR number)
+  * Supports additional build arguments: `/build-and-push-container --build-arg KEY=value`
+
+"""
+        return "\n"
+
     async def label_all_opened_pull_requests_merge_state_after_merged(self) -> None:
         """
         Labels pull requests based on their mergeable state.
@@ -754,7 +770,7 @@ async def _compare_branches(self, base_ref: str, head_ref_full: str) -> dict[str
             NOTE: This API does NOT return conflict information (mergeable/mergeable_state).
         """
         try:
-            _headers, data = await asyncio.to_thread(
+            _, data = await asyncio.to_thread(
                 self.repository._requester.requestJsonAndCheck,
                 "GET",
                 f"{self.repository.url}/compare/{base_ref}...{head_ref_full}",

webhook_server/tests/test_app_utils.py

@@ -7,7 +7,7 @@
 import pytest
 from fastapi import HTTPException
 
-from webhook_server.utils.app_utils import parse_datetime_string, verify_signature
+from webhook_server.utils.app_utils import format_duration, parse_datetime_string, verify_signature
 
 
 class TestVerifySignature:
@@ -90,3 +90,45 @@ def test_parse_datetime_string_empty_string(self) -> None:
         result = parse_datetime_string(datetime_str, "test_field")
         # Empty string is falsy, so it returns None (same as None input)
         assert result is None
+
+
+class TestFormatDuration:
+    """Test suite for format_duration function."""
+
+    def test_format_duration_milliseconds_only(self) -> None:
+        """Test format_duration with less than 1 second."""
+        assert format_duration(500) == "500ms"
+        assert format_duration(0) == "0ms"
+        assert format_duration(999) == "999ms"
+
+    def test_format_duration_seconds_only(self) -> None:
+        """Test format_duration with exact seconds (no remaining ms)."""
+        assert format_duration(1000) == "1s"
+        assert format_duration(5000) == "5s"
+        assert format_duration(59000) == "59s"
+
+    def test_format_duration_seconds_with_milliseconds(self) -> None:
+        """Test format_duration with seconds and remaining milliseconds."""
+        assert format_duration(1500) == "1s500ms"
+        assert format_duration(5123) == "5s123ms"
+
+    def test_format_duration_minutes_only(self) -> None:
+        """Test format_duration with exact minutes (no remaining seconds)."""
+        assert format_duration(60000) == "1m"
+        assert format_duration(120000) == "2m"
+        assert format_duration(3540000) == "59m"
+
+    def test_format_duration_minutes_with_seconds(self) -> None:
+        """Test format_duration with minutes and remaining seconds."""
+        assert format_duration(65000) == "1m5s"
+        assert format_duration(125000) == "2m5s"
+
+    def test_format_duration_hours_only(self) -> None:
+        """Test format_duration with exact hours (no remaining minutes)."""
+        assert format_duration(3600000) == "1h"
+        assert format_duration(7200000) == "2h"
+
+    def test_format_duration_hours_with_minutes(self) -> None:
+        """Test format_duration with hours and remaining minutes."""
+        assert format_duration(3660000) == "1h1m"
+        assert format_duration(5700000) == "1h35m"

webhook_server/tests/test_prepare_retest_welcome_comment.py

@@ -1,7 +1,34 @@
+from __future__ import annotations
+
+import re
+from typing import TYPE_CHECKING, TypedDict, Unpack
+
 import pytest
 
 from webhook_server.libs.handlers.pull_request_handler import PullRequestHandler
 
+if TYPE_CHECKING:
+    from github.PullRequest import PullRequest
+
+    from webhook_server.libs.github_api import GithubWebhook
+    from webhook_server.libs.handlers.owners_files_handler import OwnersFileHandler
+
+
+class HandlerConfig(TypedDict, total=False):
+    """Configuration options for _create_handler."""
+
+    tox: bool
+    build_and_push_container: bool
+    pypi: bool
+    pre_commit: bool
+    conventional_title: bool
+    parent_committer: str
+    auto_verified_and_merged_users: list[str]
+    create_issue_for_new_pr: bool
+    issue_url_for_welcome_msg: str
+    minimum_lgtm: int
+    pull_request: PullRequest | None
+
 
 class TestPrepareRetestWelcomeMsg:
     @pytest.mark.parametrize(
@@ -95,3 +122,229 @@ def test_prepare_retest_welcome_comment(
         )
 
         assert pull_request_handler._prepare_retest_welcome_comment == expected
+
+
+class TestWelcomeMessageNewlineStructure:
+    """Regression tests for welcome message markdown formatting.
+
+    These tests ensure that conditional sections in the welcome message
+    maintain proper newline structure to prevent headers from being
+    glued to previous content.
+    """
+
+    def _create_handler(
+        self,
+        process_github_webhook: GithubWebhook,
+        owners_file_handler: OwnersFileHandler,
+        **config: Unpack[HandlerConfig],
+    ) -> PullRequestHandler:
+        """Create a PullRequestHandler with the specified configuration."""
+        # Set default values for all config options
+        defaults: HandlerConfig = {
+            "tox": False,
+            "build_and_push_container": False,
+            "pypi": False,
+            "pre_commit": False,
+            "conventional_title": False,
+            "parent_committer": "test-user",
+            "auto_verified_and_merged_users": [],
+            "create_issue_for_new_pr": False,
+            "issue_url_for_welcome_msg": "https://github.com/test/repo/issues/1",
+            "minimum_lgtm": 1,
+            "pull_request": None,
+        }
+        defaults.update(config)
+
+        for key, value in defaults.items():
+            setattr(process_github_webhook, key, value)
+
+        # Mock the owners file handler attributes needed for welcome message generation
+        owners_file_handler.all_pull_request_approvers = ["approver1", "approver2"]
+        owners_file_handler.all_pull_request_reviewers = ["reviewer1", "reviewer2"]
+
+        return PullRequestHandler(github_webhook=process_github_webhook, owners_file_handler=owners_file_handler)
+
+    def test_container_section_enabled_has_header_on_own_line(self, process_github_webhook, owners_file_handler):
+        """When container operations are enabled, the header must start on its own line."""
+        handler = self._create_handler(
+            process_github_webhook,
+            owners_file_handler,
+            build_and_push_container=True,
+            tox=True,
+        )
+
+        welcome_msg = handler._prepare_welcome_comment()
+
+        # Container Operations header should be on its own line (preceded by newline)
+        assert "\n#### Container Operations\n" in welcome_msg, (
+            "#### Container Operations header should be on its own line"
+        )
+
+    def test_cherry_pick_section_has_header_on_own_line_when_container_enabled(
+        self, process_github_webhook, owners_file_handler
+    ):
+        """When container operations are enabled, Cherry-pick header must still be on its own line."""
+        handler = self._create_handler(
+            process_github_webhook,
+            owners_file_handler,
+            build_and_push_container=True,
+            tox=True,
+        )
+
+        welcome_msg = handler._prepare_welcome_comment()
+
+        # Cherry-pick Operations header should be on its own line
+        assert "\n#### Cherry-pick Operations\n" in welcome_msg, (
+            "#### Cherry-pick Operations header should be on its own line"
+        )
+
+    def test_cherry_pick_section_has_header_on_own_line_when_container_disabled(
+        self, process_github_webhook, owners_file_handler
+    ):
+        """When container operations are disabled, Cherry-pick header must still be on its own line.
+
+        This is a critical regression test - when the container section is disabled,
+        the Cherry-pick header should not be glued to the previous section.
+        """
+        handler = self._create_handler(
+            process_github_webhook,
+            owners_file_handler,
+            build_and_push_container=False,
+            tox=True,
+        )
+
+        welcome_msg = handler._prepare_welcome_comment()
+
+        # Cherry-pick Operations header should be on its own line, not glued to previous content
+        assert "\n#### Cherry-pick Operations\n" in welcome_msg, (
+            "#### Cherry-pick Operations header should be on its own line even when container section is disabled"
+        )
+        # Ensure it's not glued to retest section content
+        assert "all`#### Cherry-pick" not in welcome_msg, "Cherry-pick header should not be glued to retest section"
+
+    def test_no_excessive_blank_lines_between_sections(self, process_github_webhook, owners_file_handler):
+        """Sections should not have more than 2 consecutive blank lines."""
+        handler = self._create_handler(
+            process_github_webhook,
+            owners_file_handler,
+            build_and_push_container=True,
+            tox=True,
+            pre_commit=True,
+        )
+
+        welcome_msg = handler._prepare_welcome_comment()
+
+        # Check for triple or more blank lines (more than 2 consecutive newlines with only whitespace)
+        triple_blank_pattern = re.compile(r"\n\s*\n\s*\n\s*\n")
+        match = triple_blank_pattern.search(welcome_msg)
+        assert match is None, (
+            f"Found excessive blank lines in welcome message at position {match.start() if match else 'N/A'}"
+        )
+
+    def test_all_level_4_headers_on_own_lines(self, process_github_webhook, owners_file_handler):
+        """All #### headers in the welcome message should start on their own line."""
+        handler = self._create_handler(
+            process_github_webhook,
+            owners_file_handler,
+            build_and_push_container=True,
+            tox=True,
+            pre_commit=True,
+            conventional_title=True,
+        )
+
+        welcome_msg = handler._prepare_welcome_comment()
+
+        # Find all #### headers
+        header_pattern = re.compile(r"#### [A-Za-z]")
+        headers = list(header_pattern.finditer(welcome_msg))
+
+        for match in headers:
+            pos = match.start()
+            # Header should be at start of message or preceded by newline
+            if pos > 0:
+                char_before = welcome_msg[pos - 1]
+                assert char_before == "\n", (
+                    f"Header '{match.group()}' at position {pos} is not on its own line. "
+                    f"Preceded by: {welcome_msg[max(0, pos - 20) : pos]!r}"
+                )
+
+    @pytest.mark.parametrize(
+        ("build_and_push_container", "tox", "expected_container_section"),
+        [
+            pytest.param(True, True, True, id="container_enabled"),
+            pytest.param(False, True, False, id="container_disabled"),
+            pytest.param(True, False, True, id="container_enabled_no_tox"),
+            pytest.param(False, False, False, id="all_disabled"),
+        ],
+    )
+    def test_section_presence_matches_config(
+        self,
+        process_github_webhook,
+        owners_file_handler,
+        build_and_push_container,
+        tox,
+        expected_container_section,
+    ):
+        """Container section should only appear when build_and_push_container is enabled."""
+        handler = self._create_handler(
+            process_github_webhook,
+            owners_file_handler,
+            build_and_push_container=build_and_push_container,
+            tox=tox,
+        )
+
+        welcome_msg = handler._prepare_welcome_comment()
+
+        container_section_present = "#### Container Operations" in welcome_msg
+        assert container_section_present == expected_container_section, (
+            f"Container section presence ({container_section_present}) does not match "
+            f"expected ({expected_container_section}) for build_and_push_container={build_and_push_container}"
+        )
+
+        # Cherry-pick section should always be present
+        assert "#### Cherry-pick Operations" in welcome_msg, "Cherry-pick Operations section should always be present"
+
+    def test_testing_validation_section_structure(self, process_github_webhook, owners_file_handler):
+        """The Testing & Validation section should have proper structure."""
+        handler = self._create_handler(
+            process_github_webhook,
+            owners_file_handler,
+            build_and_push_container=True,
+            tox=True,
+        )
+
+        welcome_msg = handler._prepare_welcome_comment()
+
+        # Find the Testing & Validation section
+        assert "#### Testing & Validation" in welcome_msg
+
+        # The order should be: Testing & Validation -> (retest content) -> Container Operations -> Cherry-pick
+        testing_pos = welcome_msg.find("#### Testing & Validation")
+        container_pos = welcome_msg.find("#### Container Operations")
+        cherry_pick_pos = welcome_msg.find("#### Cherry-pick Operations")
+
+        assert testing_pos < container_pos < cherry_pick_pos, (
+            "Sections should be in order: Testing & Validation -> Container Operations -> Cherry-pick"
+        )
+
+    def test_retest_content_between_testing_header_and_container_section(
+        self, process_github_webhook, owners_file_handler
+    ):
+        """Retest commands should appear between Testing & Validation header and Container section."""
+        handler = self._create_handler(
+            process_github_webhook,
+            owners_file_handler,
+            build_and_push_container=True,
+            tox=True,
+        )
+
+        welcome_msg = handler._prepare_welcome_comment()
+
+        testing_pos = welcome_msg.find("#### Testing & Validation")
+        container_pos = welcome_msg.find("#### Container Operations")
+
+        # Get content between the two headers
+        section_between = welcome_msg[testing_pos:container_pos]
+
+        # Should contain retest commands
+        assert "/retest tox" in section_between, "Retest tox command should be in Testing & Validation section"