From ad3c43bf8e618f7b85a3d5ed89e176af06f0c53c Mon Sep 17 00:00:00 2001 From: emil Date: Mon, 22 Jun 2026 07:11:20 +0000 Subject: [PATCH] fix: revert review_pr.py to GiteaClient (tea v0.14.1 is interactive-only) (#70) --- AGENTS.md | 6 +- scripts/ci/review_pr.py | 34 +++++++--- tests/unit/test_review_pr.py | 116 ++++++++++++++++++----------------- 3 files changed, 89 insertions(+), 67 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index dbcce6f..f572a8a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -291,12 +291,12 @@ The `tea` Gitea CLI tool is used for Gitea API interactions in CI scripts. It is - `TeaCLI.list_branches()` — Branch listing **Scripts using tea (via `gitea_cli.py`):** -- `scripts/ci/review_pr.py` — Posts PR reviews via `tea pulls review` - `scripts/ci/publish.py` — Creates Gitea releases via `tea releases create` - `scripts/ci/notify_failure.py` — Creates issues via `tea issues create` (falls back to `GiteaClient` if tea not installed) - `scripts/configure_repo.py` — Creates labels via `tea labels create` (falls back to `GiteaClient` if tea fails; branch protection still uses `GiteaClient` since tea only supports basic protect/unprotect) **Operations still using `GiteaClient` (not supported by tea):** +- PR reviews (`review_pr.py`) — tea v0.14.1 only supports interactive reviews - Wiki page management (`sync_wiki.py`) - Commit status checks (`auto_merge.py`) - Runner discovery (`discover_runners.py`) @@ -309,8 +309,8 @@ Scripts have different import requirements. Workflows must set `PYTHONPATH` acco | PYTHONPATH | When to use | Example scripts | |------------|-------------|-----------------| -| `src` | Script imports from `gitea_runner_manager` | `auto_merge.py`, `pr_review.py`, `sync_wiki.py`, `post_merge.py`, `classify_changes.py`, `discover_runners.py`, `doc_coverage.py` | -| `.:src` | Script imports from both `gitea_runner_manager` and `scripts.gitea_cli` | `review_pr.py`, `publish.py`, `notify_failure.py`, `configure_repo.py` | +| `src` | Script imports from `gitea_runner_manager` | `auto_merge.py`, `pr_review.py`, `review_pr.py`, `sync_wiki.py`, `post_merge.py`, `classify_changes.py`, `discover_runners.py`, `doc_coverage.py` | +| `.:src` | Script imports from both `gitea_runner_manager` and `scripts.gitea_cli` | `publish.py`, `notify_failure.py`, `configure_repo.py` | | `.` | Script imports from other `scripts.ci.*` modules | `release.py` (imports `classify_changes.has_user_facing_changes`) | | (none) | Script has no GRM or cross-script imports | `detect_release_commit.py`, `distribute_molecule.py`, `molecule_ci_guard.py`, `push_badges.py`, `validate_commit_msg.py` | diff --git a/scripts/ci/review_pr.py b/scripts/ci/review_pr.py index 853de73..bfc9c37 100644 --- a/scripts/ci/review_pr.py +++ b/scripts/ci/review_pr.py @@ -3,9 +3,14 @@ Used by the GRM workflow to post structured PR reviews. The review body is provided via --body and inline comments via a JSON file -(--comments-json) or stdin (--comments-stdin). This script uses the -``tea`` Gitea CLI for posting the review — the actual review analysis is -performed by the agent before invoking this tool. +(--comments-json) or stdin (--comments-stdin). + +.. note:: + The ``tea`` CLI v0.14.1 only supports interactive reviews (no + ``--approve``/``--comment`` flags), so this script uses + ``GiteaClient`` (direct HTTP API) for posting reviews. When a newer + version of tea adds non-interactive review support, this can be + switched to use ``TeaCLI.review_pr()``. Usage: REPO_TOKEN= python3 scripts/review_pr.py \ @@ -36,8 +41,10 @@ from typing import Any import click from dotenv import load_dotenv # pyright: ignore[reportMissingImports,reportUnknownVariableType] +from gitea_runner_manager.api_clients import GiteaClient +from gitea_runner_manager.config import GITEA_API_URL +from gitea_runner_manager.exceptions import APIError from gitea_runner_manager.i18n import _ -from scripts.gitea_cli import TeaCLI, TeaCLIError load_dotenv(override=True) @@ -105,7 +112,8 @@ def main( if not token: raise click.ClickException(_("ERROR: REPO_TOKEN is not set.")) - tea = TeaCLI(repo=repo) + owner, repo_name = repo.split("/") + client = GiteaClient(GITEA_API_URL, token, owner, repo_name) comments = parse_comments(comments_json, comments_stdin) @@ -129,13 +137,21 @@ def main( ) try: - tea.review_pr(repo, int(pr_number), event=event, body=body) - except TeaCLIError as e: - raise click.ClickException(_("Failed to post review: {error}", error=str(e))) from None + review = client.create_review(pr_number, event=event, body=body, comments=comments) + except APIError as e: + raise click.ClickException( + _( + "Failed to post review: HTTP {status} — {message}", + status=e.status, + message=e.message, + ) + ) from None + review_id = review.get("id", "?") click.echo( _( - "Review posted on PR #{pr_number} with event '{event}' ({num_comments} inline comments).", + "Review #{review_id} posted on PR #{pr_number} with event '{event}' ({num_comments} inline comments).", + review_id=review_id, pr_number=pr_number, event=event, num_comments=len(comments), diff --git a/tests/unit/test_review_pr.py b/tests/unit/test_review_pr.py index b6bfe64..c3c63d1 100644 --- a/tests/unit/test_review_pr.py +++ b/tests/unit/test_review_pr.py @@ -1,5 +1,6 @@ """Unit tests for scripts/ci/review_pr.py.""" +import http import json from unittest.mock import MagicMock, patch @@ -7,8 +8,8 @@ import click import pytest from click.testing import CliRunner +from gitea_runner_manager.exceptions import APIError from scripts.ci.review_pr import main, parse_comments -from scripts.gitea_cli import TeaCLIError class TestParseComments: @@ -59,25 +60,27 @@ class TestParseComments: class TestMain: @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) - @patch("scripts.ci.review_pr.TeaCLI") - def test_successful_comment_review(self, mock_tea_cls: MagicMock) -> None: - mock_tea = MagicMock() - mock_tea_cls.return_value = mock_tea + @patch("scripts.ci.review_pr.GiteaClient") + def test_successful_comment_review(self, mock_client_cls: MagicMock) -> None: + mock_client = MagicMock() + mock_client.create_review.return_value = {"id": 42} + mock_client_cls.return_value = mock_client runner = CliRunner() result = runner.invoke( main, ["5", "owner/repo", "--event", "COMMENT", "--body", "LGTM"], ) assert result.exit_code == 0 - assert "Review posted" in result.output - mock_tea.review_pr.assert_called_once_with("owner/repo", 5, event="COMMENT", body="LGTM") + assert "Review #42" in result.output + mock_client.create_review.assert_called_once_with("5", event="COMMENT", body="LGTM", comments=[]) @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) - @patch("scripts.ci.review_pr.TeaCLI") - def test_successful_approve_review(self, mock_tea_cls: MagicMock) -> None: + @patch("scripts.ci.review_pr.GiteaClient") + def test_successful_approve_review(self, mock_client_cls: MagicMock) -> None: """APPROVE requires --checklist-confirmed and substantive body.""" - mock_tea = MagicMock() - mock_tea_cls.return_value = mock_tea + mock_client = MagicMock() + mock_client.create_review.return_value = {"id": 7} + mock_client_cls.return_value = mock_client runner = CliRunner() result = runner.invoke( main, @@ -92,19 +95,20 @@ class TestMain: ], ) assert result.exit_code == 0 - mock_tea.review_pr.assert_called_once_with( - "owner/repo", - 5, + assert "Review #7" in result.output + mock_client.create_review.assert_called_once_with( + "5", event="APPROVE", body="All 10 checklist categories verified. Architecture OK, tests pass.", + comments=[], ) @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) - @patch("scripts.ci.review_pr.TeaCLI") - def test_approve_without_checklist_confirmed_fails(self, mock_tea_cls: MagicMock) -> None: + @patch("scripts.ci.review_pr.GiteaClient") + def test_approve_without_checklist_confirmed_fails(self, mock_client_cls: MagicMock) -> None: """APPROVE without --checklist-confirmed is rejected.""" - mock_tea = MagicMock() - mock_tea_cls.return_value = mock_tea + mock_client = MagicMock() + mock_client_cls.return_value = mock_client runner = CliRunner() result = runner.invoke( main, @@ -112,14 +116,14 @@ class TestMain: ) assert result.exit_code != 0 assert "checklist" in result.output.lower() - mock_tea.review_pr.assert_not_called() + mock_client.create_review.assert_not_called() @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) - @patch("scripts.ci.review_pr.TeaCLI") - def test_approve_with_trivial_body_fails(self, mock_tea_cls: MagicMock) -> None: + @patch("scripts.ci.review_pr.GiteaClient") + def test_approve_with_trivial_body_fails(self, mock_client_cls: MagicMock) -> None: """APPROVE with trivial body (< 20 chars) and no comments is rejected.""" - mock_tea = MagicMock() - mock_tea_cls.return_value = mock_tea + mock_client = MagicMock() + mock_client_cls.return_value = mock_client runner = CliRunner() result = runner.invoke( main, @@ -127,30 +131,32 @@ class TestMain: ) assert result.exit_code != 0 assert "substantive" in result.output.lower() - mock_tea.review_pr.assert_not_called() + mock_client.create_review.assert_not_called() @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) - @patch("scripts.ci.review_pr.TeaCLI") - def test_successful_with_inline_comments(self, mock_tea_cls: MagicMock, tmp_path) -> None: + @patch("scripts.ci.review_pr.GiteaClient") + def test_successful_with_inline_comments(self, mock_client_cls: MagicMock, tmp_path) -> None: comments = [{"path": "a.py", "body": "fix", "new_position": 1}] f = tmp_path / "comments.json" f.write_text(json.dumps(comments)) - mock_tea = MagicMock() - mock_tea_cls.return_value = mock_tea + mock_client = MagicMock() + mock_client.create_review.return_value = {"id": 9} + mock_client_cls.return_value = mock_client runner = CliRunner() result = runner.invoke( main, ["5", "owner/repo", "--comments-json", str(f)], ) assert result.exit_code == 0 - mock_tea.review_pr.assert_called_once_with("owner/repo", 5, event="COMMENT", body="") + mock_client.create_review.assert_called_once_with("5", event="COMMENT", body="", comments=comments) @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) - @patch("scripts.ci.review_pr.TeaCLI") - def test_successful_with_stdin_comments(self, mock_tea_cls: MagicMock) -> None: + @patch("scripts.ci.review_pr.GiteaClient") + def test_successful_with_stdin_comments(self, mock_client_cls: MagicMock) -> None: comments = [{"path": "a.py", "body": "fix", "new_position": 1}] - mock_tea = MagicMock() - mock_tea_cls.return_value = mock_tea + mock_client = MagicMock() + mock_client.create_review.return_value = {"id": 11} + mock_client_cls.return_value = mock_client runner = CliRunner() result = runner.invoke( main, @@ -158,7 +164,7 @@ class TestMain: input=json.dumps(comments), ) assert result.exit_code == 0 - mock_tea.review_pr.assert_called_once_with("owner/repo", 5, event="COMMENT", body="") + mock_client.create_review.assert_called_once_with("5", event="COMMENT", body="", comments=comments) @patch.dict("os.environ", {"REPO_TOKEN": ""}, clear=True) def test_missing_token_exits(self) -> None: @@ -168,44 +174,44 @@ class TestMain: assert "REPO_TOKEN" in result.output @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) - @patch("scripts.ci.review_pr.TeaCLI") - def test_no_body_or_comments_for_comment_event(self, mock_tea_cls: MagicMock) -> None: - mock_tea = MagicMock() - mock_tea_cls.return_value = mock_tea + @patch("scripts.ci.review_pr.GiteaClient") + def test_no_body_or_comments_for_comment_event(self, mock_client_cls: MagicMock) -> None: + mock_client = MagicMock() + mock_client_cls.return_value = mock_client runner = CliRunner() result = runner.invoke(main, ["5", "owner/repo", "--event", "COMMENT"]) assert result.exit_code == 1 assert "required" in result.output - mock_tea.review_pr.assert_not_called() + mock_client.create_review.assert_not_called() @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) - @patch("scripts.ci.review_pr.TeaCLI") - def test_no_body_or_comments_for_request_changes(self, mock_tea_cls: MagicMock) -> None: - mock_tea = MagicMock() - mock_tea_cls.return_value = mock_tea + @patch("scripts.ci.review_pr.GiteaClient") + def test_no_body_or_comments_for_request_changes(self, mock_client_cls: MagicMock) -> None: + mock_client = MagicMock() + mock_client_cls.return_value = mock_client runner = CliRunner() result = runner.invoke(main, ["5", "owner/repo", "--event", "REQUEST_CHANGES"]) assert result.exit_code == 1 assert "required" in result.output - mock_tea.review_pr.assert_not_called() + mock_client.create_review.assert_not_called() @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) - @patch("scripts.ci.review_pr.TeaCLI") - def test_tea_error_raises_click(self, mock_tea_cls: MagicMock) -> None: - mock_tea = MagicMock() - mock_tea.review_pr.side_effect = TeaCLIError("tea command failed") - mock_tea_cls.return_value = mock_tea + @patch("scripts.ci.review_pr.GiteaClient") + def test_api_error_raises_click(self, mock_client_cls: MagicMock) -> None: + mock_client = MagicMock() + mock_client.create_review.side_effect = APIError(http.HTTPStatus.INTERNAL_SERVER_ERROR, "server error") + mock_client_cls.return_value = mock_client runner = CliRunner() result = runner.invoke(main, ["5", "owner/repo", "--body", "x"]) assert result.exit_code == 1 - assert "Failed to post review" in result.output + assert "HTTP" in result.output @patch.dict("os.environ", {"REPO_TOKEN": "tok"}) - @patch("scripts.ci.review_pr.TeaCLI") - def test_invalid_event_choice(self, mock_tea_cls: MagicMock) -> None: - mock_tea = MagicMock() - mock_tea_cls.return_value = mock_tea + @patch("scripts.ci.review_pr.GiteaClient") + def test_invalid_event_choice(self, mock_client_cls: MagicMock) -> None: + mock_client = MagicMock() + mock_client_cls.return_value = mock_client runner = CliRunner() result = runner.invoke(main, ["5", "owner/repo", "--event", "Bogus"]) assert result.exit_code != 0 - mock_tea.review_pr.assert_not_called() + mock_client.create_review.assert_not_called()