From 149d7fc0fd03f04cdf247d80082761114d4b1a70 Mon Sep 17 00:00:00 2001 From: Magnus Hedemark Date: Fri, 23 Oct 2026 22:56:11 -0400 Subject: [PATCH 1/2] fix: preserve sanitized JSON errors in CI logs Signed-off-by: Magnus Hedemark --- src/pr_review_harness/cli.py | 4 +++- tests/test_cli.py | 13 ++++++++++--- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/pr_review_harness/cli.py b/src/pr_review_harness/cli.py index b73fa123..1d3209f6 100644 --- a/src/pr_review_harness/cli.py +++ b/src/pr_review_harness/cli.py @@ -1833,7 +1833,9 @@ def _emit_error(args, code: int, message: str, *, diagnostic_artifact_path: str if code == 1 and diagnostic_artifact_path: payload["diagnostic_artifact_path"] = diagnostic_artifact_path if getattr(args, "json", False): - print(json.dumps(payload, sort_keys=True, separators=(",", ":"))) + encoded = json.dumps(payload, sort_keys=True, separators=(",", ":")) + print(encoded) + print(encoded, file=sys.stderr) else: suffix = ( f"; diagnostic artifact (not a completed report): {diagnostic_artifact_path}" diff --git a/tests/test_cli.py b/tests/test_cli.py index edf90d93..5c94ff54 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -874,7 +874,9 @@ def test_preflight_rejection_stays_exit_two_without_review_dispatch(tmp_path, mo ) == 2 ) - output = capsys.readouterr().out + captured = capsys.readouterr() + output = captured.out + assert captured.err == output assert json.loads(output) == { "error": "explicit base and head revisions are required without a PR/event input", "exit_code": 2, @@ -1053,7 +1055,10 @@ def test_runtime_failure_without_a_regular_result_has_no_diagnostic_path(tmp_pat engine, "run_review", lambda *_args, **_kwargs: (_ for _ in ()).throw(ValueError("secret detail")) ) assert cli.main(argv) == 1 - payload = json.loads(capsys.readouterr().out) + captured = capsys.readouterr() + payload = json.loads(captured.out) + assert captured.err == captured.out + assert "secret detail" not in captured.err assert payload == {"error": "review_runtime_failed", "exit_code": 1} assert not output_dir.exists() @@ -1068,7 +1073,9 @@ def test_engine_typed_preflight_rejection_stays_exit_two_without_artifact_path(t lambda *_args, **_kwargs: (_ for _ in ()).throw(engine.EnginePreflightError("resume_state_invalid")), ) assert cli.main(argv) == 2 - payload = json.loads(capsys.readouterr().out) + captured = capsys.readouterr() + payload = json.loads(captured.out) + assert captured.err == captured.out assert payload == {"error": "resume_state_invalid", "exit_code": 2} assert not output_dir.exists() From e6be6f8490212f9311cbe86b22968f2b05b19450 Mon Sep 17 00:00:00 2001 From: Magnus Hedemark Date: Fri, 23 Oct 2026 23:25:15 -0400 Subject: [PATCH 2/2] test: accept exact preflight diagnostics in recovery rehearsal Signed-off-by: Magnus Hedemark --- scripts/run_recovery_rehearsal.py | 11 +++- tests/test_recovery_rehearsal.py | 87 ++++++++++++++++++++++++++++++- 2 files changed, 96 insertions(+), 2 deletions(-) diff --git a/scripts/run_recovery_rehearsal.py b/scripts/run_recovery_rehearsal.py index 8402774e..c46cb7aa 100644 --- a/scripts/run_recovery_rehearsal.py +++ b/scripts/run_recovery_rehearsal.py @@ -613,7 +613,7 @@ def invocation_remaining() -> float: raise RehearsalError("cli_output_invalid_utf8") from None if returncode != expected_exit: raise RehearsalError("cli_exit_mismatch") - if stderr_text: + if stderr_text and (expected_exit != 2 or stderr != stdout): raise RehearsalError("cli_stderr_not_empty") try: value = json.loads(stdout_text) @@ -621,6 +621,15 @@ def invocation_remaining() -> float: raise RehearsalError("cli_json_output_invalid") from None if not isinstance(value, dict): raise RehearsalError("cli_json_output_invalid") + if stderr_text and ( + set(value) != {"error", "exit_code"} + or type(value.get("exit_code")) is not int + or value["exit_code"] != expected_exit + or not isinstance(value.get("error"), str) + or not _FAILURE_CODE.fullmatch(value["error"]) + or stdout_text != json.dumps(value, sort_keys=True, separators=(",", ":")) + "\n" + ): + raise RehearsalError("cli_stderr_not_empty") return value diff --git a/tests/test_recovery_rehearsal.py b/tests/test_recovery_rehearsal.py index 170192d6..33fc168f 100644 --- a/tests/test_recovery_rehearsal.py +++ b/tests/test_recovery_rehearsal.py @@ -119,7 +119,8 @@ def test_run_cli_rejects_mismatched_process_exit_even_with_preflight_json(tmp_pa cli.write_text( "#!/usr/bin/env python3\n" "import json, sys\n" - "print(json.dumps({'error':'resume_state_invalid','exit_code':2}))\n" + "payload = json.dumps({'error':'resume_state_invalid','exit_code':2}) + '\\n'\n" + "sys.stdout.write(payload); sys.stderr.write(payload)\n" "sys.exit(1)\n", encoding="utf-8", ) @@ -135,6 +136,90 @@ def test_run_cli_rejects_mismatched_process_exit_even_with_preflight_json(tmp_pa ) +@pytest.mark.parametrize("error_code", ["preflight_rejected", "resume_state_invalid"]) +def test_run_cli_accepts_only_exact_preflight_error_mirror_and_keeps_drift_guards(tmp_path, error_code): + payload = json.dumps({"error": error_code, "exit_code": 2}, sort_keys=True, separators=(",", ":")) + "\n" + cli = tmp_path / "preflight-error" + cli.write_text( + f"#!{sys.executable}\nimport sys\n" + f"sys.stdout.write({payload!r}); sys.stderr.write({payload!r})\n" + "sys.exit(2)\n", + encoding="utf-8", + ) + cli.chmod(0o700) + result = rehearsal._run_cli( + cli, + [], + cwd=tmp_path, + env={"PATH": os.environ.get("PATH", "")}, + expected_exit=2, + deadline_at=__import__("time").monotonic() + 2, + ) + assert result == {"error": error_code, "exit_code": 2} + checkpoint = tmp_path / "checkpoint.json" + checkpoint.write_bytes(b'{"disposition":"INCOMPLETE"}\n') + digest = rehearsal._hash_file(checkpoint) + assert rehearsal._validate_profile_drift_rejection( + result, + checkpoint_path=checkpoint, + checkpoint_sha256=digest, + calls_before=2, + calls_after=2, + ) == error_code + assert rehearsal._hash_file(checkpoint) == digest + + +@pytest.mark.parametrize( + ("stdout", "stderr", "expected_exit"), + [ + pytest.param('{"status":"ok"}\n', '{"status":"ok"}\n', 0, id="success-still-rejects-stderr"), + pytest.param('{"error":"resume_state_invalid","exit_code":2}\n', "MUST_NOT_LEAK_SECRET\n", 2, + id="arbitrary-secret-stderr"), + pytest.param('{"error":"resume_state_invalid","exit_code":2}\n', + '{"error":"preflight_rejected","exit_code":2}\n', 2, id="mismatched-error"), + pytest.param('{"error":"resume_state_invalid","exit_code":2}\n', + '{"error":"resume_state_invalid","exit_code":2}\nextra\n', 2, id="extra-stderr"), + *[ + pytest.param(payload, payload, 2, id=case) + for case, payload in [ + ("extra-field", '{"error":"resume_state_invalid","exit_code":2,"extra":true}\n'), + ("secret-diagnostic", '{"diagnostic_artifact_path":"MUST_NOT_LEAK_SECRET",' + '"error":"resume_state_invalid","exit_code":2}\n'), + ("wrong-exit-code", '{"error":"resume_state_invalid","exit_code":1}\n'), + ("bool-exit-code", '{"error":"resume_state_invalid","exit_code":true}\n'), + ("float-exit-code", '{"error":"resume_state_invalid","exit_code":2.0}\n'), + ("nonstring-error", '{"error":["resume_state_invalid"],"exit_code":2}\n'), + ("unsafe-error", '{"error":"MUST_NOT_LEAK_SECRET text","exit_code":2}\n'), + ("multiline-json", '{\n"error":"resume_state_invalid",\n"exit_code":2\n}\n'), + ("extra-json-line", '{"error":"resume_state_invalid","exit_code":2}\n{}\n'), + ("duplicate-key", '{"error":"preflight_rejected","error":"resume_state_invalid",' + '"exit_code":2}\n'), + ("no-final-newline", '{"error":"resume_state_invalid","exit_code":2}'), + ] + ], + ], +) +def test_run_cli_rejects_stderr_outside_exact_preflight_mirror(tmp_path, stdout, stderr, expected_exit): + cli = tmp_path / "invalid-stderr" + cli.write_text( + f"#!{sys.executable}\nimport sys\n" + f"sys.stdout.write({stdout!r}); sys.stderr.write({stderr!r})\n" + f"sys.exit({expected_exit})\n", + encoding="utf-8", + ) + cli.chmod(0o700) + with pytest.raises(rehearsal.RehearsalError, match=r"^cli_(stderr_not_empty|json_output_invalid)$") as caught: + rehearsal._run_cli( + cli, + [], + cwd=tmp_path, + env={"PATH": os.environ.get("PATH", "")}, + expected_exit=expected_exit, + deadline_at=__import__("time").monotonic() + 2, + ) + assert "MUST_NOT_LEAK_SECRET" not in str(caught.value) + + @pytest.mark.parametrize("error_code", ["preflight_rejected", "resume_state_invalid"]) def test_profile_drift_accepts_only_known_preflight_codes_and_preserves_checkpoint(tmp_path, error_code): checkpoint = tmp_path / "run.json"