diff --git a/docs/docs/usage-guide/additional_configurations.md b/docs/docs/usage-guide/additional_configurations.md index 1afb6c4476..9adc0bdf76 100644 --- a/docs/docs/usage-guide/additional_configurations.md +++ b/docs/docs/usage-guide/additional_configurations.md @@ -109,6 +109,18 @@ expand_submodule_diffs = true When enabled, PR-Agent will fetch and attach diffs from the submodule repositories. The default is `false` to avoid extra GitLab API calls. +## Post the review as a GitLab thread + +By default, PR-Agent posts the `/review` summary as a plain note. To post it as a resolvable thread (GitLab discussion) instead, enable (default: `false`): + +```toml +[gitlab] +publish_review_as_thread = true +``` +- With `pr_reviewer.persistent_comment=true` (the default), each run updates the existing review thread and reopens it if it was resolved, so the refreshed review gets another look. +- Enabling the flag does not convert a review that was already posted as a plain note: it keeps being updated in place, and GitLab cannot promote a note to a thread. Only MRs whose first review runs after the flag is set get a thread. +- Set `pr_reviewer.persistent_comment=false` to open a new review thread on each run instead. + ## Log Level PR-Agent allows you to control the verbosity of logging by using the `log_level` configuration parameter. This is particularly useful for troubleshooting and debugging issues with your PR workflows. diff --git a/pr_agent/git_providers/git_provider.py b/pr_agent/git_providers/git_provider.py index 70e72c8f0f..bdda437068 100644 --- a/pr_agent/git_providers/git_provider.py +++ b/pr_agent/git_providers/git_provider.py @@ -340,18 +340,26 @@ def get_lines_link_original_file(self, filepath:str, component_range: Range) -> def publish_comment(self, pr_comment: str, is_temporary: bool = False): pass + def should_publish_review_as_thread(self) -> bool: + return False + + def unresolve_comment_thread(self, comment): # noqa: B027 - intentional no-op + pass + def publish_persistent_comment(self, pr_comment: str, initial_header: str, update_header: bool = True, name='review', - final_update_message=True): - return self.publish_comment(pr_comment) + final_update_message=True, + as_thread: bool = False): + return self.publish_comment(pr_comment, **({'as_thread': True} if as_thread else {})) def publish_persistent_comment_full(self, pr_comment: str, initial_header: str, update_header: bool = True, name='review', - final_update_message=True): + final_update_message=True, + as_thread: bool = False): try: prev_comments = list(self.get_issue_comments()) for comment in prev_comments: @@ -366,6 +374,14 @@ def publish_persistent_comment_full(self, pr_comment: str, get_logger().info(f"Persistent mode - updating comment {comment_url} to latest {name} message") # response = self.mr.notes.update(comment.id, {'body': pr_comment_updated}) self.edit_comment(comment, pr_comment_updated) + if as_thread: + try: + # Reopen the thread if it was resolved, so the developer revisits the updated review. + self.unresolve_comment_thread(comment) + except Exception as e: + # The review was already updated in place; a reopen failure must not reach the + # outer except, whose fallback publish would duplicate the review. + get_logger().warning(f"Failed to reopen review thread: {e}") if final_update_message: return self.publish_comment( f"**[Persistent {name}]({comment_url})** updated to latest commit {latest_commit_url}") @@ -373,7 +389,7 @@ def publish_persistent_comment_full(self, pr_comment: str, except Exception as e: get_logger().exception(f"Failed to update persistent review, error: {e}") pass - return self.publish_comment(pr_comment) + return self.publish_comment(pr_comment, **({'as_thread': True} if as_thread else {})) @abstractmethod def publish_inline_comment(self, body: str, relevant_file: str, relevant_line_in_file: str, original_suggestion=None): diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index d3b7c4e47d..9662156a70 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -510,18 +510,39 @@ def get_latest_commit_url(self): def get_comment_url(self, comment): return f"{self.mr.web_url}#note_{comment.id}" + def should_publish_review_as_thread(self) -> bool: + return bool(get_settings().get("GITLAB.PUBLISH_REVIEW_AS_THREAD", False)) + def publish_persistent_comment(self, pr_comment: str, initial_header: str, update_header: bool = True, name='review', - final_update_message=True): - self.publish_persistent_comment_full(pr_comment, initial_header, update_header, name, final_update_message) + final_update_message=True, + as_thread: bool = False): + self.publish_persistent_comment_full(pr_comment, initial_header, update_header, name, final_update_message, + as_thread=as_thread) - def publish_comment(self, mr_comment: str, is_temporary: bool = False): + def publish_comment(self, mr_comment: str, is_temporary: bool = False, as_thread: bool = False): if is_temporary and not get_settings().config.publish_output_progress: get_logger().debug(f"Skipping publish_comment for temporary comment: {mr_comment}") return None mr_comment = self.limit_output_characters(mr_comment, self.max_comment_chars) + # When as_thread is set (only the review's final comment requests this), post it as a resolvable + # thread (discussion) instead of a plain note. Temporary progress comments are never threaded. + if as_thread and not is_temporary: + try: + discussion = self.mr.discussions.create({'body': mr_comment}) + except Exception as e: + get_logger().warning(f"Failed to publish comment as a thread, falling back to a note: {e}") + else: + # Return the underlying note so callers keep note-level semantics (edit/remove/url by id). + # The thread already exists here, so a failure must not fall back to a note + # (it would duplicate the review); return None instead. + try: + return self.mr.notes.get(discussion.attributes['notes'][0]['id']) + except Exception as e: + get_logger().warning(f"Published review thread but failed to fetch its note: {e}") + return None comment = self.mr.notes.create({'body': mr_comment}) if is_temporary: self.temp_comments.append(comment) @@ -531,6 +552,23 @@ def edit_comment(self, comment, body: str): body = self.limit_output_characters(body, self.max_comment_chars) self.mr.notes.update(comment.id,{'body': body} ) + def unresolve_comment_thread(self, comment): + try: + # Notes carry their own resolution state; skip the full discussions scan (the API offers no + # note -> discussion lookup) unless the note reports it is actually resolved. + if getattr(comment, 'resolvable', None) is False or getattr(comment, 'resolved', None) is False: + return + for discussion in self.mr.discussions.list(get_all=True): + notes = discussion.attributes.get('notes', []) + if not any(note.get('id') == comment.id for note in notes): + continue + if any(note.get('resolvable') and note.get('resolved') for note in notes): + discussion.resolved = False + discussion.save() + return + except Exception as e: + get_logger().warning(f"Failed to reopen resolved review thread: {e}") + def edit_comment_from_comment_id(self, comment_id: int, body: str): body = self.limit_output_characters(body, self.max_comment_chars) comment = self.mr.notes.get(comment_id) diff --git a/pr_agent/settings/configuration.toml b/pr_agent/settings/configuration.toml index 6155a2d908..396a873011 100644 --- a/pr_agent/settings/configuration.toml +++ b/pr_agent/settings/configuration.toml @@ -286,6 +286,8 @@ push_commands = [ [gitlab] url = "https://gitlab.com" expand_submodule_diffs = false +# Post the /review summary as a resolvable thread (discussion) instead of a plain note. +publish_review_as_thread = false pr_commands = [ "/describe --pr_description.final_update_message=false", "/review", diff --git a/pr_agent/tools/pr_reviewer.py b/pr_agent/tools/pr_reviewer.py index 6e754293bf..a2d094abb5 100644 --- a/pr_agent/tools/pr_reviewer.py +++ b/pr_agent/tools/pr_reviewer.py @@ -181,14 +181,18 @@ async def run(self) -> None: return # publish the review + # Providers that support it (GitLab) can post the review's final comment as a resolvable thread. + # This intent applies to the review only - never to status comments or the output of other tools. + review_thread_kwargs = {"as_thread": True} if self.git_provider.should_publish_review_as_thread() else {} if get_settings().pr_reviewer.persistent_comment and not self.incremental.is_incremental: final_update_message = get_settings().pr_reviewer.final_update_message self.git_provider.publish_persistent_comment(pr_review, initial_header=f"{PRReviewHeader.REGULAR.value} 🔍", update_header=True, - final_update_message=final_update_message, ) + final_update_message=final_update_message, + **review_thread_kwargs) else: - self.git_provider.publish_comment(pr_review) + self.git_provider.publish_comment(pr_review, **review_thread_kwargs) self.git_provider.remove_initial_comment() except Exception as e: diff --git a/tests/unittest/test_gitlab_provider.py b/tests/unittest/test_gitlab_provider.py index 56191790f9..779e223e26 100644 --- a/tests/unittest/test_gitlab_provider.py +++ b/tests/unittest/test_gitlab_provider.py @@ -8,6 +8,15 @@ from pr_agent.git_providers.gitlab_provider import GitLabProvider +def _mock_settings(publish_review_as_thread=False): + """Settings stub whose .get() returns the GitLab review-thread flag and passes other keys through to the default.""" + settings = MagicMock() + settings.get.side_effect = lambda key, default=None: { + "GITLAB.PUBLISH_REVIEW_AS_THREAD": publish_review_as_thread, + }.get(key, default) + return settings + + class TestGitLabProvider: """Test suite for GitLab provider functionality.""" @@ -303,6 +312,239 @@ def test_publish_description_with_title_updates_both(self, gitlab_provider): assert gitlab_provider.mr.description == "Updated description" gitlab_provider.mr.save.assert_called_once() + @pytest.mark.parametrize("configured", [True, False]) + def test_should_publish_review_as_thread_reflects_config(self, gitlab_provider, configured): + with patch("pr_agent.git_providers.gitlab_provider.get_settings", + return_value=_mock_settings(publish_review_as_thread=configured)): + assert gitlab_provider.should_publish_review_as_thread() is configured + + def test_should_publish_review_as_thread_defaults_false(self, gitlab_provider): + # Key absent -> default False (the feature is opt-in). + settings = MagicMock() + settings.get.side_effect = lambda key, default=None: default + with patch("pr_agent.git_providers.gitlab_provider.get_settings", return_value=settings): + assert gitlab_provider.should_publish_review_as_thread() is False + + def test_publish_comment_defaults_to_a_note(self, gitlab_provider): + # Without as_thread (status comments, other tools), publishing stays a plain note. + gitlab_provider.mr = MagicMock() + result = gitlab_provider.publish_comment("a status comment") + + gitlab_provider.mr.notes.create.assert_called_once_with({'body': 'a status comment'}) + gitlab_provider.mr.discussions.create.assert_not_called() + assert result is gitlab_provider.mr.notes.create.return_value + + def test_publish_comment_as_thread_creates_a_discussion(self, gitlab_provider): + gitlab_provider.mr = MagicMock() + gitlab_provider.mr.discussions.create.return_value.attributes = {'notes': [{'id': 42}]} + result = gitlab_provider.publish_comment("the review", as_thread=True) + + # A resolvable thread (discussion) is opened instead of a plain note... + gitlab_provider.mr.discussions.create.assert_called_once_with({'body': 'the review'}) + gitlab_provider.mr.notes.create.assert_not_called() + # ...and the thread's underlying note is returned so callers keep note-level semantics. + gitlab_provider.mr.notes.get.assert_called_once_with(42) + assert result is gitlab_provider.mr.notes.get.return_value + + def test_publish_comment_as_thread_falls_back_to_note_on_error(self, gitlab_provider): + gitlab_provider.mr = MagicMock() + gitlab_provider.mr.discussions.create.side_effect = Exception("gitlab api error") + result = gitlab_provider.publish_comment("the review", as_thread=True) + + # Thread creation failed, so publishing must not raise and must fall back to a plain note. + gitlab_provider.mr.notes.create.assert_called_once_with({'body': 'the review'}) + assert result is gitlab_provider.mr.notes.create.return_value + + @pytest.mark.parametrize("break_response", [ + lambda mr: setattr(mr.notes.get, 'side_effect', Exception("gitlab api error")), + lambda mr: setattr(mr.discussions.create.return_value, 'attributes', {'notes': []}), + lambda mr: setattr(mr.discussions.create.return_value, 'attributes', {}), + ]) + def test_publish_comment_as_thread_returns_none_when_note_fetch_fails(self, gitlab_provider, break_response): + # The thread was created; a failure fetching its note (API error or unexpected response + # shape) must return None - not raise, and not post the review a second time as a plain note. + gitlab_provider.mr = MagicMock() + gitlab_provider.mr.discussions.create.return_value.attributes = {'notes': [{'id': 42}]} + break_response(gitlab_provider.mr) + + result = gitlab_provider.publish_comment("the review", as_thread=True) + + assert result is None + gitlab_provider.mr.discussions.create.assert_called_once() + gitlab_provider.mr.notes.create.assert_not_called() + + def test_publish_comment_as_thread_is_ignored_for_temporary(self, gitlab_provider): + gitlab_provider.mr = MagicMock() + with patch("pr_agent.git_providers.gitlab_provider.get_settings", + return_value=_mock_settings(publish_review_as_thread=True)): + result = gitlab_provider.publish_comment("Preparing review...", is_temporary=True, as_thread=True) + + # Temporary progress comments are removed shortly after, so they are never threaded. + gitlab_provider.mr.discussions.create.assert_not_called() + gitlab_provider.mr.notes.create.assert_called_once_with({'body': 'Preparing review...'}) + assert result in gitlab_provider.temp_comments + + def test_publish_review_as_thread_opens_a_new_thread_each_call(self, gitlab_provider): + # persistent_comment=false: the reviewer calls publish_comment(as_thread=True) on every run, + # so each review opens a fresh thread rather than editing or reusing a previous one. + gitlab_provider.mr = MagicMock() + gitlab_provider.mr.discussions.create.return_value.attributes = {'notes': [{'id': 1}]} + gitlab_provider.publish_comment("first review", as_thread=True) + gitlab_provider.publish_comment("second review", as_thread=True) + + assert gitlab_provider.mr.discussions.create.call_count == 2 + gitlab_provider.mr.discussions.create.assert_any_call({'body': 'first review'}) + gitlab_provider.mr.discussions.create.assert_any_call({'body': 'second review'}) + gitlab_provider.mr.notes.update.assert_not_called() + + def test_persistent_review_opens_a_thread_on_first_run(self, gitlab_provider): + # persistent_comment=true, no existing review yet: the fallback create must open a thread. + gitlab_provider.mr = MagicMock() + gitlab_provider.mr.discussions.create.return_value.attributes = {'notes': [{'id': 5}]} + gitlab_provider.get_issue_comments = MagicMock(return_value=[]) + gitlab_provider.publish_persistent_comment("## PR Review\n\nbody", + initial_header="## PR Review", + update_header=True, + final_update_message=False, + as_thread=True) + + gitlab_provider.mr.discussions.create.assert_called_once() + gitlab_provider.mr.notes.create.assert_not_called() + + def test_persistent_review_update_edits_in_place_and_reopens_thread(self, gitlab_provider): + # persistent_comment=true with an existing review thread: edit it in place + # and reopen (unresolve) it + header = "## PR Review" + existing = MagicMock() + existing.body = f"{header}\n\nprevious review" + gitlab_provider.mr = MagicMock() + gitlab_provider.get_issue_comments = MagicMock(return_value=[existing]) + gitlab_provider.get_latest_commit_url = MagicMock(return_value="https://gitlab.com/c/abc") + gitlab_provider.get_comment_url = MagicMock(return_value="https://gitlab.com/n/1") + gitlab_provider.unresolve_comment_thread = MagicMock() + gitlab_provider.publish_persistent_comment(f"{header}\n\nnew review", + initial_header=header, + update_header=True, + final_update_message=False, + as_thread=True) + + gitlab_provider.mr.notes.update.assert_called_once() + gitlab_provider.mr.discussions.create.assert_not_called() + gitlab_provider.unresolve_comment_thread.assert_called_once_with(existing) + + def test_persistent_review_update_status_message_stays_a_plain_note(self, gitlab_provider): + # final_update_message=true posts an "updated to latest commit" follow-up. It is a status + # comment, so it stays a plain note even when the review itself is threaded. + header = "## PR Review" + existing = MagicMock() + existing.body = f"{header}\n\nprevious review" + gitlab_provider.mr = MagicMock() + gitlab_provider.get_issue_comments = MagicMock(return_value=[existing]) + gitlab_provider.get_latest_commit_url = MagicMock(return_value="https://gitlab.com/c/abc") + gitlab_provider.get_comment_url = MagicMock(return_value="https://gitlab.com/n/1") + gitlab_provider.unresolve_comment_thread = MagicMock() + gitlab_provider.publish_persistent_comment(f"{header}\n\nnew review", + initial_header=header, + update_header=True, + final_update_message=True, + as_thread=True) + + gitlab_provider.mr.discussions.create.assert_not_called() + gitlab_provider.mr.notes.create.assert_called_once() + assert "updated to latest commit" in gitlab_provider.mr.notes.create.call_args.args[0]['body'] + + def test_persistent_review_update_does_not_duplicate_when_unresolve_raises(self, gitlab_provider): + # A reopen failure after the in-place edit must not reach the outer fallback, which would + # publish the review a second time. + header = "## PR Review" + existing = MagicMock() + existing.body = f"{header}\n\nprevious review" + gitlab_provider.mr = MagicMock() + gitlab_provider.get_issue_comments = MagicMock(return_value=[existing]) + gitlab_provider.get_latest_commit_url = MagicMock(return_value="https://gitlab.com/c/abc") + gitlab_provider.get_comment_url = MagicMock(return_value="https://gitlab.com/n/1") + gitlab_provider.unresolve_comment_thread = MagicMock(side_effect=Exception("reopen failed")) + gitlab_provider.publish_persistent_comment(f"{header}\n\nnew review", + initial_header=header, + update_header=True, + final_update_message=False, + as_thread=True) + + gitlab_provider.mr.notes.update.assert_called_once() + gitlab_provider.mr.discussions.create.assert_not_called() + gitlab_provider.mr.notes.create.assert_not_called() + + def test_persistent_review_update_without_thread_keeps_resolution(self, gitlab_provider): + # Without as_thread (the persistent comment isn't a thread), resolution state must not be touched. + header = "## PR Review" + existing = MagicMock() + existing.body = f"{header}\n\nprevious review" + gitlab_provider.mr = MagicMock() + gitlab_provider.get_issue_comments = MagicMock(return_value=[existing]) + gitlab_provider.get_latest_commit_url = MagicMock(return_value="https://gitlab.com/c/abc") + gitlab_provider.get_comment_url = MagicMock(return_value="https://gitlab.com/n/1") + gitlab_provider.unresolve_comment_thread = MagicMock() + gitlab_provider.publish_persistent_comment(f"{header}\n\nnew review", + initial_header=header, + update_header=True, + final_update_message=False) + + gitlab_provider.mr.notes.update.assert_called_once() + gitlab_provider.unresolve_comment_thread.assert_not_called() + + @pytest.mark.parametrize("resolvable,resolved,should_reopen", [ + (True, True, True), # resolved thread -> reopen it + (True, False, False), # already open -> leave it + (False, False, False), # not resolvable -> nothing to do + ]) + def test_unresolve_comment_thread(self, gitlab_provider, resolvable, resolved, should_reopen): + comment = MagicMock(id=42) + discussion = MagicMock() + discussion.attributes = {'notes': [{'id': 42, 'resolvable': resolvable, 'resolved': resolved}]} + gitlab_provider.mr = MagicMock() + gitlab_provider.mr.discussions.list.return_value = [discussion] + + gitlab_provider.unresolve_comment_thread(comment) + + if should_reopen: + assert discussion.resolved is False + discussion.save.assert_called_once() + else: + discussion.save.assert_not_called() + + @pytest.mark.parametrize("note_attrs", [ + {'resolved': False}, # note not resolved -> nothing to reopen + {'resolvable': False}, # note not resolvable -> nothing to reopen + ]) + def test_unresolve_comment_thread_skips_discussion_scan_when_note_not_resolved(self, gitlab_provider, note_attrs): + # The note's own resolution state rules out a resolved thread, so the (paginated) + # discussions listing must be skipped entirely. + comment = MagicMock(id=42, **note_attrs) + gitlab_provider.mr = MagicMock() + + gitlab_provider.unresolve_comment_thread(comment) + + gitlab_provider.mr.discussions.list.assert_not_called() + + def test_unresolve_comment_thread_ignores_unrelated_discussions(self, gitlab_provider): + # A resolved discussion that does not own our note must be left untouched. + comment = MagicMock(id=99) + other = MagicMock() + other.attributes = {'notes': [{'id': 1, 'resolvable': True, 'resolved': True}]} + gitlab_provider.mr = MagicMock() + gitlab_provider.mr.discussions.list.return_value = [other] + + gitlab_provider.unresolve_comment_thread(comment) + + other.save.assert_not_called() + + def test_unresolve_comment_thread_soft_fails(self, gitlab_provider): + # A GitLab API error while reopening must not raise. + gitlab_provider.mr = MagicMock() + gitlab_provider.mr.discussions.list.side_effect = Exception("gitlab api error") + + gitlab_provider.unresolve_comment_thread(MagicMock(id=1)) # must not raise + # ---- publish_labels / get_pr_labels tests ---- def _real_mr(self, snapshot_labels, update_result=None, update_error=None): diff --git a/tests/unittest/test_pr_reviewer_core.py b/tests/unittest/test_pr_reviewer_core.py index 65a8d93bd3..edb3ba819a 100644 --- a/tests/unittest/test_pr_reviewer_core.py +++ b/tests/unittest/test_pr_reviewer_core.py @@ -1,6 +1,8 @@ from types import SimpleNamespace from unittest.mock import MagicMock +import pytest + from pr_agent.config_loader import get_settings from pr_agent.tools.pr_reviewer import PRReviewer @@ -92,6 +94,65 @@ def test_get_user_answers_collects_question_and_answer_from_issue_comments(): assert answer == "/answer Because it fixes production." +@pytest.mark.asyncio +@pytest.mark.parametrize("persistent", [True, False]) +@pytest.mark.parametrize("thread_enabled", [True, False]) +async def test_run_threads_only_the_final_review_comment(monkeypatch, persistent, thread_enabled): + """`as_thread` is forwarded to the review's final publish call only when the provider opts in + (should_publish_review_as_thread), and is omitted entirely otherwise - other providers' + publish methods don't accept it. Status/progress comments are never threaded. + """ + from pr_agent.tools import pr_reviewer as pr_reviewer_module + + git_provider = MagicMock() + git_provider.should_publish_review_as_thread.return_value = thread_enabled + reviewer = _make_reviewer(git_provider) + reviewer.incremental = SimpleNamespace(is_incremental=False) + reviewer.vars = {} + reviewer.prediction = None + review_text = "## PR Reviewer Guide 🔍\n\nsome findings" + reviewer._prepare_pr_review = lambda: review_text + + async def fake_extract_tickets(git_provider, vars): + return None + + async def fake_retry(prepare_fn, model_type=None): + reviewer.prediction = "prediction" + + monkeypatch.setattr(pr_reviewer_module, "extract_and_cache_pr_tickets", fake_extract_tickets) + monkeypatch.setattr(pr_reviewer_module, "retry_with_fallback_models", fake_retry) + + settings = get_settings() + original = { + "publish_output": settings.config.publish_output, + "persistent_comment": settings.pr_reviewer.persistent_comment, + "is_auto_command": settings.config.get("is_auto_command", False), + } + try: + settings.config.publish_output = True + settings.config.is_auto_command = False + settings.pr_reviewer.persistent_comment = persistent + + await reviewer.run() + finally: + settings.config.publish_output = original["publish_output"] + settings.config.is_auto_command = original["is_auto_command"] + settings.pr_reviewer.persistent_comment = original["persistent_comment"] + + if persistent: + publish = git_provider.publish_persistent_comment + publish.assert_called_once() + else: + publish = git_provider.publish_comment + assert publish.call_args.args[0] == review_text + if thread_enabled: + assert publish.call_args.kwargs.get("as_thread") is True + else: + assert "as_thread" not in publish.call_args.kwargs + # The temporary progress comment is published without as_thread regardless of the flag. + git_provider.publish_comment.assert_any_call("Preparing review...", is_temporary=True) + + def test_init_maps_user_question_and_answer_to_correct_prompt_vars(monkeypatch): """Behavioral regression for the swapped-unpacking bug (#2496).