[KreutzM]

Summary

fractal-doctor no longer fails merely because CUDA is missing. It is now a full installation health check that separates required core health from optional capabilities:

• Core (required, ERROR => exit 1): supported Python 3.11–3.13, NumPy, Numba, Pillow, mpmath (import + version), a real CPU renderer test frame, Tkinter.
• Optional (WARNING at most, exit stays 0): FFmpeg (MP4 export) and CUDA (acceleration), with GPU/driver/compute-capability details.
• Per-check states OK / WARNING / ERROR; human-readable output groups core vs optional explicitly.
• --json emits a stable schema: schema_version, tool, exit_code, core_ok, summary{ok,warning,error}, checks[{key,label,state,required,detail,data}].
• CUDA probing still goes through gpu_info.inspect_cuda() — no duplicated detection; the CUDA-Details block reuses CudaStatus.report().
• FFmpeg is probed as an executable, not just a PATH entry: ProbeEnv.probe_ffmpeg() delegates to ffmpeg_mp4.probe_ffmpeg() (ffmpeg -version). A stale/broken binary on PATH reports WARNING with the probe error instead of a false OK (review finding, addressed in a49c6f6).

Issue

Closes #1

Design / Decisions

• Tkinter is core (TKINTER_IS_CORE = True, documented in doctor.py): the desktop GUI fractal-studio is the primary entry point of this repository, so a missing Tkinter is a real core failure for the shipped application; the detail message states that offline rendering remains possible. The constant makes the opposite policy a one-line change if desired.
• Environment probing (run_health_check(env)) is separated from CLI presentation (main, format_report) via a small ProbeEnv protocol, so every acceptance scenario is unit-testable on a headless box without GPU/FFmpeg/display. No new runtime dependency.
• Exit-code contract: 0 = core usable (even without CUDA and FFmpeg), 1 = core failure. Launcher scripts (run_windows.ps1, enable_cuda.ps1) keep working — they already treat a non-zero doctor exit as a CUDA warning, which remains correct under the new contract.
• format_report tolerates reports without a CUDA check (next(..., None) guard) because it is a public function taking any HealthReport.

Validation

• pytest -q: 348 passed (17 tests/test_doctor.py acceptance/coverage cases + existing test_gpu_info.py coverage), on Linux with the repo venv (Python 3.11.16, NumPy/Numba/Pillow/mpmath installed). CI additionally covers Windows 3.11–3.13.
• scripts/check_pan_stability.py --backend cpu: STABLE.
• Live CLI smoke on this WSL box: human output exits 0 with CUDA=WARNING and FFmpeg=OK (version line captured from a real ffmpeg -version run); --json parses with the schema above.
• Real broken-binary smoke: a PATH-shadowing ffmpeg that exits non-zero is reported WARNING ... aber nicht verwendbar with exit 0 — verifies the review fix outside the fake-env unit tests.
• Not run: physical CUDA/GPU validation (no product code outside diagnostics changed), Windows PowerShell launcher smoke (Linux dev box), GUI smoke (no GUI code changed). CI covers Windows pytest for 3.11–3.13.

Remaining / Follow-ups

• None blocking. Review findings disposition is documented in the PR comment "Review findings disposition" (2 MAJOR and 4 MINOR fixed in a49c6f6; 3 NOTE items evaluated and deliberately kept).
• A future enhancement could surface the full doctor summary in the GUI diagnostics dialog, but that is out of scope for Improve fractal-doctor into a full installation health check #1.

Handoff

• Branch agent/1-fractal-doctor-health-check, head a49c6f6 (review fixes) on top of 0beb1c9 (initial implementation), base main@e1219d7.
• Files: src/fractal_flight_studio/doctor.py (rewritten, probe/CLI split + FFmpeg executable probe), tests/test_doctor.py (new, 17 tests), CHANGELOG.md, README.md (GPU-Diagnose section).
• Next step for a reviewer/agent: re-run pytest -q if desired; merge only with explicit user approval.

[KreutzM]

External review: overall implementation matches Issue #1 well and CI is green across Ubuntu/Windows on Python 3.11–3.13 plus GUI smoke, pan stability, and wheel build. One MINOR finding before merge: check_ffmpeg() currently treats shutil.which("ffmpeg") as sufficient for OK. The repository already has ffmpeg_mp4.probe_ffmpeg() / resolve_ffmpeg(), which actually verifies that the executable can run and returns a valid version line. A stale/broken executable on PATH would currently be reported healthy by fractal-doctor even though MP4 export would fail. Please reuse/provide equivalent executable probing through the injectable probe layer and add a deterministic test for “ffmpeg path exists but probe fails => WARNING, exit remains 0”. No other blocking findings.

