GRM-55: fix: strengthen review process with deeper checks and structured checklist

2026-06-22 08:16:42 +00:00
parent d1531ac81f
commit a791800809
7 changed files with 393 additions and 48 deletions
@@ -161,32 +161,31 @@ class TestHasApprovalReview:
        ]
        assert has_approval_review(client, "5") is True

-    def test_trivial_approved_falls_back_to_no_changes(self) -> None:
-        """A bare 'LGTM' approval (< 20 chars) without comments falls back to
-        checking no REQUEST_CHANGES exist (single-token workflow)."""
+    def test_trivial_approved_without_comments_returns_false(self) -> None:
+        """A bare 'LGTM' approval (< 20 chars) without comments is not substantive."""
        client = MagicMock()
        client.get_pr_reviews.return_value = [
            {"state": "APPROVED", "body": "LGTM", "comments": []},
        ]
-        assert has_approval_review(client, "5") is True
+        assert has_approval_review(client, "5") is False

-    def test_no_approved_but_no_changes_requested(self) -> None:
-        """Single-token workflow: no APPROVE but no REQUEST_CHANGES either."""
+    def test_no_approved_returns_false(self) -> None:
+        """No APPROVE review means merge is blocked — no fallback."""
        client = MagicMock()
        client.get_pr_reviews.return_value = [{"state": "COMMENT"}]
-        assert has_approval_review(client, "5") is True
+        assert has_approval_review(client, "5") is False

    def test_changes_requested_blocks_merge(self) -> None:
-        """REQUEST_CHANGES blocks merge even in single-token workflow."""
+        """REQUEST_CHANGES blocks merge."""
        client = MagicMock()
        client.get_pr_reviews.return_value = [{"state": "REQUEST_CHANGES", "body": "Fix this"}]
        assert has_approval_review(client, "5") is False

-    def test_no_reviews_allows_merge(self) -> None:
-        """No reviews at all allows merge (single-token workflow fallback)."""
+    def test_no_reviews_returns_false(self) -> None:
+        """No reviews at all means no APPROVE — merge is blocked."""
        client = MagicMock()
        client.get_pr_reviews.return_value = []
-        assert has_approval_review(client, "5") is True
+        assert has_approval_review(client, "5") is False


 class TestValidatePrTitleMatchesVikunja:
