fix: stderr is not part of the JSON (#282)

Author: shenxianpeng

main.py

@@ -492,29 +492,66 @@ def get_pr_commit_messages() -> list[Commit]:
     return []
 
 
+#: Notices already relayed, so one repeated across every scope is shown once.
+_RELAYED_NOTICES: set[str] = set()
+
+
+def _relay_cli_notices(text: str) -> None:
+    """Put the CLI's stderr in the job log, without repeating it.
+
+    The Action runs commit-check once per scope and once per commit in the
+    pull request, and a notice about the configuration is the same every
+    time; printed on each run it would bury the findings.
+    """
+    for line in text.splitlines():
+        line = line.strip()
+        if line and line not in _RELAYED_NOTICES:
+            _RELAYED_NOTICES.add(line)
+            print(f"commit-check: {line}", file=sys.stderr)
+
+
 def run_check_json(
     args: list[str], input_text: str | None = None
 ) -> tuple[int, dict[str, Any] | None, str]:
     """Run ``commit-check --format json`` and return (exit code, parsed JSON, raw output).
 
+    The CLI's contract is that stdout holds the JSON and nothing else, while
+    stderr carries what it has to say to a person: a parent config it could
+    not fetch, a flag whose every rule the config switched off, a dry run
+    that softened its own verdict. Those lines are not JSON, so they are
+    read separately and relayed to the job log rather than handed to the
+    parser -- merged into stdout they turned a passing run into an
+    unparsable one, which ScopeResult reports as a failure.
+
     The parsed JSON is ``None`` when the CLI did not produce valid JSON; the
-    raw output is kept so callers can fall back to showing it as text.
+    raw output is kept so callers can fall back to showing it as text, and
+    in that case it carries both streams so nothing the CLI said is lost.
     """
     command = ["commit-check", "--format", "json"] + args
     result = subprocess.run(
         command,
         input=input_text,
         stdout=subprocess.PIPE,
-        stderr=subprocess.STDOUT,
+        stderr=subprocess.PIPE,
         text=True,
         encoding="utf-8",
         check=False,
     )
-    raw = result.stdout or ""
+    out = result.stdout or ""
+    err = result.stderr or ""
+    _relay_cli_notices(err)
     try:
-        return result.returncode, json.loads(raw), raw
+        return result.returncode, json.loads(out), out
     except json.JSONDecodeError:
-        return result.returncode, None, raw
+        # Whatever went wrong, the operator needs everything the CLI said,
+        # so stderr joins stdout here -- and only here, where there is no
+        # JSON left to protect. It is often the whole story: a config the
+        # CLI refused leaves stdout empty and prints "Error: ..." on stderr,
+        # and dropping it would leave an empty raw_text that reads as a pass.
+        if not err.strip():
+            return result.returncode, None, out
+        parts = [part for part in (out.strip(), err.strip()) if part]
+        return result.returncode, None, "\n".join(parts)
 
 
 def check_scope(

main_test.py

@@ -308,7 +308,7 @@ def test_input_text_is_passed_through(self):
         self.assertTrue(mock_run.call_args[1]["text"])
 
     def test_invalid_json_returns_none_with_raw_output(self):
-        mock_result = MagicMock(returncode=1, stdout="Commit rejected.\n")
+        mock_result = MagicMock(returncode=1, stdout="Commit rejected.\n", stderr="")
         with patch("main.subprocess.run", return_value=mock_result):
             rc, data, raw = main.run_check_json(["--branch"])
         self.assertEqual(rc, 1)
@@ -352,7 +352,7 @@ def test_parses_checks_into_scope(self):
         self.assertEqual(scope.failures[0]["rule_id"], "CC001")
 
     def test_invalid_json_falls_back_to_raw_text(self):
-        mock_result = MagicMock(returncode=1, stdout="unexpected output")
+        mock_result = MagicMock(returncode=1, stdout="unexpected output", stderr="")
         with patch("main.subprocess.run", return_value=mock_result):
             scope = main.check_scope("Branch", ["--branch"])
         self.assertEqual(scope.label, "Branch")
@@ -388,7 +388,7 @@ def test_failed_message_marks_scope_failed(self):
         self.assertEqual(scopes[0].sha, SHA_A)
 
     def test_unparsable_output_still_names_the_commit(self):
-        mock_result = MagicMock(returncode=1, stdout="unexpected output")
+        mock_result = MagicMock(returncode=1, stdout="unexpected output", stderr="")
         with patch("main.subprocess.run", return_value=mock_result):
             scopes = main.run_pr_message_checks([(SHA_A, "bad commit")])
         self.assertEqual(scopes[0].raw_text, "unexpected output")
@@ -2841,3 +2841,78 @@ def test_failing_message(self):
             "[CC001 message](https://commit-check.com/rules/#cc001)",
             main.render_report([scope]),
         )
+
+
+class TestStderrIsNotPartOfTheJson(unittest.TestCase):
+    """stdout is the JSON; stderr is what the CLI has to say to a person.
+
+    Merging the two fed notices to json.loads, and an unparsable response is
+    a failure as far as ScopeResult is concerned -- so a passing run went
+    red the moment the CLI gained something to say. It has several such
+    lines, printed under --format json as well: a parent config it could not
+    fetch, a flag whose every rule the config switched off, a dry run that
+    softened its own verdict.
+    """
+
+    #: What the CLI prints when --branch is asked for and the repository's
+    #: config has switched every branch rule off.
+    NOTICE = (
+        "⊘ --branch requested but no branch rules are configured "
+        "(conventional_branch = false and no require_rebase_target)\n"
+    )
+
+    def setUp(self):
+        main._RELAYED_NOTICES.clear()
+
+    def test_the_two_streams_are_read_separately(self):
+        mock_result = MagicMock(returncode=0, stdout="{}", stderr="")
+        with patch("main.subprocess.run", return_value=mock_result) as mock_run:
+            main.run_check_json(["--branch"])
+        self.assertIs(mock_run.call_args[1]["stderr"], main.subprocess.PIPE)
+
+    def test_a_notice_does_not_break_the_json(self):
+        mock_result = MagicMock(
+            returncode=0,
+            stdout=json_output(make_check("branch")),
+            stderr=self.NOTICE,
+        )
+        with patch("main.subprocess.run", return_value=mock_result):
+            with patch.object(sys, "stderr", io.StringIO()):
+                rc, data, raw = main.run_check_json(["--branch"])
+        self.assertEqual(rc, 0)
+        self.assertIsNotNone(data)
+        self.assertEqual(data["status"], "pass")
+        # And the scope built from it is a pass, not the "unparsable" fail.
+        self.assertEqual(main.check_scope("Branch", ["--branch"]).label, "Branch")
+
+    def test_a_notice_alone_does_not_fail_the_scope(self):
+        mock_result = MagicMock(returncode=0, stdout=json_output(), stderr=self.NOTICE)
+        with patch("main.subprocess.run", return_value=mock_result):
+            with patch.object(sys, "stderr", io.StringIO()):
+                scope = main.check_scope("Branch", ["--branch"])
+        self.assertEqual(scope.status, "pass")
+        self.assertEqual(scope.raw_text, "")
+
+    def test_notices_reach_the_job_log_once(self):
+        mock_result = MagicMock(
+            returncode=0, stdout=json_output(make_check("branch")), stderr=self.NOTICE
+        )
+        fake_err = io.StringIO()
+        with patch("main.subprocess.run", return_value=mock_result):
+            with patch.object(sys, "stderr", fake_err):
+                for _ in range(3):
+                    main.run_check_json(["--branch"])
+        printed = fake_err.getvalue()
+        self.assertIn("--branch requested but no branch rules are configured", printed)
+        # Once, not once per scope and per commit in the pull request.
+        self.assertEqual(printed.count("commit-check: ⊘"), 1)
+
+    def test_unparsable_output_keeps_both_streams(self):
+        mock_result = MagicMock(
+            returncode=2, stdout="", stderr="Error: cchk.toml: Expected ']'\n"
+        )
+        with patch("main.subprocess.run", return_value=mock_result):
+            with patch.object(sys, "stderr", io.StringIO()):
+                scope = main.check_scope("Branch", ["--branch"])
+        self.assertEqual(scope.status, "fail")
+        self.assertIn("Expected ']'", scope.raw_text)