Add get_authorized_comments_from_merge_request function

* Add `get_authorized_comments_from_merge_request` function

This function helps to scrape relevant comments on the MR

* Spliting to private functions and fixing small issues

* Update mcp_server/gitlab_tools.py

Co-authored-by: Nikola Forró <[email protected]>

* Reduce duplicates in the tests for comments in MR

---------

Co-authored-by: Nikola Forró <[email protected]>

Author: ljavorsk

common/models.py

@@ -5,6 +5,7 @@
 and components to ensure consistency and type safety.
 """
 
+from datetime import datetime
 from typing import Optional, Dict, Any, Union
 from pydantic import BaseModel, Field
 from pathlib import Path
@@ -278,7 +279,7 @@ class CachedMRMetadata(BaseModel):
 
 
 # ============================================================================
-# GitLab Failed Pipeline Jobs Schemas
+# GitLab Tools Schemas
 # ============================================================================
 
 class FailedPipelineJob(BaseModel):
@@ -292,3 +293,50 @@ class FailedPipelineJob(BaseModel):
     artifacts_url: str = Field(
         description="URL to browse job artifacts, empty string if no artifacts"
     )
+
+
+class CommentReply(BaseModel):
+    """Represents a reply comment in a discussion thread."""
+
+    author: str | None = Field(description="Username of the reply author")
+    message: str | None = Field(description="The reply message text")
+    created_at: datetime | None = Field(
+        description="Timestamp when reply was created"
+    )
+
+
+class MergeRequestComment(BaseModel):
+    """Represents a comment from a GitLab merge request by an authorized member."""
+
+    author: str | None = Field(description="Username of the comment author")
+    message: str | None = Field(description="The comment message text")
+    created_at: datetime | None = Field(
+        description="Timestamp when comment was created"
+    )
+    file_path: str = Field(
+        default="",
+        description="File path if comment targets specific code, "
+        "empty for general comments"
+    )
+    line_number: int | None = Field(
+        default=None,
+        description="Line number in the current state of the file. "
+        "WARNING: If subsequent commits modified the file after this comment "
+        "was made, this line number may differ from where the comment was "
+        "originally placed. None for general comments."
+    )
+    line_type: str = Field(
+        default="",
+        description="Type of line in the diff: 'new' (added line), "
+        "'old' (removed line), 'unchanged' (context line), "
+        "or empty for general comments"
+    )
+    discussion_id: str = Field(
+        default="",
+        description="Discussion/thread ID this comment belongs to"
+    )
+    replies: list[CommentReply] = Field(
+        default_factory=list,
+        description="List of replies to this comment in the thread, "
+        "ordered chronologically"
+    )

mcp_server/gitlab_tools.py

@@ -12,13 +12,17 @@
 from ogr.services.gitlab.pull_request import GitlabPullRequest
 from pydantic import Field
 
-from common.models import FailedPipelineJob
+from common.models import FailedPipelineJob, MergeRequestComment, CommentReply
 from common.validators import AbsolutePath
 from utils import clean_stale_repositories
 
 
 logger = logging.getLogger(__name__)
 
+# GitLab access levels: Guest (10), Reporter (20), Developer (30),
+# Maintainer (40), Owner (50)
+DEVELOPER_ACCESS_LEVEL = 30
+
 
 def _get_authenticated_url(repository_url: str) -> str:
     """
@@ -386,3 +390,127 @@ def get_latest_pipeline_jobs():
     except Exception as e:
         logger.error(f"Failed to get failed jobs from MR {merge_request_url}: {e}")
         raise ToolError(f"Failed to get failed jobs from merge request: {e}") from e