@@ -13,6 +13,8 @@ from scripts.ci.pr_review import (
    check_commit_conventions,
    check_documentation,
    check_function_length,
+    check_i18n,
+    check_resource_management,
    check_security,
    check_test_coverage,
    is_python_file,
@@ -238,6 +240,143 @@ class TestCheckSecurity:
        assert not result.has_issues


+class TestCheckI18n:
+    def test_raw_string_in_echo_triggers_warning(self) -> None:
+        result = ReviewResult()
+        files = [
+            {"filename": "src/gitea_runner_manager/cli.py", "patch": '@@ -1,1 +1,1 @@\n+click.echo("Hello world")\n'}
+        ]
+        check_i18n(files, result)
+        assert result.has_issues
+        assert any("i18n" in i["body"] for i in result.issues)
+
+    def test_translated_string_no_warning(self) -> None:
+        result = ReviewResult()
+        files = [
+            {"filename": "src/gitea_runner_manager/cli.py", "patch": '@@ -1,1 +1,1 @@\n+click.echo(_("Hello world"))\n'}
+        ]
+        check_i18n(files, result)
+        assert not result.has_issues
+
+    def test_fstring_in_echo_triggers_warning(self) -> None:
+        result = ReviewResult()
+        files = [
+            {"filename": "src/gitea_runner_manager/cli.py", "patch": '@@ -1,1 +1,1 @@\n+click.echo(f"Hello {name}")\n'}
+        ]
+        check_i18n(files, result)
+        assert result.has_issues
+
+    def test_raw_exception_triggers_warning(self) -> None:
+        result = ReviewResult()
+        files = [
+            {
+                "filename": "src/gitea_runner_manager/cli.py",
+                "patch": '@@ -1,1 +1,1 @@\n+raise click.ClickException("Something went wrong")\n',
+            }
+        ]
+        check_i18n(files, result)
+        assert result.has_issues
+
+    def test_non_src_file_skipped(self) -> None:
+        result = ReviewResult()
+        files = [{"filename": "scripts/ci/test.py", "patch": '@@ -1,1 +1,1 @@\n+click.echo("Hello world")\n'}]
+        check_i18n(files, result)
+        assert not result.has_issues
+
+    def test_comment_skipped(self) -> None:
+        result = ReviewResult()
+        files = [
+            {"filename": "src/gitea_runner_manager/cli.py", "patch": '@@ -1,1 +1,1 @@\n+# click.echo("Hello world")\n'}
+        ]
+        check_i18n(files, result)
+        assert not result.has_issues
+
+    def test_empty_patch_skipped(self) -> None:
+        result = ReviewResult()
+        files = [{"filename": "src/gitea_runner_manager/cli.py", "patch": ""}]
+        check_i18n(files, result)
+        assert not result.has_issues
+
+    def test_clean_code_adds_ok_summary(self) -> None:
+        result = ReviewResult()
+        files = [
+            {"filename": "src/gitea_runner_manager/cli.py", "patch": '@@ -1,1 +1,1 @@\n+click.echo(_("Hello world"))\n'}
+        ]
+        check_i18n(files, result)
+        assert any("i18n: OK" in s for s in result.summary)
+
+
+class TestCheckResourceManagement:
+    def test_open_without_with_triggers_warning(self) -> None:
+        result = ReviewResult()
+        files = [{"filename": "src/gitea_runner_manager/cli.py", "patch": '@@ -1,1 +1,1 @@\n+f = open("file.txt")\n'}]
+        check_resource_management(files, result)
+        assert result.has_issues
+        assert any("resource" in i["body"].lower() for i in result.issues)
+
+    def test_open_with_with_no_warning(self) -> None:
+        result = ReviewResult()
+        files = [
+            {
+                "filename": "src/gitea_runner_manager/cli.py",
+                "patch": '@@ -1,1 +1,1 @@\n+with open("file.txt") as f:\n+    pass\n',
+            }
+        ]
+        check_resource_management(files, result)
+        assert not result.has_issues
+
+    def test_popen_without_cleanup_triggers_warning(self) -> None:
+        result = ReviewResult()
+        files = [
+            {
+                "filename": "src/gitea_runner_manager/executor.py",
+                "patch": '@@ -1,1 +1,1 @@\n+proc = subprocess.Popen(["cmd"])\n',
+            }
+        ]
+        check_resource_management(files, result)
+        assert result.has_issues
+
+    def test_popen_with_communicate_no_warning(self) -> None:
+        result = ReviewResult()
+        files = [
+            {
+                "filename": "src/gitea_runner_manager/executor.py",
+                "patch": '@@ -1,1 +1,1 @@\n+out, err = subprocess.Popen(["cmd"], stdout=PIPE).communicate()\n',
+            }
+        ]
+        check_resource_management(files, result)
+        assert not result.has_issues
+
+    def test_comment_skipped(self) -> None:
+        result = ReviewResult()
+        files = [{"filename": "src/gitea_runner_manager/cli.py", "patch": '@@ -1,1 +1,1 @@\n+# f = open("file.txt")\n'}]
+        check_resource_management(files, result)
+        assert not result.has_issues
+
+    def test_empty_patch_skipped(self) -> None:
+        result = ReviewResult()
+        files = [{"filename": "src/gitea_runner_manager/cli.py", "patch": ""}]
+        check_resource_management(files, result)
+        assert not result.has_issues
+
+    def test_non_python_file_skipped(self) -> None:
+        result = ReviewResult()
+        files = [{"filename": "docs/config.md", "patch": '@@ -1,1 +1,1 @@\n+f = open("file.txt")\n'}]
+        check_resource_management(files, result)
+        assert not result.has_issues
+
+    def test_clean_code_adds_ok_summary(self) -> None:
+        result = ReviewResult()
+        files = [
+            {
+                "filename": "src/gitea_runner_manager/cli.py",
+                "patch": '@@ -1,1 +1,1 @@\n+with open("file.txt") as f:\n+    data = f.read()\n',
+            }
+        ]
+        check_resource_management(files, result)
+        assert any("Resource management: OK" in s for s in result.summary)
+
+
 class TestCheckFunctionLength:
    def test_long_function_triggers_warning(self) -> None:
        result = ReviewResult()
@@ -77,7 +77,7 @@ class TestMain:
    @patch.dict("os.environ", {"REPO_TOKEN": "tok"})
    @patch("scripts.ci.review_pr.GiteaClient")
    def test_successful_approve_review(self, mock_client_cls: MagicMock) -> None:
-        """APPROVE requires --checklist-confirmed and substantive body."""
+        """APPROVE requires --checklist-confirmed, --checklist-categories, and substantive body."""
        mock_client = MagicMock()
        mock_client.create_review.return_value = {"id": 7}
        mock_client_cls.return_value = mock_client
@@ -90,8 +90,10 @@ class TestMain:
                "--event",
                "APPROVE",
                "--checklist-confirmed",
+                "--checklist-categories",
+                "1,2,3,4,5,6,7,8,9,10,11,12,13",
                "--body",
-                "All 10 checklist categories verified. Architecture OK, tests pass.",
+                "All 13 checklist categories verified. Architecture OK, tests pass.",
            ],
        )
        assert result.exit_code == 0
