From 4dc8bd237bb79fd928c0563ae402780109e2ab73 Mon Sep 17 00:00:00 2001 From: Jer Miller Date: Thu, 30 Jul 2026 12:45:48 -0600 Subject: [PATCH] refactor(doctor): share the aggregate failure predicate Dropped the unreachable has_execution_error arm from summary_counts' failed count; probe.run_check already builds error results with status fail, so errors are a subset of failed by construction. Added probe.results_failed, the single aggregate rule for any execution error or any blocker-severity fail, now used by doctor.main, doctor.jsonl_summary_status, and preflight.main instead of duplicated conditions. Added preflight --json coverage asserting the structured execution_error payload and a nonzero exit for an advisory raising check. --- solstone/think/doctor.py | 13 +++---------- solstone/think/preflight.py | 8 ++------ solstone/think/probe.py | 14 ++++++++----- tests/test_preflight.py | 39 +++++++++++++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 21 deletions(-) diff --git a/solstone/think/doctor.py b/solstone/think/doctor.py index 2985756df..ceea8d99b 100644 --- a/solstone/think/doctor.py +++ b/solstone/think/doctor.py @@ -70,6 +70,7 @@ from solstone.think.probe import ( local_bin_sol_reachable_check, make_result, platform_tag, + results_failed, run_check, run_probe, status_label, @@ -1678,11 +1679,7 @@ def solstone_version() -> str: def jsonl_summary_status(results: Sequence[CheckResult]) -> str: - if any(has_execution_error(result) for result in results): - return "failed" - if any( - result.severity == "blocker" and result.status == "fail" for result in results - ): + if results_failed(results): return "failed" if any( result.status == "warn" @@ -1751,8 +1748,4 @@ def main(argv: Sequence[str] | None = None) -> int: ) else: emit_text(results, verbose=args.verbose) - blocker_failed = any( - result.severity == "blocker" and result.status == "fail" for result in results - ) - execution_failed = any(has_execution_error(result) for result in results) - return 1 if execution_failed or blocker_failed else 0 + return 1 if results_failed(results) else 0 diff --git a/solstone/think/preflight.py b/solstone/think/preflight.py index f5ee0f534..b5707ee51 100644 --- a/solstone/think/preflight.py +++ b/solstone/think/preflight.py @@ -31,11 +31,11 @@ from solstone.think.probe import ( check_result_to_json_dict, config_dir_readable_check, disk_space_check, - has_execution_error, local_bin_sol_reachable_check, make_result, platform_tag, python_version_check, + results_failed, run_check, solstone_core_rust_toolchain_check, status_label, @@ -138,8 +138,4 @@ def main(argv: Sequence[str] | None = None) -> int: emit_json(results) else: emit_text(results, verbose=args.verbose) - blocker_failed = any( - result.severity == "blocker" and result.status == "fail" for result in results - ) - execution_failed = any(has_execution_error(result) for result in results) - return 1 if execution_failed or blocker_failed else 0 + return 1 if results_failed(results) else 0 diff --git a/solstone/think/probe.py b/solstone/think/probe.py index 7df30b11c..db2534043 100644 --- a/solstone/think/probe.py +++ b/solstone/think/probe.py @@ -249,17 +249,21 @@ def run_check( def summary_counts(results: Sequence[CheckResult]) -> dict[str, int]: return { "total": len(results), - "failed": sum( - 1 - for result in results - if result.status == "fail" or has_execution_error(result) - ), + "failed": sum(1 for result in results if result.status == "fail"), "warnings": sum(1 for result in results if result.status == "warn"), "skipped": sum(1 for result in results if result.status == "skip"), "errors": sum(1 for result in results if has_execution_error(result)), } +def results_failed(results: Sequence[CheckResult]) -> bool: + return any( + has_execution_error(result) + or (result.severity == "blocker" and result.status == "fail") + for result in results + ) + + def status_label(result: CheckResult) -> str: if has_execution_error(result): return "ERROR" diff --git a/tests/test_preflight.py b/tests/test_preflight.py index 3c6b645be..d20adc6b7 100644 --- a/tests/test_preflight.py +++ b/tests/test_preflight.py @@ -170,6 +170,45 @@ def test_main_isolates_execution_error_and_continues(preflight, monkeypatch, cap ) +def test_main_json_carries_structured_execution_error(preflight, monkeypatch, capsys): + before_check = preflight.Check("before_check", "blocker", ("linux", "darwin")) + raising_check = preflight.Check("raising_check", "advisory", ("linux", "darwin")) + after_check = preflight.Check("after_check", "blocker", ("linux", "darwin")) + + def before(_args): + return preflight.make_result(before_check, "ok", "before complete") + + def raising(_args): + raise RuntimeError("boom") + + def after(_args): + return preflight.make_result(after_check, "ok", "after complete") + + monkeypatch.setattr( + preflight, + "CHECKS", + [ + (before_check, before), + (raising_check, raising), + (after_check, after), + ], + ) + + rc = preflight.main(["--json"]) + payload = json.loads(capsys.readouterr().out) + by_name = {check["name"]: check for check in payload["checks"]} + + assert rc == 1 + assert by_name["raising_check"]["execution_error"] == { + "type": "RuntimeError", + "message": "boom", + } + assert by_name["before_check"]["execution_error"] is None + assert by_name["after_check"]["execution_error"] is None + assert payload["summary"]["errors"] == 1 + assert payload["summary"]["errors"] <= payload["summary"]["failed"] + + def test_python_version_ok(preflight, probe, monkeypatch, tmp_path): repo = make_repo(tmp_path) monkeypatch.setattr(probe, "ROOT", repo) -- 2.51.2