Skip to content

Commit 5577e74

Browse files
committed
fix: stderr is not part of the JSON
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.
1 parent fd3d923 commit 5577e74

2 files changed

Lines changed: 120 additions & 8 deletions

File tree

main.py

Lines changed: 42 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -492,29 +492,66 @@ def get_pr_commit_messages() -> list[Commit]:
492492
return []
493493

494494

495+
#: Notices already relayed, so one repeated across every scope is shown once.
496+
_RELAYED_NOTICES: set[str] = set()
497+
498+
499+
def _relay_cli_notices(text: str) -> None:
500+
"""Put the CLI's stderr in the job log, without repeating it.
501+
502+
The Action runs commit-check once per scope and once per commit in the
503+
pull request, and a notice about the configuration is the same every
504+
time; printed on each run it would bury the findings.
505+
"""
506+
for line in text.splitlines():
507+
line = line.strip()
508+
if line and line not in _RELAYED_NOTICES:
509+
_RELAYED_NOTICES.add(line)
510+
print(f"commit-check: {line}", file=sys.stderr)
511+
512+
495513
def run_check_json(
496514
args: list[str], input_text: str | None = None
497515
) -> tuple[int, dict[str, Any] | None, str]:
498516
"""Run ``commit-check --format json`` and return (exit code, parsed JSON, raw output).
499517
518+
The CLI's contract is that stdout holds the JSON and nothing else, while
519+
stderr carries what it has to say to a person: a parent config it could
520+
not fetch, a flag whose every rule the config switched off, a dry run
521+
that softened its own verdict. Those lines are not JSON, so they are
522+
read separately and relayed to the job log rather than handed to the
523+
parser -- merged into stdout they turned a passing run into an
524+
unparsable one, which ScopeResult reports as a failure.
525+
500526
The parsed JSON is ``None`` when the CLI did not produce valid JSON; the
501-
raw output is kept so callers can fall back to showing it as text.
527+
raw output is kept so callers can fall back to showing it as text, and
528+
in that case it carries both streams so nothing the CLI said is lost.
502529
"""
503530
command = ["commit-check", "--format", "json"] + args
504531
result = subprocess.run(
505532
command,
506533
input=input_text,
507534
stdout=subprocess.PIPE,
508-
stderr=subprocess.STDOUT,
535+
stderr=subprocess.PIPE,
509536
text=True,
510537
encoding="utf-8",
511538
check=False,
512539
)
513-
raw = result.stdout or ""
540+
out = result.stdout or ""
541+
err = result.stderr or ""
542+
_relay_cli_notices(err)
514543
try:
515-
return result.returncode, json.loads(raw), raw
544+
return result.returncode, json.loads(out), out
516545
except json.JSONDecodeError:
517-
return result.returncode, None, raw
546+
# Whatever went wrong, the operator needs everything the CLI said,
547+
# so stderr joins stdout here -- and only here, where there is no
548+
# JSON left to protect. It is often the whole story: a config the
549+
# CLI refused leaves stdout empty and prints "Error: ..." on stderr,
550+
# and dropping it would leave an empty raw_text that reads as a pass.
551+
if not err.strip():
552+
return result.returncode, None, out
553+
parts = [part for part in (out.strip(), err.strip()) if part]
554+
return result.returncode, None, "\n".join(parts)
518555

519556