[KreutzM]

Independent Review of PR #9 — fractal-doctor health check

Overall assessment

The PR correctly addresses Issue #1 requirements. The architecture (ProbeEnv protocol, separation of probing from presentation, reuse of gpu_info.py, per-check OK/WARNING/ERROR states, exit-code contract, stable JSON schema) is sound and well-documented. Tests are passing (11/11 in test_doctor.py) and the design is unit-testable without a GPU or FFmpeg.

That said, I found 2 MAJOR, 4 MINOR, and 3 NOTE items. The most important is a diagnostic correctness gap in FFmpeg probing that the existing owner review (KreutzM) also identified.

I concur with KreutzM that the FFmpeg probe issue should be addressed before merge. I escalate its severity to MAJOR because a usable-health-check that reports a stale/broken executable as OK is a false-positive diagnostic, contrary to the doctor's stated purpose.

---

MAJOR — check_ffmpeg() only tests PATH presence, not executable health

Evidence: check_ffmpeg() (doctor.py:235–255) calls env.find_executable("ffmpeg"), which delegates to shutil.which(). This only checks whether an ffmpeg binary is on PATH — it does not verify that the binary actually runs. A broken, stale, or mismatched-arch FFmpeg executable on PATH is reported as [ OK ], but any subsequent MP4 export would fail with an opaque error.

The repository already has proven FFmpeg probing infrastructure:

• ffmpeg_mp4.resolve_ffmpeg() (ffmpeg_mp4.py:128–142) — same shutil.which() call but integrated into a proper error chain
• ffmpeg_mp4.probe_ffmpeg() (ffmpeg_mp4.py:145–169) — actually runs ffmpeg -version and validates it returns a version line with exit 0

The ProbeEnv protocol makes injecting a proper probe straightforward. A probe_ffmpeg() method should be added to the protocol, implemented in SystemProbeEnv via ffmpeg_mp4.probe_ffmpeg() (with FFmpegProbeError → WARNING), and faked in FakeEnv to support a new ffmpeg_broken parameter.

Remediation:

1. Add probe_ffmpeg() -> str | None (or similar) to ProbeEnv
2. Implement in SystemProbeEnv — catch FFmpegNotFoundError and FFmpegProbeError, return None for either
3. Update check_ffmpeg() to call the probe
4. Add a test: "ffmpeg path exists but probe fails → WARNING, exit 0"

---

MAJOR — format_report next() will crash on reports without a CUDA check

Evidence: format_report() line 349:

cuda_check = next(c for c in report.checks if c.key == "cuda")

This raises StopIteration if no check has key "cuda". While run_health_check() always appends a CUDA check, format_report is a public function that accepts any HealthReport. A caller constructing a report manually or an evolution of run_health_check that conditionally skips the CUDA check would crash silently.

Remediation: Use next(..., None) and guard:

cuda_check = next((c for c in report.checks if c.key == "cuda"), None)
if cuda_check and cuda_check.data:
    ...

---

MINOR — PR description claims 12 new tests, only 11 exist

Evidence: tests/test_doctor.py contains exactly 11 test_* functions (confirmed by pytest verbose output and file read). The PR description states "12 new tests/test_doctor.py cases." Minor inaccuracy.

Remediation: Update the PR description to say 11.

---

MINOR — "\Installation vollständig gesund" output path is untested

Evidence: format_report() lines 362–363 have a branch for the case where report.core_ok and not report.has_warnings (the entirely-healthy scenario): "Ergebnis: Installation vollständig gesund (Exit 0)." None of the 11 doctor tests exercise this path — every test either includes at least one WARNING or mocks a core failure.

Additionally, HealthReport.core_ok, has_errors, and has_warnings are public properties with no direct unit tests.

Remediation: Add a test with a CUDA-available + FFmpeg-found environment (zero warnings) that asserts the "\vollständig gesund" substring is present.

---

MINOR — "\Versionsnummer unbekannt" check_package path is untested

Evidence: When a module imports successfully but env.package_version() returns None and the module itself has no __version__ attribute, check_package() (doctor.py) emits "( Versionsnummer unbekannt )" in the detail. This edge case has no test.

Remediation: Add a FakeEnv variant that returns a module without __version__ and a package_version() that returns None, and assert the "unbekannt" message.

---

MINOR — check_package non-ImportError exception path is untested

Evidence: The except Exception as exc: clause (doctor.py:169) catches more than ImportError — it catches any runtime surprise from a broken module import (correct defensive choice). But the test only exercises the missing-package (ImportError) path, not a scenario where import_module raises a different exception.

