Merge pull request #88 from OpenFUSIONToolkit/bugfix/review-findings

Fix correctness bugs from the full codebase review

Author: matt-pharr

.github/workflows/ci.yml

@@ -65,14 +65,14 @@ jobs:
         if: ${{ matrix.python == '3.12' }}
         run: uvx ruff format --check .
 
-      - name: Tests (pytest; tolerate "no tests collected" while skeletal)
-        run: |
-          set +e
-          uv run --python ${{ matrix.python }} --with pytest pytest -q
-          code=$?
-          set -e
-          # exit 5 = no tests collected yet (skeleton); treat as success
-          if [ "$code" -ne 0 ] && [ "$code" -ne 5 ]; then exit "$code"; fi
+      # Coverage floor: a regression guard, not a target. Today's suite sits at
+      # ~79% line coverage; 75 leaves headroom for legitimately-untestable code
+      # (live transports, notebook plotting) while catching a big untested
+      # feature landing. Raise it as coverage grows; never lower it to merge.
+      - name: Tests (pytest + coverage floor)
+        run: >
+          uv run --python ${{ matrix.python }} --with pytest --with pytest-cov
+          pytest -q --cov=magnetics --cov-report=term --cov-fail-under=75
 
   typecheck:
     name: Typecheck (ty)

CLAUDE.md

@@ -12,9 +12,9 @@ The full **fetch → process → service → GUI** path runs end-to-end for **bo
   `data/datafile/` (gitignored); read back via `magnetics.data.h5source`. The GUI can trigger a
   pull from the left rail (`PullControl` → `POST /api/fetch`).
 - **Process:** `core/spectral.py` (MODESPEC) is real and pure. The SLCONTOUR quasi-stationary fit
