2026-07-07 review: safety-loop findings, test portability, preview panel

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.