Remediation: Add a FakeEnv mode where import_module raises ValueError (or similar) for a core package, and assert the check produces STATE_ERROR.

---

NOTE — CUDA-Details block always shown, even when CUDA is unavailable

Both branches of check_cuda() populate data via asdict(status), and a CudaStatus with available=False still has all fields set (some to None), so cuda_check.data is always truthy. The CUDA-Details section therefore appears even on a CUDA-less machine, showing driver/NVIDIA-smi context that may be irrelevant to the user. This is informative rather than wrong, but could be surprising.

---

NOTE — JSON CLI test uses monkeypatch instead of the real main path

test_json_cli_flag_emits_valid_json monkeypatches run_health_check at module level. This tests the argparse wiring and JSON serialization but not the full main() → SystemProbeEnv chain. Acceptable per the headless-test constraint, but worth noting that the real probe path is only covered by live smoke, not CI.

---

NOTE — No test with the real SystemProbeEnv

All 11 tests use FakeEnv; the real SystemProbeEnv path (actual CPU renderer, actual import resolution, actual shutil.which/inspect_cuda) is exercised only during ad-hoc smoke testing. The PR description correctly flags this ("Not run: physical CUDA/GPU validation"). A test exercising run_health_check() without arguments (defaulting to SystemProbeEnv) is not currently viable on headless CI, but the requirement is documented transparently.

---

Conclusion

The architecture and core implementation are solid. The FFmpeg probe gap is the only finding that would materially mislead a user and should be fixed before merge. The other issues are test-coverage gaps and minor robustness concerns that could be addressed in follow-up.

[KreutzM]

Review findings disposition — commit a49c6f6 (branch agent/1-fractal-doctor-health-check)

Every finding from the review (external review COMMENTED + the independent-review PR comment) was evaluated independently against the code. All fixes are pushed; validation: pytest 348 passed (17 doctor tests), check_pan_stability STABLE, live CLI + real broken-binary smoke on Linux.

MAJOR 1 — check_ffmpeg() only tests PATH presence: VALID, FIXED.
Agree with escalation: a health check that reports a broken binary as OK is a false-positive diagnostic. ProbeEnv now has probe_ffmpeg(executable); SystemProbeEnv delegates to ffmpeg_mp4.probe_ffmpeg() (runs ffmpeg -version, reuses resolve_ffmpeg()), so any FFmpegNotFoundError/FFmpegProbeError (and any other probe exception) yields WARNING with the probe error in detail and data.probe_error, exit stays 0. The OK path now includes the real version line in detail/data.version_line. FakeEnv gained ffmpeg_probe_ok; tests test_broken_ffmpeg_binary_is_warning_not_failure and test_working_ffmpeg_reports_version_line cover both branches. Also smoke-verified end-to-end with a real PATH-shadowing broken ffmpeg (WARNING, exit 0) and the real system ffmpeg (OK, version 4.4.2 line captured).

MAJOR 2 — format_report next() StopIteration: VALID, FIXED.
format_report is public and takes any HealthReport, so the unguarded next() was a latent crash for hand-built reports. Now next((...), None) with a None-and-data guard; regression test test_format_report_tolerates_report_without_cuda_check builds a CUDA-less report and asserts no crash and no "CUDA-Details:" section.

MINOR 1 — PR claims 12 tests, 11 existed: VALID, FIXED.
PR description now says 17 (11 original + 6 added with the review fixes) and matches pytest tests/test_doctor.py output exactly.

MINOR 2 — "vollständig gesund" path untested: VALID, FIXED.
test_fully_healthy_installation_reports_no_warnings uses CUDA-available + working-FFmpeg (zero warnings), asserts the substring, absence of "[WARNUNG]", and directly exercises the previously untested HealthReport.core_ok / has_warnings / has_errors properties. core_ok/has_errors are additionally covered by the other new tests.

MINOR 3 — "Versionsnummer unbekannt" untested: VALID, FIXED.
FakeEnv(versionless_packages=...) returns a module without __version__ and no metadata version; test_versionless_package_reports_unknown_version asserts the detail text, data.version is None, and that the check stays OK (version is informational — a package that imports is usable).

MINOR 4 — non-ImportError package path untested: VALID, FIXED.
FakeEnv(broken_imports=...) makes import_module raise ValueError; test_non_import_error_package_failure_is_core_error asserts STATE_ERROR + required, matching the defensive except Exception in check_package.