+
+
+def _get_authorized_member_ids(project: GitlabProject) -> set[int]:
+    """
+    Fetch all project members and return a set of IDs for members
+    with Developer role or higher. This avoids N+1 API calls.
+    """
+    try:
+        members = project.gitlab_repo.members_all.list(get_all=True)
+        return {
+            member.id for member in members
+            if member.access_level >= DEVELOPER_ACCESS_LEVEL
+        }
+    except Exception as e:
+        logger.warning(f"Failed to fetch project members: {e}")
+        return set()
+
+
+def _extract_position_info(note: dict) -> tuple[str, int | None, str]:
+    """Extract file path, line number, and line type from a note's position."""
+    if not (position := note.get("position")):
+        return "", None, ""
+
+    file_path = position.get("new_path", "") or position.get("old_path", "")
+    new_line = position.get("new_line")
+    old_line = position.get("old_line")
+
+    if new_line and old_line:
+        return file_path, new_line, "unchanged"
+    elif new_line:
+        return file_path, new_line, "new"
+    elif old_line:
+        return file_path, old_line, "old"
+
+    return file_path, None, ""
+
+
+def _process_reply(
+    authorized_member_ids: set[int], note: dict
+) -> CommentReply | None:
+    """Process a reply note and return CommentReply if author is authorized."""
+    if note.get("system", False):
+        return None
+
+    try:
+        author = note.get("author", {})
+        author_id = author.get("id")
+        if not author_id or author_id not in authorized_member_ids:
+            return None
+
+        return CommentReply(
+            author=author.get("username"),
+            message=note.get("body"),
+            created_at=note.get("created_at"),
+        )
+    except Exception as e:
+        logger.warning(f"Failed to process reply note: {e}")
+        return None
+
+
+async def get_authorized_comments_from_merge_request(
+    merge_request_url: Annotated[str, Field(description="URL of the merge request")],
+) -> list[MergeRequestComment]:
+    """
+    Gets all comments from a merge request, filtered to only include
+    comments from authorized members with Developer role or higher.
+    """
+    try:
+        mr = await _get_merge_request_from_url(merge_request_url)
+
+        def get_authorized_comments():
+            discussions = mr._raw_pr.discussions.list(get_all=True)
+
+            authorized_member_ids = _get_authorized_member_ids(mr.target_project)
+
+            authorized_comments = []
+            for discussion in discussions:
+                try:
+                    if not (notes := discussion.attributes.get("notes")):
+                        continue
+
+                    first_note = notes[0]
+
+                    # Skip system notes (e.g. commit added)
+                    if first_note.get("system"):
+                        continue
+
+                    author = first_note.get("author", {})
+                    author_id = author.get("id")
+                    if not author_id or author_id not in authorized_member_ids:
+                        continue
+
+                    file_path, line_number, line_type = (
+                        _extract_position_info(first_note)
+                    )
+
+                    replies = [
+                        reply for note in notes[1:]
+                        if (reply := _process_reply(authorized_member_ids, note)) is not None
+                    ]
+
+                    authorized_comments.append(
+                        MergeRequestComment(
+                            author=author.get("username"),
+                            message=first_note.get("body"),
+                            created_at=first_note.get("created_at"),
+                            file_path=file_path,
+                            line_number=line_number,
+                            line_type=line_type,
+                            discussion_id=getattr(discussion, "id", ""),
+                            replies=replies,
+                        )
+                    )
+                except Exception as e:
+                    logger.warning(f"Failed to process discussion: {e}")
+                    continue
+
+            return authorized_comments
+
+        comments = await asyncio.to_thread(get_authorized_comments)
+        return comments
+
+    except Exception as e:
+        raise ToolError(f"Failed to get authorized comments from merge request: {e}") from e

mcp_server/tests/unit/test_gitlab_tools.py

@@ -22,6 +22,7 @@
     add_blocking_merge_request_comment,
     retry_pipeline_job,
     get_failed_pipeline_jobs_from_merge_request,
+    get_authorized_comments_from_merge_request,
 )
 from test_utils import mock_git_repo_basepath
 
@@ -446,3 +447,87 @@ async def test_get_failed_pipeline_jobs_from_merge_request_invalid_url():
         await get_failed_pipeline_jobs_from_merge_request(merge_request_url=merge_request_url)
 
     assert "Could not parse merge request URL" in str(exc_info.value)
