From ad6a149e92c81bfb60036ffe0c1f8694eada9f3b Mon Sep 17 00:00:00 2001 From: Sergio Castillo Date: Mon, 29 Sep 2025 09:16:31 +0200 Subject: [PATCH 1/4] ci: add issues integration to lgtm review action --- lgtm.toml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lgtm.toml b/lgtm.toml index 829fa97..c19f68b 100644 --- a/lgtm.toml +++ b/lgtm.toml @@ -5,3 +5,7 @@ model = "gemini-2.5-pro" silent = false publish = true ai_retries = 2 + +# GitHub issues integration +issues_platform = "github" +issues_url = "https://github.com/elementsinteractive/lgtm-ai/issues" From 8384a1e5ca2b28b8b09b491664ebfb4ccfbb152f Mon Sep 17 00:00:00 2001 From: Sergio Castillo Date: Mon, 29 Sep 2025 09:40:07 +0200 Subject: [PATCH 2/4] fix: handling of target for guides, tests using lgtm.toml --- scripts/evaluate_review_quality.py | 4 ++-- src/lgtm_ai/__main__.py | 5 +++-- src/lgtm_ai/validators.py | 16 ++++++++++++++-- tests/config/conftest.py | 9 --------- tests/conftest.py | 13 +++++++++++++ tests/test_main.py | 23 ++++++++++++++++++----- tests/test_validators.py | 13 +++++++++---- 7 files changed, 59 insertions(+), 24 deletions(-) diff --git a/scripts/evaluate_review_quality.py b/scripts/evaluate_review_quality.py index a426844..ed2875e 100644 --- a/scripts/evaluate_review_quality.py +++ b/scripts/evaluate_review_quality.py @@ -15,7 +15,7 @@ from lgtm_ai.git_client.gitlab import GitlabClient from lgtm_ai.review import CodeReviewer from lgtm_ai.review.context import ContextRetriever -from lgtm_ai.validators import parse_target +from lgtm_ai.validators import _parse_target from rich.logging import RichHandler PRS_FOR_EVALUATION = { @@ -94,7 +94,7 @@ def get_git_branch() -> str: def perform_review( output_dir: str, pr_url: str, pr_name: str, sample: int, model: SupportedAIModels, git_api_key: str, ai_api_key: str ) -> None: - url = parse_target(mock.Mock(), "pr_url", pr_url) + url = _parse_target(mock.Mock(), "pr_url", pr_url) git_client = GitlabClient(client=gitlab.Gitlab(private_token=git_api_key), formatter=MarkDownFormatter()) code_reviewer = CodeReviewer( reviewer_agent=get_reviewer_agent_with_settings(), diff --git a/src/lgtm_ai/__main__.py b/src/lgtm_ai/__main__.py index 87ac756..56c3899 100644 --- a/src/lgtm_ai/__main__.py +++ b/src/lgtm_ai/__main__.py @@ -32,7 +32,7 @@ from lgtm_ai.validators import ( IntOrNoLimitType, ModelChoice, - parse_target, + TargetParser, validate_model_url, ) from rich.console import Console @@ -57,7 +57,6 @@ def entry_point() -> None: def _common_options[**P, T](func: Callable[P, T]) -> Callable[P, T]: """Wrap a click command and adds common options for lgtm commands.""" - @click.argument("target", required=True, callback=parse_target) @click.option( "--model", type=ModelChoice(SupportedAIModelsList), @@ -112,6 +111,7 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> T: return wrapper +@click.argument("target", required=True, callback=TargetParser(allow_git_repo=True)) @entry_point.command() @_common_options @click.option( @@ -216,6 +216,7 @@ def review(target: PRUrl | LocalRepository, config: str | None, verbose: int, ** logger.info("Review published successfully") +@click.argument("target", required=True, callback=TargetParser(allow_git_repo=False)) @entry_point.command() @_common_options def guide( diff --git a/src/lgtm_ai/validators.py b/src/lgtm_ai/validators.py index aa7d70c..635a314 100644 --- a/src/lgtm_ai/validators.py +++ b/src/lgtm_ai/validators.py @@ -16,7 +16,17 @@ class AllowedSchemes(StrEnum): Http = "http" -def parse_target(ctx: click.Context, param: str, value: object) -> PRUrl | LocalRepository: +class TargetParser: + """Generate a click callback that parses the `TARGET` argument.""" + + def __init__(self, allow_git_repo: bool) -> None: + self.allow_git_repo = allow_git_repo + + def __call__(self, ctx: click.Context, param: str, value: object) -> PRUrl | LocalRepository: + return _parse_target(ctx, param, value, allow_git_repo=self.allow_git_repo) + + +def _parse_target(ctx: click.Context, param: str, value: object, *, allow_git_repo: bool) -> PRUrl | LocalRepository: """Click callback that transforms a given URL into a dataclass for later use. It validates it and raises click exceptions if the URL is not valid. @@ -26,7 +36,9 @@ def parse_target(ctx: click.Context, param: str, value: object) -> PRUrl | Local parsed = urlparse(value) if not parsed.netloc: - # Check whether its a directory path + if not allow_git_repo: + raise click.BadParameter("The PR URL must be a valid URL") + try: resolved_path = pathlib.Path(value).resolve(strict=True) # just to ensure it's a valid path # Check whether it's a git repository diff --git a/tests/config/conftest.py b/tests/config/conftest.py index b6fd583..a06fd8d 100644 --- a/tests/config/conftest.py +++ b/tests/config/conftest.py @@ -3,7 +3,6 @@ from collections.abc import Iterator from contextlib import contextmanager from pathlib import Path -from unittest import mock import pytest @@ -144,11 +143,3 @@ def clean_env_secrets() -> Iterator[None]: # Restore original environment os.environ.clear() os.environ.update(original_env) - - -@pytest.fixture(autouse=True) -def mock_current_working_directory(tmp_path: Path) -> Iterator[None]: - with mock.patch("lgtm_ai.config.handler.os.getcwd", return_value=str(tmp_path)): - # We still mock the current directory because this is a python project, - # and thus it has a pyproject.toml file that will be read during the test execution! - yield diff --git a/tests/conftest.py b/tests/conftest.py index 7f5b93b..0084d62 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,10 +1,23 @@ +from collections.abc import Iterator from copy import deepcopy +from pathlib import Path from typing import Any +from unittest import mock from unittest.mock import MagicMock import pytest +@pytest.fixture(autouse=True) +def mock_current_working_directory(tmp_path: Path) -> Iterator[None]: + """Mock the current working directory for config handling in all tests. + + This is done so that the `lgtm.toml` file of `lgtm` is not used in tests. + """ + with mock.patch("lgtm_ai.config.handler.os.getcwd", return_value=str(tmp_path)): + yield + + class CopyingMock(MagicMock): """Mock which copies arguments when mocks are called with mutable arguments. diff --git a/tests/test_main.py b/tests/test_main.py index 84d7fb1..6329de9 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -1,4 +1,5 @@ import logging +from pathlib import Path from unittest import mock import click @@ -73,7 +74,14 @@ def test_review_cli_github(*args: mock.MagicMock) -> None: @mock.patch("lgtm_ai.__main__.CodeReviewer") @mock.patch("lgtm_ai.__main__.PrettyFormatter") @mock.patch("lgtm_ai.__main__.get_git_client") -def test_review_cli_local(*args: mock.MagicMock) -> None: +def test_review_cli_local( + m_get_git_client: mock.MagicMock, + m_formatter: mock.MagicMock, + m_reviewer: mock.MagicMock, + tmp_path: Path, +) -> None: + # Create a .git directory to simulate a git repository + (tmp_path / ".git").mkdir() runner = CliRunner() result = runner.invoke( review, @@ -90,14 +98,19 @@ def test_review_cli_local(*args: mock.MagicMock) -> None: @mock.patch("lgtm_ai.__main__.CodeReviewer") @mock.patch("lgtm_ai.__main__.PrettyFormatter") @mock.patch("lgtm_ai.__main__.get_git_client") -def test_review_cli_local_does_not_exist(*args: mock.MagicMock) -> None: +def test_review_cli_local_does_not_exist( + m_get_git_client: mock.MagicMock, + m_formatter: mock.MagicMock, + m_reviewer: mock.MagicMock, + tmp_path: Path, +) -> None: runner = CliRunner() result = runner.invoke( review, [ "--ai-api-key", "fake-token", - "./foo/bar/baz", + "./fake", ], catch_exceptions=False, ) @@ -162,8 +175,8 @@ def test_guide_cli_local_fails(*args: mock.MagicMock) -> None: catch_exceptions=False, ) - assert result.exit_code == 1 - assert "Aborted!" in result.stderr + assert result.exit_code == 2 + assert "Invalid value for 'TARGET': The PR URL must be a valid URL" in result.stderr @pytest.mark.parametrize( diff --git a/tests/test_validators.py b/tests/test_validators.py index be82885..a2d29bc 100644 --- a/tests/test_validators.py +++ b/tests/test_validators.py @@ -6,7 +6,7 @@ import click import pytest from lgtm_ai.base.schemas import PRSource, PRUrl -from lgtm_ai.validators import parse_target, validate_model_url +from lgtm_ai.validators import TargetParser, validate_model_url @pytest.mark.parametrize( @@ -34,12 +34,17 @@ ) def test_parse_url(url: str, expectation: AbstractContextManager[Any]) -> None: with expectation: - parse_target(mock.Mock(), "pr_url", url) + TargetParser(allow_git_repo=True)(mock.Mock(), "pr_url", url) + + +def test_parse_url_does_not_accept_git_repo() -> None: + with pytest.raises(click.exceptions.BadParameter, match="The PR URL must be a valid URL"): + TargetParser(allow_git_repo=False)(mock.Mock(), "pr_url", "./foo/bar/baz") def test_parse_url_gitlab_valid() -> None: url = "https://gitlab.com/foo/-/merge_requests/1" - parsed = parse_target(mock.Mock(), "pr_url", url) + parsed = TargetParser(allow_git_repo=True)(mock.Mock(), "pr_url", url) assert parsed == PRUrl( full_url=url, @@ -52,7 +57,7 @@ def test_parse_url_gitlab_valid() -> None: def test_parse_url_github_valid() -> None: url = "https://github.com/elementsinteractive/sheriff/pull/61" - parsed = parse_target(mock.Mock(), "pr_url", url) + parsed = TargetParser(allow_git_repo=True)(mock.Mock(), "pr_url", url) assert parsed == PRUrl( full_url=url, From 335f533906a0fad8593c2c3817b95f83e40d6a28 Mon Sep 17 00:00:00 2001 From: Sergio Castillo Date: Mon, 29 Sep 2025 09:51:53 +0200 Subject: [PATCH 3/4] fix: use suggestion by lgtm to fix evaluate script --- scripts/evaluate_review_quality.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/evaluate_review_quality.py b/scripts/evaluate_review_quality.py index ed2875e..6b1bb08 100644 --- a/scripts/evaluate_review_quality.py +++ b/scripts/evaluate_review_quality.py @@ -15,7 +15,7 @@ from lgtm_ai.git_client.gitlab import GitlabClient from lgtm_ai.review import CodeReviewer from lgtm_ai.review.context import ContextRetriever -from lgtm_ai.validators import _parse_target +from lgtm_ai.validators import TargetParser from rich.logging import RichHandler PRS_FOR_EVALUATION = { @@ -94,7 +94,7 @@ def get_git_branch() -> str: def perform_review( output_dir: str, pr_url: str, pr_name: str, sample: int, model: SupportedAIModels, git_api_key: str, ai_api_key: str ) -> None: - url = _parse_target(mock.Mock(), "pr_url", pr_url) + url = TargetParser(allow_git_repo=True)(mock.Mock(), "pr_url", pr_url) git_client = GitlabClient(client=gitlab.Gitlab(private_token=git_api_key), formatter=MarkDownFormatter()) code_reviewer = CodeReviewer( reviewer_agent=get_reviewer_agent_with_settings(), From a6a02e042346a065bf552741f128c4be350f842e Mon Sep 17 00:00:00 2001 From: Sergio Castillo Date: Mon, 29 Sep 2025 09:56:21 +0200 Subject: [PATCH 4/4] ci: add scripts to linting targets --- justfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/justfile b/justfile index 0dd6e16..c0971d7 100644 --- a/justfile +++ b/justfile @@ -4,7 +4,7 @@ venv := ".venv" bin := venv + "/bin" python_version := "python3.13" run := "poetry run" -target_dirs := "src tests" +target_dirs := "src tests scripts" image := "docker.io/elementsinteractive/lgtm-ai" # SENTINELS