NOTE 1 — CUDA-Details always shown: EVALUATED, KEPT AS-IS (deliberate).
Issue #1 asks the doctor to report CUDA availability as an optional capability; showing why CUDA is unavailable (nvidia-smi found/not found, Numba/numba-cuda versions) is precisely the actionable information a user needs to decide whether to run enable_cuda.ps1 — the "GPU seen, CUDA path inactive" case in particular is only explainable through that block. Hiding it on CUDA-less machines would remove the most useful diagnostics. Kept; the check row itself already clearly labels this as an optional WARNING, not an error.

NOTE 2 — JSON CLI test monkeypatches run_health_check: EVALUATED, KEPT AS-IS.
The test targets exactly the seam it should: argparse wiring + JSON serialization of the report contract. Patching run_health_check is legitimate because the issue forbids requiring real GPU/FFmpeg/display in automated tests; the unpatched SystemProbeEnv chain is additionally covered by mandatory live smoke (this comment's validation and the prior owner smoke), which is the strongest available CI-honest coverage.

NOTE 3 — no test with the real SystemProbeEnv: EVALUATED, KEPT AS-IS.
Same constraint as NOTE 2: a headless-safe test of run_health_check() with zero arguments would assert on host-dependent state (this box's FFmpeg, Python patch level, WSL nvidia-smi quirk) and produce flaky CI on GitHub's Windows runners. The PR description flags this explicitly as ad-hoc-smoke-only; that transparency is the right resolution rather than a fake assertion of host state.

Docs updated with the behavior change: CHANGELOG.md (FFmpeg executable-health probe bullet) and README.md GPU-Diagnose section.

Not merged, per policy; ready for re-review.

[KreutzM]

Re-review of PR #9 — fractal-doctor health check (Round 2)

Commit under review: a49c6f6
Previous review: 0beb1c9 (2 MAJOR, 4 MINOR, 3 NOTE)
Validation: 348/348 tests pass (17 doctor-specific), full regression clean

---

VERIFIED FIXED — 6 of 6 actionable findings

MAJOR — check_ffmpeg() PATH-only, no executable health probe — VERIFIED FIXED.
ProbeEnv.probe_ffmpeg() runs ffmpeg -version via ffmpeg_mp4. check_ffmpeg catches any Exception to WARNING. Test covers both probe_ok and probe_broken branches. The real FFmpeg infrastructure is reused appropriately.

MAJOR — format_report next() StopIteration on CUDA-less report — VERIFIED FIXED.
next((...), None) + cuda_check is not None guard. Test builds CUDA-less HealthReport, asserts no crash and no CUDA-Details section.

MINOR — PR claimed 12 tests, 11 existed — VERIFIED FIXED.
Now 17 (11 original + 6 new). Confirmed: pytest lists exactly 17 test functions. PR body updated.

MINOR — "vollständig gesund" path untested — VERIFIED FIXED.
test_fully_healthy_installation_reports_no_warnings covers zero-warning path, asserts substring and the core_ok/has_warnings/has_errors properties directly.

MINOR — "Versionsnummer unbekannt" untested — VERIFIED FIXED.
test_versionless_package_reports_unknown_version covers the edge case where a module imports but has neither version nor metadata version. Check remains OK (version is informational).

MINOR — non-ImportError exception path untested — VERIFIED FIXED.
test_non_import_error_package_failure_is_core_error covers the ValueError path through the defensive except Exception in check_package.

The 3 NOTE items (CUDA-Details always shown, JSON CLI monkeypatch, no real SystemProbeEnv test) were EVALUATED AND KEPT. I concur with the published rationale.

---

NEW FINDINGS — 0

No regressions, new defects, architecture violations, or misleading diagnostics found. Specifically verified in the review-fix delta (a49c6f6):

• check_ffmpeg exception safety: Any probe failure degrades gracefully to WARNING — appropriate for a diagnostic tool.
• Double shutil.which() call (find_executable then resolve_ffmpeg): Harmless redundancy, no defect.
• Lazy import of ffmpeg_mp4 inside SystemProbeEnv.probe_ffmpeg: correct — no import cost when FFmpeg is not on PATH.
• format_report CUDA guard: correctly handles both nonexistent check and check with empty data.
• Documentation: CHANGELOG.md and README.md updated.
• Full regression: 348 tests pass. Only test_doctor.py was modified for coverage.

---

VERDICT: READY TO MERGE

All 6 previous findings are independently verified as correctly fixed. Zero unresolved issues. Zero new findings. Tests pass, documentation updated. The PR makes fractal-doctor a correct, robust, and testable health check.

(I will not merge; approval requires explicit user authorization per policy.)