@@ -99,7 +101,7 @@ class TestMain:
        mock_client.create_review.assert_called_once_with(
            "5",
            event="APPROVE",
-            body="All 10 checklist categories verified. Architecture OK, tests pass.",
+            body="All 13 checklist categories verified. Architecture OK, tests pass.",
            comments=[],
        )

@@ -120,14 +122,72 @@ class TestMain:

    @patch.dict("os.environ", {"REPO_TOKEN": "tok"})
    @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."""
+    def test_approve_without_checklist_categories_fails(self, mock_client_cls: MagicMock) -> None:
+        """APPROVE without --checklist-categories is rejected."""
        mock_client = MagicMock()
        mock_client_cls.return_value = mock_client
        runner = CliRunner()
        result = runner.invoke(
            main,
-            ["5", "owner/repo", "--event", "APPROVE", "--checklist-confirmed", "--body", "LGTM"],
+            [
+                "5",
+                "owner/repo",
+                "--event",
+                "APPROVE",
+                "--checklist-confirmed",
+                "--body",
+                "All categories verified. Architecture OK, tests pass, docs updated.",
+            ],
+        )
+        assert result.exit_code != 0
+        assert "checklist-categories" in result.output.lower()
+        mock_client.create_review.assert_not_called()
+
+    @patch.dict("os.environ", {"REPO_TOKEN": "tok"})
+    @patch("scripts.ci.review_pr.GiteaClient")
+    def test_approve_with_too_few_categories_fails(self, mock_client_cls: MagicMock) -> None:
+        """APPROVE with fewer than 8 categories is rejected."""
+        mock_client = MagicMock()
+        mock_client_cls.return_value = mock_client
+        runner = CliRunner()
+        result = runner.invoke(
+            main,
+            [
+                "5",
+                "owner/repo",
+                "--event",
+                "APPROVE",
+                "--checklist-confirmed",
+                "--checklist-categories",
+                "1,2,3",
+                "--body",
+                "All categories verified. Architecture OK, tests pass, docs updated.",
+            ],
+        )
+        assert result.exit_code != 0
+        assert "8 of 13" in result.output
+        mock_client.create_review.assert_not_called()
+
+    @patch.dict("os.environ", {"REPO_TOKEN": "tok"})
+    @patch("scripts.ci.review_pr.GiteaClient")
+    def test_approve_with_trivial_body_fails(self, mock_client_cls: MagicMock) -> None:
+        """APPROVE with trivial body (< 50 chars) and no comments is rejected."""
+        mock_client = MagicMock()
+        mock_client_cls.return_value = mock_client
+        runner = CliRunner()
+        result = runner.invoke(
+            main,
+            [
+                "5",
+                "owner/repo",
+                "--event",
+                "APPROVE",
+                "--checklist-confirmed",
+                "--checklist-categories",
+                "1,2,3,4,5,6,7,8",
+                "--body",
+                "LGTM",
+            ],
        )
        assert result.exit_code != 0
        assert "substantive" in result.output.lower()