+
+
+@pytest.mark.parametrize(
+    "discussions,members,expected_count",
+    [
+        pytest.param(
+            [
+                flexmock(id="d1", attributes={"notes": [{"author": {"id": 1, "username": "dev"}, "body": "Dev", "created_at": "2024-01-15T10:00:00Z", "system": False}]}),
+                flexmock(id="d2", attributes={"notes": [{"author": {"id": 2, "username": "reporter"}, "body": "Rep", "created_at": "2024-01-15T11:00:00Z", "system": False}]}),
+                flexmock(id="d3", attributes={"notes": [{"author": {"id": 3, "username": "guest"}, "body": "Guest", "created_at": "2024-01-15T12:00:00Z", "system": False}]}),
+            ],
+            [flexmock(id=1, access_level=30), flexmock(id=2, access_level=20)],
+            1,
+            id="filters_unauthorized",
+        ),
+        pytest.param(
+            [],
+            [],
+            0,
+            id="no_comments",
+        ),
+        pytest.param(
+            [
+                flexmock(id="d1", attributes={"notes": [{"author": {"id": 1, "username": "dev1"}, "body": "General", "created_at": "2024-01-15T10:00:00Z", "system": False}]}),
+                flexmock(id="d2", attributes={"notes": [{"author": {"id": 2, "username": "dev2"}, "body": "Line", "created_at": "2024-01-15T11:00:00Z", "system": False, "position": {"new_path": "f.py", "old_path": "f.py", "new_line": 42, "old_line": None}}]}),
+            ],
+            [flexmock(id=1, access_level=30), flexmock(id=2, access_level=30)],
+            2,
+            id="with_line_context",
+        ),
+        pytest.param(
+            [
+                flexmock(id="d1", attributes={"notes": [
+                    {"author": {"id": 1, "username": "dev1"}, "body": "Q", "created_at": "2024-01-15T10:00:00Z", "system": False},
+                    {"author": {"id": 2, "username": "dev2"}, "body": "A", "created_at": "2024-01-15T10:30:00Z", "system": False},
+                    {"author": {"id": 3, "username": "guest"}, "body": "?", "created_at": "2024-01-15T10:45:00Z", "system": False},
+                ]}),
+            ],
+            [flexmock(id=1, access_level=30), flexmock(id=2, access_level=30), flexmock(id=3, access_level=10)],
+            1,
+            id="with_replies",
+        ),
+    ],
+)
+@pytest.mark.asyncio
+async def test_get_authorized_comments_from_merge_request(discussions, members, expected_count):
+    merge_request_url = "https://gitlab.com/redhat/centos-stream/rpms/bash/-/merge_requests/123"
+
+    flexmock(GitlabService).should_receive("get_project_from_url").with_args(
+        url="https://gitlab.com/redhat/centos-stream/rpms/bash"
+    ).and_return(
+        flexmock().should_receive("get_pr").with_args(123).and_return(
+            flexmock(
+                _raw_pr=flexmock(
+                    discussions=flexmock().should_receive("list").with_args(
+                        get_all=True
+                    ).and_return(discussions).mock()
+                ),
+                target_project=flexmock(
+                    namespace="redhat/centos-stream/rpms",
+                    repo="bash",
+                    gitlab_repo=flexmock(
+                        members_all=flexmock().should_receive("list").with_args(
+                            get_all=True
+                        ).and_return(members).mock()
+                    )
+                ),
+            )
+        ).mock()
+    )
+
+    result = await get_authorized_comments_from_merge_request(merge_request_url=merge_request_url)
+
+    assert len(result) == expected_count
+
+
+@pytest.mark.asyncio
+async def test_get_authorized_comments_invalid_url():
+    """Test that invalid URLs raise appropriate errors."""
+    with pytest.raises(Exception) as exc_info:
+        await get_authorized_comments_from_merge_request(
+            merge_request_url="https://github.com/user/repo/pull/123"
+        )
+    assert "Could not parse merge request URL" in str(exc_info.value)