From 2854cb4f0fe73501110658162e35acb7348b84df Mon Sep 17 00:00:00 2001 From: Sergey Petrov Date: Thu, 14 May 2026 18:51:48 +0300 Subject: [PATCH 1/7] feat(gitlab): support incremental review via /review -i MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When push_commands re-runs /review on every MR update, the full diff is re-evaluated and the persistent review comment is rewritten β€” losing the context of prior findings. The /review -i path already supported this on GitHub but was a no-op for GitLab. Mirror the GitHub provider's incremental flow for GitLab: - Locate the most recent prior review note (## PR Reviewer Guide). - Use its timestamp to partition MR commits into seen vs. new. - Use repository_compare(last_seen_sha, head_sha) to derive the diff restricted to commits added since the last review, anchoring file contents to last_seen_sha instead of the MR base. - Adapt GitLab commits / notes to the PyGithub-shaped attributes (`.sha`, `.commit.author.date`, `.created_at`, `.html_url`) consumed by the shared incremental code path in PRReviewer. Existing default push_commands are unchanged; users opt in by setting `push_commands = ["/describe", "/review -i"]` in their GitLab config. --- pr_agent/git_providers/gitlab_provider.py | 203 +++++++++++++++++++++- tests/unittest/test_gitlab_provider.py | 203 +++++++++++++++++++++- 2 files changed, 398 insertions(+), 8 deletions(-) diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index b3f54920d0..1ce6ef8c8a 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -2,6 +2,8 @@ import hashlib import re import urllib.parse +from datetime import datetime, timezone +from types import SimpleNamespace from typing import Any, Optional, Tuple, Union from urllib.parse import parse_qs, urlparse @@ -15,18 +17,76 @@ from ..algo.file_filter import filter_ignored from ..algo.git_patch_processing import decode_if_bytes from ..algo.language_handler import is_valid_file -from ..algo.utils import (clip_tokens, +from ..algo.utils import (PRReviewHeader, clip_tokens, find_line_number_of_relevant_line_in_file, load_large_diff) from ..config_loader import get_settings from ..log import get_logger -from .git_provider import MAX_FILES_ALLOWED_FULL, GitProvider +from .git_provider import MAX_FILES_ALLOWED_FULL, GitProvider, IncrementalPR class DiffNotFoundError(Exception): """Raised when the diff for a merge request cannot be found.""" pass + +def _parse_gitlab_iso_datetime(value) -> Optional[datetime]: + """Parse a GitLab ISO 8601 datetime string into a naive UTC datetime. + + GitLab returns timestamps with timezone info (e.g. "2024-01-15T14:30:00.000+02:00"). + We normalise to naive UTC so they can be compared with PyGithub-style naive datetimes + used in the shared incremental-review code path. + """ + if value is None: + return None + if isinstance(value, datetime): + if value.tzinfo is not None: + return value.astimezone(timezone.utc).replace(tzinfo=None) + return value + if not isinstance(value, str): + return None + try: + s = value.replace('Z', '+00:00') + dt = datetime.fromisoformat(s) + if dt.tzinfo is not None: + dt = dt.astimezone(timezone.utc).replace(tzinfo=None) + return dt + except (ValueError, AttributeError): + return None + + +class _GitlabIncrementalCommit: + """Adapter exposing a GitLab ProjectCommit with the attribute shape PyGithub Commit objects use. + + Shared incremental-review code reads `.sha` and `.commit.author.date`; we mimic that surface + so the provider can plug into `IncrementalPR.first_new_commit_sha` and the threshold checks + in `_can_run_incremental_review` without further branching. + """ + + def __init__(self, gl_commit): + self._gl_commit = gl_commit + self.sha = getattr(gl_commit, 'id', None) + date = _parse_gitlab_iso_datetime( + getattr(gl_commit, 'committed_date', None) + or getattr(gl_commit, 'authored_date', None) + or getattr(gl_commit, 'created_at', None) + ) + self.commit = SimpleNamespace(author=SimpleNamespace(date=date)) + + +class _GitlabIncrementalNote: + """Adapter exposing a GitLab note (issue comment) with attributes the reviewer expects. + + The reviewer reads `.created_at` (datetime) and `.html_url` from `previous_review`. + """ + + def __init__(self, note, mr_web_url: Optional[str] = None): + self._note = note + self.id = getattr(note, 'id', None) + self.body = getattr(note, 'body', '') or '' + self.created_at = _parse_gitlab_iso_datetime(getattr(note, 'created_at', None)) + self.html_url = f"{mr_web_url}#note_{self.id}" if mr_web_url else "" + class GitLabProvider(GitProvider): def __init__(self, merge_request_url: Optional[str] = None, incremental: Optional[bool] = False): @@ -347,6 +407,118 @@ def _set_merge_request(self, merge_request_url: str): get_logger().error(f"Could not get diff for merge request {self.id_mr}") raise DiffNotFoundError(f"Could not get diff for merge request {self.id_mr}") from e + def get_incremental_commits(self, incremental: Optional[IncrementalPR] = None): + """Populate state needed for an incremental review. + + Mirrors `GithubProvider.get_incremental_commits`: locates the previous review note, + determines which commits have arrived since, and pre-computes the diff set restricted + to those commits. Falls back silently to a full review when no previous review exists. + """ + if incremental is None: + incremental = IncrementalPR(False) + self.incremental = incremental + if not self.incremental.is_incremental: + return + self.unreviewed_files_set = {} + self._get_incremental_commits() + + def _get_incremental_commits(self): + if not getattr(self, 'mr_commits', None): + # gitlab returns commits newest-first; reverse to match PyGithub's oldest-first ordering + self.mr_commits = list(self.mr.commits())[::-1] + + self.previous_review = self.get_previous_review(full=True, incremental=True) + if not self.previous_review: + get_logger().info("No previous review found, will review the entire MR") + self.incremental.is_incremental = False + return + + self.incremental.commits_range = self.get_commit_range() + if not self.incremental.commits_range: + return + + last_seen_sha = self.incremental.last_seen_commit_sha + try: + head_sha = self.mr.diff_refs['head_sha'] + except (KeyError, TypeError, AttributeError): + head_sha = None + self._incremental_head_sha = head_sha + + if not last_seen_sha or not head_sha: + # The previous review predates every commit on the branch (or refs unavailable); + # nothing to anchor an incremental diff against, fall back to a full review. + get_logger().info( + "Incremental review cannot anchor a base commit (no last_seen_sha or head_sha); " + "falling back to a full review" + ) + self.incremental.is_incremental = False + return + + try: + project = self.gl.projects.get(self.id_project) + compare_result = project.repository_compare(last_seen_sha, head_sha) + except Exception as e: + get_logger().error( + f"Failed to compare commits {last_seen_sha}..{head_sha} for incremental review: {e}" + ) + self.incremental.is_incremental = False + return + + if isinstance(compare_result, dict): + diffs = compare_result.get('diffs', []) or [] + else: + diffs = getattr(compare_result, 'diffs', []) or [] + + for diff in diffs: + new_path = diff.get('new_path') if isinstance(diff, dict) else None + if new_path: + self.unreviewed_files_set[new_path] = diff + + def get_commit_range(self): + last_review_time = getattr(self.previous_review, 'created_at', None) + if last_review_time is None: + return [] + first_new_commit_index = None + for index in range(len(self.mr_commits) - 1, -1, -1): + adapter = _GitlabIncrementalCommit(self.mr_commits[index]) + commit_time = adapter.commit.author.date + if commit_time is not None and commit_time > last_review_time: + self.incremental.first_new_commit = adapter + first_new_commit_index = index + else: + self.incremental.last_seen_commit = adapter + break + return self.mr_commits[first_new_commit_index:] if first_new_commit_index is not None else [] + + def get_previous_review(self, *, full: bool, incremental: bool): + if not (full or incremental): + raise ValueError("At least one of full or incremental must be True") + if not getattr(self, '_incremental_notes_cache', None): + try: + self._incremental_notes_cache = list(self.mr.notes.list(get_all=True)) + except Exception as e: + get_logger().error(f"Failed to list MR notes for incremental review: {e}") + return None + prefixes = [] + if full: + prefixes.append(PRReviewHeader.REGULAR.value) + if incremental: + prefixes.append(PRReviewHeader.INCREMENTAL.value) + # gitlab returns notes newest-first; pick the most recent matching note + notes_sorted = sorted( + (n for n in self._incremental_notes_cache if getattr(n, 'body', None)), + key=lambda n: ( + _parse_gitlab_iso_datetime(getattr(n, 'created_at', None)) + or datetime.min + ), + reverse=True, + ) + mr_web_url = getattr(self.mr, 'web_url', None) + for note in notes_sorted: + if any(note.body.startswith(prefix) for prefix in prefixes): + return _GitlabIncrementalNote(note, mr_web_url=mr_web_url) + return None + def get_pr_file_content(self, file_path: str, branch: str) -> str: try: file_obj = self.gl.projects.get(self.id_project).files.get(file_path, branch) @@ -405,9 +577,22 @@ def get_diff_files(self) -> list[FilePatchInfo]: if self.diff_files: return self.diff_files - # filter files using [ignore] patterns - raw_changes = self.mr.changes().get('changes', []) - raw_changes = self._expand_submodule_changes(raw_changes) + incremental_active = bool( + getattr(self, 'incremental', None) + and getattr(self.incremental, 'is_incremental', False) + and getattr(self, 'unreviewed_files_set', None) + ) + + if incremental_active: + raw_changes = list(self.unreviewed_files_set.values()) + base_sha_for_content = self.incremental.last_seen_commit_sha + head_sha_for_content = getattr(self, '_incremental_head_sha', None) \ + or self.mr.diff_refs['head_sha'] + else: + raw_changes = self.mr.changes().get('changes', []) + raw_changes = self._expand_submodule_changes(raw_changes) + base_sha_for_content = self.mr.diff_refs['base_sha'] + head_sha_for_content = self.mr.diff_refs['head_sha'] diffs_original = raw_changes diffs = filter_ignored(diffs_original, 'gitlab') if diffs != diffs_original: @@ -432,8 +617,8 @@ def get_diff_files(self) -> list[FilePatchInfo]: # allow only a limited number of files to be fully loaded. We can manage the rest with diffs only counter_valid += 1 if counter_valid < MAX_FILES_ALLOWED_FULL or not diff['diff']: - original_file_content_str = self.get_pr_file_content(diff['old_path'], self.mr.diff_refs['base_sha']) - new_file_content_str = self.get_pr_file_content(diff['new_path'], self.mr.diff_refs['head_sha']) + original_file_content_str = self.get_pr_file_content(diff['old_path'], base_sha_for_content) + new_file_content_str = self.get_pr_file_content(diff['new_path'], head_sha_for_content) else: if counter_valid == MAX_FILES_ALLOWED_FULL: get_logger().info(f"Too many files in PR, will avoid loading full content for rest of files") @@ -477,6 +662,10 @@ def get_diff_files(self) -> list[FilePatchInfo]: return diff_files def get_files(self) -> list: + if (getattr(self, 'incremental', None) + and getattr(self.incremental, 'is_incremental', False) + and getattr(self, 'unreviewed_files_set', None)): + return list(self.unreviewed_files_set.keys()) if not self.git_files: raw_changes = self.mr.changes().get('changes', []) raw_changes = self._expand_submodule_changes(raw_changes) diff --git a/tests/unittest/test_gitlab_provider.py b/tests/unittest/test_gitlab_provider.py index dadc0903cb..ea0985b2ee 100644 --- a/tests/unittest/test_gitlab_provider.py +++ b/tests/unittest/test_gitlab_provider.py @@ -1,3 +1,4 @@ +from datetime import datetime from unittest.mock import MagicMock, patch import pytest @@ -5,7 +6,13 @@ from gitlab.exceptions import GitlabGetError from gitlab.v4.objects import Project, ProjectFile -from pr_agent.git_providers.gitlab_provider import GitLabProvider +from pr_agent.git_providers.git_provider import IncrementalPR +from pr_agent.git_providers.gitlab_provider import ( + GitLabProvider, + _GitlabIncrementalCommit, + _GitlabIncrementalNote, + _parse_gitlab_iso_datetime, +) class TestGitLabProvider: @@ -192,3 +199,197 @@ def test_compare_submodule_cached(self, gitlab_provider): assert first == second == [{"diff": "d"}] m_pbp.assert_called_once_with("grp/repo") proj.repository_compare.assert_called_once_with("old", "new") + + +class TestGitLabIncrementalHelpers: + """Pure-function tests for the incremental-review helpers.""" + + @pytest.mark.parametrize("value,expected", [ + ("2024-05-01T10:00:00.000Z", datetime(2024, 5, 1, 10, 0, 0)), + ("2024-05-01T12:00:00+02:00", datetime(2024, 5, 1, 10, 0, 0)), + ("2024-05-01T10:00:00", datetime(2024, 5, 1, 10, 0, 0)), + (datetime(2024, 5, 1, 10, 0, 0), datetime(2024, 5, 1, 10, 0, 0)), + (None, None), + ("not a date", None), + (12345, None), + ]) + def test_parse_iso_datetime(self, value, expected): + assert _parse_gitlab_iso_datetime(value) == expected + + def test_commit_adapter_exposes_pygithub_shape(self): + gl_commit = MagicMock() + gl_commit.id = "abc123" + gl_commit.committed_date = "2024-05-01T10:00:00.000Z" + gl_commit.authored_date = "2024-04-30T10:00:00.000Z" + + adapter = _GitlabIncrementalCommit(gl_commit) + + assert adapter.sha == "abc123" + # committed_date takes precedence over authored_date + assert adapter.commit.author.date == datetime(2024, 5, 1, 10, 0, 0) + + def test_commit_adapter_falls_back_to_authored_date(self): + gl_commit = MagicMock(spec=["id", "authored_date"]) + gl_commit.id = "abc" + gl_commit.authored_date = "2024-04-30T10:00:00Z" + + adapter = _GitlabIncrementalCommit(gl_commit) + + assert adapter.commit.author.date == datetime(2024, 4, 30, 10, 0, 0) + + def test_note_adapter_builds_html_url(self): + note = MagicMock() + note.id = 42 + note.body = "## PR Reviewer Guide πŸ”\n..." + note.created_at = "2024-05-01T10:00:00Z" + + adapter = _GitlabIncrementalNote(note, mr_web_url="https://gitlab.com/x/y/-/merge_requests/1") + + assert adapter.id == 42 + assert adapter.html_url == "https://gitlab.com/x/y/-/merge_requests/1#note_42" + assert adapter.created_at == datetime(2024, 5, 1, 10, 0, 0) + + +class TestGitLabIncrementalReview: + """Tests for the GitLab incremental-review flow.""" + + @pytest.fixture + def mock_gitlab_client(self): + return MagicMock() + + @pytest.fixture + def mock_project(self): + return MagicMock() + + @pytest.fixture + def gitlab_provider(self, mock_gitlab_client, mock_project): + with patch('pr_agent.git_providers.gitlab_provider.gitlab.Gitlab', return_value=mock_gitlab_client), \ + patch('pr_agent.git_providers.gitlab_provider.get_settings') as mock_settings: + mock_settings.return_value.get.side_effect = lambda key, default=None: { + "GITLAB.URL": "https://gitlab.com", + "GITLAB.PERSONAL_ACCESS_TOKEN": "fake_token", + }.get(key, default) + mock_gitlab_client.projects.get.return_value = mock_project + provider = GitLabProvider("https://gitlab.com/test/repo/-/merge_requests/1") + provider.gl = mock_gitlab_client + provider.id_project = "test/repo" + provider.mr = MagicMock() + provider.mr.web_url = "https://gitlab.com/test/repo/-/merge_requests/1" + provider.mr.diff_refs = {"base_sha": "base", "head_sha": "head", "start_sha": "base"} + return provider + + @staticmethod + def _make_note(note_id, body, created_at): + n = MagicMock() + n.id = note_id + n.body = body + n.created_at = created_at + return n + + @staticmethod + def _make_commit(sha, committed_date): + c = MagicMock(spec=["id", "committed_date", "authored_date", "created_at"]) + c.id = sha + c.committed_date = committed_date + c.authored_date = committed_date + c.created_at = committed_date + return c + + def test_get_incremental_commits_no_previous_review_falls_back(self, gitlab_provider): + gitlab_provider.mr.notes.list.return_value = [ + self._make_note(1, "Just a comment", "2024-05-01T10:00:00Z"), + ] + gitlab_provider.mr.commits.return_value = [ + self._make_commit("c1", "2024-05-02T10:00:00Z"), + ] + + gitlab_provider.get_incremental_commits(IncrementalPR(True)) + + assert gitlab_provider.incremental.is_incremental is False + + def test_get_incremental_commits_picks_commits_after_review(self, gitlab_provider, mock_project): + # Previous review at T=10:00. Commit c0 at 09:00 (before), c1 and c2 at 11:00 (after). + gitlab_provider.mr.notes.list.return_value = [ + self._make_note(7, "## PR Reviewer Guide πŸ”\nbody", "2024-05-01T10:00:00Z"), + self._make_note(1, "older note", "2024-04-01T10:00:00Z"), + ] + # gitlab returns commits newest-first + gitlab_provider.mr.commits.return_value = [ + self._make_commit("c2", "2024-05-01T11:30:00Z"), + self._make_commit("c1", "2024-05-01T11:00:00Z"), + self._make_commit("c0", "2024-05-01T09:00:00Z"), + ] + mock_project.repository_compare.return_value = { + "diffs": [ + {"new_path": "a.py", "old_path": "a.py", "diff": "@@ -1 +1 @@\n-old\n+new\n", + "new_file": False, "deleted_file": False, "renamed_file": False}, + {"new_path": "b.py", "old_path": "b.py", "diff": "@@ ... @@", + "new_file": True, "deleted_file": False, "renamed_file": False}, + ] + } + + gitlab_provider.get_incremental_commits(IncrementalPR(True)) + + assert gitlab_provider.incremental.is_incremental is True + assert gitlab_provider.incremental.first_new_commit_sha == "c1" + assert gitlab_provider.incremental.last_seen_commit_sha == "c0" + assert set(gitlab_provider.unreviewed_files_set.keys()) == {"a.py", "b.py"} + mock_project.repository_compare.assert_called_once_with("c0", "head") + + def test_get_incremental_commits_no_new_commits_yields_empty_set(self, gitlab_provider, mock_project): + gitlab_provider.mr.notes.list.return_value = [ + self._make_note(7, "## PR Reviewer Guide πŸ”\nbody", "2024-05-01T20:00:00Z"), + ] + gitlab_provider.mr.commits.return_value = [ + self._make_commit("c0", "2024-05-01T09:00:00Z"), + ] + + gitlab_provider.get_incremental_commits(IncrementalPR(True)) + + # is_incremental stays True so the reviewer publishes the "no new files" message; + # unreviewed_files_set is empty. + assert gitlab_provider.incremental.is_incremental is True + assert gitlab_provider.unreviewed_files_set == {} + mock_project.repository_compare.assert_not_called() + + def test_get_incremental_commits_no_anchor_commit_falls_back(self, gitlab_provider, mock_project): + # All commits are after the previous review -> no last_seen_commit -> can't anchor. + gitlab_provider.mr.notes.list.return_value = [ + self._make_note(7, "## PR Reviewer Guide πŸ”\nbody", "2024-05-01T08:00:00Z"), + ] + gitlab_provider.mr.commits.return_value = [ + self._make_commit("c1", "2024-05-01T11:00:00Z"), + ] + + gitlab_provider.get_incremental_commits(IncrementalPR(True)) + + assert gitlab_provider.incremental.is_incremental is False + mock_project.repository_compare.assert_not_called() + + def test_get_files_uses_incremental_set_when_active(self, gitlab_provider): + gitlab_provider.incremental = IncrementalPR(True) + gitlab_provider.unreviewed_files_set = {"a.py": {"new_path": "a.py"}} + + assert gitlab_provider.get_files() == ["a.py"] + gitlab_provider.mr.changes.assert_not_called() + + def test_get_files_falls_back_to_mr_changes_when_not_incremental(self, gitlab_provider): + gitlab_provider.incremental = IncrementalPR(False) + gitlab_provider.git_files = None + gitlab_provider.mr.changes.return_value = {"changes": [{"new_path": "x.py"}]} + + assert gitlab_provider.get_files() == ["x.py"] + + def test_get_previous_review_returns_most_recent_match(self, gitlab_provider): + from pr_agent.algo.utils import PRReviewHeader + + gitlab_provider.mr.notes.list.return_value = [ + self._make_note(1, f"{PRReviewHeader.REGULAR.value} πŸ”\nold", "2024-04-01T10:00:00Z"), + self._make_note(2, f"{PRReviewHeader.REGULAR.value} πŸ”\nnew", "2024-05-01T10:00:00Z"), + self._make_note(3, "unrelated", "2024-06-01T10:00:00Z"), + ] + + result = gitlab_provider.get_previous_review(full=True, incremental=True) + + assert result is not None + assert result.id == 2 From acd1c171f692370605e712fb20f822875f0d0e65 Mon Sep 17 00:00:00 2001 From: Sergey Petrov Date: Thu, 14 May 2026 19:12:20 +0300 Subject: [PATCH 2/7] fix(gitlab): address Qodo review feedback on incremental flow - get_commit_range: skip commits whose timestamp fails to parse so they never become last_seen_commit. PRReviewer compares that field with `>` against a datetime and would raise TypeError if the date were None. - get_diff_files (incremental branch): run raw_changes through _expand_submodule_changes so GITLAB.EXPAND_SUBMODULE_DIFFS keeps working under /review -i, matching the full-review path. - get_diff_files (incremental branch): drop the unsafe diff_refs index fallback for head_sha; rely on _incremental_head_sha (always populated when incremental_active) and a defensive .get() for the rare callers that arrange state by hand. Tests cover both fixes: a dateless commit no longer anchors the incremental window, and _expand_submodule_changes is invoked on the incremental path. --- pr_agent/git_providers/gitlab_provider.py | 20 +++++++-- tests/unittest/test_gitlab_provider.py | 55 +++++++++++++++++++++++ 2 files changed, 72 insertions(+), 3 deletions(-) diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index 1ce6ef8c8a..c894964d8f 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -482,7 +482,15 @@ def get_commit_range(self): for index in range(len(self.mr_commits) - 1, -1, -1): adapter = _GitlabIncrementalCommit(self.mr_commits[index]) commit_time = adapter.commit.author.date - if commit_time is not None and commit_time > last_review_time: + if commit_time is None: + # A commit without a parseable timestamp cannot be placed on the timeline; + # skip it so it never lands in last_seen_commit (PRReviewer compares that + # date with `>`, which would TypeError against None). + get_logger().warning( + f"Skipping commit {adapter.sha} with unparseable timestamp during incremental review" + ) + continue + if commit_time > last_review_time: self.incremental.first_new_commit = adapter first_new_commit_index = index else: @@ -585,9 +593,15 @@ def get_diff_files(self) -> list[FilePatchInfo]: if incremental_active: raw_changes = list(self.unreviewed_files_set.values()) + # Apply submodule expansion symmetrically with the full-review path so that + # `GITLAB.EXPAND_SUBMODULE_DIFFS` keeps working under `/review -i`. + raw_changes = self._expand_submodule_changes(raw_changes) base_sha_for_content = self.incremental.last_seen_commit_sha - head_sha_for_content = getattr(self, '_incremental_head_sha', None) \ - or self.mr.diff_refs['head_sha'] + # `_incremental_head_sha` is populated by `_get_incremental_commits()` whenever + # incremental_active is true; we still guard for defensive callers. + head_sha_for_content = getattr(self, '_incremental_head_sha', None) + if not head_sha_for_content: + head_sha_for_content = (self.mr.diff_refs or {}).get('head_sha') else: raw_changes = self.mr.changes().get('changes', []) raw_changes = self._expand_submodule_changes(raw_changes) diff --git a/tests/unittest/test_gitlab_provider.py b/tests/unittest/test_gitlab_provider.py index ea0985b2ee..e3cc368553 100644 --- a/tests/unittest/test_gitlab_provider.py +++ b/tests/unittest/test_gitlab_provider.py @@ -393,3 +393,58 @@ def test_get_previous_review_returns_most_recent_match(self, gitlab_provider): assert result is not None assert result.id == 2 + + def test_commit_with_unparseable_date_is_skipped_not_anchored(self, gitlab_provider, mock_project): + # Anchor commit (c0) has a valid date older than the review; a stray dateless + # commit (cX) sits between the new commits and must not become last_seen_commit. + gitlab_provider.mr.notes.list.return_value = [ + self._make_note(7, "## PR Reviewer Guide πŸ”\nbody", "2024-05-01T10:00:00Z"), + ] + bad_commit = MagicMock(spec=["id", "committed_date", "authored_date", "created_at"]) + bad_commit.id = "cX" + bad_commit.committed_date = "not-a-date" + bad_commit.authored_date = None + bad_commit.created_at = None + gitlab_provider.mr.commits.return_value = [ + self._make_commit("c1", "2024-05-01T11:00:00Z"), + bad_commit, + self._make_commit("c0", "2024-05-01T09:00:00Z"), + ] + mock_project.repository_compare.return_value = { + "diffs": [{"new_path": "a.py", "old_path": "a.py", "diff": "@@ ... @@", + "new_file": False, "deleted_file": False, "renamed_file": False}], + } + + gitlab_provider.get_incremental_commits(IncrementalPR(True)) + + # The dateless commit must be ignored: anchor falls through to c0 (valid date). + assert gitlab_provider.incremental.is_incremental is True + assert gitlab_provider.incremental.last_seen_commit_sha == "c0" + assert gitlab_provider.incremental.last_seen_commit.commit.author.date is not None + assert gitlab_provider.incremental.first_new_commit_sha == "c1" + + def test_incremental_get_diff_files_expands_submodule_changes(self, gitlab_provider): + # Set up incremental state directly to isolate get_diff_files behaviour. + gitlab_provider.incremental = IncrementalPR(True) + gitlab_provider.unreviewed_files_set = { + "libs/sub": {"new_path": "libs/sub", "old_path": "libs/sub", + "diff": "-Subproject commit aaa\n+Subproject commit bbb\n", + "new_file": False, "deleted_file": False, "renamed_file": False} + } + gitlab_provider._incremental_head_sha = "head" + gitlab_provider.incremental.last_seen_commit = _GitlabIncrementalCommit( + self._make_commit("c0", "2024-05-01T09:00:00Z") + ) + + expanded = [{ + "new_path": "libs/sub/file.py", "old_path": "libs/sub/file.py", + "diff": "@@ ... @@", "new_file": False, "deleted_file": False, "renamed_file": False, + }] + with patch.object(gitlab_provider, "_expand_submodule_changes", return_value=expanded) as m_exp, \ + patch.object(gitlab_provider, "get_pr_file_content", return_value=""): + files = gitlab_provider.get_diff_files() + + # _expand_submodule_changes was called with the incremental raw_changes, + # and the resulting file list reflects the expanded entries. + m_exp.assert_called_once() + assert [f.filename for f in files] == ["libs/sub/file.py"] From cd141c007ba41604fb5b4d11a083ae472b4cef15 Mon Sep 17 00:00:00 2001 From: Sergey Petrov Date: Thu, 14 May 2026 19:30:07 +0300 Subject: [PATCH 3/7] fix(gitlab): stabilize notes cache for MRs with no notes `get_previous_review()` guarded the notes cache with `not getattr(self, '_incremental_notes_cache', None)`, which treats an empty list as "not cached" and re-fetches from GitLab on every call. Switch to `hasattr` so a legitimately empty notes list is preserved as a cached sentinel; the API call only happens once. --- pr_agent/git_providers/gitlab_provider.py | 4 +++- tests/unittest/test_gitlab_provider.py | 10 ++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index c894964d8f..e9abb0da5a 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -501,7 +501,9 @@ def get_commit_range(self): def get_previous_review(self, *, full: bool, incremental: bool): if not (full or incremental): raise ValueError("At least one of full or incremental must be True") - if not getattr(self, '_incremental_notes_cache', None): + # Use hasattr (not truthy) so a legitimately empty notes list still counts as cached; + # otherwise we'd re-fetch from GitLab on every call for MRs that have no notes. + if not hasattr(self, '_incremental_notes_cache'): try: self._incremental_notes_cache = list(self.mr.notes.list(get_all=True)) except Exception as e: diff --git a/tests/unittest/test_gitlab_provider.py b/tests/unittest/test_gitlab_provider.py index e3cc368553..2286e36a68 100644 --- a/tests/unittest/test_gitlab_provider.py +++ b/tests/unittest/test_gitlab_provider.py @@ -423,6 +423,16 @@ def test_commit_with_unparseable_date_is_skipped_not_anchored(self, gitlab_provi assert gitlab_provider.incremental.last_seen_commit.commit.author.date is not None assert gitlab_provider.incremental.first_new_commit_sha == "c1" + def test_get_previous_review_caches_empty_notes_list(self, gitlab_provider): + # An MR with no notes must still cache the result; falsy-checks would re-fetch each call. + gitlab_provider.mr.notes.list.return_value = [] + + first = gitlab_provider.get_previous_review(full=True, incremental=True) + second = gitlab_provider.get_previous_review(full=True, incremental=True) + + assert first is None and second is None + assert gitlab_provider.mr.notes.list.call_count == 1 + def test_incremental_get_diff_files_expands_submodule_changes(self, gitlab_provider): # Set up incremental state directly to isolate get_diff_files behaviour. gitlab_provider.incremental = IncrementalPR(True) From 4e4b931936f60ad8acd44105c162949145e5ba42 Mon Sep 17 00:00:00 2001 From: Sergey Petrov Date: Fri, 15 May 2026 10:08:25 +0300 Subject: [PATCH 4/7] fix(gitlab): fall back to full review when timeline can't be anchored MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_get_incremental_commits` previously returned early with `is_incremental=True` whenever `commits_range` came back empty, which conflated two distinct cases: - legitimately no new commits since the previous review (last_seen_commit set) β€” the reviewer should publish "Incremental Review Skipped"; - a broken timeline (previous review's timestamp didn't parse, or every post-review commit had an unparseable date) β€” `last_seen_commit` was never set, but the run still silently produced "no new files". Distinguish the two by checking whether `last_seen_commit` was anchored. When it wasn't, disable `is_incremental` so the run falls back to a full review instead of silently dropping it. Two new tests cover both failure modes (unparseable review timestamp and all-dateless commits). --- pr_agent/git_providers/gitlab_provider.py | 14 ++++++++++ tests/unittest/test_gitlab_provider.py | 33 +++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index e9abb0da5a..e3e35dd38d 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -435,6 +435,20 @@ def _get_incremental_commits(self): self.incremental.commits_range = self.get_commit_range() if not self.incremental.commits_range: + # Disambiguate two cases: + # - last_seen_commit is set: we successfully walked the commit timeline and + # found that all commits are at-or-before the previous review, i.e. legitimately + # no new commits since the last review. Keep is_incremental=True so the reviewer + # surfaces "Incremental Review Skipped β€” no files changed". + # - last_seen_commit is unset: we couldn't anchor any commit on the timeline + # (the previous review's timestamp didn't parse, or every post-review commit was + # dateless). Fall back to a full review rather than silently dropping the run. + if self.incremental.last_seen_commit is None: + get_logger().info( + "Could not establish a commit timeline against the previous review " + "(missing/unparseable timestamps); falling back to a full review" + ) + self.incremental.is_incremental = False return last_seen_sha = self.incremental.last_seen_commit_sha diff --git a/tests/unittest/test_gitlab_provider.py b/tests/unittest/test_gitlab_provider.py index 2286e36a68..618662529f 100644 --- a/tests/unittest/test_gitlab_provider.py +++ b/tests/unittest/test_gitlab_provider.py @@ -423,6 +423,39 @@ def test_commit_with_unparseable_date_is_skipped_not_anchored(self, gitlab_provi assert gitlab_provider.incremental.last_seen_commit.commit.author.date is not None assert gitlab_provider.incremental.first_new_commit_sha == "c1" + def test_unparseable_review_timestamp_falls_back_to_full(self, gitlab_provider, mock_project): + # If the previous review's created_at didn't parse, we can't position commits on the + # timeline; we must fall back to a full review rather than silently report "no new files". + gitlab_provider.mr.notes.list.return_value = [ + self._make_note(7, "## PR Reviewer Guide πŸ”\nbody", "not-a-date"), + ] + gitlab_provider.mr.commits.return_value = [ + self._make_commit("c1", "2024-05-01T11:00:00Z"), + ] + + gitlab_provider.get_incremental_commits(IncrementalPR(True)) + + assert gitlab_provider.incremental.is_incremental is False + mock_project.repository_compare.assert_not_called() + + def test_all_post_review_commits_dateless_falls_back_to_full(self, gitlab_provider, mock_project): + # If every commit after the previous review has an unparseable timestamp, we can't + # anchor a last_seen_commit. The fix must fall back to full review, not produce a + # spurious "Incremental Review Skipped" message. + gitlab_provider.mr.notes.list.return_value = [ + self._make_note(7, "## PR Reviewer Guide πŸ”\nbody", "2024-05-01T10:00:00Z"), + ] + bad1 = MagicMock(spec=["id", "committed_date", "authored_date", "created_at"]) + bad1.id, bad1.committed_date, bad1.authored_date, bad1.created_at = "cX1", None, None, None + bad2 = MagicMock(spec=["id", "committed_date", "authored_date", "created_at"]) + bad2.id, bad2.committed_date, bad2.authored_date, bad2.created_at = "cX2", "garbage", None, None + gitlab_provider.mr.commits.return_value = [bad1, bad2] + + gitlab_provider.get_incremental_commits(IncrementalPR(True)) + + assert gitlab_provider.incremental.is_incremental is False + mock_project.repository_compare.assert_not_called() + def test_get_previous_review_caches_empty_notes_list(self, gitlab_provider): # An MR with no notes must still cache the result; falsy-checks would re-fetch each call. gitlab_provider.mr.notes.list.return_value = [] From 595684f0f2b1b4aa7b29056e217ed3a9a11a04ca Mon Sep 17 00:00:00 2001 From: Sergey Petrov Date: Fri, 15 May 2026 10:41:17 +0300 Subject: [PATCH 5/7] feat(gitlab): support incremental /improve via /improve -i MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/improve` was unaffected by the GitLab incremental work: each push to an MR re-scanned the full diff and re-posted the same `**Suggestion:** ...` notes on the same lines, producing duplicate inline comments. Witness note IDs 158625 / 158636 / 158637 on MR teamtravelata/travelata!1115 β€” three identical suggestions for the same `parseInt(...)` line across three push events. Mirror what `/review -i` does for incremental scope, but anchor on the last /improve output rather than the last /review: - GitLabProvider.get_incremental_commits gains a `kind="review"` kwarg. `kind="suggestions"` anchors on the most recent prior `## PR Code Suggestions ✨` summary OR inline `**Suggestion:**` note, whichever is newer. `kind="review"` keeps existing /review -i semantics. - Extracted shared `_find_anchor_note(prefixes)` helper so review and suggestions paths share the cache + sort logic. - PRCodeSuggestions parses `-i` exactly like PRReviewer.parse_incremental and activates `kind="suggestions"` incremental on the provider when the kwarg is supported. If the provider doesn't accept the kwarg, the flag degrades to a no-op (full /improve), so older providers keep working unchanged. - `run()` short-circuits with a log line when incremental is active but no files changed since the last suggestions pass, instead of falling through to a full /improve pass. Tests cover three new branches: anchoring on a suggestion note ahead of a later review note, fallback when no prior suggestion exists, and the default `kind="review"` still ignores suggestion notes. --- pr_agent/git_providers/gitlab_provider.py | 56 ++++++++++++++++----- pr_agent/tools/pr_code_suggestions.py | 35 +++++++++++++- tests/unittest/test_gitlab_provider.py | 59 +++++++++++++++++++++++ 3 files changed, 136 insertions(+), 14 deletions(-) diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index e3e35dd38d..f3bd2d780c 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -407,12 +407,26 @@ def _set_merge_request(self, merge_request_url: str): get_logger().error(f"Could not get diff for merge request {self.id_mr}") raise DiffNotFoundError(f"Could not get diff for merge request {self.id_mr}") from e - def get_incremental_commits(self, incremental: Optional[IncrementalPR] = None): - """Populate state needed for an incremental review. - - Mirrors `GithubProvider.get_incremental_commits`: locates the previous review note, - determines which commits have arrived since, and pre-computes the diff set restricted - to those commits. Falls back silently to a full review when no previous review exists. + # Anchor-note prefixes per incremental "kind". An incremental run looks for the most recent + # prior note matching ANY of these prefixes and uses its timestamp as the timeline anchor. + _INCREMENTAL_ANCHOR_PREFIXES = { + "review": ( + PRReviewHeader.REGULAR.value, # "## PR Reviewer Guide" + PRReviewHeader.INCREMENTAL.value, # "## Incremental PR Reviewer Guide" + ), + "suggestions": ( + "## PR Code Suggestions ✨", # summary-table mode + "**Suggestion:**", # commitable-suggestions inline mode + ), + } + + def get_incremental_commits(self, incremental: Optional[IncrementalPR] = None, kind: str = "review"): + """Populate state needed for an incremental run. + + Mirrors `GithubProvider.get_incremental_commits` for `/review -i`, and also supports + `/improve -i` via `kind="suggestions"` β€” in that case we anchor on the most recent prior + `## PR Code Suggestions` or inline `**Suggestion:**` note instead of a review note, so + re-runs of `/improve` only act on commits added since the last suggestions pass. """ if incremental is None: incremental = IncrementalPR(False) @@ -420,6 +434,7 @@ def get_incremental_commits(self, incremental: Optional[IncrementalPR] = None): if not self.incremental.is_incremental: return self.unreviewed_files_set = {} + self._incremental_kind = kind self._get_incremental_commits() def _get_incremental_commits(self): @@ -427,9 +442,13 @@ def _get_incremental_commits(self): # gitlab returns commits newest-first; reverse to match PyGithub's oldest-first ordering self.mr_commits = list(self.mr.commits())[::-1] - self.previous_review = self.get_previous_review(full=True, incremental=True) + kind = getattr(self, '_incremental_kind', 'review') + prefixes = self._INCREMENTAL_ANCHOR_PREFIXES.get(kind, ()) + self.previous_review = self._find_anchor_note(prefixes) if prefixes else None if not self.previous_review: - get_logger().info("No previous review found, will review the entire MR") + get_logger().info( + f"No previous {kind} comment found, will fall back to a full run" + ) self.incremental.is_incremental = False return @@ -515,6 +534,22 @@ def get_commit_range(self): def get_previous_review(self, *, full: bool, incremental: bool): if not (full or incremental): raise ValueError("At least one of full or incremental must be True") + prefixes = [] + if full: + prefixes.append(PRReviewHeader.REGULAR.value) + if incremental: + prefixes.append(PRReviewHeader.INCREMENTAL.value) + return self._find_anchor_note(prefixes) + + def _find_anchor_note(self, prefixes): + """Return the most recent MR note whose body starts with any of `prefixes`. + + Used by incremental flows (`/review -i`, `/improve -i`) to find the timestamp + we anchor the commit timeline on. Returns a `_GitlabIncrementalNote` adapter + with `.created_at` parsed to a naive UTC datetime, or `None` if no match. + """ + if not prefixes: + return None # Use hasattr (not truthy) so a legitimately empty notes list still counts as cached; # otherwise we'd re-fetch from GitLab on every call for MRs that have no notes. if not hasattr(self, '_incremental_notes_cache'): @@ -523,11 +558,6 @@ def get_previous_review(self, *, full: bool, incremental: bool): except Exception as e: get_logger().error(f"Failed to list MR notes for incremental review: {e}") return None - prefixes = [] - if full: - prefixes.append(PRReviewHeader.REGULAR.value) - if incremental: - prefixes.append(PRReviewHeader.INCREMENTAL.value) # gitlab returns notes newest-first; pick the most recent matching note notes_sorted = sorted( (n for n in self._incremental_notes_cache if getattr(n, 'body', None)), diff --git a/pr_agent/tools/pr_code_suggestions.py b/pr_agent/tools/pr_code_suggestions.py index 6372396c8b..4f0f1922aa 100644 --- a/pr_agent/tools/pr_code_suggestions.py +++ b/pr_agent/tools/pr_code_suggestions.py @@ -24,7 +24,7 @@ from pr_agent.git_providers import (AzureDevopsProvider, GithubProvider, GitLabProvider, get_git_provider, get_git_provider_with_context) -from pr_agent.git_providers.git_provider import get_main_pr_language, GitProvider +from pr_agent.git_providers.git_provider import IncrementalPR, get_main_pr_language, GitProvider from pr_agent.log import get_logger from pr_agent.servers.help import HelpMessage from pr_agent.tools.pr_description import insert_br_after_x_chars @@ -36,6 +36,21 @@ def __init__(self, pr_url: str, cli_mode=False, args: list = None, ai_handler: partial[BaseAiHandler,] = LiteLLMAIHandler): self.git_provider = get_git_provider_with_context(pr_url) + self.args = args + self.incremental = self._parse_incremental(args) + # When invoked as `/improve -i`, narrow `git_provider.get_diff_files()` to the files + # changed since the previous suggestions pass. Falls back to full when the provider + # doesn't support incremental scope or no prior suggestion comment exists. + if self.incremental.is_incremental and hasattr(self.git_provider, "get_incremental_commits"): + try: + self.git_provider.get_incremental_commits(self.incremental, kind="suggestions") + except TypeError: + # Older provider signature without the `kind` kwarg β€” skip incremental scope. + get_logger().info( + "Provider does not support kind-based incremental commits; " + "running /improve on the full MR diff" + ) + self.incremental = IncrementalPR(False) self.main_language = get_main_pr_language( self.git_provider.get_languages(), self.git_provider.get_files() ) @@ -90,8 +105,26 @@ def __init__(self, pr_url: str, cli_mode=False, args: list = None, self.progress = build_progress_comment() self.progress_response = None + @staticmethod + def _parse_incremental(args): + """Parse the `-i` flag for `/improve` exactly like `PRReviewer.parse_incremental`.""" + is_incremental = bool(args and len(args) >= 1 and args[0] == "-i") + return IncrementalPR(is_incremental) + async def run(self): try: + if (self.incremental.is_incremental + and hasattr(self.git_provider, "unreviewed_files_set") + and not self.git_provider.unreviewed_files_set): + # Anchor note exists and the timeline walked cleanly, but no files changed in the + # commits added since the previous suggestions pass. Skip silently instead of + # re-running on the full MR (which would re-post identical suggestions). + get_logger().info( + f"Incremental /improve for {self.pr_url}: no files changed since the previous " + f"suggestions pass; skipping" + ) + return None + if not self.git_provider.get_files(): get_logger().info(f"PR has no files: {self.pr_url}, skipping code suggestions") return None diff --git a/tests/unittest/test_gitlab_provider.py b/tests/unittest/test_gitlab_provider.py index d021b6389f..e4a1b1f613 100644 --- a/tests/unittest/test_gitlab_provider.py +++ b/tests/unittest/test_gitlab_provider.py @@ -506,6 +506,65 @@ def test_get_previous_review_caches_empty_notes_list(self, gitlab_provider): assert first is None and second is None assert gitlab_provider.mr.notes.list.call_count == 1 + def test_incremental_kind_suggestions_anchors_on_suggestion_note(self, gitlab_provider, mock_project): + # When kind="suggestions", we anchor on the latest /improve output (either the + # "## PR Code Suggestions ✨" summary or an inline "**Suggestion:**" note), + # NOT on a /review note posted later in the same CI run. + gitlab_provider.mr.notes.list.return_value = [ + # Most recent: a review-incremental note posted AFTER the last /improve run. + self._make_note(9, "## Incremental PR Reviewer Guide πŸ”\nbody", "2026-05-15T10:05:00Z"), + # The actual /improve anchor we want to pick. + self._make_note(8, "**Suggestion:** Π˜ΡΠΏΠΎΠ»ΡŒΠ·ΡƒΠΉΡ‚Π΅ Number вмСсто parseInt...", "2026-05-15T10:00:00Z"), + # An older /review note that should NOT win over the suggestion above. + self._make_note(5, "## PR Reviewer Guide πŸ”\nold", "2026-05-15T09:00:00Z"), + ] + gitlab_provider.mr.commits.return_value = [ + self._make_commit("c2", "2026-05-15T10:10:00Z"), # after the suggestion note + self._make_commit("c0", "2026-05-15T09:30:00Z"), # before the suggestion note + ] + mock_project.repository_compare.return_value = { + "diffs": [{"new_path": "a.py", "old_path": "a.py", "diff": "@@ ... @@", + "new_file": False, "deleted_file": False, "renamed_file": False}], + } + + gitlab_provider.get_incremental_commits(IncrementalPR(True), kind="suggestions") + + assert gitlab_provider.incremental.is_incremental is True + assert gitlab_provider.incremental.first_new_commit_sha == "c2" + assert gitlab_provider.incremental.last_seen_commit_sha == "c0" + mock_project.repository_compare.assert_called_once_with("c0", "head") + + def test_incremental_kind_suggestions_falls_back_when_no_prior_suggestion(self, gitlab_provider, mock_project): + # A /review note exists, but no /improve has ever run. /improve -i must fall back to + # a full pass, not anchor on the review note. + gitlab_provider.mr.notes.list.return_value = [ + self._make_note(5, "## PR Reviewer Guide πŸ”\nbody", "2026-05-15T09:00:00Z"), + ] + gitlab_provider.mr.commits.return_value = [ + self._make_commit("c0", "2026-05-15T08:30:00Z"), + ] + + gitlab_provider.get_incremental_commits(IncrementalPR(True), kind="suggestions") + + assert gitlab_provider.incremental.is_incremental is False + mock_project.repository_compare.assert_not_called() + + def test_get_incremental_commits_default_kind_is_review(self, gitlab_provider, mock_project): + # Sanity-check backward compatibility: no kind kwarg β‡’ behaves like a review run. + gitlab_provider.mr.notes.list.return_value = [ + # A /improve note that must be IGNORED in default (review) mode. + self._make_note(8, "**Suggestion:** xyz", "2026-05-15T10:00:00Z"), + ] + gitlab_provider.mr.commits.return_value = [ + self._make_commit("c0", "2026-05-15T09:30:00Z"), + ] + + gitlab_provider.get_incremental_commits(IncrementalPR(True)) + + # No review note exists -> fallback to full review (NOT anchoring on the suggestion note). + assert gitlab_provider.incremental.is_incremental is False + mock_project.repository_compare.assert_not_called() + def test_incremental_get_diff_files_expands_submodule_changes(self, gitlab_provider): # Set up incremental state directly to isolate get_diff_files behaviour. gitlab_provider.incremental = IncrementalPR(True) From 1fc55a14c48638e4fda3a1883035c9ff19ed585c Mon Sep 17 00:00:00 2001 From: Sergey Petrov Date: Fri, 15 May 2026 11:14:39 +0300 Subject: [PATCH 6/7] fix(gitlab): exclude target-branch merge content from incremental scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When `git merge ` is run on the MR's source branch between two incremental passes, `repository_compare(last_seen_sha, head_sha)` walks through the merge commit and surfaces every file the target branch touched since the MR's merge-base β€” files that are NOT part of the MR's own contribution. Reproduction (teamtravelata/travelata!1115): 1. MR open on `test-pr`, only frontend JS files changed. 2. /improve and /review run, post initial suggestions/review. 3. User runs `git merge master`, which brings in CI config changes (notably `.gitlab-ci.yml` from !1116) via a merge commit. 4. Next /review -i posts a suggestion on `.gitlab-ci.yml`, a file the MR has never touched. `mr.changes()` is anchored on the MR's merge-base with target, so it correctly excludes target-side content. Intersect the file set from `repository_compare` with `mr.changes()` to drop these "phantom" files. Files where the MR also has its own changes still go through (with their full compare diff β€” partial overlap with target content is rare and benign next to the original bug). --- pr_agent/git_providers/gitlab_provider.py | 32 +++++++++++++++-- tests/unittest/test_gitlab_provider.py | 42 +++++++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index f3bd2d780c..0a558d92dc 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -502,10 +502,38 @@ def _get_incremental_commits(self): else: diffs = getattr(compare_result, 'diffs', []) or [] + # `repository_compare(last_seen_sha, head_sha)` walks every commit on the path between + # the two SHAs, so if `git merge ` was run on the MR branch since the last + # incremental pass, files that only changed in the target branch (and were brought in + # via the merge) appear in `diffs` β€” even though they are not part of the MR's own + # contribution and would never appear in a full /review. + # + # `mr.changes()` is anchored on the MR's merge-base with target, so it correctly excludes + # target-side changes. Intersect file paths to drop "phantom" files brought in via merge. + mr_change_paths = None + try: + mr_change_paths = { + c.get('new_path') + for c in self.mr.changes().get('changes', []) + if c.get('new_path') + } + except Exception as e: + get_logger().warning( + f"Could not fetch mr.changes() to filter incremental scope; " + f"merge-from-target changes may leak into the review: {e}" + ) + for diff in diffs: new_path = diff.get('new_path') if isinstance(diff, dict) else None - if new_path: - self.unreviewed_files_set[new_path] = diff + if not new_path: + continue + if mr_change_paths is not None and new_path not in mr_change_paths: + get_logger().debug( + f"Excluding {new_path} from incremental scope: not part of the MR diff " + f"(likely brought in via a merge from the target branch)" + ) + continue + self.unreviewed_files_set[new_path] = diff def get_commit_range(self): last_review_time = getattr(self.previous_review, 'created_at', None) diff --git a/tests/unittest/test_gitlab_provider.py b/tests/unittest/test_gitlab_provider.py index e4a1b1f613..a24330b41e 100644 --- a/tests/unittest/test_gitlab_provider.py +++ b/tests/unittest/test_gitlab_provider.py @@ -367,6 +367,11 @@ def test_get_incremental_commits_picks_commits_after_review(self, gitlab_provide "new_file": True, "deleted_file": False, "renamed_file": False}, ] } + # mr.changes() is intersected with repository_compare to exclude files brought in + # via a merge from the target branch. Here both files are part of the MR. + gitlab_provider.mr.changes.return_value = { + "changes": [{"new_path": "a.py"}, {"new_path": "b.py"}] + } gitlab_provider.get_incremental_commits(IncrementalPR(True)) @@ -434,6 +439,42 @@ def test_get_previous_review_returns_most_recent_match(self, gitlab_provider): assert result is not None assert result.id == 2 + def test_master_merge_files_are_excluded_from_incremental_scope(self, gitlab_provider, mock_project): + # Reproduction of the MR !1115 bug: user ran `git merge master` on the feature branch, + # which brought CI/config changes into the branch via a merge commit. Those files are + # NOT part of mr.changes() (the MR's actual contribution against its merge-base), but + # repository_compare(last_seen, head) walks through the merge and surfaces them. + # The fix intersects with mr.changes() to drop these "phantom" files. + gitlab_provider.mr.notes.list.return_value = [ + self._make_note(7, "## PR Reviewer Guide πŸ”\nbody", "2024-05-01T10:00:00Z"), + ] + gitlab_provider.mr.commits.return_value = [ + self._make_commit("merge", "2024-05-01T11:30:00Z"), # merge from master + self._make_commit("feat", "2024-05-01T11:00:00Z"), # author commit on feature + self._make_commit("c0", "2024-05-01T09:00:00Z"), # anchor (pre-review) + ] + mock_project.repository_compare.return_value = { + "diffs": [ + # MR's own change to a frontend file β€” must be reviewed + {"new_path": "src/feature.js", "old_path": "src/feature.js", "diff": "@@ ... @@", + "new_file": False, "deleted_file": False, "renamed_file": False}, + # Pulled in via merge from master, NOT part of the MR β€” must be excluded + {"new_path": ".gitlab-ci.yml", "old_path": ".gitlab-ci.yml", "diff": "@@ ... @@", + "new_file": False, "deleted_file": False, "renamed_file": False}, + ] + } + # mr.changes() returns only the MR's actual contribution; .gitlab-ci.yml is absent + # because the change to it lives in the target branch already. + gitlab_provider.mr.changes.return_value = { + "changes": [{"new_path": "src/feature.js"}] + } + + gitlab_provider.get_incremental_commits(IncrementalPR(True)) + + assert gitlab_provider.incremental.is_incremental is True + assert set(gitlab_provider.unreviewed_files_set.keys()) == {"src/feature.js"} + assert ".gitlab-ci.yml" not in gitlab_provider.unreviewed_files_set + def test_commit_with_unparseable_date_is_skipped_not_anchored(self, gitlab_provider, mock_project): # Anchor commit (c0) has a valid date older than the review; a stray dateless # commit (cX) sits between the new commits and must not become last_seen_commit. @@ -526,6 +567,7 @@ def test_incremental_kind_suggestions_anchors_on_suggestion_note(self, gitlab_pr "diffs": [{"new_path": "a.py", "old_path": "a.py", "diff": "@@ ... @@", "new_file": False, "deleted_file": False, "renamed_file": False}], } + gitlab_provider.mr.changes.return_value = {"changes": [{"new_path": "a.py"}]} gitlab_provider.get_incremental_commits(IncrementalPR(True), kind="suggestions") From a534a03ae498f781fe582f6d30cfabe20b3cee9d Mon Sep 17 00:00:00 2001 From: Sergey Petrov Date: Fri, 15 May 2026 11:56:59 +0300 Subject: [PATCH 7/7] fix(gitlab): address Qodo round-4 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six findings from the latest bot review: 1. `_find_anchor_note` calls `body.startswith` without checking the body is a string β€” add an `isinstance(body, str)` guard. Paranoid but cheap. 2. Anchor selection was wrong when the newest matching note's timestamp couldn't be parsed: the local `sorted(...)` with a `datetime.min` fallback demoted the newest match below older parseable ones, so incremental runs anchored on an older note and re-reviewed already-reviewed commits. Drop the local sort entirely β€” GitLab returns notes in created_at-DESC order by default, so iterating in natural order picks the newest match. When that match has an unparseable timestamp, `_get_incremental_commits` already falls back to a full run via the `last_seen_commit is None` branch. 3. `_parse_gitlab_iso_datetime` used single-quoted string literals β€” switched to double quotes to match the repo's Ruff convention. 4. `_get_incremental_commits` extracted `new_path` via dict access only; non-dict diff entries (a stricter library or a stubbed client) were silently dropped. Add a `getattr(diff, 'new_path', None)` fallback so the object-shape branch isn't dead code. 5. `pr_code_suggestions.py` import line wasn't isort-clean after adding `IncrementalPR` β€” reorder to `GitProvider, IncrementalPR, get_main_pr_language`. 6. `PRCodeSuggestions.__init__` called `get_main_pr_language(get_languages(), get_files())` before `run()` short-circuited the no-op incremental path. For GitLab, `get_files()` falls back to a full `mr.changes()` call when `unreviewed_files_set` is empty β€” that's a wasted API round-trip on every push that finds nothing new to review. Set `self._incremental_empty_scope = True` and return early from `__init__`; `run()` short-circuits on the same flag. Bot finding #2 ("Commit shape mismatch") was a false positive β€” python- gitlab's `RESTObject` supports both attribute and item access on the same object, so the inconsistency the bot flagged doesn't manifest in practice. --- pr_agent/git_providers/gitlab_provider.py | 36 ++++++++++++++--------- pr_agent/tools/pr_code_suggestions.py | 25 +++++++++++----- tests/unittest/test_gitlab_provider.py | 7 +++-- 3 files changed, 44 insertions(+), 24 deletions(-) diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index 0a558d92dc..2e388d3bcd 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -46,7 +46,7 @@ def _parse_gitlab_iso_datetime(value) -> Optional[datetime]: if not isinstance(value, str): return None try: - s = value.replace('Z', '+00:00') + s = value.replace("Z", "+00:00") dt = datetime.fromisoformat(s) if dt.tzinfo is not None: dt = dt.astimezone(timezone.utc).replace(tzinfo=None) @@ -524,7 +524,13 @@ def _get_incremental_commits(self): ) for diff in diffs: - new_path = diff.get('new_path') if isinstance(diff, dict) else None + # `repository_compare` normally yields dict entries, but defend against object-shaped + # responses too β€” otherwise a stricter library or stubbed client silently empties the + # incremental set and we degrade to "no new files". + if isinstance(diff, dict): + new_path = diff.get('new_path') + else: + new_path = getattr(diff, 'new_path', None) if not new_path: continue if mr_change_paths is not None and new_path not in mr_change_paths: @@ -574,7 +580,15 @@ def _find_anchor_note(self, prefixes): Used by incremental flows (`/review -i`, `/improve -i`) to find the timestamp we anchor the commit timeline on. Returns a `_GitlabIncrementalNote` adapter - with `.created_at` parsed to a naive UTC datetime, or `None` if no match. + with `.created_at` parsed to a naive UTC datetime (possibly `None` when the + GitLab payload had an unexpected shape), or `None` if no match. + + We rely on GitLab returning notes in `created_at DESC` order (the API default) + and take the first match β€” re-sorting locally with a `datetime.min` fallback + for unparseable timestamps would silently demote the newest match in favour + of an older parseable one, leading to commits being re-reviewed. If the chosen + anchor's timestamp doesn't parse, `_get_incremental_commits` falls back to a + full run via the existing `last_seen_commit is None` branch. """ if not prefixes: return None @@ -586,18 +600,12 @@ def _find_anchor_note(self, prefixes): except Exception as e: get_logger().error(f"Failed to list MR notes for incremental review: {e}") return None - # gitlab returns notes newest-first; pick the most recent matching note - notes_sorted = sorted( - (n for n in self._incremental_notes_cache if getattr(n, 'body', None)), - key=lambda n: ( - _parse_gitlab_iso_datetime(getattr(n, 'created_at', None)) - or datetime.min - ), - reverse=True, - ) mr_web_url = getattr(self.mr, 'web_url', None) - for note in notes_sorted: - if any(note.body.startswith(prefix) for prefix in prefixes): + for note in self._incremental_notes_cache: + body = getattr(note, 'body', None) + if not isinstance(body, str): + continue + if any(body.startswith(prefix) for prefix in prefixes): return _GitlabIncrementalNote(note, mr_web_url=mr_web_url) return None diff --git a/pr_agent/tools/pr_code_suggestions.py b/pr_agent/tools/pr_code_suggestions.py index 4f0f1922aa..47f08f76bf 100644 --- a/pr_agent/tools/pr_code_suggestions.py +++ b/pr_agent/tools/pr_code_suggestions.py @@ -24,7 +24,7 @@ from pr_agent.git_providers import (AzureDevopsProvider, GithubProvider, GitLabProvider, get_git_provider, get_git_provider_with_context) -from pr_agent.git_providers.git_provider import IncrementalPR, get_main_pr_language, GitProvider +from pr_agent.git_providers.git_provider import GitProvider, IncrementalPR, get_main_pr_language from pr_agent.log import get_logger from pr_agent.servers.help import HelpMessage from pr_agent.tools.pr_description import insert_br_after_x_chars @@ -36,8 +36,10 @@ def __init__(self, pr_url: str, cli_mode=False, args: list = None, ai_handler: partial[BaseAiHandler,] = LiteLLMAIHandler): self.git_provider = get_git_provider_with_context(pr_url) + self.pr_url = pr_url # set early so the no-op log line in `run()` can reference it self.args = args self.incremental = self._parse_incremental(args) + self._incremental_empty_scope = False # When invoked as `/improve -i`, narrow `git_provider.get_diff_files()` to the files # changed since the previous suggestions pass. Falls back to full when the provider # doesn't support incremental scope or no prior suggestion comment exists. @@ -51,6 +53,16 @@ def __init__(self, pr_url: str, cli_mode=False, args: list = None, "running /improve on the full MR diff" ) self.incremental = IncrementalPR(False) + # If incremental is active but the scope came back empty (no files changed since the + # previous suggestions pass), short-circuit init now. `run()` checks the same flag and + # exits without touching the model. This avoids a wasted `mr.changes()` round-trip via + # `get_files()` β€” when `unreviewed_files_set` is `{}` it's falsy and `get_files()` falls + # back to the full MR file list, which is pure waste on the "nothing new" path. + if (self.incremental.is_incremental + and hasattr(self.git_provider, "unreviewed_files_set") + and not self.git_provider.unreviewed_files_set): + self._incremental_empty_scope = True + return self.main_language = get_main_pr_language( self.git_provider.get_languages(), self.git_provider.get_files() ) @@ -61,7 +73,6 @@ def __init__(self, pr_url: str, cli_mode=False, args: list = None, self.ai_handler.main_pr_language = self.main_language self.patches_diff = None self.prediction = None - self.pr_url = pr_url self.cli_mode = cli_mode self.pr_description, self.pr_description_files = ( self.git_provider.get_pr_description(split_changes_walkthrough=True)) @@ -113,12 +124,10 @@ def _parse_incremental(args): async def run(self): try: - if (self.incremental.is_incremental - and hasattr(self.git_provider, "unreviewed_files_set") - and not self.git_provider.unreviewed_files_set): - # Anchor note exists and the timeline walked cleanly, but no files changed in the - # commits added since the previous suggestions pass. Skip silently instead of - # re-running on the full MR (which would re-post identical suggestions). + if getattr(self, "_incremental_empty_scope", False): + # Set by `__init__` when incremental anchored cleanly but no files changed + # since the previous suggestions pass. Skip silently β€” re-running on the + # full MR diff here would just re-post the same inline suggestions. get_logger().info( f"Incremental /improve for {self.pr_url}: no files changed since the previous " f"suggestions pass; skipping" diff --git a/tests/unittest/test_gitlab_provider.py b/tests/unittest/test_gitlab_provider.py index a24330b41e..91dd485f37 100644 --- a/tests/unittest/test_gitlab_provider.py +++ b/tests/unittest/test_gitlab_provider.py @@ -428,10 +428,13 @@ def test_get_files_falls_back_to_mr_changes_when_not_incremental(self, gitlab_pr def test_get_previous_review_returns_most_recent_match(self, gitlab_provider): from pr_agent.algo.utils import PRReviewHeader + # GitLab returns notes in created_at-DESC order. The helper relies on that order + # (no local sort) β€” the unrelated newest note must be skipped, the newer matching + # note must win over the older matching note. gitlab_provider.mr.notes.list.return_value = [ - self._make_note(1, f"{PRReviewHeader.REGULAR.value} πŸ”\nold", "2024-04-01T10:00:00Z"), - self._make_note(2, f"{PRReviewHeader.REGULAR.value} πŸ”\nnew", "2024-05-01T10:00:00Z"), self._make_note(3, "unrelated", "2024-06-01T10:00:00Z"), + self._make_note(2, f"{PRReviewHeader.REGULAR.value} πŸ”\nnew", "2024-05-01T10:00:00Z"), + self._make_note(1, f"{PRReviewHeader.REGULAR.value} πŸ”\nold", "2024-04-01T10:00:00Z"), ] result = gitlab_provider.get_previous_review(full=True, incremental=True)