520557
def check_scope(

main_test.py

Lines changed: 78 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,7 @@ def test_input_text_is_passed_through(self):
308308
self.assertTrue(mock_run.call_args[1]["text"])
309309

310310
def test_invalid_json_returns_none_with_raw_output(self):
311-
mock_result = MagicMock(returncode=1, stdout="Commit rejected.\n")
311+
mock_result = MagicMock(returncode=1, stdout="Commit rejected.\n", stderr="")
312312
with patch("main.subprocess.run", return_value=mock_result):
313313
rc, data, raw = main.run_check_json(["--branch"])
314314
self.assertEqual(rc, 1)
@@ -352,7 +352,7 @@ def test_parses_checks_into_scope(self):
352352
self.assertEqual(scope.failures[0]["rule_id"], "CC001")
353353

354354
def test_invalid_json_falls_back_to_raw_text(self):
355-
mock_result = MagicMock(returncode=1, stdout="unexpected output")
355+
mock_result = MagicMock(returncode=1, stdout="unexpected output", stderr="")
356356
with patch("main.subprocess.run", return_value=mock_result):
357357
scope = main.check_scope("Branch", ["--branch"])
358358
self.assertEqual(scope.label, "Branch")
@@ -388,7 +388,7 @@ def test_failed_message_marks_scope_failed(self):
388388
self.assertEqual(scopes[0].sha, SHA_A)
389389

390390
def test_unparsable_output_still_names_the_commit(self):
391-
mock_result = MagicMock(returncode=1, stdout="unexpected output")
391+
mock_result = MagicMock(returncode=1, stdout="unexpected output", stderr="")
392392
with patch("main.subprocess.run", return_value=mock_result):
393393
scopes = main.run_pr_message_checks([(SHA_A, "bad commit")])
394394
self.assertEqual(scopes[0].raw_text, "unexpected output")
@@ -2841,3 +2841,78 @@ def test_failing_message(self):
28412841
"[CC001 message](https://commit-check.com/rules/#cc001)",
28422842
main.render_report([scope]),
28432843
)
2844+
2845+
2846+
class TestStderrIsNotPartOfTheJson(unittest.TestCase):
2847+
"""stdout is the JSON; stderr is what the CLI has to say to a person.
2848+
2849+
Merging the two fed notices to json.loads, and an unparsable response is
2850+
a failure as far as ScopeResult is concerned -- so a passing run went
2851+
red the moment the CLI gained something to say. It has several such
2852+
lines, printed under --format json as well: a parent config it could not
2853+
fetch, a flag whose every rule the config switched off, a dry run that
2854+
softened its own verdict.
2855+
"""
2856+
2857+
#: What the CLI prints when --branch is asked for and the repository's
2858+
#: config has switched every branch rule off.
2859+
NOTICE = (
2860+
"⊘ --branch requested but no branch rules are configured "
2861+
"(conventional_branch = false and no require_rebase_target)\n"
2862+
)
2863+
2864+
def setUp(self):
2865+
main._RELAYED_NOTICES.clear()
2866+
2867+
def test_the_two_streams_are_read_separately(self):
2868+
mock_result = MagicMock(returncode=0, stdout="{}", stderr="")
2869+
with patch("main.subprocess.run", return_value=mock_result) as mock_run:
2870+
main.run_check_json(["--branch"])
2871+
self.assertIs(mock_run.call_args[1]["stderr"], main.subprocess.PIPE)
2872+
2873+
def test_a_notice_does_not_break_the_json(self):
2874+
mock_result = MagicMock(
2875+
returncode=0,
2876+
stdout=json_output(make_check("branch")),
2877+
stderr=self.NOTICE,
2878+
)
2879+
with patch("main.subprocess.run", return_value=mock_result):
2880+
with patch.object(sys, "stderr", io.StringIO()):
2881+
rc, data, raw = main.run_check_json(["--branch"])
2882+
self.assertEqual(rc, 0)
2883+
self.assertIsNotNone(data)
2884+
self.assertEqual(data["status"], "pass")
2885+
# And the scope built from it is a pass, not the "unparsable" fail.
2886+
self.assertEqual(main.check_scope("Branch", ["--branch"]).label, "Branch")
2887+
2888+
def test_a_notice_alone_does_not_fail_the_scope(self):
2889+
mock_result = MagicMock(returncode=0, stdout=json_output(), stderr=self.NOTICE)
2890+
with patch("main.subprocess.run", return_value=mock_result):
2891+
with patch.object(sys, "stderr", io.StringIO()):
2892+
scope = main.check_scope("Branch", ["--branch"])
2893+
self.assertEqual(scope.status, "pass")
2894+
self.assertEqual(scope.raw_text, "")
2895+
2896+
def test_notices_reach_the_job_log_once(self):
2897+
mock_result = MagicMock(
2898+
returncode=0, stdout=json_output(make_check("branch")), stderr=self.NOTICE
2899+
)
2900+
fake_err = io.StringIO()
2901+
with patch("main.subprocess.run", return_value=mock_result):
2902+
with patch.object(sys, "stderr", fake_err):
2903+
for _ in range(3):
2904+
main.run_check_json(["--branch"])
2905+
printed = fake_err.getvalue()
2906+
self.assertIn("--branch requested but no branch rules are configured", printed)
2907+
# Once, not once per scope and per commit in the pull request.
2908+
self.assertEqual(printed.count("commit-check: ⊘"), 1)
2909+
2910+
def test_unparsable_output_keeps_both_streams(self):
2911+
mock_result = MagicMock(
2912+
returncode=2, stdout="", stderr="Error: cchk.toml: Expected ']'\n"
2913+
)
2914+
with patch("main.subprocess.run", return_value=mock_result):
2915+
with patch.object(sys, "stderr", io.StringIO()):
2916+
scope = main.check_scope("Branch", ["--branch"])
2917+
self.assertEqual(scope.status, "fail")
2918+
self.assertIn("Expected ']'", scope.raw_text)

0 commit comments

Comments
 (0)