-  runs end-to-end via the reference pipeline in `magnetics._slcontour/` (xarray, self-contained
-  OMFIT shim) adapted by `core/qs_bridge` — real K / χ² / modes for shots pulled with the Bp LFS
-  midplane array. A pure `core/quasistationary` port exists but is not yet wired in production (#40).
+  runs end-to-end via the shim-free `core/qs_*` modules (`qs_io_data` → `qs_prep` → `qs_fit`,
+  adapted to nodes by `qs_bridge`; the former `_slcontour` translation, promoted into core) — real
+  K / χ² / modes for shots pulled with the Bp LFS midplane array.
 - **Service:** `service/app.py` — `GET /api/node/{shot}/{node_id}` serves `kind`-nodes from
   `service/nodes.py`; `/api/machines` lists fetched shots (mock fallback when none).
 - **Nodes / seam:** `nodes.py` forwards GUI query params and serves the core's real `mode_number` /
@@ -31,9 +31,10 @@ The full **fetch → process → service → GUI** path runs end-to-end for **bo
   the rotating/MODESPEC nodes and the Sensors view render those shots too.
 
 Known gaps / open work: per-sensor σ from the data layer (the QS fit currently uses a constant σ;
-helicity is computed from Ip·Bt); finishing the pure `core/quasistationary` port and wiring it in
-place of the `_slcontour` reference pipeline (#40); real equilibrium plotting in the Sensors tab
-(#43); Br saddle-loop geometry corrections (#44).
+helicity is computed from Ip·Bt); real equilibrium plotting in the Sensors tab (#43); the
+xarray-heavy `qs_*` modules are excluded from the `ty` typecheck (see `[tool.ty.src]` in
+pyproject.toml); the cursor analyses (`phase_fit` / mode shape) read from a 1–25 kHz cached
+array spectrum, so modes above 25 kHz aren't phase-fittable at the cursor.
 
 ## The API contract is FLEXIBLE — change it, don't fake around it
 The `kind`-node contract (`core/contracts.py` ⇄ `gui/web/src/lib/contract.ts`, plus the
@@ -59,16 +60,27 @@ rather than fabricating data in the GUI. Keep `contracts.py` and `contract.ts` i
 
 ## Layout
 The Python project **is the repo root** (a uv project, served as a webapp). `src/magnetics/`:
-- `core/` — device-agnostic math (geometry, basis, design metrics, `quasistationary`, `spectral`).
+- `core/` — device-agnostic math: `spectral` (MODESPEC), `mode_shape`, `geometry`, the
+  quasi-stationary fit (`qs_io_data`/`qs_prep`/`qs_fit`/`qs_bridge`/`qs_device`/`qs_run`/
+  `qs_plots`), and the `contracts` node shapes.
 - `data/` — sources + `fetch/` (toksearch/mdsthin pulls, cluster orchestration); device configs
   in `data/device/*.json`.
 - `service/` — FastAPI; the built GUI is bundled at `service/webapp/` and served here.
-- `_slcontour/` — the reference SLCONTOUR translation (self-contained OMFIT shim), pending port
-  into `core/quasistationary` (issue #40); **excluded from lint/typecheck** until then.
 
 Tests in `tests/`, maintainer scripts in `scripts/`. `gui/web/` — React + Vite + TypeScript
 frontend (its `dist/` is staged into `service/webapp/` for the wheel).
 
+## Running the tests
+- **Python:** `uv run pytest` from the repo root — the whole suite is offline and deterministic
+  (~5 s; synthetic fixtures are generated at collection time, no real data needed). Live-network
+  tests (GA gateway / PPPL flux) are env-gated and skip unless `MAGNETICS_GA_USER` /
+  `MAGNETICS_FLUX_USER` is set; they are manual-only, never wired into CI.
+- **Frontend:** `cd gui/web && npm run test` (vitest, one-shot; `npm run test:watch` to iterate).
+- **Everything CI runs:** `uv run ruff format --check .` + `uvx ruff check .` + `uv run pytest`
+  + `uv run ty check src/magnetics` (Python), and `npm run lint` + `npm run typecheck` +
+  `npm run test` + `npm run build` (in `gui/web/`). Run these locally before committing —
+  a plain `pytest` + `tsc` pass does NOT cover everything CI checks.
+
 ## Conventions
 - Physics lives in `src/magnetics/core` (pure, device-agnostic, testable); **no physics in the
   service routes**.

gui/web/src/components/PullControl.tsx

@@ -8,6 +8,7 @@
 // decimation to make a pull seconds instead of minutes.
 import { useEffect, useRef, useState } from "react";
 import { apiBase, startFetch, usingLiveBackend, type DeviceInfo } from "../lib/api";
+import { deviceDefaults } from "../lib/deviceDefaults";
 import { useStore } from "../store";
 
 export default function PullControl() {
@@ -57,21 +58,33 @@ export default function PullControl() {
   const esRef = useRef<EventSource | null>(null);
 
   // Snap backend / sensor-set / window / shot to sensible per-device defaults on a
-  // device change. A tree device (NSTX/KSTAR) has no cluster and no analysis→signal
-  // map, so it uses mdsthin over a NARROW window (its raw signals are ~15 MHz and
-  // seconds long — a wide window is gigabytes). An NSTX-style tree device requires a
-  // named sensor set; KSTAR's transport defaults to its declared arrays when none is
-  // chosen. Called from the device onChange handler — NOT a synchronous effect
-  // (react-hooks/set-state-in-effect).
+  // device change. The decision lives in lib/deviceDefaults.ts (pure + unit-tested,
+  // issue #64); this just applies it to the form state.
   function snapDeviceDefaults(d: DeviceInfo) {
-    const tree = d.access === "mdsplus_tree";
-    setBackend(d.remote_capable ? "remote" : "mdsthin");
-    setSensorSet(tree && !d.needs_ssh_creds ? (d.sensor_sets[0] ?? "") : "");
-    setTmin(tree ? "250" : "1000");
-    setTmax(tree ? "350" : "5000");
-    if (d.default_shot != null) setShot(String(d.default_shot));
+    const def = deviceDefaults(d);
+    setBackend(def.backend);
+    setSensorSet(def.sensorSet);
+    setTmin(def.tmin);
+    setTmax(def.tmax);
+    if (def.shot != null) setShot(def.shot);
   }
 
+  // The store owns `device`, and the left rail's Device picker (App.tsx) changes it
+  // WITHOUT going through this component's select — snapping only in the select's
+  // onChange left the backend/window/sensor-set at the previous device's values (a
+  // rail switch to a tree device then POSTed backend "remote" to a device with no
+  // cluster while the select displayed "mdsthin"). Snap once per device id, from
+  // wherever the change originated.
+  const snappedDevice = useRef<string | null>(null);
+  useEffect(() => {
+    if (!device || snappedDevice.current === device) return;
+    snappedDevice.current = device;
+    const d = devices.find((x) => x.id === device);
+    // eslint-disable-next-line react-hooks/set-state-in-effect -- per-device defaults must follow an external (rail) device change
+    if (d) snapDeviceDefaults(d);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [device, devices]);
+
   // close any open pull stream when the component unmounts
   useEffect(() => () => esRef.current?.close(), []);
   // Device is owned by the store; derive its capabilities. A tree device (NSTX/KSTAR)
@@ -150,11 +163,7 @@ export default function PullControl() {
       <h3>Pull a shot (live)</h3>
       {devices.length > 0 && (
         <select className="pull-input" value={device} aria-label="device"
-          onChange={(e) => {
-            const d = devices.find((x) => x.id === e.target.value);
-            setDevice(e.target.value);
-            if (d) snapDeviceDefaults(d);
-          }}>
+          onChange={(e) => setDevice(e.target.value)}>
           {devices.map((d) => (
             <option key={d.id} value={d.id}>{d.name}</option>
           ))}