Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion scripts/run_recovery_rehearsal.py
Original file line number Diff line number Diff line change
Expand Up @@ -613,14 +613,23 @@ 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)
except json.JSONDecodeError:
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


Expand Down
4 changes: 3 additions & 1 deletion src/pr_review_harness/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Expand Down
13 changes: 10 additions & 3 deletions tests/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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()

Expand All @@ -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()

Expand Down
87 changes: 86 additions & 1 deletion tests/test_recovery_rehearsal.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
)
Expand All @@ -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"
Expand Down
Loading