refactor: clean up PR commit validation flow

Author: shenxianpeng

main.py

@@ -55,9 +55,9 @@ def is_pr_event() -> bool:
 def parse_commit_messages(output: str) -> list[str]:
     """Split git log output into individual commit messages."""
     return [
-        message.rstrip("\n")
+        message.strip("\n")
         for message in output.split(COMMIT_MESSAGE_DELIMITER)
-        if message.rstrip("\n")
+        if message.strip("\n")
     ]
 
 
@@ -124,19 +124,26 @@ def get_pr_commit_messages() -> list[str]:
 
 
 def run_check_command(
-    args: list[str], result_file: TextIO, input_text: str | None = None
+    args: list[str],
+    result_file: TextIO,
+    input_text: str | None = None,
+    output_prefix: str | None = None,
 ) -> int:
     """Run commit-check and write both stdout and stderr to the result file."""
     command = ["commit-check"] + args
     print(" ".join(command))
     result = subprocess.run(
         command,
         input=input_text,
-        stdout=result_file,
+        stdout=subprocess.PIPE,
         stderr=subprocess.STDOUT,
         text=True,
         check=False,
     )
+    if result.stdout:
+        if output_prefix:
+            result_file.write(output_prefix)
+        result_file.write(result.stdout)
     return result.returncode
 
 
@@ -149,11 +156,12 @@ def run_pr_message_checks(pr_messages: list[str], result_file: TextIO) -> int:
     total_messages = len(pr_messages)
     for index, msg in enumerate(pr_messages, start=1):
         subject = msg.splitlines()[0] if msg else "<empty commit message>"
-        result_file.write(f"\n--- Commit {index}/{total_messages}: {subject}\n")
-        has_failure = (
-            run_check_command(["--message"], result_file, input_text=msg) != 0
-            or has_failure
-        )
+        has_failure = run_check_command(
+            ["--message"],
+            result_file,
+            input_text=msg,
+            output_prefix=f"\n--- Commit {index}/{total_messages}: {subject}\n",
+        ) != 0 or has_failure
     return 1 if has_failure else 0
 
 

main_test.py

@@ -62,28 +62,28 @@ def test_message_and_branch(self):
 
 
 class TestParseCommitMessages(unittest.TestCase):
-    def test_splits_messages_and_trims_trailing_newlines(self):
-        result = main.parse_commit_messages("fix: first\n\x00feat: second\n\n\x00")
+    def test_splits_messages_and_trims_surrounding_newlines(self):
+        result = main.parse_commit_messages("\nfix: first\n\x00\nfeat: second\n\n\x00")
         self.assertEqual(result, ["fix: first", "feat: second"])
 
 
 class TestRunCheckCommand(unittest.TestCase):
     def test_with_args_calls_subprocess(self):
-        mock_result = MagicMock(returncode=0)
+        mock_result = MagicMock(returncode=0, stdout="")
         with patch("main.subprocess.run", return_value=mock_result) as mock_run:
             rc = main.run_check_command(["--branch"], io.StringIO())
         self.assertEqual(rc, 0)
         self.assertEqual(mock_run.call_args[0][0], ["commit-check", "--branch"])
 
     def test_with_input_uses_text_mode(self):
-        mock_result = MagicMock(returncode=0)
+        mock_result = MagicMock(returncode=0, stdout="")
         with patch("main.subprocess.run", return_value=mock_result) as mock_run:
             main.run_check_command(["--message"], io.StringIO(), input_text="fix: demo")
         self.assertEqual(mock_run.call_args[1]["input"], "fix: demo")
         self.assertTrue(mock_run.call_args[1]["text"])
 
     def test_prints_command(self):
-        mock_result = MagicMock(returncode=0)
+        mock_result = MagicMock(returncode=0, stdout="")
         with patch("main.subprocess.run", return_value=mock_result):
             with patch("builtins.print") as mock_print:
                 main.run_check_command(["--branch"], io.StringIO())
@@ -92,20 +92,29 @@ def test_prints_command(self):
 
 class TestRunPrMessageChecks(unittest.TestCase):
     def test_single_message_pass(self):
