Check if the blocking thread is already present and skip comment if yes

[ljavorsk]

Agents spam the blocking comments when run repeatedly, so we should check for it and only have one thread

[gemini-code-assist]

Code Review

This pull request adds a useful check to prevent posting duplicate blocking comments on GitLab merge requests. The implementation is sound, correctly using asyncio.to_thread for blocking calls. The new functionality is also well-covered by unit tests. I've provided a couple of suggestions to enhance code conciseness and maintainability in both the application logic and the test suite.

[gemini-code-assist]

The loop for checking existing comments can be made more concise. By combining the checks into a single if statement, you can reduce the number of lines and improve readability without sacrificing clarity.

Suggested change

	for discussion in discussions:
	notes = discussion.attributes.get("notes", [])
	if not notes:
	continue
	# Check first note in discussion for exact match (regardless of resolved status)
	first_note = notes[0]
	note_body = first_note.get("body", "").strip()
	if note_body == blocking_comment_message:
	return True
	for discussion in discussions:
	notes = discussion.attributes.get("notes", [])
	# Check first note in discussion for exact match (regardless of resolved status)
	if notes and notes[0].get("body", "").strip() == blocking_comment_message:
	return True

[gemini-code-assist]

The two test cases, test_add_blocking_merge_request_comment_already_exists and test_add_blocking_merge_request_comment_resolved_discussion_exists, are very similar and primarily differ by the resolved status of the mock discussion. To reduce code duplication and improve maintainability, you can combine them into a single parameterized test using pytest.mark.parametrize.

@pytest.mark.parametrize("is_resolved", [True, False])
@pytest.mark.asyncio
async def test_add_blocking_merge_request_comment_already_exists(is_resolved):
    merge_request_url = "https://gitlab.com/redhat/rhel/rpms/bash/-/merge_requests/123"
    comment = "**Blocking Merge Request**\n\nTest comment"

    existing_discussion = flexmock(
        id="disc1",
        attributes={
            "notes": [{"body": "**Blocking Merge Request**\n\nTest comment"}],
            "resolved": is_resolved,
        }
    )

    flexmock(GitlabService).should_receive("get_project_from_url").with_args(
        url=merge_request_url.rsplit("/-/merge_requests/", 1)[0],
    ).and_return(
        flexmock().should_receive("get_pr").and_return(
            flexmock(
                id=123,
                _raw_pr=flexmock(
                    discussions=flexmock()
                        .should_receive("list").with_args(get_all=True).and_return([existing_discussion]).mock()
                ),
            ),
        ).mock()
    )

    result = await add_blocking_merge_request_comment(
        merge_request_url=merge_request_url,
        comment=comment
    )

    # Should NOT add a new comment since the exact same one already exists (even if resolved)
    assert "already exists" in result
    assert merge_request_url in result

[ljavorsk]

/gemini review

[gemini-code-assist]

Code Review

This pull request correctly implements a check to prevent duplicate blocking comments on GitLab merge requests. My review focuses on improving code clarity and maintainability. I've suggested refactoring the comment-checking logic in gitlab_tools.py to be more concise and Pythonic, and also pointed out a minor style violation. For the tests, I've recommended consolidating two repetitive test cases into a single parameterized test to reduce code duplication and improve the test suite's structure.

[gemini-code-assist]

The check_existing_comment function can be made more concise and Pythonic by using a generator expression with any(). This improves readability by expressing the intent of the check more directly. Using an assignment expression (walrus operator :=) makes it even cleaner, assuming the project uses Python 3.8 or newer.

        def check_existing_comment():
            discussions = mr._raw_pr.discussions.list(get_all=True)
            blocking_comment_message = comment.strip()

            return any(
                (notes := discussion.attributes.get("notes"))
                and notes[0].get("body", "").strip() == blocking_comment_message
                for discussion in discussions
            )

[gemini-code-assist]

For improved readability and adherence to PEP 8 style guidelines, it's better to avoid single-line compound statements. Placing the return statement on a new line makes the code easier to read and maintain.

        if exists:
            return f"Comment already exists in merge request {merge_request_url}, not adding duplicate"

[gemini-code-assist]

The tests test_add_blocking_merge_request_comment_already_exists and test_add_blocking_merge_request_comment_resolved_discussion_exists are highly repetitive. They can be combined into a single, parameterized test using pytest.mark.parametrize. This will make the test suite more concise and easier to maintain. I've also used the comment variable instead of a hardcoded string for the note body and renamed the test for clarity.

@pytest.mark.asyncio
@pytest.mark.parametrize(
    "is_resolved",
    [False, True],
    ids=["unresolved_discussion_exists", "resolved_discussion_exists"],
)
async def test_add_blocking_merge_request_comment_if_already_exists(is_resolved):
    merge_request_url = "https://gitlab.com/redhat/rhel/rpms/bash/-/merge_requests/123"
    comment = "**Blocking Merge Request**\n\nTest comment"

    existing_discussion = flexmock(
        id="disc1",
        attributes={
            "notes": [{"body": comment}],
            "resolved": is_resolved,
        }
    )

    flexmock(GitlabService).should_receive("get_project_from_url").with_args(
        url=merge_request_url.rsplit("/-/merge_requests/", 1)[0],
    ).and_return(
        flexmock().should_receive("get_pr").and_return(
            flexmock(
                id=123,
                _raw_pr=flexmock(
                    discussions=flexmock()
                        .should_receive("list").with_args(get_all=True).and_return([existing_discussion]).mock()
                ),
            ),
        ).mock()
    )

    result = await add_blocking_merge_request_comment(
        merge_request_url=merge_request_url,
        comment=comment
    )

    # Should NOT add a new comment since the exact same one already exists (even if resolved)
    assert "already exists" in result
    assert merge_request_url in result

[ljavorsk]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a check to prevent duplicate blocking comments on GitLab merge requests. The implementation correctly identifies existing comments by fetching all discussions and comparing the content of the first note. The changes are well-tested, with new unit tests covering cases where a comment already exists (both resolved and unresolved).

My main feedback is a suggestion to refactor the comment-checking logic into a separate helper function to improve code structure and maintainability. Overall, this is a good improvement to prevent spamming.

[gemini-code-assist]

For better readability and modularity, consider extracting this checking logic into a separate private helper function at the module level. The current implementation nests the check_existing_comment function definition within add_blocking_merge_request_comment, which makes the parent function longer and mixes concerns.

A standalone helper function would make the code cleaner and the separation of synchronous (API call) and asynchronous logic more explicit. For example:

def _check_existing_comment_sync(mr: 'GitlabPullRequest', comment: str) -> bool:
    """Synchronously checks if a comment already exists in an MR's discussions."""
    discussions = mr._raw_pr.discussions.list(get_all=True)
    blocking_comment_message = comment.strip()

    for discussion in discussions:
        notes = discussion.attributes.get("notes", [])
        # Check first note in discussion for exact match
        if notes and notes[0].get("body", "").strip() == blocking_comment_message:
            return True
    return False

# ... inside add_blocking_merge_request_comment:
# exists = await asyncio.to_thread(_check_existing_comment_sync, mr, comment)

This would improve the overall structure and maintainability of the code.

[ljavorsk]

We don't need a separate function anywhere else, so maybe not for now