From 5577e74fd89e7d2871614ffe2bf20ad05b31161f Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Fri, 11 Sep 2026 09:57:26 +0300 Subject: [PATCH] fix: stderr is not part of the JSON MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit run_check_json ran commit-check with stderr=subprocess.STDOUT and handed the merged stream to json.loads. The CLI's contract is the opposite: stdout is the JSON and nothing else, stderr is what it has to say to a person. Merged, any such line makes the response unparsable -- and an unparsable response is a failure as far as ScopeResult is concerned, so a passing run goes red: $ commit-check --format json --branch # conventional_branch = false ⊘ --branch requested but no branch rules are configured (conventional_branch = false and no require_rebase_target) { "status": "pass", "warnings": 0, "checks": [] } -> json.loads fails -> ScopeResult(raw_text=...) -> status "fail" -> exit_code_for -> 1, on a run where the CLI itself exited 0. commit-check 2.17.1 adds two lines that reach this path, and the action asks for branch checks by default: the notice above (any repository whose config switched the branch rules off) and an inherit_from that could not be fetched (a private .github repository, a typo'd URL, a network blip). A dry run prints one too. None of them is a verdict, and the CLI fails open on the second on purpose. Read the two streams separately: stdout goes to the parser, stderr goes to the job log, deduplicated -- 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. When there is no JSON to protect the two are joined again, so an unparsable run still shows everything the CLI said. That path matters more than it looks: a config the CLI refuses leaves stdout empty and prints "Error: ..." on stderr, and dropping it would leave an empty raw_text that reads as a pass. --- main.py | 47 ++++++++++++++++++++++++++---- main_test.py | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 120 insertions(+), 8 deletions(-) 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)