-        mock_result = MagicMock(returncode=0)
+        mock_result = MagicMock(returncode=0, stdout="")
         result_file = io.StringIO()
         with patch("main.subprocess.run", return_value=mock_result) as mock_run:
             rc = main.run_pr_message_checks(["fix: something"], result_file)
         self.assertEqual(rc, 0)
         self.assertEqual(mock_run.call_args[0][0], ["commit-check", "--message"])
         self.assertEqual(mock_run.call_args[1]["input"], "fix: something")
+        self.assertEqual(result_file.getvalue(), "")
+
+    def test_failed_message_writes_header_and_output(self):
+        mock_result = MagicMock(returncode=1, stdout="Commit rejected.\n")
+        result_file = io.StringIO()
+        with patch("main.subprocess.run", return_value=mock_result):
+            rc = main.run_pr_message_checks(["fix: something"], result_file)
+        self.assertEqual(rc, 1)
         self.assertIn("--- Commit 1/1: fix: something", result_file.getvalue())
+        self.assertIn("Commit rejected.", result_file.getvalue())
 
     def test_multiple_messages_partial_failure(self):
         results = [
-            MagicMock(returncode=0),
-            MagicMock(returncode=1),
-            MagicMock(returncode=0),
+            MagicMock(returncode=0, stdout=""),
+            MagicMock(returncode=1, stdout="Commit rejected.\n"),
+            MagicMock(returncode=0, stdout=""),
         ]
         with patch("main.subprocess.run", side_effect=results):
             rc = main.run_pr_message_checks(["ok", "bad", "ok"], io.StringIO())
@@ -126,7 +135,7 @@ def test_empty_args_returns_zero(self):
         mock_run.assert_not_called()
 
     def test_with_args_returns_returncode(self):
-        mock_result = MagicMock(returncode=1)
+        mock_result = MagicMock(returncode=1, stdout="branch check failed\n")
         with patch("main.subprocess.run", return_value=mock_result):
             rc = main.run_other_checks(["--branch", "--author-name"], io.StringIO())
         self.assertEqual(rc, 1)
@@ -182,19 +191,15 @@ def test_falls_back_to_base_ref_when_merge_ref_is_unavailable(self):
     def test_exception_returns_empty(self):
         with (
             patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}),
-            patch(
-                "main.get_messages_from_merge_ref", side_effect=Exception("git failed")
-            ),
+            patch("main.get_messages_from_merge_ref", side_effect=Exception("git failed")),
         ):
             result = main.get_pr_commit_messages()
         self.assertEqual(result, [])
 
 
 class TestGitMessageReaders(unittest.TestCase):
     def test_get_messages_from_merge_ref(self):
-        mock_result = MagicMock(
-            returncode=0, stdout="fix: first\n\x00feat: second\n\x00"
-        )
+        mock_result = MagicMock(returncode=0, stdout="fix: first\n\x00feat: second\n\x00")
         with patch("main.subprocess.run", return_value=mock_result) as mock_run:
             result = main.get_messages_from_merge_ref()
         self.assertEqual(result, ["fix: first", "feat: second"])
@@ -421,9 +426,7 @@ def test_same_repo_not_fork(self):
                 "base": {"repo": {"full_name": "owner/repo"}},
             }
         }
-        with tempfile.NamedTemporaryFile(
-            mode="w", suffix=".json", delete=False
-        ) as file_obj:
+        with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False) as file_obj:
             json.dump(event, file_obj)
             event_path = file_obj.name
         with patch.dict(os.environ, {"GITHUB_EVENT_PATH": event_path}):
@@ -440,9 +443,7 @@ def test_different_repo_is_fork(self):
                 "base": {"repo": {"full_name": "owner/repo"}},
             }
         }
-        with tempfile.NamedTemporaryFile(
-            mode="w", suffix=".json", delete=False
-        ) as file_obj:
+        with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False) as file_obj:
             json.dump(event, file_obj)
             event_path = file_obj.name
         with patch.dict(os.environ, {"GITHUB_EVENT_PATH": event_path}):