diff --git a/main.py b/main.py index 2d6095c..77a2c0a 100755 --- a/main.py +++ b/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( diff --git a/main_test.py b/main_test.py index a078b0c..8b0be1f 100644 --- a/main_test.py +++ b/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)