From 059790362d17be709f531d3614d8f26c22ed8437 Mon Sep 17 00:00:00 2001 From: P S Kesavan Date: Sun, 6 Sep 2026 02:08:47 +0530 Subject: [PATCH] fix: refuse to start a run on an uncalibrated embryo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing checked. `POST /api/devices/timelapse/start` validated `interval_seconds > 0` and that the embryo ids existed, and started. The acquisition layer did not fail either. It invented the geometry, and said so in a comment: # Get calibration parameters (use defaults if not calibrated) galvo_amplitude = cal.get("galvo_amplitude", 0.5) galvo_center = cal.get("galvo_center", 0.0) piezo_amplitude = cal.get("piezo_amplitude", 25.0) piezo_center = cal.get("piezo_center", 50.0) slope = cal.get("slope_um_per_deg", 100.0) So an adaptive timelapse could run to completion on embryos that had never been calibrated, on five literals, and report success. The resulting volumes are indistinguishable from real ones, which is the whole problem — a refusal gets noticed, silently invented scan geometry does not. It also inverted the ordering the team stated out loud on 2026-08-07. Ryan: "the main thing is just making sure that we can get the calibration to work". Kesavan: "Calibration has to work. Embryo navigation has to work. Then timelapse setup has to work." The workflow has a hard dependency; now the code has one too. `gently/harness/calibration_gate.py` holds the single predicate, so the answer cannot differ between the surface an operator drives and the tool an agent calls. Calibrated means a finite non-zero `slope_um_per_deg` — the field every successful fit writes (`calibration_tools.py:891`) and the number the scan geometry is derived from. Guarded: `timelapse/start`, `operate/run-tactic`, `acquire/volume`, and the `acquire_volume` agent tool. Server-side on purpose: a check in operate.js is a check the agent walks past. 409 rather than 400 — the request is well formed, the instrument is not in a state to honour it. JUDGEMENT CALLS, flagged for reversal: 1. **Refuse rather than warn**, with `allow_uncalibrated=true` as an explicit escape. Someone who means it can say so; the point is that they have to say it. A warning in a toast is not a decision anyone records. 2. **Fit quality is not judged.** `r_squared` is hardcoded to 0.85 on the vision-guided path (`calibration_tools.py:903`), so it is not currently a measurement — a threshold against a literal would be theatre. Presence is checked; quality is left for the operator. Worth its own issue. 3. **`embryo_id` on `acquire/volume` is optional.** Manual-mode snapping images the current stage position with no embryo in mind, which is legitimate and has nothing to check against. Operate's single mode now sends it, so that path is covered. Which incidentally closes half of the audit's finding 5: the route previously received no embryo id at all, so it imaged wherever the stage happened to be and could not have checked anything even if it had wanted to. 21 tests: the predicate against partial calibrations, zero and junk slopes, and the routes against the refusal, the override, the null-roster case, and that a bad interval is still a 400 rather than masked by the gate. Refs the audit in docs/devices-tab-audit.md. Co-Authored-By: Claude Opus 5 (1M context) --- gently/app/tools/acquisition_tools.py | 18 +++- gently/harness/calibration_gate.py | 86 +++++++++++++++++ gently/ui/web/routes/data.py | 35 +++++++ gently/ui/web/static/js/operate.js | 7 ++ tests/test_calibration_gate.py | 127 +++++++++++++++++++++++++ tests/test_calibration_gate_routes.py | 130 ++++++++++++++++++++++++++ 6 files changed, 401 insertions(+), 2 deletions(-) create mode 100644 gently/harness/calibration_gate.py create mode 100644 tests/test_calibration_gate.py create mode 100644 tests/test_calibration_gate_routes.py diff --git a/gently/app/tools/acquisition_tools.py b/gently/app/tools/acquisition_tools.py index bfc892ea..204d210e 100644 --- a/gently/app/tools/acquisition_tools.py +++ b/gently/app/tools/acquisition_tools.py @@ -11,6 +11,7 @@ import numpy as np +from gently.harness import calibration_gate from gently.harness.tools.helpers import ctx_get, get_embryo_or_error from gently.harness.tools.registry import ToolCategory, ToolExample, tool @@ -103,9 +104,17 @@ async def acquire_volume( num_slices: int = 50, exposure_ms: float = 10.0, z_buffer_um: float | None = None, + allow_uncalibrated: bool = False, context: dict | None = None, ) -> str: - """Acquire single volume - moves to embryo first, uses calibration""" + """Acquire single volume - moves to embryo first, uses calibration. + + Refuses an uncalibrated embryo unless ``allow_uncalibrated``. The four + galvo/piezo parameters below used to fall back to literals with the comment + "use defaults if not calibrated" — so a volume acquired from an embryo that + had never been calibrated used invented geometry and reported success. The + resulting data is indistinguishable from real data, which is the problem. + """ agent = ctx_get(context, "agent") client = ctx_get(context, "client") @@ -116,13 +125,18 @@ async def acquire_volume( if err: return err + if not allow_uncalibrated and not calibration_gate.is_calibrated(embryo): + return f"Error: {calibration_gate.refusal_detail([embryo_id])}" + try: # Move to embryo position first pos = embryo.stage_position if pos and pos.get("x") is not None and pos.get("y") is not None: await client.move_to_position(pos["x"], pos["y"]) - # Get calibration parameters (use defaults if not calibrated) + # Calibrated by the time we get here, unless the caller explicitly + # accepted a guess. The literals below are that guess, and they are + # only ever reached on that path. cal = embryo.calibration or {} galvo_amplitude = cal.get("galvo_amplitude", 0.5) galvo_center = cal.get("galvo_center", 0.0) diff --git a/gently/harness/calibration_gate.py b/gently/harness/calibration_gate.py new file mode 100644 index 00000000..b4a98d38 --- /dev/null +++ b/gently/harness/calibration_gate.py @@ -0,0 +1,86 @@ +"""Has this embryo actually been calibrated? + +One predicate, used by the run routes and by `acquire_volume`, so the answer +cannot differ between the surface an operator drives and the tool an agent +calls. + +WHY THIS EXISTS + +Nothing checked. `POST /api/devices/timelapse/start` validated +`interval_seconds > 0` and that the embryo ids existed, and started. And the +acquisition layer did not fail on an uncalibrated embryo either — it invented a +complete set of scan geometry, with the comment saying so: + + # Get calibration parameters (use defaults if not calibrated) + galvo_amplitude = cal.get("galvo_amplitude", 0.5) + galvo_center = cal.get("galvo_center", 0.0) + piezo_amplitude = cal.get("piezo_amplitude", 25.0) + piezo_center = cal.get("piezo_center", 50.0) + slope = cal.get("slope_um_per_deg", 100.0) + +So a timelapse could run to completion on embryos that had never been +calibrated, using five made-up numbers, and report success. The data looks +exactly like real data. That is worse than a refusal, because a refusal is +noticed. + +It also inverted the order the team stated out loud on 2026-08-07 — Ryan: "the +main thing is just making sure that we can get the calibration to work"; +Kesavan: "Calibration has to work. Embryo navigation has to work. Then +timelapse setup has to work." The workflow has a hard dependency; now the code +has one too. + +WHAT COUNTS AS CALIBRATED + +A finite `slope_um_per_deg` in `embryo.calibration`. Every successful fit +writes it (`calibration_tools.py:891`), and it is the number the scan geometry +is derived from — without it there is nothing to derive from but a guess. + +Fit *quality* is deliberately not judged here. `r_squared` is hardcoded to 0.85 +on the vision-guided path, so it is not currently a measurement, and inventing +a threshold against a literal would be theatre. Presence is checked; quality is +reported for the operator to judge. +""" + +from __future__ import annotations + +from typing import Any + +# The one field that means a fit happened. +FIT_KEY = "slope_um_per_deg" + + +def is_calibrated(embryo: Any) -> bool: + """True when this embryo carries a usable galvo→piezo fit.""" + cal = getattr(embryo, "calibration", None) or {} + if not isinstance(cal, dict): + return False + slope = cal.get(FIT_KEY) + try: + value = float(slope) # type: ignore[arg-type] + except (TypeError, ValueError): + return False + # A zero slope is not a calibration either: the scan would have no extent. + return value == value and value not in (float("inf"), float("-inf")) and value != 0.0 + + +def uncalibrated(embryos: dict[str, Any], embryo_ids: list[str] | None) -> list[str]: + """Which of `embryo_ids` have no fit. `None` means every known embryo. + + Unknown ids are not reported here — the routes already reject those, and + conflating "does not exist" with "not calibrated" would send an operator + to the wrong fix. + """ + ids = list(embryo_ids) if embryo_ids else list(embryos.keys()) + return [eid for eid in ids if eid in embryos and not is_calibrated(embryos[eid])] + + +def refusal_detail(missing: list[str]) -> str: + """The message an operator gets, naming the embryos and the way past it.""" + names = ", ".join(missing) + return ( + f"not calibrated: {names}. " + "An uncalibrated embryo has no galvo/piezo fit, so the scan geometry " + "would be invented rather than measured. Calibrate them, or pass " + "allow_uncalibrated=true to acquire anyway and accept that the volume " + "geometry is a guess." + ) diff --git a/gently/ui/web/routes/data.py b/gently/ui/web/routes/data.py index f0ed9a0b..759968a2 100644 --- a/gently/ui/web/routes/data.py +++ b/gently/ui/web/routes/data.py @@ -7,6 +7,7 @@ import yaml from fastapi import APIRouter, Body, Depends, HTTPException +from gently.harness import calibration_gate from gently.ui.web.auth import require_control logger = logging.getLogger(__name__) @@ -100,6 +101,30 @@ def _require_agent_with_experiment(): raise HTTPException(status_code=503, detail="Agent not ready") return agent + def _require_calibrated(embryo_ids, payload): + """Refuse to start unless every named embryo carries a real fit. + + Server-side on purpose: the agent reaches these routes too, and a check + that lives in operate.js is a check the agent walks past. See + gently/harness/calibration_gate.py for why an uncalibrated run is worse + than a refused one — the acquisition layer invents five numbers and + reports success. + + `allow_uncalibrated: true` still goes through. Someone who means it + should be able to say so; the point is that they have to say it. + """ + if payload.get("allow_uncalibrated"): + return + bridge = getattr(server, "agent_bridge", None) + agent = bridge.agent if bridge is not None else None + experiment = getattr(agent, "experiment", None) if agent else None + embryos = getattr(experiment, "embryos", None) + if not embryos: + return # nothing known to check against + missing = calibration_gate.uncalibrated(embryos, embryo_ids) + if missing: + raise HTTPException(status_code=409, detail=calibration_gate.refusal_detail(missing)) + @router.put("/api/embryos/{embryo_id}/position", dependencies=[Depends(require_control)]) async def update_embryo_position( embryo_id: str, @@ -1389,6 +1414,7 @@ async def operate_run_tactic(payload: dict = Body(...)): # noqa: B008 ) agent = _require_agent_with_experiment() + _require_calibrated(payload.get("embryo_ids") or None, payload) tactic = payload.get("tactic") lib_id = payload.get("library_id") if tactic is None and lib_id: @@ -1540,6 +1566,12 @@ async def acquire_volume(payload: dict = Body(...)): # noqa: B008 can send "ALL OFF" for brightfield-safe Manual-view captures. piezo_center and galvo_center capture at the dialled focal plane. """ + # Optional on purpose: Manual-mode snapping images the current stage + # position with no embryo in mind, which is legitimate. When a caller + # DOES name an embryo, it gets checked. + eid = payload.get("embryo_id") + if eid: + _require_calibrated([str(eid)], payload) client = _resolve_client() if client is None: raise HTTPException(status_code=503, detail="Microscope not connected") @@ -1617,6 +1649,9 @@ async def timelapse_start(payload: dict = Body(...)): # noqa: B008 stop_condition = str(payload.get("stop_condition") or "manual") embryo_ids = payload.get("embryo_ids") or None + # A timelapse is the longest-running thing this UI starts; an invented + # scan geometry would be baked into every timepoint of it. + _require_calibrated(embryo_ids, payload) condition_value = payload.get("condition_value") monitoring_mode = payload.get("monitoring_mode") or None diff --git a/gently/ui/web/static/js/operate.js b/gently/ui/web/static/js/operate.js index a468f93c..9c8775ea 100644 --- a/gently/ui/web/static/js/operate.js +++ b/gently/ui/web/static/js/operate.js @@ -1151,6 +1151,13 @@ const OperateManager = (function () { _acquiring = true; renderSubnavMeta(); try { await postJSON('/api/devices/acquire/volume', { + // Say WHICH embryo. The route used to receive no id at + // all, so it imaged wherever the stage happened to be + // and could not check whether that embryo had ever been + // calibrated. Manual-mode snapping still omits it on + // purpose — a test shot at the current position is a + // real thing to want. + embryo_id: _selected, num_slices: Math.max(1, Number(($('op-vol-slices') || {}).value) || 50), exposure_ms: Math.max(1, Number(($('op-vol-exp') || {}).value) || 10), }); diff --git a/tests/test_calibration_gate.py b/tests/test_calibration_gate.py new file mode 100644 index 00000000..9fbc1a90 --- /dev/null +++ b/tests/test_calibration_gate.py @@ -0,0 +1,127 @@ +"""A run must not start on an uncalibrated embryo. + +Nothing checked. `POST /api/devices/timelapse/start` validated +`interval_seconds > 0` and that the ids existed, and started — and the +acquisition layer did not fail either, it invented the scan geometry: + + # Get calibration parameters (use defaults if not calibrated) + galvo_amplitude = cal.get("galvo_amplitude", 0.5) + ... + slope = cal.get("slope_um_per_deg", 100.0) + +So a timelapse could run to completion on embryos that had never been +calibrated, using five literals, and report success. The output is +indistinguishable from real data, which is why a refusal is the kinder answer. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from typing import Any + +from gently.harness import calibration_gate + + +@dataclass +class _Embryo: + calibration: dict[str, Any] = field(default_factory=dict) + + +def _fit(slope: float = 42.0) -> dict[str, Any]: + """The shape a successful fit writes (calibration_tools.py:891).""" + return {"slope_um_per_deg": slope, "offset_um": 1.0, "r_squared": 0.85} + + +# ── is_calibrated ──────────────────────────────────────────────────────────── + + +def test_a_real_fit_counts() -> None: + assert calibration_gate.is_calibrated(_Embryo(_fit())) is True + + +def test_an_empty_calibration_does_not() -> None: + assert calibration_gate.is_calibrated(_Embryo({})) is False + + +def test_scan_parameters_without_a_fit_do_not_count() -> None: + """The exact trap: the literals the old code substituted are not a fit. + + An embryo carrying galvo/piezo values but no slope has never been + calibrated — those are the invented defaults, not a measurement. + """ + partial = { + "galvo_amplitude": 0.5, + "galvo_center": 0.0, + "piezo_amplitude": 25.0, + "piezo_center": 50.0, + } + assert calibration_gate.is_calibrated(_Embryo(partial)) is False + + +def test_a_zero_slope_is_not_a_calibration() -> None: + """Zero slope means the scan has no extent — a fit that failed, not one.""" + assert calibration_gate.is_calibrated(_Embryo(_fit(0.0))) is False + + +def test_junk_in_the_slope_is_not_a_calibration() -> None: + junk: list[Any] = [None, "", "n/a", float("nan"), float("inf"), [], {}] + for bad in junk: + assert calibration_gate.is_calibrated(_Embryo({"slope_um_per_deg": bad})) is False, bad + + +def test_a_missing_calibration_attribute_does_not_raise() -> None: + class Bare: + pass + + assert calibration_gate.is_calibrated(Bare()) is False + + +# ── uncalibrated() ─────────────────────────────────────────────────────────── + + +def _roster() -> dict[str, _Embryo]: + return { + "e1": _Embryo(_fit()), + "e2": _Embryo({}), + "e3": _Embryo(_fit()), + "e4": _Embryo({"galvo_center": 0.0}), + } + + +def test_reports_only_the_uncalibrated_ones() -> None: + assert calibration_gate.uncalibrated(_roster(), ["e1", "e2", "e3", "e4"]) == ["e2", "e4"] + + +def test_none_means_every_known_embryo() -> None: + """`embryo_ids: null` on the route means "all active embryos".""" + assert calibration_gate.uncalibrated(_roster(), None) == ["e2", "e4"] + + +def test_a_fully_calibrated_subset_is_allowed_through() -> None: + assert calibration_gate.uncalibrated(_roster(), ["e1", "e3"]) == [] + + +def test_unknown_ids_are_not_reported_as_uncalibrated() -> None: + """ "Does not exist" and "not calibrated" send an operator to different fixes. + + The routes already reject unknown ids; conflating the two here would tell + someone to calibrate an embryo that is not there. + """ + assert calibration_gate.uncalibrated(_roster(), ["e1", "ghost"]) == [] + + +def test_an_empty_roster_reports_nothing() -> None: + assert calibration_gate.uncalibrated({}, None) == [] + assert calibration_gate.uncalibrated({}, ["e1"]) == [] + + +# ── the message ────────────────────────────────────────────────────────────── + + +def test_the_refusal_names_the_embryos_and_the_way_past_it() -> None: + msg = calibration_gate.refusal_detail(["e2", "e4"]) + assert "e2" in msg and "e4" in msg + # An operator at a microscope needs the remedy in the message, not in a + # dependency file or a route signature. + assert "allow_uncalibrated" in msg + assert "invented" in msg or "guess" in msg diff --git a/tests/test_calibration_gate_routes.py b/tests/test_calibration_gate_routes.py new file mode 100644 index 00000000..49720aaf --- /dev/null +++ b/tests/test_calibration_gate_routes.py @@ -0,0 +1,130 @@ +"""The calibration gate must live on the routes, not in the browser. + +The agent reaches these endpoints too, so a check in `operate.js` is a check +the agent walks past. These tests pin the refusal to the HTTP boundary. + +409 rather than 400: the request is well formed, the instrument is simply not +in a state where it can be honoured. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from typing import Any +from unittest.mock import AsyncMock, MagicMock + +from fastapi import FastAPI +from fastapi.testclient import TestClient + +import gently.ui.web.auth as auth +from gently.ui.web.routes.data import create_router + + +@dataclass +class _Embryo: + calibration: dict[str, Any] = field(default_factory=dict) + + +FIT = {"slope_um_per_deg": 42.0, "offset_um": 1.0, "r_squared": 0.85} + + +def _app(embryos: dict[str, _Embryo]): + """Routes wired against a roster, with a working orchestrator and client.""" + server = MagicMock() + agent = server.agent_bridge.agent + agent.experiment.embryos = embryos + agent.timelapse_orchestrator.start_timelapse = AsyncMock(return_value={"success": True}) + agent.client.acquire_volume = AsyncMock(return_value={"success": True}) + app = FastAPI() + app.include_router(create_router(server)) + app.dependency_overrides[auth.require_control] = lambda: True + return TestClient(app) + + +ROSTER = {"e1": _Embryo(dict(FIT)), "e2": _Embryo({})} + + +# ── timelapse ──────────────────────────────────────────────────────────────── + + +def test_timelapse_refuses_an_uncalibrated_embryo() -> None: + r = _app(ROSTER).post( + "/api/devices/timelapse/start", + json={"interval_seconds": 120, "embryo_ids": ["e1", "e2"]}, + ) + assert r.status_code == 409, r.text + detail = r.json()["detail"] + assert "e2" in detail + assert "e1" not in detail, "a calibrated embryo must not be named in the refusal" + assert "allow_uncalibrated" in detail + + +def test_timelapse_allows_a_fully_calibrated_set() -> None: + r = _app(ROSTER).post( + "/api/devices/timelapse/start", + json={"interval_seconds": 120, "embryo_ids": ["e1"]}, + ) + assert r.status_code != 409, r.text + + +def test_null_embryo_ids_still_checks_the_whole_roster() -> None: + """`embryo_ids: null` means "all active embryos" — including the bad one.""" + r = _app(ROSTER).post("/api/devices/timelapse/start", json={"interval_seconds": 120}) + assert r.status_code == 409, r.text + assert "e2" in r.json()["detail"] + + +def test_the_override_is_honoured() -> None: + """Someone who means it can say so. The point is that they must say it.""" + r = _app(ROSTER).post( + "/api/devices/timelapse/start", + json={"interval_seconds": 120, "embryo_ids": ["e1", "e2"], "allow_uncalibrated": True}, + ) + assert r.status_code != 409, r.text + + +def test_the_gate_runs_before_the_interval_check_is_irrelevant() -> None: + """A bad interval must still be a 400 — the gate does not mask validation.""" + r = _app(ROSTER).post( + "/api/devices/timelapse/start", + json={"interval_seconds": 0, "embryo_ids": ["e1"]}, + ) + assert r.status_code == 400, r.text + + +# ── single volume ──────────────────────────────────────────────────────────── + + +def test_volume_refuses_a_named_uncalibrated_embryo() -> None: + r = _app(ROSTER).post("/api/devices/acquire/volume", json={"embryo_id": "e2"}) + assert r.status_code == 409, r.text + + +def test_volume_without_an_embryo_id_is_not_gated() -> None: + """Manual-mode snapping images the current stage position deliberately. + + Optional by design, not by oversight: a test shot with no embryo in mind is + a real thing to want, and there is nothing to check it against. + """ + r = _app(ROSTER).post("/api/devices/acquire/volume", json={"num_slices": 5}) + assert r.status_code != 409, r.text + + +# ── tactics ────────────────────────────────────────────────────────────────── + + +def test_run_tactic_refuses_an_uncalibrated_embryo() -> None: + r = _app(ROSTER).post( + "/api/operate/run-tactic", + json={"library_id": "whatever", "embryo_ids": ["e2"]}, + ) + assert r.status_code == 409, r.text + + +# ── no roster ──────────────────────────────────────────────────────────────── + + +def test_an_empty_roster_does_not_block() -> None: + """With nothing known, there is nothing to assert — do not invent a refusal.""" + r = _app({}).post("/api/devices/timelapse/start", json={"interval_seconds": 120}) + assert r.status_code != 409, r.text