diff --git a/docs/reviews/2026-07-07-review.md b/docs/reviews/2026-07-07-review.md new file mode 100644 index 0000000..54ab1ba --- /dev/null +++ b/docs/reviews/2026-07-07-review.md @@ -0,0 +1,149 @@ +# ScanFlow review — 2026-07-07 + +Reviewed at `b27a4c2` (fresh clone of `main`). Reviewer: Peter + Claude, +read-only — no functional code touched; this document is the deliverable. +The full test suite was run on macOS in mock mode: **292 passed** (10 +preview-panel tests initially failed for an environmental reason — finding +4 below). Focus areas: the safety layer end-to-end (`core/safety.py`, +`core/motion.py`, the runner's poll loop), COM threading, contracts and +sidecars, and the recent churn (tunneling-loss stop, temperature readback, +preview panel). + +## What is genuinely good (keep doing this) + +- **The safety monitor fails closed.** Persistent current-readback failure + escalates to a run abort instead of silently continuing unprotected; + NaN/Inf readings are treated as *failed* readings everywhere (a NaN + passes every `> threshold` comparison as False — the comment in + `measure_current_A` shows the failure mode was understood, not + stumbled past). +- **Closed-loop XY motion.** `set_offset_nm` iterates against the + instrument's own `SCAN.OFFSET.*.NM` readback, so a wrong V/nm + calibration slows convergence instead of producing silently wrong + motion. The motion policy layer (single-move limit, readback + verification, refuse-while-scanning) is the right shape. +- **The runner's lifecycle hygiene**: control-flag reset at run start + (with a comment explaining the stale-escalation failure it prevents), + per-thread COM re-dispatch (`bind_thread`), the wait-for-idle after + stop (the "STMAFM crashed after I clicked Stop" fix), and + `activity.begin/end` to silence background pollers during runs. +- **`tests/test_import_boundaries.py`** — enforcing "contracts are + stdlib-only, core is Qt-free" with tests instead of documentation is + exactly how the layered architecture survives contributors. +- The 0 V constant-current guard, DST suppression for overnight runs, + and the two-stage Stop (graceful → emergency retract) are all + thoughtful operator-facing safety. + +## Findings (severity-ordered) + +### 1. HIGH — Pause suspends safety polling while the scan keeps running + +`AutomationRunner._wait_if_paused` (`automation/runner.py:1463`) blocks in +a plain `time.sleep(0.2)` loop, and it is called from inside the +scan-wait/safety loop (`runner.py:1366`) *before* `_check_safety()`. Pause +is reachable at any moment from the Sweep and Mosaic panels (the Pause +button is never disabled during a scan). Sequence: user pauses mid-scan → +the Createc scan **keeps scanning** (nothing stops it) → the safety loop +is parked in the pause sleep → tip-crash protection is off until Resume. +A crash during a paused-but-scanning stretch is exactly the unattended +scenario the monitor exists for. + +**Fix options:** call `self._check_safety()` inside the pause sleep loop +(minimal, preserves current semantics), and/or make pause defer to the +next step boundary while `scan.is_running` (clearer semantics: "pause +between steps", which is what the runner can actually honour). + +### 2. MED — Tunneling-loss detection is blind on the fallback read path + +When the ADC read fails, `SafetyMonitor.measure_current_A` falls back to +the live frame and returns `max(|I|)` over the **entire partial frame** +(`core/safety.py:137`). For crash detection that maximum is conservative +(good). For the new tunneling-loss check it is the opposite: rows scanned +*before* the tip left the surface keep the frame maximum above +`min_tunnel_current_A`, so `tunnel_lost` can never fire while the monitor +is on the fallback path — precisely the degraded-readback situation where +the tip is most likely to be wandering. Consider evaluating the floor +against the most recent complete rows only, or explicitly documenting +that tunnel-loss protection requires the ADC path. + +### 3. MED — A tunneling-loss halt reports `FINISHED` + +The `tunnel_lost` branch of `_check_safety` (`runner.py:1453`) sets +`_stop_requested = True`; `_execute` returns normally and `run()` ends +with `RunnerState.FINISHED`. The operator (or a supervising script +watching the state) sees a successfully finished run that actually +halted for cause mid-recipe. The `error` signal does fire, but the +terminal state contradicts it. A distinct terminal state (`HALTED`) — or +reusing `ERROR` — would make overnight-run post-mortems unambiguous. + +### 4. MED — Tests hard-require a sibling `ProbeFlow` checkout + +`tests/test_preview_panel.py:23` builds the fixture path as +`parents[2] / "ProbeFlow" / "test_data" / …` — i.e. a ProbeFlow git +checkout **next to** the ScanFlow checkout, with no skip guard. On any +other layout (fresh clone, CI runner, this review) 10 tests fail with +`FileNotFoundError`. Since `probeflow` must already be importable for +these tests, resolve the fixture from the installed package +(`Path(probeflow.__file__).parent.parent / "test_data"`) with +`pytest.skip` fallback, or vendor the one `.dat` fixture (~1 MB) into +ScanFlow's own `test_data/`. + +### 5. LOW — `int(getp(...)) or 9` turns a legitimate 0 into 9 + +`core/safety.py:122`: a preamp gain exponent read of `0` becomes `9` +via the `or`, which would mis-scale the measured current by 10⁹. +Exponent 0 is physically unlikely on this rig, but the idiom is +bug-shaped; an explicit `None` check costs one line. + +### 6. LOW — A scan that never starts is treated as a success + +`_wait_for_scan_with_live_emit` gives the DSP three polls to flip into +SCANNING, then falls through the `while scan.is_running` loop and +returns `True`. If STMAFM rejected the scan (bad parameter, wrong mode), +the step "completes", `_scan_and_save` saves whatever file appears (or +none), and the recipe marches on. Distinguishing "never started" from +"finished" (e.g. require SCANNING to be observed at least once, or check +a scan counter) would catch mis-armed steps. + +### 7. Structure — `preview_panel.py` is 4,157 lines and bypasses the contract + +158 methods, four `QThread` worker classes, and two dialogs in one file; +it imports six ProbeFlow surfaces directly (`analysis.preview`, +`analysis.features`, `analysis.helpers`, `core.scan_loader`, +`processing.geometry`) rather than going through the +detector/planner contract that `docs/long_term_architecture.md` §13 +defines — the reserved `ProbeFlowFeatureDetector` slot is still empty. +Direct imports are legal in the GUI layer per the dependency rules, but +this file is now the de-facto integration point and the single hardest +thing in the repo to review. Suggested seams: workers → +`automation/workers/`, the two dialogs → own modules, detection calls → +the contract. + +Related (action on the ProbeFlow side, noted here for visibility): those +six imports plus the `probeflow --open-survey` CLI flag are an informal +public API. ProbeFlow will grow a pinning test that imports exactly this +surface so refactors over there break CI instead of ScanFlow. + +## Smaller notes + +- `io/sidecar.py` uses `.with_suffix(".scanflow.json")` — correct for + dotted Createc names, and the `.scanflow.json` namespace cannot collide + with ProbeFlow's `.probeflow.json` provenance sidecars. Good. +- `_save_image_preview` (runner.py) imports matplotlib on the runner + thread with the Agg backend — fine, but note it runs `_level_correct` + from `analysis/` inside the control package's process; keep an eye on + import-boundary creep here. +- The recent temperature work (getparam→getp fallback, MEMO poke, + real-channels-only) reads as sound rig-debugging; the poller stands + down during runs via `activity`, consistent with the H7 fix. + +## Reviewer's overall read + +The control core (safety, motion, runner lifecycle, COM handling) is in +better shape than most lab automation code ever gets — the failure modes +that matter were clearly reasoned about, and the comments record *why*. +The risk concentration is (a) the pause/safety interaction in finding 1, +which is a genuine unattended-operation hazard and worth fixing before +the next overnight campaign, and (b) the preview panel, which is growing +into a second application without the architectural guardrails the rest +of the repo enforces on itself.