From f449bcdd787c65530419dd0d8bf3a822114b1ec2 Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Sun, 3 Aug 2025 11:25:43 +0100
Subject: [PATCH 01/18] Turn scan_data into a BaseModel with validation for
 custom formatting

---
 .../scan_directories.py                       | 126 ++++++++++++++-
 .../things/smart_scan.py                      | 151 +++++++-----------
 2 files changed, 181 insertions(+), 96 deletions(-)

diff --git a/src/openflexure_microscope_server/scan_directories.py b/src/openflexure_microscope_server/scan_directories.py
index 6f235191..8104c69c 100644
--- a/src/openflexure_microscope_server/scan_directories.py
+++ b/src/openflexure_microscope_server/scan_directories.py
@@ -6,8 +6,9 @@ import re
 import shutil
 import zipfile
 import threading
+from datetime import datetime, timedelta
 
-from pydantic import BaseModel
+from pydantic import BaseModel, field_validator, field_serializer
 
 from openflexure_microscope_server.utilities import requires_lock
 
@@ -17,6 +18,8 @@ SCAN_ZERO_PAD_DIGITS = 4
 STITCH_REGEX = re.compile(r"stitched\.jpe?g$")
 IMAGE_REGEX = re.compile(r"-?[0-9]+_-?[0-9]+\.jpe?g$")
 
+SCAN_DATA_FILENAME = "scan_data.json"
+
 
 class NotEnoughFreeSpaceError(IOError):
     """An exception raised if there is not enough free space on disk to scan."""
@@ -33,6 +36,96 @@ class ScanInfo(BaseModel):
     dzi: Optional[str]
 
 
+class ScanData(BaseModel):
+    """Data about a scan to be saved to a JSON file in the directory.
+
+    This serialises into a human readable format where possible with
+
+    timestamps in %H_%M_%S-%d_%m_%Y format
+    timedeltas in %H:%M:%S format
+
+    Properties that are not known until the end have ``None`` serialised as "Unknown"
+    """
+
+    # TODO: Docs for each variable
+    # TODO: Make note about timestamp format. Is it too ambiguous?
+    scan_name: str
+    overlap: float
+    max_dist: int
+    dx: int
+    dy: int
+    autofocus_range: int
+    autofocus_on: bool
+    start_time: datetime
+    skip_background: str
+    stitch_automatically: str
+    stitch_resize: int
+    save_resolution: tuple[int, int]
+    final_image_count: Optional[int] = None
+    duration: Optional[timedelta] = None
+    scan_result: Optional[str] = None
+
+    def set_final_data(self, result: str, final_image_count: int):
+        """Set the final data for the scan, scan duration is automatically calculated.
+
+        :param result: A string describing the result.
+        :param final_image_count: The total number of images captures.
+        """
+        self.duration = datetime.now() - self.start_time
+        self.final_image_count = final_image_count
+        self.scan_result = result
+
+    @field_validator("start_time", mode="before")
+    @classmethod
+    def parse_timestamp(cls, value: str | datetime) -> datetime:
+        """Validate a timestamp that may be a string in Hrs_Min_Sec-Day_Month_Year format."""
+        if isinstance(value, str):
+            return datetime.strptime(value, "%H_%M_%S-%d_%m_%Y")
+        return value
+
+    @field_serializer("start_time")
+    def serialize_timestamp(self, value: datetime) -> str:
+        """Serialise timestamp to Hrs_Min_Sec-Day_Month_Year format."""
+        return value.strftime("%H_%M_%S-%d_%m_%Y")
+
+    @field_validator("duration", mode="before")
+    @classmethod
+    def parse_timedelta(cls, value: Optional[str | timedelta]) -> Optional[timedelta]:
+        """Validate a timedelta that may be a string in Hrs:Min:Sec format or "Unknown"."""
+        if isinstance(value, str):
+            if value == "Unknown":
+                return None
+            hrs, mins, secs = map(int, value.split(":"))
+            return timedelta(hours=hrs, minutes=mins, seconds=secs)
+        return value
+
+    @field_serializer("duration")
+    def serialize_timedelta(self, value: Optional[timedelta]) -> str:
+        """Serialise timedelta to Hrs:Min:Sec (or "Unknown" if None)."""
+        if value is None:
+            return "Unknown"
+        # Calculate the string manually rather than use str(), as this may convert to
+        # days for a very very long scan, and then it is much harder to parse.
+        total_secs = value.total_seconds()
+        hrs = int(total_secs // 3600)
+        mins = int((total_secs % 3600) // 60)
+        secs = int((total_secs % 60))
+        return f"{hrs}:{mins}:{secs}"
+
+    @field_validator("final_image_count", "scan_result", mode="before")
+    @classmethod
+    def parse_unknown_as_none(cls, value: Optional[str | int]) -> Optional[str | int]:
+        """Validate the string "Unknown" as None."""
+        if value == "Unknown":
+            return None
+        return value
+
+    @field_serializer("final_image_count", "scan_result")
+    def serialize_none_as_unknown(self, value: Optional[str | int]) -> str:
+        """Serialise None as "Unknown" for a more human readable result."""
+        return "Unknown" if value is None else str(value)
+
+
 class ScanDirectoryManager:
     """A class for managing interactions with scan directories."""
 
@@ -111,6 +204,17 @@ class ScanDirectoryManager:
             return None
         return self.get_file_path_from_img_dir(scan_name, stitch_fname)
 
+    @requires_lock
+    def get_scan_data_path(self, scan_name: str) -> Optional[str]:
+        """Return the file full path scan data JSON file.
+
+        If no scan data JSON file is found, return None
+        """
+        scan_data_path = ScanDirectory(scan_name, self.base_dir).scan_data_path
+        if not os.path.isfile(scan_data_path):
+            return None
+        return scan_data_path
+
     @property
     @requires_lock
     def all_scans(self) -> list[str]:
@@ -250,6 +354,16 @@ class ScanDirectory:
             return im_path
         return None
 
+    @property
+    def scan_data_path(self) -> Optional[str]:
+        """The path to the scan data json file for this directory.
+
+        Returns None if there is no images dir to write to.
+        """
+        if self.images_dir is not None:
+            return os.path.join(self.images_dir, SCAN_DATA_FILENAME)
+        return None
+
     @property
     def created_time(self) -> float:
         """The time the directory was created on disk."""
@@ -341,6 +455,16 @@ class ScanDirectory:
                 files.append(os.path.relpath(full_path, self.dir_path))
         return files
 
+    def save_scan_data(self, scan_data: ScanData):
+        """Save the scan data for this scan to disk."""
+        if self.scan_data_path is None:
+            raise FileNotFoundError(
+                "There is no images directory to save scan data into."
+            )
+
+        with open(self.scan_data_path, "w", encoding="utf-8") as f:
+            f.write(scan_data.model_dump_json(indent=4))
+
     def zip_files(self, final_version: bool = False) -> str:
         """Zips any images from the scan not yet zipped, return full path to zip.
 
diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py
index caad1acc..6b129ab0 100644
--- a/src/openflexure_microscope_server/things/smart_scan.py
+++ b/src/openflexure_microscope_server/things/smart_scan.py
@@ -12,7 +12,6 @@ import threading
 import os
 import time
 import json
-from datetime import datetime
 from subprocess import CompletedProcess, Popen, PIPE, SubprocessError, STDOUT
 
 from fastapi import HTTPException
@@ -40,7 +39,7 @@ AutofocusDep = lt.deps.direct_thing_client_dependency(AutofocusThing, "/autofocu
 JPEGBlob = lt.blob.blob_type("image/jpeg")
 ZipBlob = lt.blob.blob_type("application/zip")
 
-SCAN_DATA_FILENAME = "scan_data.json"
+
 STITCHING_CMD = "openflexure-stitch"
 
 STITCHING_RESOLUTION = (820, 616)
@@ -111,7 +110,7 @@ class SmartScanThing(lt.Thing):
         self._capture_thread: Optional[ErrorCapturingThread] = None
         self._scan_images_taken: Optional[int] = None
         # TODO Scan data is a dict during refactoring, should become a dataclass
-        self._scan_data: Optional[dict] = None
+        self._scan_data: Optional[scan_directories.ScanData] = None
 
     @lt.thing_action
     def sample_scan(
@@ -141,6 +140,7 @@ class SmartScanThing(lt.Thing):
         self._autofocus = autofocus
         self._stage = stage
         self._cam = cam
+        # TODO check if metadata_getter this can be removed without error?
         self._metadata_getter = metadata_getter
         self._csm = csm
         self._capture_thread = None
@@ -280,14 +280,11 @@ class SmartScanThing(lt.Thing):
         return dx, dy
 
     @_scan_running
-    def _set_scan_data(self):
-        """Set date for this scan to the ``self._scan_data`` variable.
-
-        This needs to become a dataclass at some point.
-        """
+    def _collect_scan_data(self) -> scan_directories.ScanData:
+        """Collect and return the data for this scan so it cannot be changed mid-scan."""
         overlap = self.overlap
         dx, dy = self._calc_displacement_from_test_image(overlap)
-        stitch_resize = STITCHING_RESOLUTION[0] / self.save_resolution[0]
+        stitch_resize = STITCHING_RESOLUTION[0] // self.save_resolution[0]
 
         self._scan_logger.debug(
             f"Resizing images when stitching by a factor of {stitch_resize}"
@@ -308,78 +305,33 @@ class SmartScanThing(lt.Thing):
             autofocus_dz = 0
 
         # Fix scan parameters in case UI is updated during scan.
-        self._scan_data = {
-            "scan_name": self._ongoing_scan.name,
-            "overlap": overlap,
-            "max_dist": self.max_range,
-            "dx": dx,
-            "dy": dy,
-            "autofocus_dz": autofocus_dz,
-            "autofocus_on": bool(autofocus_dz),
-            "start_time": time.strftime("%H_%M_%S-%d_%m_%Y"),
-            "skip_background": self.skip_background,
-            "stitch_automatically": self.stitch_automatically,
-            "stitch_resize": stitch_resize,
-            "save_resolution": self.save_resolution,
-        }
-
-    @_scan_running
-    def _save_scan_inputs_json(self):
-        """Save scan inputs as a JSON file in the scan folder.
-
-        This file allows the user to review the settings used in the scan.
-        """
-        # Should this be a method of the scan_data dataclass?
-
-        data = {
-            "scan_name": self._ongoing_scan.name,
-            "overlap": self._scan_data["overlap"],
-            "autofocus range": self._scan_data["autofocus_dz"],
-            "dx": self._scan_data["dx"],
-            "dy": self._scan_data["dy"],
-            "start time": self._scan_data["start_time"],
-            "skipping background": self._scan_data["skip_background"],
-            "capture resolution": self._scan_data["save_resolution"],
-        }
-
-        scan_inputs_fname = os.path.join(
-            self._ongoing_scan.images_dir, SCAN_DATA_FILENAME
+        return scan_directories.ScanData(
+            scan_name=self._ongoing_scan.name,
+            overlap=overlap,
+            max_dist=self.max_range,
+            dx=dx,
+            dy=dy,
+            autofocus_dz=autofocus_dz,
+            autofocus_on=bool(autofocus_dz),
+            start_time=time.strftime("%H_%M_%S-%d_%m_%Y"),
+            skip_background=self.skip_background,
+            stitch_automatically=self.stitch_automatically,
+            stitch_resize=stitch_resize,
+            save_resolution=self.save_resolution,
         )
-        with open(scan_inputs_fname, "w", encoding="utf-8") as f:
-            json.dump(data, f, ensure_ascii=False, indent=4)
 
     @_scan_running
-    def _update_scan_data_json(self, scan_result: str):
+    def _save_final_scan_data(self, scan_result: str):
         """Update scan data JSON file with data only known at the end of the scan.
 
         Takes scan_result, a string that is either "success", "cancelled by user",
         or the error that ended the scan.
         """
-        # Should this be a method of the scan_data dataclass?
-        current_time = datetime.now().replace(microsecond=0)
-        start_time = datetime.strptime(
-            self._scan_data["start_time"], "%H_%M_%S-%d_%m_%Y"
-        ).replace(microsecond=0)
-
-        duration = current_time - start_time
-
-        outputs = {
-            "image_count": self._scan_images_taken,
-            "duration": str(duration),
-            "scan_result": scan_result,
-        }
-
-        scan_data_fname = os.path.join(
-            self._ongoing_scan.images_dir, SCAN_DATA_FILENAME
+        self.scan_data.set_final_data(
+            result=scan_result,
+            final_image_count=self._scan_images_taken,
         )
-
-        with open(scan_data_fname, encoding="utf-8") as f:
-            data = json.load(f)
-
-        data.update(outputs)
-
-        with open(scan_data_fname, "w", encoding="utf-8") as f:
-            json.dump(data, f, ensure_ascii=False, indent=4)
+        self._ongoing_scan.save_scan_data(self._scan_data)
 
     @_scan_running
     def _manage_stitching_threads(self):
@@ -388,7 +340,7 @@ class SmartScanThing(lt.Thing):
         # well constrained.
         if self._scan_images_taken > 3:
             if not self._preview_stitch_running():
-                self._preview_stitch_start(overlap=self._scan_data["overlap"])
+                self._preview_stitch_start(overlap=self._scan_data.overlap)
 
     @_scan_running
     def _run_scan(self):
@@ -404,25 +356,25 @@ class SmartScanThing(lt.Thing):
 
         try:
             self._cam.start_streaming(main_resolution=(3280, 2464))
-            self._set_scan_data()
-            self._save_scan_inputs_json()
+            self._scan_data = self._collect_scan_data()
+            self._ongoing_scan.save_scan_data(self._scan_data)
             if self._scan_images_taken != 0:
                 msg = "_scan_images_taken should be zero before starting scanning"
                 raise RuntimeError(msg)
 
             # This is the main loop of the scan!
             self._main_scan_loop()
-            self._update_scan_data_json(scan_result="success")
+            self._save_final_scan_data(scan_result="success")
 
         except lt.exceptions.InvocationCancelledError:
             scan_successful = False
             # Reset the cancel event so it can be thrown again
             self._cancel.clear()
             self._scan_logger.info("Stopping scan because it was cancelled.")
-            self._update_scan_data_json(scan_result="cancelled by user")
+            self._save_final_scan_data(scan_result="cancelled by user")
         except scan_directories.NotEnoughFreeSpaceError as e:
             scan_successful = False
-            self._update_scan_data_json(scan_result=str(e))
+            self._save_final_scan_data(scan_result=str(e))
             self._scan_logger.error(
                 f"Stopping scan to avoid filling up the disk: {e}",
                 exc_info=e,
@@ -430,6 +382,7 @@ class SmartScanThing(lt.Thing):
             raise e
         except Exception as e:
             scan_successful = False
+            self._save_final_scan_data(scan_result=str(e))
             self._scan_logger.error(
                 f"The scan stopped because of an error: {e} "
                 "Attempting to stitch and archive the images acquired so far.",
@@ -475,9 +428,9 @@ class SmartScanThing(lt.Thing):
         # have multiple starting positions, each of which will be visited before the
         # scan can end.
         planner_settings = {
-            "dx": self._scan_data["dx"],
-            "dy": self._scan_data["dy"],
-            "max_dist": self._scan_data["max_dist"],
+            "dx": self._scan_data.dx,
+            "dy": self._scan_data.dy,
+            "max_dist": self._scan_data.max_dist,
         }
         route_planner = scan_planners.SmartSpiral(
             initial_position=(self._stage.position["x"], self._stage.position["y"]),
@@ -502,7 +455,7 @@ class SmartScanThing(lt.Thing):
 
             capture_image = True
             # If skipping background, take an image to check if current field of view is background
-            if self._scan_data["skip_background"]:
+            if self._scan_data.skip_background:
                 capture_image, bg_message = self._cam.image_is_sample()
 
             if not capture_image:
@@ -515,8 +468,8 @@ class SmartScanThing(lt.Thing):
 
             focused, focused_height = self._autofocus.run_smart_stack(
                 images_dir=self._ongoing_scan.images_dir,
-                autofocus_dz=self._scan_data["autofocus_dz"],
-                save_resolution=self._scan_data["save_resolution"],
+                autofocus_dz=self._scan_data.autofocus_dz,
+                save_resolution=self._scan_data.save_resolution,
             )
 
             current_pos_xyz = (new_pos_xyz[0], new_pos_xyz[1], focused_height)
@@ -552,14 +505,14 @@ class SmartScanThing(lt.Thing):
 
         self._preview_stitch_wait()
         try:
-            if self._scan_data["stitch_automatically"]:
+            if self._scan_data.stitch_automatically:
                 self._scan_logger.info("Stitching final image (may take some time)...")
                 self.stitch_scan(
                     logger=self._scan_logger,
                     cancel=self._cancel,
                     scan_name=self._ongoing_scan.name,
-                    stitch_resize=self._scan_data["stitch_resize"],
-                    overlap=self._scan_data["overlap"],
+                    stitch_resize=self._scan_data.stitch_resize,
+                    overlap=self._scan_data.overlap,
                 )
         except SubprocessError as e:
             self._scan_logger.error(f"Stitching failed: {e}", exc_info=e)
@@ -798,7 +751,7 @@ class SmartScanThing(lt.Thing):
                     "--minimum_overlap",
                     f"{min_overlap}",
                     "--resize",
-                    f"{self._scan_data['stitch_resize']}",
+                    f"{self._scan_data.stitch_resize}",
                     self._ongoing_scan.images_dir,
                 ]
             )
@@ -885,9 +838,9 @@ class SmartScanThing(lt.Thing):
         Note that as this is a lt.thing_action it needs the logger passed as
         a variable if called from another thing action
         """
-        json_fpath = self._scan_dir_manager.get_file_path_from_img_dir(
-            scan_name=scan_name, filename=SCAN_DATA_FILENAME
-        )
+        # TODO tidy this function
+        # TODO handle if json_path is None (easier once tidied)
+        json_fpath = self._scan_dir_manager.get_scan_data_path()
 
         if self.stitch_tiff:
             tiff_arg = "--stitch_tiff"
@@ -904,7 +857,7 @@ class SmartScanThing(lt.Thing):
                 # the file not being there, it not being json in the file,
                 # or the imported data not being indexable
                 logger.warning(
-                    f"Couldn't read scan data, is {SCAN_DATA_FILENAME} missing or corrupt? "
+                    f"Couldn't read scan data, is {json_fpath} missing or corrupt? "
                     "Attempting stitch with overlap value of 0.1"
                 )
                 overlap = 0.1
@@ -919,20 +872,28 @@ class SmartScanThing(lt.Thing):
             try:
                 with open(json_fpath, "r", encoding="utf-8") as data_file:
                     data_loaded = json.load(data_file)
-                save_resolution = data_loaded["capture resolution"]
+
+                # Handle "capture resolution" being used to store the save resolution
+                # in old scans.
+                key = (
+                    "capture resolution"
+                    if "capture resolution" in data_loaded
+                    else "save_resolution"
+                )
+                save_resolution = data_loaded[key]
                 stitch_resize = STITCHING_RESOLUTION[0] / save_resolution[0]
             except (json.decoder.JSONDecodeError, FileNotFoundError, TypeError):
                 # As there is no schema or pydantic model this should handle
                 # the file not being there, it not being json in the file,
                 # or the imported data not being indexable
                 logger.warning(
-                    f"Couldn't read scan data, is {SCAN_DATA_FILENAME} missing or corrupt? "
+                    f"Couldn't read scan data, is {json_fpath} missing or corrupt? "
                     "Attempting stitch with resize value of 0.5"
                 )
                 stitch_resize = 0.5
             except KeyError:
                 logger.warning(
-                    "Value for capture resolution not found in scan data. "
+                    "Value for save resolution not found in scan data. "
                     "Attempting stitch with resize value of 0.5"
                 )
                 stitch_resize = 0.5

From 6ceb1a98458b35b53ac3f08f1865c6bd83130b41 Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Sun, 3 Aug 2025 14:33:45 +0100
Subject: [PATCH 02/18] Pull stitching out of SmartScanThing

---
 .../scan_directories.py                       |  22 +-
 .../stitching.py                              | 295 ++++++++++++++++++
 .../things/smart_scan.py                      | 241 +++-----------
 3 files changed, 358 insertions(+), 200 deletions(-)
 create mode 100644 src/openflexure_microscope_server/stitching.py

diff --git a/src/openflexure_microscope_server/scan_directories.py b/src/openflexure_microscope_server/scan_directories.py
index 8104c69c..6533c589 100644
--- a/src/openflexure_microscope_server/scan_directories.py
+++ b/src/openflexure_microscope_server/scan_directories.py
@@ -1,11 +1,12 @@
 """Functionality to manage file system operations for scan directories."""
 
-from typing import Optional
+from typing import Optional, Any
 import os
 import re
 import shutil
 import zipfile
 import threading
+import json
 from datetime import datetime, timedelta
 
 from pydantic import BaseModel, field_validator, field_serializer
@@ -59,7 +60,9 @@ class ScanData(BaseModel):
     start_time: datetime
     skip_background: str
     stitch_automatically: str
-    stitch_resize: int
+    # TODO: Think about changing stitch_resize name as it is NOT the resize for
+    # just for correlation stitching
+    stitch_resize: float
     save_resolution: tuple[int, int]
     final_image_count: Optional[int] = None
     duration: Optional[timedelta] = None
@@ -215,6 +218,21 @@ class ScanDirectoryManager:
             return None
         return scan_data_path
 
+    def get_scan_data_dict(self, scan_name: str) -> Optional[dict[str, Any]]:
+        """Return the scan data read from a JSON file as a dict.
+
+        This is a dictionary not a base models as the data format has changed
+        somewhat over time.
+        """
+        json_fpath = self.get_scan_data_path(scan_name)
+        if json_fpath is None:
+            return None
+        try:
+            with open(json_fpath, "r", encoding="utf-8") as data_file:
+                return json.load(data_file)
+        except (json.decoder.JSONDecodeError, IOError):
+            return None
+
     @property
     @requires_lock
     def all_scans(self) -> list[str]:
diff --git a/src/openflexure_microscope_server/stitching.py b/src/openflexure_microscope_server/stitching.py
new file mode 100644
index 00000000..27461e1e
--- /dev/null
+++ b/src/openflexure_microscope_server/stitching.py
@@ -0,0 +1,295 @@
+"""Communicate with OpenFlexure Stitching to perform stitches for scans.
+
+This includes both live stitching and final stitching. This is done via subprocess
+to call openflexure-stitching over CLI. This cannot be done via Threading dut to the
+CPU intensity of stitching causing scanning problems due to the Python Global
+Interpreter Lock (GIL). May be possible to shift to multiprocessing int the future.
+"""
+
+from typing import Optional, Any
+import threading
+import subprocess
+import os
+import time
+
+import labthings_fastapi as lt
+
+STITCHING_CMD = "openflexure-stitch"
+STITCHING_RESOLUTION = (820, 616)
+
+DEFAULT_OVERLAP = 0.5
+DEFAULT_RESIZE = 0.5
+
+
+class BaseStitcher:
+    """A base stitching class for all stitchers. Don't initialise this directly."""
+
+    def __init__(self, images_dir: str, *, overlap: float, correlation_resize: float):
+        """Initialise a stitcher.
+
+        All args except images_dir are positional only.
+
+        :param images_dir: The images directory of the scan to stitch.
+        :param overlap: The scan overlap.
+        :param correlation_resize: The fraction to resize images by when correlating.
+        """
+        # Set minimum overlap to 90% of the scan overlap to catch only images
+        # directly adjacent, not images with overlapping corners.
+        self.images_dir = images_dir
+        self.min_overlap = round(overlap * 0.9, 2)
+        self.correlation_resize = correlation_resize
+        self._mode = "all"
+        self._extra_args = []
+
+    @property
+    def command(self) -> list[str]:
+        """The command to run with subprocess.Popen."""
+        # The command, and the mode
+        initial_args = [STITCHING_CMD, "--stitching_mode", self._mode]
+        setting_args = [
+            "--minimum_overlap",
+            f"{self.min_overlap}",
+            "--resize",
+            f"{self.correlation_resize}",
+        ]
+        return initial_args + self._extra_args + setting_args + [self.images_dir]
+
+    def start(self) -> None:
+        """Start stitching a stitching process.
+
+        This should be overridden by any child class.
+        """
+        raise NotImplementedError(
+            "Child stitchers should implement their own ``start`` method."
+        )
+
+
+class PreviewStitcher(BaseStitcher):
+    """A stitcher for stitching an ongoing scan in preview mode.
+
+    Use ``start()`` to start a scan, and ``running`` to check if it is complete, or
+    ``wait()`` to wait for it to complete.
+
+    The same stitcher object can be run multiple times to update the preview. However,
+    one preview must finish before another can be started.
+    """
+
+    def __init__(self, images_dir: str, *, overlap: float, correlation_resize: float):
+        """Initialise a preview stitcher.
+
+        All args except images_dir are positional only.
+
+        :param images_dir: The images directory of the scan to stitch.
+        :param overlap: The scan overlap.
+        :param correlation_resize: The fraction to resize images by when correlating.
+        """
+        super().__init__(
+            images_dir, overlap=overlap, correlation_resize=correlation_resize
+        )
+        self._popen_lock = threading.Lock()
+        self._popen_obj: Optional[subprocess.Popen] = None
+
+        self._mode = "preview_stitch"
+
+    @property
+    def command(self) -> list[str]:
+        """The command to run with subprocess.Popen."""
+        return [
+            STITCHING_CMD,
+            "--stitching_mode",
+            "preview_stitch",
+            "--minimum_overlap",
+            f"{self.min_overlap}",
+            "--resize",
+            f"{self.correlation_resize}",
+            self.images_dir,
+        ]
+
+    def start(self) -> None:
+        """Start stitching a preview of the scan in a background subprocess.
+
+        This uses popen and returns immediately.
+        """
+        if self.running:
+            raise RuntimeError("Cannot start stitch. It is already running.")
+        with self._popen_lock:
+            self._popen_obj = subprocess.Popen(self.command)
+
+    @property
+    def running(self) -> bool:
+        """Whether the preview stitch is running in a subprocess."""
+        with self._popen_lock:
+            if self._popen_obj is None:
+                return False
+            if self._popen_obj.poll() is None:
+                return True
+            return False
+
+    def wait(self):
+        """Wait for this preview stitch to return."""
+        if self.running:
+            with self._popen_lock:
+                self._popen_obj.wait()
+
+
+class FinalStitcher(BaseStitcher):
+    """A class to handle the final stich for a scan."""
+
+    def __init__(
+        self,
+        images_dir,
+        *,
+        logger: lt.deps.InvocationLogger,
+        overlap: Optional[float] = None,
+        correlation_resize: Optional[float] = None,
+        stitch_tiff: bool = False,
+        scan_data_dict: Optional[dict[str, Any]] = None,
+    ):
+        """Initialise a final stitcher, this has more args than the base class.
+
+        All args except images_dir are positional only.
+
+        :param images_dir: The images directory of the scan to stitch.
+        :param logger: The invocation logger for this stitch process.
+        :param overlap: The scan overlap, if not known enter None. A value will be
+            chosen from the scan_data_dict, or set to a default value if no value is
+            available in the scan_data.
+        :param correlation_resize: The fraction to resize images by when correlating,
+            if not known enter None. A value will be chosen from the scan_data_dict, or
+            set to a default value if no value is available in the scan_data.
+        :param stitch_tiff: Whether to stitch a pyramidal TIFF.
+        :param scan_data_dict: The ScanData for this scan as a dictionary. This is used
+            to read/calculate overlap and correlation_resize if they are not provided.
+        """
+        self.logger = logger
+        overlap, correlation_resize = self._process_inputs(
+            overlap=overlap,
+            correlation_resize=correlation_resize,
+            scan_data_dict=scan_data_dict,
+        )
+        super().__init__(
+            images_dir, overlap=overlap, correlation_resize=correlation_resize
+        )
+        self._mode = "all"
+
+        tiff_arg = "--stitch_tiff" if stitch_tiff else "--no-stitch_tiff"
+        self._extra_args = ["--stitch-dzi", tiff_arg]
+
+    def _process_inputs(
+        self,
+        overlap: float,
+        correlation_resize: float,
+        scan_data_dict: Optional[dict[str, Any]],
+    ) -> tuple[float, float]:
+        """Process inputs to ensure ``overlap`` and ``correlation_resize`` have values.
+
+        First the scan_data_dict is inspected for values to allow ``overlap`` and
+        ``correlation_resize`` to be set correctly, if these values are not available
+        then default values are used, and a warning is logged to the invocation logger.
+
+        :param overlap: overlap as input to __init__
+        :param correlation_resize: correlation_resize as input to __init__
+        :param scan_data_dict: scan_data_dict as input to __init__
+
+        :returns: overlap and correlation_resize as floats.
+        """
+        if overlap is None:
+            if scan_data_dict is not None and "overlap" in scan_data_dict:
+                overlap = scan_data_dict["overlap"]
+
+            # Warn if still None and set to default.
+            if overlap is None:
+                overlap = DEFAULT_OVERLAP
+                self.logger.warning(
+                    "No value set for overlap. Attempting stitch with overlap "
+                    f"value of {DEFAULT_OVERLAP}"
+                )
+
+        if correlation_resize is None:
+            if scan_data_dict is not None:
+                # Handle "capture resolution" being used to store the save resolution
+                # in old scans.
+                key = (
+                    "capture resolution"
+                    if "capture resolution" in scan_data_dict
+                    else "save_resolution"
+                )
+                if key in scan_data_dict:
+                    save_resolution = scan_data_dict[key]
+                    correlation_resize = STITCHING_RESOLUTION[0] / save_resolution[0]
+            # Warn if still None and set to default.
+            if correlation_resize is None:
+                correlation_resize = DEFAULT_RESIZE
+                self.logger.warning(
+                    "No information available to calculate stitch resize. Attempting "
+                    f"stitch with resize value of {DEFAULT_RESIZE}"
+                )
+        return overlap, correlation_resize
+
+    def start(
+        self,
+        cancel: lt.deps.CancelHook,
+    ) -> None:
+        """Run the final stitch logging any output.
+
+        Raises:
+            ChildProcessError if exit code is not zero
+            InvocationCancelledError if the action is cancelled.
+
+        """
+        cmd = self.command
+        self.logger.debug(f"Running command in subprocess: `{' '.join(cmd)}`")
+
+        # Run the command piping stdout into the process for reading and
+        # forwarding the stdrerr to stdout
+        process = subprocess.Popen(
+            cmd,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.STDOUT,
+            bufsize=1,
+            universal_newlines=True,
+        )
+        # Stop opening pipe blocking writing to it
+        os.set_blocking(process.stdout.fileno(), False)
+
+        # TODO check if we still want this? Is it for debugging, or should it have
+        # more explanation.
+        self.logger.info(
+            time.strftime("%Y-%m-%d %H:%M:%S", time.localtime(time.time()))
+        )
+
+        self._log_ongoing(process, cancel=cancel)
+
+        if process.poll() == 0:
+            self.logger.info("Stitching complete")
+        else:
+            raise ChildProcessError(f"Subprocess {cmd[0]} exited with an error.")
+
+    def _log_ongoing(
+        self,
+        process: subprocess.Popen,
+        cancel: lt.deps.CancelHook,
+    ) -> None:
+        """Log the ongoing process unless it is cancelled."""
+        # Poll returns None while running, will return the error code when finished
+        while process.poll() is None:
+            self.log_buffer(process.stdout)
+            # Once buffer is clear sleep for 0.2s before trying again.
+
+            try:
+                # Note that using cancel.sleep allows the InvocationCancelledError to
+                # be thrown
+                cancel.sleep(0.2)
+            except lt.exceptions.InvocationCancelledError as e:
+                self.logger.info("Stitching cancelled by user")
+                process.kill()
+                raise e
+
+        # Print everything in the buffer when program finishes
+        self.log_buffer(process.stdout)
+
+    def log_buffer(self, buffer):
+        # TODO work out type
+        """Log everything in the buffer at INFO level."""
+        while line := buffer.readline():
+            self.logger.info(line)
diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py
index 6b129ab0..cb1ef83d 100644
--- a/src/openflexure_microscope_server/things/smart_scan.py
+++ b/src/openflexure_microscope_server/things/smart_scan.py
@@ -11,8 +11,7 @@ from typing import Optional, Mapping
 import threading
 import os
 import time
-import json
-from subprocess import CompletedProcess, Popen, PIPE, SubprocessError, STDOUT
+from subprocess import SubprocessError
 
 from fastapi import HTTPException
 from fastapi.responses import FileResponse
@@ -24,6 +23,7 @@ import labthings_fastapi as lt
 from openflexure_microscope_server.utilities import ErrorCapturingThread
 from openflexure_microscope_server import scan_directories
 from openflexure_microscope_server import scan_planners
+from openflexure_microscope_server import stitching
 
 # Things
 from .autofocus import AutofocusThing
@@ -40,11 +40,6 @@ JPEGBlob = lt.blob.blob_type("image/jpeg")
 ZipBlob = lt.blob.blob_type("application/zip")
 
 
-STITCHING_CMD = "openflexure-stitch"
-
-STITCHING_RESOLUTION = (820, 616)
-
-
 class ScanNotRunningError(RuntimeError):
     """Exception called when scan not running that requires a scan to be running."""
 
@@ -85,8 +80,6 @@ class SmartScanThing(lt.Thing):
             HTTP interface.
         """
         self._scan_dir_manager = scan_directories.ScanDirectoryManager(scans_folder)
-        self._preview_stitch_popen = None
-        self._preview_stitch_popen_lock = threading.Lock()
         self._scan_lock = threading.Lock()
 
         # Variables set by the scan
@@ -106,11 +99,13 @@ class SmartScanThing(lt.Thing):
         self._metadata_getter: Optional[lt.deps.GetThingStates] = None
         self._csm: Optional[CSMDep] = None
         self._ongoing_scan: Optional[scan_directories.ScanDirectory] = None
+        # TODO see if starting position can go into ScanData
         self._starting_position: Optional[Mapping[str, int]] = None
         self._capture_thread: Optional[ErrorCapturingThread] = None
         self._scan_images_taken: Optional[int] = None
         # TODO Scan data is a dict during refactoring, should become a dataclass
         self._scan_data: Optional[scan_directories.ScanData] = None
+        self._preview_stitcher: Optional[stitching.PreviewStitcher] = None
 
     @lt.thing_action
     def sample_scan(
@@ -181,6 +176,8 @@ class SmartScanThing(lt.Thing):
             self._scan_images_taken = None
             self._scan_data = None
             self._scan_lock.release()
+            # Ensure any PreviewStitcher created cannot be reused.
+            self._preview_stitcher = None
 
     @_scan_running
     def _check_background_and_csm_set(self):
@@ -284,7 +281,7 @@ class SmartScanThing(lt.Thing):
         """Collect and return the data for this scan so it cannot be changed mid-scan."""
         overlap = self.overlap
         dx, dy = self._calc_displacement_from_test_image(overlap)
-        stitch_resize = STITCHING_RESOLUTION[0] // self.save_resolution[0]
+        stitch_resize = stitching.STITCHING_RESOLUTION[0] / self.save_resolution[0]
 
         self._scan_logger.debug(
             f"Resizing images when stitching by a factor of {stitch_resize}"
@@ -339,8 +336,8 @@ class SmartScanThing(lt.Thing):
         # Assume 4 images means at least one offset in x and y, making the stitching
         # well constrained.
         if self._scan_images_taken > 3:
-            if not self._preview_stitch_running():
-                self._preview_stitch_start(overlap=self._scan_data.overlap)
+            if not self._preview_stitcher.running:
+                self._preview_stitcher.start()
 
     @_scan_running
     def _run_scan(self):
@@ -358,6 +355,12 @@ class SmartScanThing(lt.Thing):
             self._cam.start_streaming(main_resolution=(3280, 2464))
             self._scan_data = self._collect_scan_data()
             self._ongoing_scan.save_scan_data(self._scan_data)
+            self._preview_stitcher = stitching.PreviewStitcher(
+                self._ongoing_scan.images_dir,
+                overlap=self._scan_data.overlap,
+                correlation_resize=self._scan_data.stitch_resize,
+            )
+
             if self._scan_images_taken != 0:
                 msg = "_scan_images_taken should be zero before starting scanning"
                 raise RuntimeError(msg)
@@ -390,6 +393,8 @@ class SmartScanThing(lt.Thing):
             )
             raise e
         finally:
+            # Don't set Preview Stitcher to None yet. It is used by _perform_final_scan.
+
             # Start streaming in the default resolution again as soon as possible
             self._cam.start_streaming()
             if self._capture_thread:
@@ -503,19 +508,17 @@ class SmartScanThing(lt.Thing):
 
         self._scan_logger.info("Waiting for background processes to finish...")
 
-        self._preview_stitch_wait()
-        try:
-            if self._scan_data.stitch_automatically:
-                self._scan_logger.info("Stitching final image (may take some time)...")
-                self.stitch_scan(
-                    logger=self._scan_logger,
-                    cancel=self._cancel,
-                    scan_name=self._ongoing_scan.name,
-                    stitch_resize=self._scan_data.stitch_resize,
-                    overlap=self._scan_data.overlap,
-                )
-        except SubprocessError as e:
-            self._scan_logger.error(f"Stitching failed: {e}", exc_info=e)
+        self._preview_stitcher.wait()
+
+        if self._scan_data.stitch_automatically:
+            self._scan_logger.info("Stitching final image (may take some time)...")
+            self.stitch_scan(
+                logger=self._scan_logger,
+                cancel=self._cancel,
+                scan_name=self._ongoing_scan.name,
+                stitch_resize=self._scan_data.stitch_resize,
+                overlap=self._scan_data.overlap,
+            )
 
     @lt.fastapi_endpoint(
         "get",
@@ -727,103 +730,6 @@ class SmartScanThing(lt.Thing):
             raise HTTPException(404, "File not found")
         return FileResponse(preview_path)
 
-    @_scan_running
-    def _preview_stitch_start(self, overlap: float) -> None:
-        """Start stitching a preview of the scan in a background subprocess.
-
-        This uses popen and returns immediately
-
-        - self._preview_stitch_popen holds the popen for polling
-        - self._preview_stitch_popen_lock is a lock acquired while interacting
-              with Popen
-        """
-        # Set minimum overlap to 90% of the scan overlap to catch only images directly adjacent,
-        # not images with overlapping corners.
-        min_overlap = round(overlap * 0.9, 2)
-        if self._preview_stitch_running():
-            raise RuntimeError("Only one subprocess is allowed at a time")
-        with self._preview_stitch_popen_lock:
-            self._preview_stitch_popen = Popen(
-                [
-                    STITCHING_CMD,
-                    "--stitching_mode",
-                    "preview_stitch",
-                    "--minimum_overlap",
-                    f"{min_overlap}",
-                    "--resize",
-                    f"{self._scan_data.stitch_resize}",
-                    self._ongoing_scan.images_dir,
-                ]
-            )
-
-    @_scan_running
-    def _preview_stitch_running(self) -> bool:
-        """Whether there is a preview stitch running in a subprocess."""
-        with self._preview_stitch_popen_lock:
-            if self._preview_stitch_popen is None:
-                return False
-            if self._preview_stitch_popen.poll() is None:
-                return True
-            return False
-
-    @_scan_running
-    def _preview_stitch_wait(self):
-        """Wait for an ongoing preview stitch to return."""
-        if self._preview_stitch_running():
-            with self._preview_stitch_popen_lock:
-                self._preview_stitch_popen.wait()
-
-    def run_subprocess(
-        self,
-        logger: lt.deps.InvocationLogger,
-        cancel: lt.deps.CancelHook,
-        cmd: list[str],
-    ) -> CompletedProcess:
-        """Run a  subprocess and log any output.
-
-        Raises:
-            ChildProcessError if exit code is not zero
-            InvocationCancelledError if the action is cancelled.
-
-        """
-        logger.info(f"Running command in subprocess: `{' '.join(cmd)}`")
-
-        def log_buffer(buffer):
-            """Log everything in the buffer at INFO level."""
-            while line := buffer.readline():
-                logger.info(line)
-
-        # Run the command piping stdout into the process for reading and
-        # forwarding the stdrerr to stdout
-        process = Popen(
-            cmd, stdout=PIPE, stderr=STDOUT, bufsize=1, universal_newlines=True
-        )
-        # Stop opening pipe blocking writing to it
-        os.set_blocking(process.stdout.fileno(), False)
-        logger.info(time.strftime("%Y-%m-%d %H:%M:%S", time.localtime(time.time())))
-
-        # Poll returns None while running, will return the error code when finished
-        while process.poll() is None:
-            log_buffer(process.stdout)
-            # Once buffer is clear sleep for 0.2s before trying again.
-
-            try:
-                # Note that using cancel.sleep allows the InvocationCancelledError to
-                # be thrown
-                cancel.sleep(0.2)
-            except lt.exceptions.InvocationCancelledError as e:
-                logger.info("Stitching cancelled by user")
-                process.kill()
-                raise e
-
-        # Print everything in the buffer when program finishes
-        log_buffer(process.stdout)
-
-        if process.poll() == 0:
-            logger.info("Stitching complete")
-        else:
-            raise ChildProcessError(f"Subprocess {cmd[0]} exited with an error.")
-
     @lt.thing_action
     def stitch_scan(
         self,
@@ -831,7 +737,7 @@ class SmartScanThing(lt.Thing):
         cancel: lt.deps.CancelHook,
         scan_name: str,
         stitch_resize: Optional[float] = None,
-        overlap: float = 0.0,
+        overlap: Optional[float] = None,
     ) -> None:
         """Generate a stitched image based on stage position metadata.
 
@@ -840,85 +746,24 @@ class SmartScanThing(lt.Thing):
         """
         # TODO tidy this function
         # TODO handle if json_path is None (easier once tidied)
-        json_fpath = self._scan_dir_manager.get_scan_data_path()
-
-        if self.stitch_tiff:
-            tiff_arg = "--stitch_tiff"
-        else:
-            tiff_arg = "--no-stitch_tiff"
-
-        if overlap == 0.0:
-            try:
-                with open(json_fpath, "r", encoding="utf-8") as data_file:
-                    data_loaded = json.load(data_file)
-                overlap = data_loaded["overlap"]
-            except (json.decoder.JSONDecodeError, FileNotFoundError, TypeError):
-                # As there is no schema or pydantic model this should handle
-                # the file not being there, it not being json in the file,
-                # or the imported data not being indexable
-                logger.warning(
-                    f"Couldn't read scan data, is {json_fpath} missing or corrupt? "
-                    "Attempting stitch with overlap value of 0.1"
-                )
-                overlap = 0.1
-            except KeyError:
-                logger.warning(
-                    "Value for overlap not found in scan data. "
-                    "Attempting stitch with overlap value of 0.1"
-                )
-                overlap = 0.1
-
-        if stitch_resize is None:
-            try:
-                with open(json_fpath, "r", encoding="utf-8") as data_file:
-                    data_loaded = json.load(data_file)
-
-                # Handle "capture resolution" being used to store the save resolution
-                # in old scans.
-                key = (
-                    "capture resolution"
-                    if "capture resolution" in data_loaded
-                    else "save_resolution"
-                )
-                save_resolution = data_loaded[key]
-                stitch_resize = STITCHING_RESOLUTION[0] / save_resolution[0]
-            except (json.decoder.JSONDecodeError, FileNotFoundError, TypeError):
-                # As there is no schema or pydantic model this should handle
-                # the file not being there, it not being json in the file,
-                # or the imported data not being indexable
-                logger.warning(
-                    f"Couldn't read scan data, is {json_fpath} missing or corrupt? "
-                    "Attempting stitch with resize value of 0.5"
-                )
-                stitch_resize = 0.5
-            except KeyError:
-                logger.warning(
-                    "Value for save resolution not found in scan data. "
-                    "Attempting stitch with resize value of 0.5"
-                )
-                stitch_resize = 0.5
-
+        scan_data_dict = self._scan_dir_manager.get_scan_data_dict(scan_name)
+        if scan_data_dict is None:
+            logger.warning("Couldn't read scan data it may be missing or corrupt.")
+        final_stitcher = stitching.FinalStitcher(
+            self._scan_dir_manager.img_dir_for(scan_name),
+            logger=logger,
+            overlap=overlap,
+            correlation_resize=stitch_resize,
+            stitch_tiff=self.stitch_tiff,
+            scan_data_dict=scan_data_dict,
+        )
         try:
-            self.run_subprocess(
-                logger=logger,
-                cancel=cancel,
-                cmd=[
-                    STITCHING_CMD,
-                    "--stitching_mode",
-                    "all",
-                    f"{tiff_arg}",
-                    "--stitch_dzi",
-                    "--minimum_overlap",
-                    f"{round(overlap * 0.9, 2)}",
-                    "--resize",
-                    f"{stitch_resize}",
-                    self._scan_dir_manager.img_dir_for(scan_name),
-                ],
-            )
+            final_stitcher.start(cancel)
         except lt.exceptions.InvocationCancelledError:
             # Sleep for 1 second just to allow invocation logs to pass to user.
             time.sleep(1)
-            pass
+        except SubprocessError as e:
+            self._scan_logger.error(f"Stitching failed: {e}", exc_info=e)
 
     @lt.thing_action
     def download_zip(

From 573fc20dec435cb18b29c80a8db3f11f9f93b4b1 Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Sun, 3 Aug 2025 16:42:38 +0100
Subject: [PATCH 03/18] Add new tests new scan directory function and for scan
 data serialisation

---
 .../scan_directories.py                       |  14 +-
 .../things/smart_scan.py                      |   2 +-
 tests/test_scan_data.py                       | 120 ++++++++++++++++++
 tests/test_scan_directories.py                |  79 ++++++++++++
 4 files changed, 209 insertions(+), 6 deletions(-)
 create mode 100644 tests/test_scan_data.py

diff --git a/src/openflexure_microscope_server/scan_directories.py b/src/openflexure_microscope_server/scan_directories.py
index 6533c589..122ff2ce 100644
--- a/src/openflexure_microscope_server/scan_directories.py
+++ b/src/openflexure_microscope_server/scan_directories.py
@@ -48,6 +48,8 @@ class ScanData(BaseModel):
     Properties that are not known until the end have ``None`` serialised as "Unknown"
     """
 
+    model_config = {"extra": "forbid"}
+
     # TODO: Docs for each variable
     # TODO: Make note about timestamp format. Is it too ambiguous?
     scan_name: str
@@ -58,8 +60,8 @@ class ScanData(BaseModel):
     autofocus_range: int
     autofocus_on: bool
     start_time: datetime
-    skip_background: str
-    stitch_automatically: str
+    skip_background: bool
+    stitch_automatically: bool
     # TODO: Think about changing stitch_resize name as it is NOT the resize for
     # just for correlation stitching
     stitch_resize: float
@@ -113,7 +115,7 @@ class ScanData(BaseModel):
         hrs = int(total_secs // 3600)
         mins = int((total_secs % 3600) // 60)
         secs = int((total_secs % 60))
-        return f"{hrs}:{mins}:{secs}"
+        return f"{hrs}:{mins:02}:{secs:02}"
 
     @field_validator("final_image_count", "scan_result", mode="before")
     @classmethod
@@ -124,9 +126,9 @@ class ScanData(BaseModel):
         return value
 
     @field_serializer("final_image_count", "scan_result")
-    def serialize_none_as_unknown(self, value: Optional[str | int]) -> str:
+    def serialize_none_as_unknown(self, value: Optional[str | int]) -> str | int:
         """Serialise None as "Unknown" for a more human readable result."""
-        return "Unknown" if value is None else str(value)
+        return "Unknown" if value is None else value
 
 
 class ScanDirectoryManager:
@@ -214,6 +216,8 @@ class ScanDirectoryManager:
         If no scan data JSON file is found, return None
         """
         scan_data_path = ScanDirectory(scan_name, self.base_dir).scan_data_path
+        if scan_data_path is None:
+            return None
         if not os.path.isfile(scan_data_path):
             return None
         return scan_data_path
diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py
index cb1ef83d..dc391731 100644
--- a/src/openflexure_microscope_server/things/smart_scan.py
+++ b/src/openflexure_microscope_server/things/smart_scan.py
@@ -324,7 +324,7 @@ class SmartScanThing(lt.Thing):
         Takes scan_result, a string that is either "success", "cancelled by user",
         or the error that ended the scan.
         """
-        self.scan_data.set_final_data(
+        self._scan_data.set_final_data(
             result=scan_result,
             final_image_count=self._scan_images_taken,
         )
diff --git a/tests/test_scan_data.py b/tests/test_scan_data.py
new file mode 100644
index 00000000..8c2ae113
--- /dev/null
+++ b/tests/test_scan_data.py
@@ -0,0 +1,120 @@
+"""Test the functionality in the scan_directories module."""
+
+from datetime import datetime, timedelta
+from copy import copy
+import json
+from math import floor
+
+from openflexure_microscope_server.scan_directories import ScanData
+
+MOCK_START_TIME = datetime(
+    year=2024,
+    month=12,
+    day=25,
+    hour=11,
+    minute=0,
+    second=0,
+    microsecond=1234,
+)
+
+MOCK_END_TIME = datetime(
+    year=2024,
+    month=12,
+    day=25,
+    hour=12,
+    minute=29,
+    second=3,
+    microsecond=5678,
+)
+
+
+def _fake_scan_data(**kwargs) -> ScanData:
+    """Make fake scan data, the start time is now. Final properties are not added.
+
+    :param **kwargs: Key word arguments can be used to override other values.
+    """
+    data_dict = {
+        "scan_name": "fake_scan_0001",
+        "overlap": 0.1,
+        "max_dist": 100000,
+        "dx": 100,
+        "dy": 100,
+        "autofocus_range": 1000,
+        "autofocus_on": True,
+        "start_time": copy(MOCK_START_TIME),
+        "skip_background": True,
+        "stitch_automatically": True,
+        "stitch_resize": 0.25,
+        "save_resolution": (1000, 1000),
+    }
+    for key, value in kwargs.items():
+        data_dict[key] = value
+    return ScanData(**data_dict)
+
+
+def test_set_final_data():
+    """Check that adding final data to a ScanData object works as expected."""
+    scan_data = _fake_scan_data()
+
+    assert scan_data.final_image_count is None
+    assert scan_data.duration is None
+    assert scan_data.scan_result is None
+
+    # Should set duration based of finishing at datetime.now()
+    scan_data.set_final_data(result="Success", final_image_count=123)
+    expected_duration = datetime.now() - MOCK_START_TIME
+    expected_duration_s = expected_duration.total_seconds()
+    scan_duration_s = scan_data.duration.total_seconds()
+
+    assert scan_data.final_image_count == 123
+    assert expected_duration_s - 1 < scan_duration_s < expected_duration_s + 1
+    assert scan_data.scan_result == "Success"
+
+
+def test_custom_serialisation():
+    """Check that the custom serialisation in ScanData works as expected."""
+    scan_data = _fake_scan_data()
+    # Serialise to string then load directly as json
+    scan_data_dict = json.loads(scan_data.model_dump_json())
+    assert scan_data_dict["start_time"] == "11_00_00-25_12_2024"
+    assert scan_data_dict["final_image_count"] == "Unknown"
+    assert scan_data_dict["duration"] == "Unknown"
+    assert scan_data_dict["scan_result"] == "Unknown"
+
+    scan_data.set_final_data(result="Success", final_image_count=123)
+    # Can't mock datetime.now as datetime is immutable. So just replace duraction
+    scan_data.duration = MOCK_END_TIME - scan_data.start_time
+    scan_data_dict = json.loads(scan_data.model_dump_json())
+    assert scan_data_dict["final_image_count"] == 123
+    assert scan_data_dict["duration"] == "1:29:03"
+    assert scan_data_dict["scan_result"] == "Success"
+
+
+def test_round_trip_not_finalised():
+    """Check that ScanData without final data can be serialised and deserialised."""
+    scan_data = _fake_scan_data()
+    scan_data_dict = json.loads(scan_data.model_dump_json())
+    scan_data_reloaded = ScanData(**scan_data_dict)
+
+    # For the round trip to be equal we must remove microseconds from the start
+    # time as they are not saved
+    scan_data.start_time = scan_data.start_time.replace(microsecond=0)
+    assert scan_data == scan_data_reloaded
+
+
+def test_round_trip_finalised():
+    """Check that finalised ScanData can be serialised and deserialised."""
+    scan_data = _fake_scan_data()
+    # Finalise the data.
+    scan_data.set_final_data(result="Success", final_image_count=123)
+
+    scan_data_dict = json.loads(scan_data.model_dump_json())
+    scan_data_reloaded = ScanData(**scan_data_dict)
+
+    # For the round trip to be equal we must remove microseconds from the start
+    # time and duration as they are not saved
+    scan_data.model_copy(update={})
+    scan_data.start_time = scan_data.start_time.replace(microsecond=0)
+    scan_data.duration = timedelta(seconds=floor(scan_data.duration.total_seconds()))
+
+    assert scan_data == scan_data_reloaded
diff --git a/tests/test_scan_directories.py b/tests/test_scan_directories.py
index 1f6c0609..3fb317cc 100644
--- a/tests/test_scan_directories.py
+++ b/tests/test_scan_directories.py
@@ -7,6 +7,7 @@ import shutil
 import logging
 import random
 import time
+import json
 from collections import namedtuple
 
 import pytest
@@ -17,8 +18,10 @@ from openflexure_microscope_server.scan_directories import (
     ScanInfo,
     get_files_in_zip,
     NotEnoughFreeSpaceError,
+    SCAN_DATA_FILENAME,
 )
 
+from .test_scan_data import _fake_scan_data
 from .utilities import assert_unique_of_length
 
 # A global logger to pass in as an Invocation Logger
@@ -281,6 +284,46 @@ def test_get_final_stitch():
     assert scan_dir.get_final_stitch_name() == fake_scan_name
 
 
+def test_get_scan_data_path():
+    """Check that a scan data path behaves as expected."""
+    _clear_scan_dir()
+    scan_dir_manager = ScanDirectoryManager(BASE_SCAN_DIR)
+    scan_dir = scan_dir_manager.new_scan_dir("fake_scan")
+    scan_name = scan_dir.name
+    expected_path = os.path.join(scan_dir.images_dir, SCAN_DATA_FILENAME)
+    # Asking the scan manager for the path will return None as there is no file.
+    assert scan_dir_manager.get_scan_data_path(scan_name) is None
+    # Asking the scan directly will return the location the file should be located
+    # this is so it can be created.
+    assert scan_dir.scan_data_path == expected_path
+
+    # Remove the images directory.
+    shutil.rmtree(scan_dir.images_dir)
+    assert scan_dir_manager.get_scan_data_path(scan_name) is None
+
+
+def test_get_scan_data_dict():
+    """Check that the dictionary for the scan data is returned, or None if doesn't exist."""
+    _clear_scan_dir()
+    scan_dir_manager = ScanDirectoryManager(BASE_SCAN_DIR)
+    scan_dir = scan_dir_manager.new_scan_dir("fake_scan")
+    scan_name = scan_dir.name
+    # Doesn't yet exist
+    assert scan_dir_manager.get_scan_data_dict(scan_name) is None
+
+    fake_data = {"foo": 1, "bar": "foobar"}
+    with open(scan_dir.scan_data_path, "w", encoding="utf-8") as json_file:
+        json.dump(fake_data, json_file)
+
+    # Should now be able to load this fake data from disk
+    assert scan_dir_manager.get_scan_data_dict(scan_name) == fake_data
+
+    # Check None is returned if the data cannot be read.
+    with open(scan_dir.scan_data_path, "w", encoding="utf-8") as json_file:
+        json_file.write("this is not json")
+    assert scan_dir_manager.get_scan_data_dict(scan_name) is None
+
+
 def test_empty_scan_info():
     """Test the scan info is correct even if the scan is empty."""
     _clear_scan_dir()
@@ -339,6 +382,42 @@ def test_zipping_scan_data():
             assert not file.endswith(".dzi")
 
 
+def test_saving_and_loading_scan_data():
+    """Test that scan data is saved and loaded as expected."""
+    _clear_scan_dir()
+    scan_dir_manager = ScanDirectoryManager(BASE_SCAN_DIR)
+    scan_dir = scan_dir_manager.new_scan_dir("fake_scan")
+    scan_name = scan_dir.name
+
+    # Should start without a scan data file.
+    assert not os.path.isfile(scan_dir.scan_data_path)
+    # Create
+    scan_data_obj = _fake_scan_data()
+    scan_dir.save_scan_data(scan_data_obj)
+    # File should now exist
+    assert os.path.isfile(scan_dir.scan_data_path)
+
+    # Dump the scan json to a string an reload it
+    # Note that more detailed checking of the dumping and loading of ScanData is in
+    # tests/test_scan_data.py
+    scan_data_dict = json.loads(scan_data_obj.model_dump_json())
+    # What is loaded from file should be the same as from dumping and loading.
+    assert scan_dir_manager.get_scan_data_dict(scan_name) == scan_data_dict
+
+
+def test_saving_scan_data_error():
+    """Test that saving scan data if there is no images directory raises FileNotFoundError."""
+    _clear_scan_dir()
+    scan_dir_manager = ScanDirectoryManager(BASE_SCAN_DIR)
+    scan_dir = scan_dir_manager.new_scan_dir("fake_scan")
+
+    # Remove the images directory.
+    shutil.rmtree(scan_dir.images_dir)
+    # Should raise FileNotFoundError.
+    with pytest.raises(FileNotFoundError):
+        scan_dir.save_scan_data(_fake_scan_data())
+
+
 def test_all_files():
     """Test all_files returns the path, and respects skipped directories."""
     _clear_scan_dir()

From a632ddee92690c6159ceb84a810ecb1eab368281 Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Sun, 3 Aug 2025 17:50:12 +0100
Subject: [PATCH 04/18] Add unit tests for generated stitching commands.

---
 .../stitching.py                              |   2 +-
 tests/test_stitching.py                       | 150 ++++++++++++++++++
 2 files changed, 151 insertions(+), 1 deletion(-)
 create mode 100644 tests/test_stitching.py

diff --git a/src/openflexure_microscope_server/stitching.py b/src/openflexure_microscope_server/stitching.py
index 27461e1e..2bc91357 100644
--- a/src/openflexure_microscope_server/stitching.py
+++ b/src/openflexure_microscope_server/stitching.py
@@ -17,7 +17,7 @@ import labthings_fastapi as lt
 STITCHING_CMD = "openflexure-stitch"
 STITCHING_RESOLUTION = (820, 616)
 
-DEFAULT_OVERLAP = 0.5
+DEFAULT_OVERLAP = 0.1
 DEFAULT_RESIZE = 0.5
 
 
diff --git a/tests/test_stitching.py b/tests/test_stitching.py
new file mode 100644
index 00000000..3e8d1d98
--- /dev/null
+++ b/tests/test_stitching.py
@@ -0,0 +1,150 @@
+"""Test that the code that talks to the external stitching process acts as expected.
+
+This does not actually run stitching. Instead it checks that the expected commands are
+generated, and the subprocess calling works as expected.
+"""
+
+import logging
+from copy import copy
+
+import pytest
+
+from openflexure_microscope_server.stitching import (
+    BaseStitcher,
+    PreviewStitcher,
+    FinalStitcher,
+    STITCHING_RESOLUTION,
+)
+
+# A global logger pretending to be an Invocation Logger
+LOGGER = logging.getLogger("mock-invocation_logger")
+FAKE_DIR = "a/dir/that/is/fake"
+
+
+def test_base_stitcher():
+    """Test the logic in BaseStitcher.
+
+    Pretty much the only logic in base stitcher is forming a command, and calculating
+    the min_overlap from overlap.
+
+    The BaseStitcher can't start as the start method is explicitly NotImplemented.
+    """
+    # Overlaps and expected minimum overlap to be in command line argument.
+    overlaps = [(0.1, "0.09"), (0.4, "0.36"), (0.8, "0.72")]
+
+    for overlap, min_overlap in overlaps:
+        expected_command = [
+            "openflexure-stitch",
+            "--stitching_mode",
+            "all",
+            "--minimum_overlap",
+            min_overlap,
+            "--resize",
+            "0.5",
+            FAKE_DIR,
+        ]
+        stitcher = BaseStitcher(FAKE_DIR, overlap=overlap, correlation_resize=0.5)
+        assert stitcher.command == expected_command
+        # check that a BaseSticher can't start
+        with pytest.raises(NotImplementedError):
+            stitcher.start()
+
+
+def test_preview_stitcher_command():
+    """Check preview stitcher command for a specific example."""
+    expected_command = [
+        "openflexure-stitch",
+        "--stitching_mode",
+        "preview_stitch",
+        "--minimum_overlap",
+        "0.09",
+        "--resize",
+        "0.5",
+        FAKE_DIR,
+    ]
+    stitcher = PreviewStitcher(FAKE_DIR, overlap=0.1, correlation_resize=0.5)
+    assert stitcher.command == expected_command
+
+
+FINAL_EXPECTED_COMMAND = [
+    "openflexure-stitch",
+    "--stitching_mode",
+    "all",
+    "--stitch-dzi",
+    "--no-stitch_tiff",
+    "--minimum_overlap",
+    "0.09",
+    "--resize",
+    "0.5",
+    FAKE_DIR,
+]
+
+
+def test_final_stitcher_command_defaults(caplog):
+    """Check the FinalStitcher stitches with expected default values.
+
+    It should warn when default values are used as they are a fallback.
+    """
+    n_logs = 0
+    # Test with no dictionary data and with irrelevant dictionary data.
+    with caplog.at_level(logging.WARNING):
+        for data_dict in [None, {"irrelevant": "data"}]:
+            stitcher = FinalStitcher(FAKE_DIR, logger=LOGGER, scan_data_dict=data_dict)
+            # Should log for overlap being None and correlation_resize being None
+            n_logs += 2
+            assert len(caplog.records) == n_logs
+        assert stitcher.command == FINAL_EXPECTED_COMMAND
+
+
+def test_final_stitcher_command_tiff(caplog):
+    """Check that the tiff can be requested."""
+    # Modify defaults
+    expected_command = copy(FINAL_EXPECTED_COMMAND)
+    expected_command[4] = "--stitch_tiff"
+    stitcher = FinalStitcher(FAKE_DIR, logger=LOGGER, stitch_tiff=True)
+    # Should log for overlap being None and correlation_resize being None
+    assert len(caplog.records) == 2
+    assert stitcher.command == expected_command
+
+
+def test_final_stitcher_command_set_val_directly():
+    """Check that values are set as expected when directly input."""
+    # Modify defaults
+    expected_command = copy(FINAL_EXPECTED_COMMAND)
+    expected_command[6] = "0.36"
+    expected_command[8] = "0.25"
+    # Test with no data dictionary, irrelevant data, and also the wrong data
+    # When wrong data is submitted, it should take the directly input data.
+    dict_vals = [
+        None,
+        {"irrelevant": "data"},
+        {"overlap": 0.2, "save_resolution": [5, 5]},
+    ]
+    for data_dict in dict_vals:
+        stitcher = FinalStitcher(
+            FAKE_DIR,
+            logger=LOGGER,
+            overlap=0.4,
+            correlation_resize=0.25,
+            scan_data_dict=data_dict,
+        )
+        assert stitcher.command == expected_command
+
+
+def test_final_stitcher_command_set_with_dict():
+    """Check that values are set as expected when set from a ScanData dictionary."""
+    # Modify defaults
+    expected_command = copy(FINAL_EXPECTED_COMMAND)
+    expected_command[6] = "0.36"
+    expected_command[8] = "0.25"
+    # Check same thing works with a dictionary, resize is calculated from the saved image
+    # resolution. Make 4x bigger than STITCHING_RESOLUTION to get 0.25
+    resolution = [dim * 4 for dim in STITCHING_RESOLUTION]
+    # Check legacy key as well as current one:
+    for resolution_key in ["save_resolution", "capture resolution"]:
+        stitcher = FinalStitcher(
+            FAKE_DIR,
+            logger=LOGGER,
+            scan_data_dict={"overlap": 0.4, resolution_key: resolution},
+        )
+        assert stitcher.command == expected_command

From 7569c7d6b249c47d4f056c93e557014d449b0d55 Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Sun, 3 Aug 2025 18:54:11 +0100
Subject: [PATCH 05/18] Add validation checks to prevent arbitrary code
 execution via stitching.

---
 .../scan_directories.py                       |  2 +
 .../stitching.py                              | 51 ++++++++++++-------
 .../utilities.py                              | 41 +++++++++++++++
 tests/test_scan_directories.py                | 10 ++++
 tests/test_stitching.py                       | 29 ++++++++++-
 5 files changed, 114 insertions(+), 19 deletions(-)

diff --git a/src/openflexure_microscope_server/scan_directories.py b/src/openflexure_microscope_server/scan_directories.py
index 122ff2ce..da93085e 100644
--- a/src/openflexure_microscope_server/scan_directories.py
+++ b/src/openflexure_microscope_server/scan_directories.py
@@ -12,6 +12,7 @@ from datetime import datetime, timedelta
 from pydantic import BaseModel, field_validator, field_serializer
 
 from openflexure_microscope_server.utilities import requires_lock
+from openflexure_microscope_server.utilities import make_name_safe
 
 IMG_DIR_NAME = "images"
 SCAN_ZERO_PAD_DIGITS = 4
@@ -257,6 +258,7 @@ class ScanDirectoryManager:
 
         For more explanation on the scan naming see `new_scan_dir`
         """
+        scan_name = make_name_safe(scan_name)
         # A regex with the scan name and a group for the numbers
         scan_regex = re.compile(
             "^" + scan_name + "_([0-9]{" + str(SCAN_ZERO_PAD_DIGITS) + "})$"
diff --git a/src/openflexure_microscope_server/stitching.py b/src/openflexure_microscope_server/stitching.py
index 2bc91357..ec4fe5b6 100644
--- a/src/openflexure_microscope_server/stitching.py
+++ b/src/openflexure_microscope_server/stitching.py
@@ -14,6 +14,8 @@ import time
 
 import labthings_fastapi as lt
 
+from openflexure_microscope_server.utilities import make_path_safe
+
 STITCHING_CMD = "openflexure-stitch"
 STITCHING_RESOLUTION = (820, 616)
 
@@ -21,6 +23,10 @@ DEFAULT_OVERLAP = 0.1
 DEFAULT_RESIZE = 0.5
 
 
+class StitcherValidationError(RuntimeError):
+    """The stitcher received values that it deems unsafe to create a command from."""
+
+
 class BaseStitcher:
     """A base stitching class for all stitchers. Don't initialise this directly."""
 
@@ -36,24 +42,49 @@ class BaseStitcher:
         # Set minimum overlap to 90% of the scan overlap to catch only images
         # directly adjacent, not images with overlapping corners.
         self.images_dir = images_dir
+        try:
+            overlap = float(overlap)
+            correlation_resize = float(correlation_resize)
+        except ValueError as e:
+            raise StitcherValidationError(
+                "Stitching inputs overlap or correlation_resize were not floats as "
+                "expected."
+            ) from e
+        self.validate_path()
+
         self.min_overlap = round(overlap * 0.9, 2)
-        self.correlation_resize = correlation_resize
+        self.correlation_resize = float(correlation_resize)
         self._mode = "all"
         self._extra_args = []
 
     @property
     def command(self) -> list[str]:
         """The command to run with subprocess.Popen."""
+        # Revalidate for good measure,
+        self.validate_path()
         # The command, and the mode
         initial_args = [STITCHING_CMD, "--stitching_mode", self._mode]
+        # Use float() just to really ensure that anything input is a float
         setting_args = [
             "--minimum_overlap",
-            f"{self.min_overlap}",
+            f"{float(self.min_overlap)}",
             "--resize",
-            f"{self.correlation_resize}",
+            f"{float(self.correlation_resize)}",
         ]
         return initial_args + self._extra_args + setting_args + [self.images_dir]
 
+    def validate_path(self):
+        """Check path is safe before making a command to run with subprocess.
+
+        This is essential for stopping arbitrary code execution.
+
+        :raises RuntimeError: if inputs are unsafe.
+        """
+        if self.images_dir != make_path_safe(self.images_dir):
+            raise StitcherValidationError(
+                "Invalid directory path contains unsafe characters."
+            )
+
     def start(self) -> None:
         """Start stitching a stitching process.
 
@@ -91,20 +122,6 @@ class PreviewStitcher(BaseStitcher):
 
         self._mode = "preview_stitch"
 
-    @property
-    def command(self) -> list[str]:
-        """The command to run with subprocess.Popen."""
-        return [
-            STITCHING_CMD,
-            "--stitching_mode",
-            "preview_stitch",
-            "--minimum_overlap",
-            f"{self.min_overlap}",
-            "--resize",
-            f"{self.correlation_resize}",
-            self.images_dir,
-        ]
-
     def start(self) -> None:
         """Start stitching a preview of the scan in a background subprocess.
 
diff --git a/src/openflexure_microscope_server/utilities.py b/src/openflexure_microscope_server/utilities.py
index 21099216..1031aa23 100644
--- a/src/openflexure_microscope_server/utilities.py
+++ b/src/openflexure_microscope_server/utilities.py
@@ -3,6 +3,7 @@
 from typing import TypeVar, Callable, ParamSpec
 import os
 import re
+import sys
 from threading import Thread
 import logging
 from importlib.metadata import version
@@ -126,6 +127,46 @@ def _wrap_and_catch_errors(target, error_buffer, *args, **kwargs):
         error_buffer.append(e)
 
 
+# Compiled regular expressions for unsafe characters
+_WINDOWS_UNSAFE_PATTERN = re.compile(r"[^a-zA-Z0-9_.\-:/\\]")
+_POSIX_UNSAFE_PATTERN = re.compile(r"[^a-zA-Z0-9_.\-/]")
+_NAME_UNSAFE_PATTERN = re.compile(r"[^a-zA-Z0-9_.\-]")
+
+
+def make_path_safe(unsafe_path_string: str) -> str:
+    """Replace unsafe characters in a file path with underscores, preserving separators.
+
+    This can be used to check that user inputs have not been concatenated into an
+    unsafe filepath.
+
+    This ensures compatibility across platforms by preserving only valid characters,
+    including path separators (forward slash on POSIX, and forward
+    slash/backslash/colon on Windows).
+
+    :param unsafe_path_string: The original path string to sanitise.
+    :returns: A version of the input string safe to use as a file path.
+    """
+    unsafe_character_pattern = (
+        _WINDOWS_UNSAFE_PATTERN
+        if sys.platform.startswith("win")
+        else _POSIX_UNSAFE_PATTERN
+    )
+    return unsafe_character_pattern.sub("_", unsafe_path_string)
+
+
+def make_name_safe(unsafe_name_string: str) -> str:
+    """Replace unsafe characters in a filename or identifier with underscores.
+
+    This excludes all path separators, ensuring the result is safe to use as a
+    standalone filename or identifier component.
+
+    :param unsafe_name_string: The original name string to sanitise.
+
+    :returns: A version of the input string safe to use as a file name or identifier.
+    """
+    return _NAME_UNSAFE_PATTERN.sub("_", unsafe_name_string)
+
+
 class VersionData(BaseModel):
     """A BaseModel containing the information about the server version.
 
diff --git a/tests/test_scan_directories.py b/tests/test_scan_directories.py
index 3fb317cc..04c4b2d1 100644
--- a/tests/test_scan_directories.py
+++ b/tests/test_scan_directories.py
@@ -160,6 +160,16 @@ def test_basic_directory_operations():
     assert not os.path.isdir(scan_path)
 
 
+def test_bad_scan_names():
+    """Check scan names with spaces, or worse BASH commands are not allowed."""
+    _clear_scan_dir()
+    scan_dir_manager = ScanDirectoryManager(BASE_SCAN_DIR)
+    scan_dir = scan_dir_manager.new_scan_dir("fake scan")
+    assert scan_dir.name == "fake_scan_0001"
+    scan_dir = scan_dir_manager.new_scan_dir("fake scan;rm -rf /;")
+    assert scan_dir.name == "fake_scan_rm_-rf____0001"
+
+
 def test_scan_sequence_and_listing():
     """Check created scans are added in order and listed correctly."""
     _clear_scan_dir()
diff --git a/tests/test_stitching.py b/tests/test_stitching.py
index 3e8d1d98..92189bd7 100644
--- a/tests/test_stitching.py
+++ b/tests/test_stitching.py
@@ -5,6 +5,7 @@ generated, and the subprocess calling works as expected.
 """
 
 import logging
+import os
 from copy import copy
 
 import pytest
@@ -14,11 +15,12 @@ from openflexure_microscope_server.stitching import (
     PreviewStitcher,
     FinalStitcher,
     STITCHING_RESOLUTION,
+    StitcherValidationError,
 )
 
 # A global logger pretending to be an Invocation Logger
 LOGGER = logging.getLogger("mock-invocation_logger")
-FAKE_DIR = "a/dir/that/is/fake"
+FAKE_DIR = os.path.join("a", "dir", "that", "is", "fake")
 
 
 def test_base_stitcher():
@@ -45,7 +47,7 @@ def test_base_stitcher():
         ]
         stitcher = BaseStitcher(FAKE_DIR, overlap=overlap, correlation_resize=0.5)
         assert stitcher.command == expected_command
-        # check that a BaseSticher can't start
+        # check that a BaseStitcher can't start
         with pytest.raises(NotImplementedError):
             stitcher.start()
 
@@ -148,3 +150,26 @@ def test_final_stitcher_command_set_with_dict():
             scan_data_dict={"overlap": 0.4, resolution_key: resolution},
         )
         assert stitcher.command == expected_command
+
+
+def _validation_error_tester(scan_path, **kwargs):
+    """Check each type of stitcher throws a validation error for the given init args."""
+    # If scan_data_dict is in the kwargs only test the scan_data_dict
+    if "scan_data_dict" not in kwargs:
+        with pytest.raises(StitcherValidationError):
+            BaseStitcher(scan_path, **kwargs).command
+        with pytest.raises(StitcherValidationError):
+            PreviewStitcher(scan_path, **kwargs).command
+    with pytest.raises(StitcherValidationError):
+        FinalStitcher(scan_path, logger=LOGGER, **kwargs).command
+
+
+def test_validation_error():
+    """Test a number of way to try to inject mallicious arguments into the stitcher.
+
+    The stitcher should throw a validation error each attempt.
+    """
+    _validation_error_tester("/dir;rm -rf /;", overlap=".2", correlation_resize=".25")
+    _validation_error_tester(FAKE_DIR, overlap=".2", correlation_resize=".25;rm -rf /;")
+    _validation_error_tester(FAKE_DIR, overlap=".2;rm -rf /;", correlation_resize=".25")
+    _validation_error_tester(FAKE_DIR, scan_data_dict={"overlap": ".2;rm -rf /;"})

From cf74ea135115a542fdc42a94d990996f45a77a1d Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Sun, 3 Aug 2025 21:29:22 +0100
Subject: [PATCH 06/18] More complete testing of code that runs stitching
 subprocess.

---
 .../stitching.py                              |  27 ++++-
 .../utilities.py                              |   1 +
 tests/mock_stitching/mock-stitch.py           |  19 +++
 tests/test_stitching.py                       | 110 +++++++++++++++++-
 4 files changed, 151 insertions(+), 6 deletions(-)
 create mode 100644 tests/mock_stitching/mock-stitch.py

diff --git a/src/openflexure_microscope_server/stitching.py b/src/openflexure_microscope_server/stitching.py
index ec4fe5b6..47c61cca 100644
--- a/src/openflexure_microscope_server/stitching.py
+++ b/src/openflexure_microscope_server/stitching.py
@@ -10,6 +10,7 @@ from typing import Optional, Any
 import threading
 import subprocess
 import os
+import shlex
 import time
 
 import labthings_fastapi as lt
@@ -27,6 +28,21 @@ class StitcherValidationError(RuntimeError):
     """The stitcher received values that it deems unsafe to create a command from."""
 
 
+def validate_command(cmd: list[str]):
+    """Validate that the command only characters that are allowed in a path.
+
+    The values in the commands should be numbers, commandline flags, paths, and
+    executables. All of these should be allowed by ``make_path_safe``.
+
+    :raises StitcherValidationError: if any element in the command is not safe.
+    """
+    for element in cmd:
+        if element != make_path_safe(element):
+            raise StitcherValidationError(
+                "Invalid stiching command: Contains unsafe characters."
+            )
+
+
 class BaseStitcher:
     """A base stitching class for all stitchers. Don't initialise this directly."""
 
@@ -62,8 +78,9 @@ class BaseStitcher:
         """The command to run with subprocess.Popen."""
         # Revalidate for good measure,
         self.validate_path()
+
         # The command, and the mode
-        initial_args = [STITCHING_CMD, "--stitching_mode", self._mode]
+        initial_args = shlex.split(STITCHING_CMD) + ["--stitching_mode", self._mode]
         # Use float() just to really ensure that anything input is a float
         setting_args = [
             "--minimum_overlap",
@@ -71,7 +88,9 @@ class BaseStitcher:
             "--resize",
             f"{float(self.correlation_resize)}",
         ]
-        return initial_args + self._extra_args + setting_args + [self.images_dir]
+        full_cmd = initial_args + self._extra_args + setting_args + [self.images_dir]
+        validate_command(full_cmd)
+        return full_cmd
 
     def validate_path(self):
         """Check path is safe before making a command to run with subprocess.
@@ -82,7 +101,7 @@ class BaseStitcher:
         """
         if self.images_dir != make_path_safe(self.images_dir):
             raise StitcherValidationError(
-                "Invalid directory path contains unsafe characters."
+                "Invalid directory path: Contains unsafe characters."
             )
 
     def start(self) -> None:
@@ -280,7 +299,7 @@ class FinalStitcher(BaseStitcher):
         if process.poll() == 0:
             self.logger.info("Stitching complete")
         else:
-            raise ChildProcessError(f"Subprocess {cmd[0]} exited with an error.")
+            raise ChildProcessError(f"Subprocess {STITCHING_CMD} exited with an error.")
 
     def _log_ongoing(
         self,
diff --git a/src/openflexure_microscope_server/utilities.py b/src/openflexure_microscope_server/utilities.py
index 1031aa23..07bc4fea 100644
--- a/src/openflexure_microscope_server/utilities.py
+++ b/src/openflexure_microscope_server/utilities.py
@@ -144,6 +144,7 @@ def make_path_safe(unsafe_path_string: str) -> str:
     slash/backslash/colon on Windows).
 
     :param unsafe_path_string: The original path string to sanitise.
+
     :returns: A version of the input string safe to use as a file path.
     """
     unsafe_character_pattern = (
diff --git a/tests/mock_stitching/mock-stitch.py b/tests/mock_stitching/mock-stitch.py
new file mode 100644
index 00000000..b4c9d16a
--- /dev/null
+++ b/tests/mock_stitching/mock-stitch.py
@@ -0,0 +1,19 @@
+"""CLI script used for testing subprocess calls that should go to openflexure-stitch."""
+
+import sys
+import time
+
+
+def main():
+    """Echo command-line arguments with a short delay between each."""
+    input_arguments = sys.argv[1:]
+    for arg in input_arguments:
+        # This is used to check we catch errors correctly.
+        if arg == "ERROR":
+            raise RuntimeError("I was told to do this.")
+        print(arg, flush=True)
+        time.sleep(0.2)
+
+
+if __name__ == "__main__":
+    main()
diff --git a/tests/test_stitching.py b/tests/test_stitching.py
index 92189bd7..a680f71f 100644
--- a/tests/test_stitching.py
+++ b/tests/test_stitching.py
@@ -7,9 +7,15 @@ generated, and the subprocess calling works as expected.
 import logging
 import os
 from copy import copy
+import uuid
+import re
+import threading
+from time import sleep
 
 import pytest
 
+import labthings_fastapi as lt
+
 from openflexure_microscope_server.stitching import (
     BaseStitcher,
     PreviewStitcher,
@@ -20,7 +26,9 @@ from openflexure_microscope_server.stitching import (
 
 # A global logger pretending to be an Invocation Logger
 LOGGER = logging.getLogger("mock-invocation_logger")
-FAKE_DIR = os.path.join("a", "dir", "that", "is", "fake")
+FAKE_DIR: list[str] = os.path.join("a", "dir", "that", "is", "fake")
+THIS_DIR: str = os.path.dirname(os.path.realpath(__file__))
+MOCK_STITCHER: str = os.path.join(THIS_DIR, "mock_stitching", "mock-stitch.py")
 
 
 def test_base_stitcher():
@@ -165,7 +173,7 @@ def _validation_error_tester(scan_path, **kwargs):
 
 
 def test_validation_error():
-    """Test a number of way to try to inject mallicious arguments into the stitcher.
+    """Test a number of way to try to inject malicious arguments into the stitcher.
 
     The stitcher should throw a validation error each attempt.
     """
@@ -173,3 +181,101 @@ def test_validation_error():
     _validation_error_tester(FAKE_DIR, overlap=".2", correlation_resize=".25;rm -rf /;")
     _validation_error_tester(FAKE_DIR, overlap=".2;rm -rf /;", correlation_resize=".25")
     _validation_error_tester(FAKE_DIR, scan_data_dict={"overlap": ".2;rm -rf /;"})
+
+
+def test_extra_arg_validation():
+    """Test that malicious arguments in extra_args also throw validation error.
+
+    Currently extra args do not come from user input. But this makes checks more
+    future-proof.
+    """
+    stitcher = FinalStitcher(FAKE_DIR, logger=LOGGER)
+    stitcher._extra_args = ["&&rm -rf /&&"]
+    with pytest.raises(StitcherValidationError):
+        stitcher.command
+
+
+def test_preview_stitching_command(caplog, mocker):
+    """Check the preview process runs in a background thread and doesn't log."""
+    mock_cmd = "python -m mock_command.py"
+
+    mocker.patch("openflexure_microscope_server.stitching.STITCHING_CMD", mock_cmd)
+
+    with caplog.at_level(logging.INFO):
+        stitcher = PreviewStitcher(FAKE_DIR, overlap=0.1, correlation_resize=0.5)
+        stitcher.start()
+        # Should take a second or so to run so will still be running
+        assert stitcher.running
+        # Can't start another time, instead get a runtime error
+        with pytest.raises(RuntimeError):
+            stitcher.start()
+        # Wait for it to complete
+        stitcher.wait()
+        # It is now not running
+        assert not stitcher.running
+        assert len(caplog.records) == 0
+
+
+def test_final_stitching_command(caplog, mocker):
+    """Check the final stitch runs until completion, and print statements are logged."""
+    mock_cmd = f"python {MOCK_STITCHER}"
+
+    mocker.patch("openflexure_microscope_server.stitching.STITCHING_CMD", mock_cmd)
+
+    with caplog.at_level(logging.INFO):
+        # Input values to prevent logging
+        stitcher = FinalStitcher(
+            FAKE_DIR, logger=LOGGER, overlap=0.1, correlation_resize=0.5
+        )
+        # For the final stitcher it will always complete before returning.
+        stitcher.start(cancel=lt.deps.CancelHook(id=uuid.uuid4()))
+        # The mock command logs the inputs so should be 1 less than
+        # FINAL_EXPECTED_COMMAND as the command itself is not logged. However, there
+        # is two extra logs, The date (before the program starts) and
+        # "Stitching complete" when it ends.
+        assert len(caplog.records) == len(FINAL_EXPECTED_COMMAND) + 1
+        for i, record in enumerate(caplog.records):
+            msg = record.message
+            if i == 0:
+                assert re.match(r"^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}", msg)
+            elif i == len(FINAL_EXPECTED_COMMAND):
+                assert msg.strip() == "Stitching complete"
+            else:
+                assert msg.strip() == FINAL_EXPECTED_COMMAND[i]
+
+
+def test_final_stitching_command_cancelled(caplog, mocker):
+    """Check that final stitch can be cancelled."""
+    mock_cmd = f"python {MOCK_STITCHER}"
+
+    mocker.patch("openflexure_microscope_server.stitching.STITCHING_CMD", mock_cmd)
+
+    with caplog.at_level(logging.INFO):
+        stitcher = FinalStitcher(FAKE_DIR, logger=LOGGER)
+        cancel_hook = lt.deps.CancelHook(id=uuid.uuid4())
+
+        # Start stitching in a thread.
+        thread = threading.Thread(target=stitcher.start, kwargs={"cancel": cancel_hook})
+        thread.start()
+        # Sleep long enough for at least 1 log.
+        sleep(0.5)
+        # use set() to cancel!
+        cancel_hook.set()
+        thread.join()
+        assert len(caplog.records) < len(FINAL_EXPECTED_COMMAND) + 1
+        assert caplog.records[-1].message == "Stitching cancelled by user"
+
+
+def test_final_stitching_command_error(caplog, mocker):
+    """Check that ChildProcessError is raised if the final stitch errors."""
+    mock_cmd = f"python {MOCK_STITCHER}"
+
+    mocker.patch("openflexure_microscope_server.stitching.STITCHING_CMD", mock_cmd)
+
+    with caplog.at_level(logging.INFO):
+        stitcher = FinalStitcher(FAKE_DIR, logger=LOGGER)
+        # Send in the argument ERROR to mock-stitch and it will raise an error rather
+        # than echo.
+        stitcher._extra_args = ["ERROR"]
+        with pytest.raises(ChildProcessError):
+            stitcher.start(cancel=lt.deps.CancelHook(id=uuid.uuid4()))

From d7a7ac4c7d49b5ac8d9a087ccf42f746f19f1715 Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Mon, 4 Aug 2025 00:55:21 +0100
Subject: [PATCH 07/18] More complete testing of code that runs stitching
 subprocess.

---
 .../scan_directories.py                       |   2 +-
 .../things/smart_scan.py                      |  29 +----
 tests/mock_things/mock_camera.py              |  20 ++--
 tests/test_scan_data.py                       |   2 +-
 tests/test_smart_scan.py                      | 110 +++++++++++++++++-
 5 files changed, 126 insertions(+), 37 deletions(-)

diff --git a/src/openflexure_microscope_server/scan_directories.py b/src/openflexure_microscope_server/scan_directories.py
index da93085e..d0ec6e91 100644
--- a/src/openflexure_microscope_server/scan_directories.py
+++ b/src/openflexure_microscope_server/scan_directories.py
@@ -58,7 +58,7 @@ class ScanData(BaseModel):
     max_dist: int
     dx: int
     dy: int
-    autofocus_range: int
+    autofocus_dz: int
     autofocus_on: bool
     start_time: datetime
     skip_background: bool
diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py
index dc391731..28782c19 100644
--- a/src/openflexure_microscope_server/things/smart_scan.py
+++ b/src/openflexure_microscope_server/things/smart_scan.py
@@ -101,7 +101,6 @@ class SmartScanThing(lt.Thing):
         self._ongoing_scan: Optional[scan_directories.ScanDirectory] = None
         # TODO see if starting position can go into ScanData
         self._starting_position: Optional[Mapping[str, int]] = None
-        self._capture_thread: Optional[ErrorCapturingThread] = None
         self._scan_images_taken: Optional[int] = None
         # TODO Scan data is a dict during refactoring, should become a dataclass
         self._scan_data: Optional[scan_directories.ScanData] = None
@@ -138,7 +137,6 @@ class SmartScanThing(lt.Thing):
         # TODO check if metadata_getter this can be removed without error?
         self._metadata_getter = metadata_getter
         self._csm = csm
-        self._capture_thread = None
         self._scan_images_taken = 0
 
         # Set _scan_data to None. This is needed just in case an exception is raised
@@ -171,7 +169,6 @@ class SmartScanThing(lt.Thing):
             self._cam = None
             self._metadata_getter = None
             self._csm = None
-            self._capture_thread = None
             self._ongoing_scan = None
             self._scan_images_taken = None
             self._scan_data = None
@@ -347,10 +344,6 @@ class SmartScanThing(lt.Thing):
         scan should be stitched and whether  the microscope should return to the
         starting x,y,z position.
         """
-        # Used to check if finally was reached via exception (except
-        # cancel by user)
-        scan_successful = True
-
         try:
             self._cam.start_streaming(main_resolution=(3280, 2464))
             self._scan_data = self._collect_scan_data()
@@ -370,13 +363,11 @@ class SmartScanThing(lt.Thing):
             self._save_final_scan_data(scan_result="success")
 
         except lt.exceptions.InvocationCancelledError:
-            scan_successful = False
             # Reset the cancel event so it can be thrown again
             self._cancel.clear()
             self._scan_logger.info("Stopping scan because it was cancelled.")
             self._save_final_scan_data(scan_result="cancelled by user")
         except scan_directories.NotEnoughFreeSpaceError as e:
-            scan_successful = False
             self._save_final_scan_data(scan_result=str(e))
             self._scan_logger.error(
                 f"Stopping scan to avoid filling up the disk: {e}",
@@ -384,7 +375,6 @@ class SmartScanThing(lt.Thing):
             )
             raise e
         except Exception as e:
-            scan_successful = False
             self._save_final_scan_data(scan_result=str(e))
             self._scan_logger.error(
                 f"The scan stopped because of an error: {e} "
@@ -397,21 +387,7 @@ class SmartScanThing(lt.Thing):
 
             # Start streaming in the default resolution again as soon as possible
             self._cam.start_streaming()
-            if self._capture_thread:
-                # If the capture thread had an error, we capture it here
-                try:
-                    self._capture_thread.join()
-                except Exception as e:
-                    # If the scan has already ended due to an exception,
-                    # ignore any exceptions. If it appeared to be successful,
-                    # log the error.
-                    if scan_successful:
-                        self._scan_logger.error(
-                            "The scan appears to have completed successfully, however "
-                            f"the final capture raised the following error: {e}."
-                            "Attempting to stitch and archive images.",
-                            exc_info=e,
-                        )
+
 
         # This is what happens if the scan completes successfully or the
         # user cancels it.
@@ -508,7 +484,8 @@ class SmartScanThing(lt.Thing):
 
         self._scan_logger.info("Waiting for background processes to finish...")
 
-        self._preview_stitcher.wait()
+        if self._preview_stitcher is not None:
+            self._preview_stitcher.wait()
 
         if self._scan_data.stitch_automatically:
             self._scan_logger.info("Stitching final image (may take some time)...")
diff --git a/tests/mock_things/mock_camera.py b/tests/mock_things/mock_camera.py
index 81104dde..59b947f6 100644
--- a/tests/mock_things/mock_camera.py
+++ b/tests/mock_things/mock_camera.py
@@ -11,18 +11,24 @@ from openflexure_microscope_server.background_detect import (
     BackgroundDetectorStatus,
     ColourChannelDetectSettings,
 )
+from unittest.mock import Mock, PropertyMock
 
 
-class MockCameraThing:
+class MockCameraThing(Mock):
     """A mock camera Thing that imports no code from ``BaseCamera``.
 
     The class needs functionality added to it over time as more complex
-    mocking is needed. It imports no code from ``BaseCamera or any other
+    mocking is needed. It imports no code from ``BaseCamera`` or any other
     camera Thing, so that coverage is not artificially inflated.
     """
 
-    background_detector_status = BackgroundDetectorStatus(
-        ready=True,
-        settings=ColourChannelDetectSettings().model_dump(),
-        settings_schema={"fake": "schema"},
-    )
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+
+        self.background_detector_status = PropertyMock(
+            return_value=BackgroundDetectorStatus(
+                ready=True,
+                settings=ColourChannelDetectSettings().model_dump(),
+                settings_schema={"fake": "schema"},
+            )
+        )
diff --git a/tests/test_scan_data.py b/tests/test_scan_data.py
index 8c2ae113..fae70236 100644
--- a/tests/test_scan_data.py
+++ b/tests/test_scan_data.py
@@ -39,7 +39,7 @@ def _fake_scan_data(**kwargs) -> ScanData:
         "max_dist": 100000,
         "dx": 100,
         "dy": 100,
-        "autofocus_range": 1000,
+        "autofocus_dz": 1000,
         "autofocus_on": True,
         "start_time": copy(MOCK_START_TIME),
         "skip_background": True,
diff --git a/tests/test_smart_scan.py b/tests/test_smart_scan.py
index 1a0be750..90d3d242 100644
--- a/tests/test_smart_scan.py
+++ b/tests/test_smart_scan.py
@@ -18,6 +18,7 @@ import tempfile
 import os
 import shutil
 import logging
+from datetime import datetime
 
 from fastapi import HTTPException
 import pytest
@@ -26,6 +27,7 @@ from openflexure_microscope_server.things.smart_scan import (
     SmartScanThing,
     ScanNotRunningError,
 )
+from openflexure_microscope_server.scan_directories import ScanData
 
 from .mock_things.mock_csm import MockCSMThing
 from .mock_things.mock_autofocus import MockAutoFocusThing
@@ -191,7 +193,6 @@ def _run_only_outer_scan(adjust_initial_state: Optional[Callable] = None):
             assert self._cam is cam_mock
             assert self._metadata_getter is meta_mock
             assert self._csm is csm_mock
-            assert self._capture_thread is None
             assert self._scan_images_taken == 0
 
     # mock smart scan thing
@@ -224,7 +225,6 @@ def _run_only_outer_scan(adjust_initial_state: Optional[Callable] = None):
     assert mock_ss_thing._cam is None
     assert mock_ss_thing._metadata_getter is None
     assert mock_ss_thing._csm is None
-    assert mock_ss_thing._capture_thread is None
     assert mock_ss_thing._scan_images_taken is None
 
     # Return the mock thing for further state testing, and the
@@ -253,3 +253,109 @@ def test_outer_scan_wo_sample_skip():
     assert exec_info is None
     # Checked the mocked _run_scan was run exactly once
     assert mock_ss_thing.mock_call_count["_run_scan"] == 1
+
+
+def _expected_scan_data():
+    """Return the expected ScanData object for a SmartScan with default properties."""
+    expected_dict = {
+        "scan_name": "test_name_0001",
+        "overlap": 0.45,
+        "max_dist": 45000,
+        "dx": 100,
+        "dy": 100,
+        "autofocus_dz": 1000,
+        "autofocus_on": True,
+        "skip_background": True,
+        "stitch_automatically": True,
+        "stitch_resize": 0.5,
+        "save_resolution": (1640, 1232),
+    }
+    return ScanData(start_time=datetime.now(), **expected_dict)
+
+
+@pytest.fixture
+def scan_thing_mocked_for_scan_data(smart_scan_thing, mocker):
+    """A scan thing that is mocked so that _collect_scan_data will return."""
+    # Give the scan thing a scan invocation logger so it thinks a scan is running.
+    smart_scan_thing._scan_logger = LOGGER
+    mocker.patch.object(
+        smart_scan_thing, "_calc_displacement_from_test_image", return_value=[100, 100]
+    )
+    mock_ongoing_scan = mocker.Mock()
+    type(mock_ongoing_scan).name = mocker.PropertyMock(return_value="test_name_0001")
+    type(mock_ongoing_scan).images_dir = mocker.PropertyMock(
+        return_value="scans/test_name_0001/images/"
+    )
+    smart_scan_thing._ongoing_scan = mock_ongoing_scan
+    return smart_scan_thing
+
+
+def test_collect_scan_data(scan_thing_mocked_for_scan_data):
+    """Run _collect_scan_data, and check the ScanData object has the expected values."""
+    scan_thing = scan_thing_mocked_for_scan_data
+
+    data = scan_thing._collect_scan_data()
+    expected_data = _expected_scan_data()
+    time_diff = expected_data.start_time - data.start_time
+    assert abs(time_diff.total_seconds()) < 1
+    # Set times to exactly the same before final comparison
+    expected_data.start_time = data.start_time
+    assert data == expected_data
+
+
+def test_save_final_scan_data(scan_thing_mocked_for_scan_data):
+    """Run _save_final_scan_data, check save is called with final results in ScanData."""
+    scan_thing = scan_thing_mocked_for_scan_data
+
+    scan_thing._scan_data = scan_thing._collect_scan_data()
+    scan_thing._scan_images_taken = 44
+    scan_thing._save_final_scan_data("Mocked!")
+    # _ongoing_scan is a mock so we can check that save_scan data was called and get
+    # the value
+    scan_thing._ongoing_scan.save_scan_data.assert_called()
+    final_data = scan_thing._ongoing_scan.save_scan_data.call_args[0][0]
+    assert isinstance(final_data, ScanData)
+    assert final_data.scan_result == "Mocked!"
+    assert final_data.final_image_count == 44
+    assert final_data.duration.total_seconds() < 1
+
+
+@pytest.fixture
+def scan_thing_mocked_for_run_scan(scan_thing_mocked_for_scan_data, mocker):
+    """Return a scan_thing mocked so _run_scan works.
+
+    main_scan_loop(), _return_to_starting_position(), _perform_final_stitch(), and
+    purge_empty_scans() are Mocks
+    _cam is a MockCameraThing
+    """
+    scan_thing = scan_thing_mocked_for_scan_data
+    scan_thing._cam = MockCameraThing()
+    scan_thing._scan_images_taken = 0
+    mocker.patch.object(scan_thing, "_main_scan_loop")
+    mocker.patch.object(scan_thing, "_return_to_starting_position")
+    mocker.patch.object(scan_thing, "_perform_final_stitch")
+    mocker.patch.object(scan_thing, "purge_empty_scans")
+
+    return scan_thing
+
+
+def test_run_scan(scan_thing_mocked_for_run_scan):
+    """Run _save_final_scan_data, check save is called with final results in ScanData."""
+    scan_thing = scan_thing_mocked_for_run_scan
+    scan_thing._scan_data = scan_thing._run_scan()
+
+    scan_thing._cam.start_streaming.assert_called()
+    # Save scan data is called at the start and the end!
+    assert scan_thing._ongoing_scan.save_scan_data.call_count == 2
+    # The scan was a success
+    final_scan_data = scan_thing._ongoing_scan.save_scan_data.call_args[0][0]
+    assert final_scan_data.scan_result == "success"
+
+    # The preview stitcher object should still exist. And images dir should be set.
+    assert scan_thing._preview_stitcher.images_dir == "scans/test_name_0001/images/"
+
+    # Other calls should be:
+    scan_thing._main_scan_loop.assert_called()
+    scan_thing._return_to_starting_position.assert_called()
+    scan_thing._perform_final_stitch.assert_called()
+    scan_thing.purge_empty_scans.assert_called()

From 82691a6b7867055b0c8e1621a234dfff9cf02ca3 Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Mon, 4 Aug 2025 00:57:18 +0100
Subject: [PATCH 08/18] Tweak error reporting in _run_scan and add a lot more
 testing for _run_scan

---
 .../things/smart_scan.py                      |   7 +-
 tests/mock_things/mock_camera.py              |   1 +
 tests/test_smart_scan.py                      | 175 ++++++++++++++++--
 3 files changed, 162 insertions(+), 21 deletions(-)

diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py
index 28782c19..20e1e673 100644
--- a/src/openflexure_microscope_server/things/smart_scan.py
+++ b/src/openflexure_microscope_server/things/smart_scan.py
@@ -20,7 +20,6 @@ from PIL import Image
 
 import labthings_fastapi as lt
 
-from openflexure_microscope_server.utilities import ErrorCapturingThread
 from openflexure_microscope_server import scan_directories
 from openflexure_microscope_server import scan_planners
 from openflexure_microscope_server import stitching
@@ -368,14 +367,15 @@ class SmartScanThing(lt.Thing):
             self._scan_logger.info("Stopping scan because it was cancelled.")
             self._save_final_scan_data(scan_result="cancelled by user")
         except scan_directories.NotEnoughFreeSpaceError as e:
-            self._save_final_scan_data(scan_result=str(e))
+            self._save_final_scan_data(scan_result=f"NotEnoughFreeSpaceError: {e}")
             self._scan_logger.error(
                 f"Stopping scan to avoid filling up the disk: {e}",
                 exc_info=e,
             )
             raise e
         except Exception as e:
-            self._save_final_scan_data(scan_result=str(e))
+            err_name = type(e).__name__
+            self._save_final_scan_data(scan_result=f"{err_name}: {e}")
             self._scan_logger.error(
                 f"The scan stopped because of an error: {e} "
                 "Attempting to stitch and archive the images acquired so far.",
@@ -388,7 +388,6 @@ class SmartScanThing(lt.Thing):
             # Start streaming in the default resolution again as soon as possible
             self._cam.start_streaming()
 
-
         # This is what happens if the scan completes successfully or the
         # user cancels it.
         self._return_to_starting_position()
diff --git a/tests/mock_things/mock_camera.py b/tests/mock_things/mock_camera.py
index 59b947f6..a817a47d 100644
--- a/tests/mock_things/mock_camera.py
+++ b/tests/mock_things/mock_camera.py
@@ -23,6 +23,7 @@ class MockCameraThing(Mock):
     """
 
     def __init__(self, *args, **kwargs):
+        """Initialise the mock camera, args and kwargs are the mock args and kwargs."""
         super().__init__(*args, **kwargs)
 
         self.background_detector_status = PropertyMock(
diff --git a/tests/test_smart_scan.py b/tests/test_smart_scan.py
index 90d3d242..d71336e1 100644
--- a/tests/test_smart_scan.py
+++ b/tests/test_smart_scan.py
@@ -23,11 +23,16 @@ from datetime import datetime
 from fastapi import HTTPException
 import pytest
 
+from labthings_fastapi.exceptions import InvocationCancelledError
+
 from openflexure_microscope_server.things.smart_scan import (
     SmartScanThing,
     ScanNotRunningError,
 )
-from openflexure_microscope_server.scan_directories import ScanData
+from openflexure_microscope_server.scan_directories import (
+    ScanData,
+    NotEnoughFreeSpaceError,
+)
 
 from .mock_things.mock_csm import MockCSMThing
 from .mock_things.mock_autofocus import MockAutoFocusThing
@@ -275,7 +280,7 @@ def _expected_scan_data():
 
 @pytest.fixture
 def scan_thing_mocked_for_scan_data(smart_scan_thing, mocker):
-    """A scan thing that is mocked so that _collect_scan_data will return."""
+    """Return a scan thing that is mocked so that _collect_scan_data will run."""
     # Give the scan thing a scan invocation logger so it thinks a scan is running.
     smart_scan_thing._scan_logger = LOGGER
     mocker.patch.object(
@@ -331,6 +336,7 @@ def scan_thing_mocked_for_run_scan(scan_thing_mocked_for_scan_data, mocker):
     scan_thing = scan_thing_mocked_for_scan_data
     scan_thing._cam = MockCameraThing()
     scan_thing._scan_images_taken = 0
+    mocker.patch.object(scan_thing, "_cancel")
     mocker.patch.object(scan_thing, "_main_scan_loop")
     mocker.patch.object(scan_thing, "_return_to_starting_position")
     mocker.patch.object(scan_thing, "_perform_final_stitch")
@@ -339,23 +345,158 @@ def scan_thing_mocked_for_run_scan(scan_thing_mocked_for_scan_data, mocker):
     return scan_thing
 
 
-def test_run_scan(scan_thing_mocked_for_run_scan):
-    """Run _save_final_scan_data, check save is called with final results in ScanData."""
-    scan_thing = scan_thing_mocked_for_run_scan
-    scan_thing._scan_data = scan_thing._run_scan()
+def check_run_scan(scan_thing, caplog, expected_exception=None):
+    """Run _run_scan with a mocked scan_thing, capture logs and call counts.
 
-    scan_thing._cam.start_streaming.assert_called()
-    # Save scan data is called at the start and the end!
-    assert scan_thing._ongoing_scan.save_scan_data.call_count == 2
-    # The scan was a success
-    final_scan_data = scan_thing._ongoing_scan.save_scan_data.call_args[0][0]
-    assert final_scan_data.scan_result == "success"
+    This can be used to check how run_scan executes in different exit modes.
+    A separate tests is above for scan completing with no exception.
 
+    :returns: a tuple of:
+
+        * The final result as reported in scan_data
+        * The logging records
+        * a dictionary of call counts
+    """
+    if expected_exception is None:
+        with caplog.at_level(logging.WARNING):
+            scan_thing._scan_data = scan_thing._run_scan()
+    else:
+        with pytest.raises(expected_exception), caplog.at_level(logging.WARNING):
+            scan_thing._scan_data = scan_thing._run_scan()
     # The preview stitcher object should still exist. And images dir should be set.
     assert scan_thing._preview_stitcher.images_dir == "scans/test_name_0001/images/"
 
-    # Other calls should be:
-    scan_thing._main_scan_loop.assert_called()
-    scan_thing._return_to_starting_position.assert_called()
-    scan_thing._perform_final_stitch.assert_called()
-    scan_thing.purge_empty_scans.assert_called()
+    final_scan_data = scan_thing._ongoing_scan.save_scan_data.call_args[0][0]
+    calls = {
+        "cam_start_streaming_calls": scan_thing._cam.start_streaming.call_count,
+        "main_scan_loop_calls": scan_thing._main_scan_loop.call_count,
+        "return_to_start_calls": scan_thing._return_to_starting_position.call_count,
+        "perform_final_stitch_calls": scan_thing._perform_final_stitch.call_count,
+        "purge_empty_scans_calls": scan_thing.purge_empty_scans.call_count,
+        "save_scan_data_calls": scan_thing._ongoing_scan.save_scan_data.call_count,
+    }
+    return final_scan_data.scan_result, caplog.records, calls
+
+
+def test_run_scan(scan_thing_mocked_for_run_scan, caplog):
+    """Run _save_final_scan_data, check save is called with final results in ScanData."""
+    result, logs, calls = check_run_scan(scan_thing_mocked_for_run_scan, caplog)
+
+    assert result == "success"
+    assert len(logs) == 0
+
+    expected_calls_numbers = {
+        "cam_start_streaming_calls": 2,
+        "main_scan_loop_calls": 1,
+        "return_to_start_calls": 1,
+        "perform_final_stitch_calls": 1,
+        "purge_empty_scans_calls": 1,
+        "save_scan_data_calls": 2,
+    }
+    assert calls == expected_calls_numbers
+
+
+def test_run_scan_wrong_image_value(scan_thing_mocked_for_run_scan, caplog):
+    """Check correct methods called if _scan_images_taken doesn't start at 0.
+
+    This should never happen but shows a good clear path for an error before
+    _main_scan_loop starts.
+    """
+    scan_thing = scan_thing_mocked_for_run_scan
+    scan_thing._scan_images_taken = 2
+
+    result, logs, calls = check_run_scan(scan_thing, caplog, RuntimeError)
+
+    assert result.startswith("RuntimeError:")
+    assert len(logs) == 1
+    assert logs[0].levelno == logging.ERROR
+
+    # Main loop not run, nor are return to start, final stitch, or purging of empty
+    # scans. Save scan data is still called twice
+    expected_calls_numbers = {
+        "cam_start_streaming_calls": 2,
+        "main_scan_loop_calls": 0,
+        "return_to_start_calls": 0,
+        "perform_final_stitch_calls": 0,
+        "purge_empty_scans_calls": 0,
+        "save_scan_data_calls": 2,
+    }
+    assert calls == expected_calls_numbers
+
+
+def test_run_scan_err_in_main_loop(scan_thing_mocked_for_run_scan, caplog, mocker):
+    """Check correct methods called if main_loop errors."""
+    scan_thing = scan_thing_mocked_for_run_scan
+    mocker.patch.object(
+        scan_thing, "_main_scan_loop", side_effect=FileNotFoundError("mocked")
+    )
+
+    result, logs, calls = check_run_scan(scan_thing, caplog, FileNotFoundError)
+
+    assert result.startswith("FileNotFoundError:")
+    assert len(logs) == 1
+    assert logs[0].levelno == logging.ERROR
+
+    # Main loop not run, nor are return to start, final stitch, or purging of empty
+    # scans. Save scan data is still called twice
+    expected_calls_numbers = {
+        "cam_start_streaming_calls": 2,
+        "main_scan_loop_calls": 1,
+        "return_to_start_calls": 0,
+        "perform_final_stitch_calls": 0,
+        "purge_empty_scans_calls": 0,
+        "save_scan_data_calls": 2,
+    }
+    assert calls == expected_calls_numbers
+
+
+def test_run_scan_cancelled(scan_thing_mocked_for_run_scan, caplog, mocker):
+    """Check correct methods called scan is cancelled."""
+    scan_thing = scan_thing_mocked_for_run_scan
+    mocker.patch.object(
+        scan_thing, "_main_scan_loop", side_effect=InvocationCancelledError()
+    )
+
+    result, logs, calls = check_run_scan(scan_thing, caplog)
+
+    assert result == "cancelled by user"
+    # No logs at warning level.
+    assert len(logs) == 0
+
+    # Main loop not run, nor are return to start, final stitch, or purging of empty
+    # scans. Save scan data is still called twice
+    expected_calls_numbers = {
+        "cam_start_streaming_calls": 2,
+        "main_scan_loop_calls": 1,
+        "return_to_start_calls": 1,
+        "perform_final_stitch_calls": 1,
+        "purge_empty_scans_calls": 1,
+        "save_scan_data_calls": 2,
+    }
+    assert calls == expected_calls_numbers
+
+
+def test_run_scan_fill_disk(scan_thing_mocked_for_run_scan, caplog, mocker):
+    """Check correct methods called if disk fills up."""
+    scan_thing = scan_thing_mocked_for_run_scan
+    mocker.patch.object(
+        scan_thing, "_main_scan_loop", side_effect=NotEnoughFreeSpaceError()
+    )
+
+    result, logs, calls = check_run_scan(scan_thing, caplog, NotEnoughFreeSpaceError)
+
+    assert result.startswith("NotEnoughFreeSpaceError:")
+    assert len(logs) == 1
+    assert logs[0].levelno == logging.ERROR
+
+    # Main loop not run, nor are return to start, final stitch, or purging of empty
+    # scans. Save scan data is still called twice
+    expected_calls_numbers = {
+        "cam_start_streaming_calls": 2,
+        "main_scan_loop_calls": 1,
+        "return_to_start_calls": 0,
+        "perform_final_stitch_calls": 0,
+        "purge_empty_scans_calls": 0,
+        "save_scan_data_calls": 2,
+    }
+    assert calls == expected_calls_numbers

From 4e9b07c78fa2abead1b0539a73e7cc970296c79a Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Mon, 4 Aug 2025 10:09:39 +0100
Subject: [PATCH 09/18] Consolidate information into ScanData, remove ad-hoc
 differences for specific error in _run_scan

---
 .../scan_directories.py                       | 13 ++--
 .../things/smart_scan.py                      | 65 +++++++------------
 tests/test_scan_data.py                       | 19 ++++--
 tests/test_smart_scan.py                      | 59 +++++------------
 4 files changed, 56 insertions(+), 100 deletions(-)

diff --git a/src/openflexure_microscope_server/scan_directories.py b/src/openflexure_microscope_server/scan_directories.py
index d0ec6e91..9f3ad9ce 100644
--- a/src/openflexure_microscope_server/scan_directories.py
+++ b/src/openflexure_microscope_server/scan_directories.py
@@ -1,6 +1,6 @@
 """Functionality to manage file system operations for scan directories."""
 
-from typing import Optional, Any
+from typing import Optional, Any, Mapping
 import os
 import re
 import shutil
@@ -54,6 +54,7 @@ class ScanData(BaseModel):
     # TODO: Docs for each variable
     # TODO: Make note about timestamp format. Is it too ambiguous?
     scan_name: str
+    starting_position: Mapping[str, int]
     overlap: float
     max_dist: int
     dx: int
@@ -67,18 +68,16 @@ class ScanData(BaseModel):
     # just for correlation stitching
     stitch_resize: float
     save_resolution: tuple[int, int]
-    final_image_count: Optional[int] = None
+    image_count: int = 0
     duration: Optional[timedelta] = None
     scan_result: Optional[str] = None
 
-    def set_final_data(self, result: str, final_image_count: int):
+    def set_final_data(self, result: str):
         """Set the final data for the scan, scan duration is automatically calculated.
 
         :param result: A string describing the result.
-        :param final_image_count: The total number of images captures.
         """
         self.duration = datetime.now() - self.start_time
-        self.final_image_count = final_image_count
         self.scan_result = result
 
     @field_validator("start_time", mode="before")
@@ -118,7 +117,7 @@ class ScanData(BaseModel):
         secs = int((total_secs % 60))
         return f"{hrs}:{mins:02}:{secs:02}"
 
-    @field_validator("final_image_count", "scan_result", mode="before")
+    @field_validator("scan_result", mode="before")
     @classmethod
     def parse_unknown_as_none(cls, value: Optional[str | int]) -> Optional[str | int]:
         """Validate the string "Unknown" as None."""
@@ -126,7 +125,7 @@ class ScanData(BaseModel):
             return None
         return value
 
-    @field_serializer("final_image_count", "scan_result")
+    @field_serializer("scan_result")
     def serialize_none_as_unknown(self, value: Optional[str | int]) -> str | int:
         """Serialise None as "Unknown" for a more human readable result."""
         return "Unknown" if value is None else value
diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py
index 20e1e673..a01d014f 100644
--- a/src/openflexure_microscope_server/things/smart_scan.py
+++ b/src/openflexure_microscope_server/things/smart_scan.py
@@ -7,7 +7,7 @@ It also controls external processes for live stitching composite images, and
 the creation of the final stitched images.
 """
 
-from typing import Optional, Mapping
+from typing import Optional
 import threading
 import os
 import time
@@ -88,20 +88,16 @@ class SmartScanThing(lt.Thing):
         # when the `sample_scan` lt.thing_action is called. It is saved as
         # private class variable along with many others here.
         # Access to these variables requires a scan to be running,
-        # any method that calls these should be decorrected with
+        # any method that calls these should be decorated with
         # @_scan_running
         self._scan_logger: Optional[lt.deps.InvocationLogger] = None
         self._cancel: Optional[lt.deps.CancelHook] = None
         self._autofocus: Optional[AutofocusDep] = None
         self._stage: Optional[StageDep] = None
         self._cam: Optional[CamDep] = None
-        self._metadata_getter: Optional[lt.deps.GetThingStates] = None
         self._csm: Optional[CSMDep] = None
+
         self._ongoing_scan: Optional[scan_directories.ScanDirectory] = None
-        # TODO see if starting position can go into ScanData
-        self._starting_position: Optional[Mapping[str, int]] = None
-        self._scan_images_taken: Optional[int] = None
-        # TODO Scan data is a dict during refactoring, should become a dataclass
         self._scan_data: Optional[scan_directories.ScanData] = None
         self._preview_stitcher: Optional[stitching.PreviewStitcher] = None
 
@@ -113,7 +109,6 @@ class SmartScanThing(lt.Thing):
         autofocus: AutofocusDep,
         stage: StageDep,
         cam: CamDep,
-        metadata_getter: lt.deps.GetThingStates,
         csm: CSMDep,
         scan_name: str = "",
     ):
@@ -133,22 +128,14 @@ class SmartScanThing(lt.Thing):
         self._autofocus = autofocus
         self._stage = stage
         self._cam = cam
-        # TODO check if metadata_getter this can be removed without error?
-        self._metadata_getter = metadata_getter
         self._csm = csm
-        self._scan_images_taken = 0
-
-        # Set _scan_data to None. This is needed just in case an exception is raised
-        # before _run_scan (which sets the real data). As we check this in the `except`
+        # Confirm scan data is None as start of scan.
         self._scan_data = None
-
         try:
             self._check_background_and_csm_set()
             self._ongoing_scan = self._scan_dir_manager.new_scan_dir(scan_name)
             self._latest_scan_name = self._ongoing_scan.name
             self._autofocus.looping_autofocus(dz=self.autofocus_dz, start="centre")
-            # record starting position so we can return there
-            self._starting_position = self._stage.position
             self._run_scan()
         except Exception as e:
             # If _scan_data is set then scan started
@@ -156,6 +143,9 @@ class SmartScanThing(lt.Thing):
                 self._return_to_starting_position()
                 if not isinstance(e, scan_directories.NotEnoughFreeSpaceError):
                     # Don't stitch if drive is full (already logged)
+                    self._scan_logger.info(
+                        "Attempting to stitch and archive the images acquired so far."
+                    )
                     self._perform_final_stitch()
             # Error must be raised so UI gives correct output
             raise e
@@ -166,10 +156,8 @@ class SmartScanThing(lt.Thing):
             self._autofocus = None
             self._stage = None
             self._cam = None
-            self._metadata_getter = None
             self._csm = None
             self._ongoing_scan = None
-            self._scan_images_taken = None
             self._scan_data = None
             self._scan_lock.release()
             # Ensure any PreviewStitcher created cannot be reused.
@@ -275,6 +263,8 @@ class SmartScanThing(lt.Thing):
     @_scan_running
     def _collect_scan_data(self) -> scan_directories.ScanData:
         """Collect and return the data for this scan so it cannot be changed mid-scan."""
+        # Record starting position so it can be returned to at end of scan.
+        starting_position = self._stage.position
         overlap = self.overlap
         dx, dy = self._calc_displacement_from_test_image(overlap)
         stitch_resize = stitching.STITCHING_RESOLUTION[0] / self.save_resolution[0]
@@ -300,6 +290,7 @@ class SmartScanThing(lt.Thing):
         # Fix scan parameters in case UI is updated during scan.
         return scan_directories.ScanData(
             scan_name=self._ongoing_scan.name,
+            starting_position=starting_position,
             overlap=overlap,
             max_dist=self.max_range,
             dx=dx,
@@ -320,10 +311,7 @@ class SmartScanThing(lt.Thing):
         Takes scan_result, a string that is either "success", "cancelled by user",
         or the error that ended the scan.
         """
-        self._scan_data.set_final_data(
-            result=scan_result,
-            final_image_count=self._scan_images_taken,
-        )
+        self._scan_data.set_final_data(result=scan_result)
         self._ongoing_scan.save_scan_data(self._scan_data)
 
     @_scan_running
@@ -331,7 +319,7 @@ class SmartScanThing(lt.Thing):
         """Manage the stitching threads, starting them if needed and not already running."""
         # Assume 4 images means at least one offset in x and y, making the stitching
         # well constrained.
-        if self._scan_images_taken > 3:
+        if self._scan_data.image_count > 3:
             if not self._preview_stitcher.running:
                 self._preview_stitcher.start()
 
@@ -353,10 +341,6 @@ class SmartScanThing(lt.Thing):
                 correlation_resize=self._scan_data.stitch_resize,
             )
 
-            if self._scan_images_taken != 0:
-                msg = "_scan_images_taken should be zero before starting scanning"
-                raise RuntimeError(msg)
-
             # This is the main loop of the scan!
             self._main_scan_loop()
             self._save_final_scan_data(scan_result="success")
@@ -366,24 +350,19 @@ class SmartScanThing(lt.Thing):
             self._cancel.clear()
             self._scan_logger.info("Stopping scan because it was cancelled.")
             self._save_final_scan_data(scan_result="cancelled by user")
-        except scan_directories.NotEnoughFreeSpaceError as e:
-            self._save_final_scan_data(scan_result=f"NotEnoughFreeSpaceError: {e}")
-            self._scan_logger.error(
-                f"Stopping scan to avoid filling up the disk: {e}",
-                exc_info=e,
-            )
-            raise e
         except Exception as e:
             err_name = type(e).__name__
-            self._save_final_scan_data(scan_result=f"{err_name}: {e}")
+            if self._scan_data is not None:
+                self._save_final_scan_data(scan_result=f"{err_name}: {e}")
             self._scan_logger.error(
-                f"The scan stopped because of an error: {e} "
-                "Attempting to stitch and archive the images acquired so far.",
+                f"The scan stopped because of an error: {e}",
                 exc_info=e,
             )
             raise e
         finally:
-            # Don't set Preview Stitcher to None yet. It is used by _perform_final_scan.
+            # Don't set Preview Stitcher to None yet. It is used by
+            # _perform_final_stitch, which may also be run after this function completes
+            # if it ended due to an exception.
 
             # Start streaming in the default resolution again as soon as possible
             self._cam.start_streaming()
@@ -459,7 +438,7 @@ class SmartScanThing(lt.Thing):
             )
 
             # increment capture counter as thread has completed
-            self._scan_images_taken += 1
+            self._scan_data.image_count += 1
             # Add it to the incremental zip
             self._ongoing_scan.zip_files()
 
@@ -467,15 +446,15 @@ class SmartScanThing(lt.Thing):
     def _return_to_starting_position(self):
         """Return to the initial scan position, if set."""
         self._scan_logger.info("Returning to starting position.")
-        if self._starting_position is not None:
+        if self._scan_data is not None:
             self._stage.move_absolute(
-                **self._starting_position, block_cancellation=True
+                **self._scan_data.starting_position, block_cancellation=True
             )
 
     @_scan_running
     def _perform_final_stitch(self):
         """Update the scan zip and perform final stitch of the data."""
-        if self._scan_images_taken <= 3:
+        if self._scan_data.image_count <= 3:
             self._scan_logger.info("Not performing a stitch as 3 or fewer images taken")
             return
 
diff --git a/tests/test_scan_data.py b/tests/test_scan_data.py
index fae70236..a7956848 100644
--- a/tests/test_scan_data.py
+++ b/tests/test_scan_data.py
@@ -35,6 +35,7 @@ def _fake_scan_data(**kwargs) -> ScanData:
     """
     data_dict = {
         "scan_name": "fake_scan_0001",
+        "starting_position": {"x": 123, "y": 456, "z": 789},
         "overlap": 0.1,
         "max_dist": 100000,
         "dx": 100,
@@ -56,17 +57,19 @@ def test_set_final_data():
     """Check that adding final data to a ScanData object works as expected."""
     scan_data = _fake_scan_data()
 
-    assert scan_data.final_image_count is None
+    assert scan_data.image_count == 0
     assert scan_data.duration is None
     assert scan_data.scan_result is None
 
+    # Quickly take 123 images!
+    scan_data.image_count += 123
     # Should set duration based of finishing at datetime.now()
-    scan_data.set_final_data(result="Success", final_image_count=123)
+    scan_data.set_final_data(result="Success")
     expected_duration = datetime.now() - MOCK_START_TIME
     expected_duration_s = expected_duration.total_seconds()
     scan_duration_s = scan_data.duration.total_seconds()
 
-    assert scan_data.final_image_count == 123
+    assert scan_data.image_count == 123
     assert expected_duration_s - 1 < scan_duration_s < expected_duration_s + 1
     assert scan_data.scan_result == "Success"
 
@@ -77,15 +80,16 @@ def test_custom_serialisation():
     # Serialise to string then load directly as json
     scan_data_dict = json.loads(scan_data.model_dump_json())
     assert scan_data_dict["start_time"] == "11_00_00-25_12_2024"
-    assert scan_data_dict["final_image_count"] == "Unknown"
+    assert scan_data_dict["image_count"] == 0
     assert scan_data_dict["duration"] == "Unknown"
     assert scan_data_dict["scan_result"] == "Unknown"
 
-    scan_data.set_final_data(result="Success", final_image_count=123)
+    scan_data.image_count += 123
+    scan_data.set_final_data(result="Success")
     # Can't mock datetime.now as datetime is immutable. So just replace duraction
     scan_data.duration = MOCK_END_TIME - scan_data.start_time
     scan_data_dict = json.loads(scan_data.model_dump_json())
-    assert scan_data_dict["final_image_count"] == 123
+    assert scan_data_dict["image_count"] == 123
     assert scan_data_dict["duration"] == "1:29:03"
     assert scan_data_dict["scan_result"] == "Success"
 
@@ -106,7 +110,8 @@ def test_round_trip_finalised():
     """Check that finalised ScanData can be serialised and deserialised."""
     scan_data = _fake_scan_data()
     # Finalise the data.
-    scan_data.set_final_data(result="Success", final_image_count=123)
+    scan_data.image_count += 123
+    scan_data.set_final_data(result="Success")
 
     scan_data_dict = json.loads(scan_data.model_dump_json())
     scan_data_reloaded = ScanData(**scan_data_dict)
diff --git a/tests/test_smart_scan.py b/tests/test_smart_scan.py
index d71336e1..05e49787 100644
--- a/tests/test_smart_scan.py
+++ b/tests/test_smart_scan.py
@@ -177,7 +177,6 @@ def _run_only_outer_scan(adjust_initial_state: Optional[Callable] = None):
     af_mock = MockAutoFocusThing()
     stage_mock = MockStageThing()
     cam_mock = MockCameraThing()
-    meta_mock = 5  # not called
     csm_mock = MockCSMThing()
 
     class MockedSmartScanThing(SmartScanThing):
@@ -196,9 +195,7 @@ def _run_only_outer_scan(adjust_initial_state: Optional[Callable] = None):
             assert self._autofocus is af_mock
             assert self._stage is stage_mock
             assert self._cam is cam_mock
-            assert self._metadata_getter is meta_mock
             assert self._csm is csm_mock
-            assert self._scan_images_taken == 0
 
     # mock smart scan thing
     mock_ss_thing = MockedSmartScanThing(SCAN_DIR)
@@ -214,7 +211,6 @@ def _run_only_outer_scan(adjust_initial_state: Optional[Callable] = None):
             autofocus=af_mock,
             stage=stage_mock,
             cam=cam_mock,
-            metadata_getter=meta_mock,
             csm=csm_mock,
             scan_name="FooBar",
         )
@@ -228,9 +224,7 @@ def _run_only_outer_scan(adjust_initial_state: Optional[Callable] = None):
     assert mock_ss_thing._autofocus is None
     assert mock_ss_thing._stage is None
     assert mock_ss_thing._cam is None
-    assert mock_ss_thing._metadata_getter is None
     assert mock_ss_thing._csm is None
-    assert mock_ss_thing._scan_images_taken is None
 
     # Return the mock thing for further state testing, and the
     # exec_info of any uncaught exceptions that were raised
@@ -260,10 +254,16 @@ def test_outer_scan_wo_sample_skip():
     assert mock_ss_thing.mock_call_count["_run_scan"] == 1
 
 
+MOCK_SCAN_NAME = "test_name_0001"
+MOCK_SCAN_DIR = "scans/test_name_0001/images/"
+MOCK_START_POS = {"x": 123, "y": 456, "z": 789}
+
+
 def _expected_scan_data():
     """Return the expected ScanData object for a SmartScan with default properties."""
     expected_dict = {
-        "scan_name": "test_name_0001",
+        "scan_name": MOCK_SCAN_NAME,
+        "starting_position": MOCK_START_POS,
         "overlap": 0.45,
         "max_dist": 45000,
         "dx": 100,
@@ -287,11 +287,13 @@ def scan_thing_mocked_for_scan_data(smart_scan_thing, mocker):
         smart_scan_thing, "_calc_displacement_from_test_image", return_value=[100, 100]
     )
     mock_ongoing_scan = mocker.Mock()
-    type(mock_ongoing_scan).name = mocker.PropertyMock(return_value="test_name_0001")
-    type(mock_ongoing_scan).images_dir = mocker.PropertyMock(
-        return_value="scans/test_name_0001/images/"
-    )
+    type(mock_ongoing_scan).name = mocker.PropertyMock(return_value=MOCK_SCAN_NAME)
+    type(mock_ongoing_scan).images_dir = mocker.PropertyMock(return_value=MOCK_SCAN_DIR)
+    mock_stage = mocker.Mock()
+    type(mock_stage).position = mocker.PropertyMock(return_value=MOCK_START_POS)
+
     smart_scan_thing._ongoing_scan = mock_ongoing_scan
+    smart_scan_thing._stage = mock_stage
     return smart_scan_thing
 
 
@@ -313,7 +315,7 @@ def test_save_final_scan_data(scan_thing_mocked_for_scan_data):
     scan_thing = scan_thing_mocked_for_scan_data
 
     scan_thing._scan_data = scan_thing._collect_scan_data()
-    scan_thing._scan_images_taken = 44
+    scan_thing._scan_data.image_count = 44
     scan_thing._save_final_scan_data("Mocked!")
     # _ongoing_scan is a mock so we can check that save_scan data was called and get
     # the value
@@ -321,7 +323,7 @@ def test_save_final_scan_data(scan_thing_mocked_for_scan_data):
     final_data = scan_thing._ongoing_scan.save_scan_data.call_args[0][0]
     assert isinstance(final_data, ScanData)
     assert final_data.scan_result == "Mocked!"
-    assert final_data.final_image_count == 44
+    assert final_data.image_count == 44
     assert final_data.duration.total_seconds() < 1
 
 
@@ -335,7 +337,6 @@ def scan_thing_mocked_for_run_scan(scan_thing_mocked_for_scan_data, mocker):
     """
     scan_thing = scan_thing_mocked_for_scan_data
     scan_thing._cam = MockCameraThing()
-    scan_thing._scan_images_taken = 0
     mocker.patch.object(scan_thing, "_cancel")
     mocker.patch.object(scan_thing, "_main_scan_loop")
     mocker.patch.object(scan_thing, "_return_to_starting_position")
@@ -364,7 +365,7 @@ def check_run_scan(scan_thing, caplog, expected_exception=None):
         with pytest.raises(expected_exception), caplog.at_level(logging.WARNING):
             scan_thing._scan_data = scan_thing._run_scan()
     # The preview stitcher object should still exist. And images dir should be set.
-    assert scan_thing._preview_stitcher.images_dir == "scans/test_name_0001/images/"
+    assert scan_thing._preview_stitcher.images_dir == MOCK_SCAN_DIR
 
     final_scan_data = scan_thing._ongoing_scan.save_scan_data.call_args[0][0]
     calls = {
@@ -396,34 +397,6 @@ def test_run_scan(scan_thing_mocked_for_run_scan, caplog):
     assert calls == expected_calls_numbers
 
 
-def test_run_scan_wrong_image_value(scan_thing_mocked_for_run_scan, caplog):
-    """Check correct methods called if _scan_images_taken doesn't start at 0.
-
-    This should never happen but shows a good clear path for an error before
-    _main_scan_loop starts.
-    """
-    scan_thing = scan_thing_mocked_for_run_scan
-    scan_thing._scan_images_taken = 2
-
-    result, logs, calls = check_run_scan(scan_thing, caplog, RuntimeError)
-
-    assert result.startswith("RuntimeError:")
-    assert len(logs) == 1
-    assert logs[0].levelno == logging.ERROR
-
-    # Main loop not run, nor are return to start, final stitch, or purging of empty
-    # scans. Save scan data is still called twice
-    expected_calls_numbers = {
-        "cam_start_streaming_calls": 2,
-        "main_scan_loop_calls": 0,
-        "return_to_start_calls": 0,
-        "perform_final_stitch_calls": 0,
-        "purge_empty_scans_calls": 0,
-        "save_scan_data_calls": 2,
-    }
-    assert calls == expected_calls_numbers
-
-
 def test_run_scan_err_in_main_loop(scan_thing_mocked_for_run_scan, caplog, mocker):
     """Check correct methods called if main_loop errors."""
     scan_thing = scan_thing_mocked_for_run_scan

From 7d83aadf8d240b2cb07a4007cd44be596747137b Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Mon, 4 Aug 2025 11:10:21 +0100
Subject: [PATCH 10/18] Squash some TODOs

---
 src/openflexure_microscope_server/stitching.py         | 4 ++--
 src/openflexure_microscope_server/things/smart_scan.py | 2 --
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/openflexure_microscope_server/stitching.py b/src/openflexure_microscope_server/stitching.py
index 47c61cca..225a0666 100644
--- a/src/openflexure_microscope_server/stitching.py
+++ b/src/openflexure_microscope_server/stitching.py
@@ -10,6 +10,7 @@ from typing import Optional, Any
 import threading
 import subprocess
 import os
+from io import TextIOWrapper
 import shlex
 import time
 
@@ -324,8 +325,7 @@ class FinalStitcher(BaseStitcher):
         # Print everything in the buffer when program finishes
         self.log_buffer(process.stdout)
 
-    def log_buffer(self, buffer):
-        # TODO work out type
+    def log_buffer(self, buffer: TextIOWrapper):
         """Log everything in the buffer at INFO level."""
         while line := buffer.readline():
             self.logger.info(line)
diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py
index a01d014f..2dbde354 100644
--- a/src/openflexure_microscope_server/things/smart_scan.py
+++ b/src/openflexure_microscope_server/things/smart_scan.py
@@ -699,8 +699,6 @@ class SmartScanThing(lt.Thing):
         Note that as this is a lt.thing_action it needs the logger passed as
         a variable if called from another thing action
         """
-        # TODO tidy this function
-        # TODO handle if json_path is None (easier once tidied)
         scan_data_dict = self._scan_dir_manager.get_scan_data_dict(scan_name)
         if scan_data_dict is None:
             logger.warning("Couldn't read scan data it may be missing or corrupt.")

From f9fcdf628bedf18fc28bd9918aeff5c8b868e8c5 Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Mon, 4 Aug 2025 12:28:04 +0100
Subject: [PATCH 11/18] Squash final todos for SmartScan refactor

---
 .../scan_directories.py                       | 53 +++++++++++++++----
 .../stitching.py                              |  7 ---
 .../things/smart_scan.py                      | 17 +++---
 tests/test_scan_data.py                       |  4 +-
 tests/test_smart_scan.py                      |  2 +-
 tests/test_stitching.py                       | 19 +++----
 6 files changed, 62 insertions(+), 40 deletions(-)

diff --git a/src/openflexure_microscope_server/scan_directories.py b/src/openflexure_microscope_server/scan_directories.py
index 9f3ad9ce..2f4091ba 100644
--- a/src/openflexure_microscope_server/scan_directories.py
+++ b/src/openflexure_microscope_server/scan_directories.py
@@ -43,7 +43,7 @@ class ScanData(BaseModel):
 
     This serialises into a human readable format where possible with
 
-    timestamps in %H_%M_%S-%d_%m_%Y format
+    timestamps in %Y-%m-%s_%H:%M:%S format
     timedeltas in %H:%M:%S format
 
     Properties that are not known until the end have ``None`` serialised as "Unknown"
@@ -51,26 +51,59 @@ class ScanData(BaseModel):
 
     model_config = {"extra": "forbid"}
 
-    # TODO: Docs for each variable
-    # TODO: Make note about timestamp format. Is it too ambiguous?
     scan_name: str
+    """The name of the scan i.e. scan_0001"""
+
     starting_position: Mapping[str, int]
+    """The starting position in dictionary format."""
+
     overlap: float
+    """The overlap between adjacent images as a fraction of the image size."""
+
     max_dist: int
+    """The maximum distance the scan could move (in steps) from the starting position."""
+
     dx: int
+    """The number of steps between adjacent images in x."""
+
     dy: int
+    """The number of steps between adjacent images in y."""
+
     autofocus_dz: int
+    """The z range used for autofocus (in steps)."""
+
     autofocus_on: bool
+    """Whether autofocus is on."""
+
     start_time: datetime
+    """The time the scan started."""
+
     skip_background: bool
+    """Whether automatic background detection is on, skipping locations with no sample."""
+
     stitch_automatically: bool
-    # TODO: Think about changing stitch_resize name as it is NOT the resize for
-    # just for correlation stitching
-    stitch_resize: float
+    """Whether the scan is set to automatically stitch when complete."""
+
+    correlation_resize: float
+    """The resize factor applied to images when the stitching program is correlating."""
+
     save_resolution: tuple[int, int]
+    """The resolution that scan images are saved at."""
+
     image_count: int = 0
+    """The number of images taken."""
+
     duration: Optional[timedelta] = None
+    """The duration of the scan.
+
+    This is automatically set when ``set_final_data()`` is run.
+    """
+
     scan_result: Optional[str] = None
+    """The result of the scan.
+
+    This should be set with ``set_final_data()`` to ensure duration is set.
+    """
 
     def set_final_data(self, result: str):
         """Set the final data for the scan, scan duration is automatically calculated.
@@ -83,15 +116,15 @@ class ScanData(BaseModel):
     @field_validator("start_time", mode="before")
     @classmethod
     def parse_timestamp(cls, value: str | datetime) -> datetime:
-        """Validate a timestamp that may be a string in Hrs_Min_Sec-Day_Month_Year format."""
+        """Validate a timestamp that may be a string in Year-Month-Day_Hrs:Min:Sec format."""
         if isinstance(value, str):
-            return datetime.strptime(value, "%H_%M_%S-%d_%m_%Y")
+            return datetime.strptime(value, "%Y-%m-%d_%H:%M:%S")
         return value
 
     @field_serializer("start_time")
     def serialize_timestamp(self, value: datetime) -> str:
-        """Serialise timestamp to Hrs_Min_Sec-Day_Month_Year format."""
-        return value.strftime("%H_%M_%S-%d_%m_%Y")
+        """Serialise timestamp to Year-Month-Day_Hrs:Min:Sec format."""
+        return value.strftime("%Y-%m-%d_%H:%M:%S")
 
     @field_validator("duration", mode="before")
     @classmethod
diff --git a/src/openflexure_microscope_server/stitching.py b/src/openflexure_microscope_server/stitching.py
index 225a0666..d5706915 100644
--- a/src/openflexure_microscope_server/stitching.py
+++ b/src/openflexure_microscope_server/stitching.py
@@ -12,7 +12,6 @@ import subprocess
 import os
 from io import TextIOWrapper
 import shlex
-import time
 
 import labthings_fastapi as lt
 
@@ -289,12 +288,6 @@ class FinalStitcher(BaseStitcher):
         # Stop opening pipe blocking writing to it
         os.set_blocking(process.stdout.fileno(), False)
 
-        # TODO check if we still want this? Is it for debugging, or should it have
-        # more explanation.
-        self.logger.info(
-            time.strftime("%Y-%m-%d %H:%M:%S", time.localtime(time.time()))
-        )
-
         self._log_ongoing(process, cancel=cancel)
 
         if process.poll() == 0:
diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py
index 2dbde354..d2bd4921 100644
--- a/src/openflexure_microscope_server/things/smart_scan.py
+++ b/src/openflexure_microscope_server/things/smart_scan.py
@@ -11,6 +11,7 @@ from typing import Optional
 import threading
 import os
 import time
+from datetime import datetime
 from subprocess import SubprocessError
 
 from fastapi import HTTPException
@@ -267,10 +268,10 @@ class SmartScanThing(lt.Thing):
         starting_position = self._stage.position
         overlap = self.overlap
         dx, dy = self._calc_displacement_from_test_image(overlap)
-        stitch_resize = stitching.STITCHING_RESOLUTION[0] / self.save_resolution[0]
+        correlation_resize = stitching.STITCHING_RESOLUTION[0] / self.save_resolution[0]
 
         self._scan_logger.debug(
-            f"Resizing images when stitching by a factor of {stitch_resize}"
+            f"Resizing images when correlating by a factor of {correlation_resize}"
         )
 
         self._scan_logger.info(
@@ -297,10 +298,10 @@ class SmartScanThing(lt.Thing):
             dy=dy,
             autofocus_dz=autofocus_dz,
             autofocus_on=bool(autofocus_dz),
-            start_time=time.strftime("%H_%M_%S-%d_%m_%Y"),
+            start_time=datetime.now(),
             skip_background=self.skip_background,
             stitch_automatically=self.stitch_automatically,
-            stitch_resize=stitch_resize,
+            correlation_resize=correlation_resize,
             save_resolution=self.save_resolution,
         )
 
@@ -338,7 +339,7 @@ class SmartScanThing(lt.Thing):
             self._preview_stitcher = stitching.PreviewStitcher(
                 self._ongoing_scan.images_dir,
                 overlap=self._scan_data.overlap,
-                correlation_resize=self._scan_data.stitch_resize,
+                correlation_resize=self._scan_data.correlation_resize,
             )
 
             # This is the main loop of the scan!
@@ -471,7 +472,7 @@ class SmartScanThing(lt.Thing):
                 logger=self._scan_logger,
                 cancel=self._cancel,
                 scan_name=self._ongoing_scan.name,
-                stitch_resize=self._scan_data.stitch_resize,
+                correlation_resize=self._scan_data.correlation_resize,
                 overlap=self._scan_data.overlap,
             )
 
@@ -691,7 +692,7 @@ class SmartScanThing(lt.Thing):
         logger: lt.deps.InvocationLogger,
         cancel: lt.deps.CancelHook,
         scan_name: str,
-        stitch_resize: Optional[float] = None,
+        correlation_resize: Optional[float] = None,
         overlap: Optional[float] = None,
     ) -> None:
         """Generate a stitched image based on stage position metadata.
@@ -706,7 +707,7 @@ class SmartScanThing(lt.Thing):
             self._scan_dir_manager.img_dir_for(scan_name),
             logger=logger,
             overlap=overlap,
-            correlation_resize=stitch_resize,
+            correlation_resize=correlation_resize,
             stitch_tiff=self.stitch_tiff,
             scan_data_dict=scan_data_dict,
         )
diff --git a/tests/test_scan_data.py b/tests/test_scan_data.py
index a7956848..df6dfe25 100644
--- a/tests/test_scan_data.py
+++ b/tests/test_scan_data.py
@@ -45,7 +45,7 @@ def _fake_scan_data(**kwargs) -> ScanData:
         "start_time": copy(MOCK_START_TIME),
         "skip_background": True,
         "stitch_automatically": True,
-        "stitch_resize": 0.25,
+        "correlation_resize": 0.25,
         "save_resolution": (1000, 1000),
     }
     for key, value in kwargs.items():
@@ -79,7 +79,7 @@ def test_custom_serialisation():
     scan_data = _fake_scan_data()
     # Serialise to string then load directly as json
     scan_data_dict = json.loads(scan_data.model_dump_json())
-    assert scan_data_dict["start_time"] == "11_00_00-25_12_2024"
+    assert scan_data_dict["start_time"] == "2024-12-25_11:00:00"
     assert scan_data_dict["image_count"] == 0
     assert scan_data_dict["duration"] == "Unknown"
     assert scan_data_dict["scan_result"] == "Unknown"
diff --git a/tests/test_smart_scan.py b/tests/test_smart_scan.py
index 05e49787..ab619e8a 100644
--- a/tests/test_smart_scan.py
+++ b/tests/test_smart_scan.py
@@ -272,7 +272,7 @@ def _expected_scan_data():
         "autofocus_on": True,
         "skip_background": True,
         "stitch_automatically": True,
-        "stitch_resize": 0.5,
+        "correlation_resize": 0.5,
         "save_resolution": (1640, 1232),
     }
     return ScanData(start_time=datetime.now(), **expected_dict)
diff --git a/tests/test_stitching.py b/tests/test_stitching.py
index a680f71f..13d1f6a8 100644
--- a/tests/test_stitching.py
+++ b/tests/test_stitching.py
@@ -8,7 +8,6 @@ import logging
 import os
 from copy import copy
 import uuid
-import re
 import threading
 from time import sleep
 
@@ -229,19 +228,15 @@ def test_final_stitching_command(caplog, mocker):
         )
         # For the final stitcher it will always complete before returning.
         stitcher.start(cancel=lt.deps.CancelHook(id=uuid.uuid4()))
-        # The mock command logs the inputs so should be 1 less than
-        # FINAL_EXPECTED_COMMAND as the command itself is not logged. However, there
-        # is two extra logs, The date (before the program starts) and
-        # "Stitching complete" when it ends.
-        assert len(caplog.records) == len(FINAL_EXPECTED_COMMAND) + 1
+        # The mock command logs the inputs (but not the initial command) and the
+        # stitcher logs # "Stitching complete" when it ends.
+        assert len(caplog.records) == len(FINAL_EXPECTED_COMMAND)
         for i, record in enumerate(caplog.records):
-            msg = record.message
-            if i == 0:
-                assert re.match(r"^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}", msg)
-            elif i == len(FINAL_EXPECTED_COMMAND):
-                assert msg.strip() == "Stitching complete"
+            msg = record.message.strip()
+            if i == len(FINAL_EXPECTED_COMMAND) - 1:
+                assert msg == "Stitching complete"
             else:
-                assert msg.strip() == FINAL_EXPECTED_COMMAND[i]
+                assert msg == FINAL_EXPECTED_COMMAND[i + 1]
 
 
 def test_final_stitching_command_cancelled(caplog, mocker):

From 6739a84d7c790133b269339313a39730bf2b0bb5 Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Wed, 6 Aug 2025 14:44:48 +0000
Subject: [PATCH 12/18] Apply suggestions from code review of branch
 yet-another-smart-scan-refactor

Co-authored-by: Joe Knapper <joe.knapper@glasgow.ac.uk>
---
 src/openflexure_microscope_server/stitching.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/openflexure_microscope_server/stitching.py b/src/openflexure_microscope_server/stitching.py
index d5706915..ba423941 100644
--- a/src/openflexure_microscope_server/stitching.py
+++ b/src/openflexure_microscope_server/stitching.py
@@ -1,7 +1,7 @@
 """Communicate with OpenFlexure Stitching to perform stitches for scans.
 
 This includes both live stitching and final stitching. This is done via subprocess
-to call openflexure-stitching over CLI. This cannot be done via Threading dut to the
+to call openflexure-stitching over CLI. This cannot be done via Threading due to the
 CPU intensity of stitching causing scanning problems due to the Python Global
 Interpreter Lock (GIL). May be possible to shift to multiprocessing int the future.
 """

From 2e6ba733452a5977a75e72c1434fcfb415176e7a Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Wed, 6 Aug 2025 15:52:52 +0000
Subject: [PATCH 13/18] Apply suggestions from code review of branch
 yet-another-smart-scan-refactor

Co-authored-by: Joe Knapper <joe.knapper@glasgow.ac.uk>
---
 src/openflexure_microscope_server/things/smart_scan.py | 3 ++-
 src/openflexure_microscope_server/utilities.py         | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py
index d2bd4921..559aea47 100644
--- a/src/openflexure_microscope_server/things/smart_scan.py
+++ b/src/openflexure_microscope_server/things/smart_scan.py
@@ -702,7 +702,7 @@ class SmartScanThing(lt.Thing):
         """
         scan_data_dict = self._scan_dir_manager.get_scan_data_dict(scan_name)
         if scan_data_dict is None:
-            logger.warning("Couldn't read scan data it may be missing or corrupt.")
+            logger.warning("Couldn't read scan data - it may be missing or corrupt.")
         final_stitcher = stitching.FinalStitcher(
             self._scan_dir_manager.img_dir_for(scan_name),
             logger=logger,
@@ -712,6 +712,7 @@ class SmartScanThing(lt.Thing):
             scan_data_dict=scan_data_dict,
         )
         try:
+            # start the final stitch, providing the cancel hook to allow aborting
             final_stitcher.start(cancel)
         except lt.exceptions.InvocationCancelledError:
             # Sleep for 1 second just to allow invocation logs to pass to user.
diff --git a/src/openflexure_microscope_server/utilities.py b/src/openflexure_microscope_server/utilities.py
index 07bc4fea..bbad56f9 100644
--- a/src/openflexure_microscope_server/utilities.py
+++ b/src/openflexure_microscope_server/utilities.py
@@ -128,8 +128,11 @@ def _wrap_and_catch_errors(target, error_buffer, *args, **kwargs):
 
 
 # Compiled regular expressions for unsafe characters
+# Matches anything that isn't a-z, A-Z, 0-9, _, ., -, :, /, \
 _WINDOWS_UNSAFE_PATTERN = re.compile(r"[^a-zA-Z0-9_.\-:/\\]")
+# Matches anything that isn't a-z, A-Z, 0-9, _, ., -, \
 _POSIX_UNSAFE_PATTERN = re.compile(r"[^a-zA-Z0-9_.\-/]")
+# Matches anything that isn't a-z, A-Z, 0-9, _, ., -
 _NAME_UNSAFE_PATTERN = re.compile(r"[^a-zA-Z0-9_.\-]")
 
 

From 5f092835ace9a5e547a4ed9260560cb0ec49f5d7 Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Wed, 6 Aug 2025 15:57:31 +0000
Subject: [PATCH 14/18] Apply suggestions from code review of branch
 yet-another-smart-scan-refactor

---
 src/openflexure_microscope_server/things/smart_scan.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py
index 559aea47..2af113ac 100644
--- a/src/openflexure_microscope_server/things/smart_scan.py
+++ b/src/openflexure_microscope_server/things/smart_scan.py
@@ -130,7 +130,9 @@ class SmartScanThing(lt.Thing):
         self._stage = stage
         self._cam = cam
         self._csm = csm
-        # Confirm scan data is None as start of scan.
+        # `scan_data` should already be None. This is added as a precaution as
+        # the presence of `scan_data` is used during error handling to
+        # determine whether the scan started.
         self._scan_data = None
         try:
             self._check_background_and_csm_set()

From cd311f08bf972ee773bd26face8f037035bf1a9d Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Wed, 6 Aug 2025 17:47:33 +0100
Subject: [PATCH 15/18] Update stiching to differentiate bwtween stitchers that
 use a thread and those that block

---
 src/openflexure_microscope_server/stitching.py | 18 +++++++-----------
 .../things/smart_scan.py                       |  2 +-
 tests/test_stitching.py                        |  9 +++------
 3 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/src/openflexure_microscope_server/stitching.py b/src/openflexure_microscope_server/stitching.py
index ba423941..54b367f9 100644
--- a/src/openflexure_microscope_server/stitching.py
+++ b/src/openflexure_microscope_server/stitching.py
@@ -44,7 +44,12 @@ def validate_command(cmd: list[str]):
 
 
 class BaseStitcher:
-    """A base stitching class for all stitchers. Don't initialise this directly."""
+    """A base stitching class for all stitchers. Don't initialise this directly.
+
+    The base class has no way to run the command. Child classes should either implement
+    a ``start``, ``running``, and ``wait`` methods if they stitch in a thread, or ``run``
+    if they stitch in this thread and return once complete.
+    """
 
     def __init__(self, images_dir: str, *, overlap: float, correlation_resize: float):
         """Initialise a stitcher.
@@ -104,15 +109,6 @@ class BaseStitcher:
                 "Invalid directory path: Contains unsafe characters."
             )
 
-    def start(self) -> None:
-        """Start stitching a stitching process.
-
-        This should be overridden by any child class.
-        """
-        raise NotImplementedError(
-            "Child stitchers should implement their own ``start`` method."
-        )
-
 
 class PreviewStitcher(BaseStitcher):
     """A stitcher for stitching an ongoing scan in preview mode.
@@ -262,7 +258,7 @@ class FinalStitcher(BaseStitcher):
                 )
         return overlap, correlation_resize
 
-    def start(
+    def run(
         self,
         cancel: lt.deps.CancelHook,
     ) -> None:
diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py
index 2af113ac..c4206bcc 100644
--- a/src/openflexure_microscope_server/things/smart_scan.py
+++ b/src/openflexure_microscope_server/things/smart_scan.py
@@ -715,7 +715,7 @@ class SmartScanThing(lt.Thing):
         )
         try:
             # start the final stitch, providing the cancel hook to allow aborting
-            final_stitcher.start(cancel)
+            final_stitcher.run(cancel)
         except lt.exceptions.InvocationCancelledError:
             # Sleep for 1 second just to allow invocation logs to pass to user.
             time.sleep(1)
diff --git a/tests/test_stitching.py b/tests/test_stitching.py
index 13d1f6a8..75d5ad98 100644
--- a/tests/test_stitching.py
+++ b/tests/test_stitching.py
@@ -54,9 +54,6 @@ def test_base_stitcher():
         ]
         stitcher = BaseStitcher(FAKE_DIR, overlap=overlap, correlation_resize=0.5)
         assert stitcher.command == expected_command
-        # check that a BaseStitcher can't start
-        with pytest.raises(NotImplementedError):
-            stitcher.start()
 
 
 def test_preview_stitcher_command():
@@ -227,7 +224,7 @@ def test_final_stitching_command(caplog, mocker):
             FAKE_DIR, logger=LOGGER, overlap=0.1, correlation_resize=0.5
         )
         # For the final stitcher it will always complete before returning.
-        stitcher.start(cancel=lt.deps.CancelHook(id=uuid.uuid4()))
+        stitcher.run(cancel=lt.deps.CancelHook(id=uuid.uuid4()))
         # The mock command logs the inputs (but not the initial command) and the
         # stitcher logs # "Stitching complete" when it ends.
         assert len(caplog.records) == len(FINAL_EXPECTED_COMMAND)
@@ -250,7 +247,7 @@ def test_final_stitching_command_cancelled(caplog, mocker):
         cancel_hook = lt.deps.CancelHook(id=uuid.uuid4())
 
         # Start stitching in a thread.
-        thread = threading.Thread(target=stitcher.start, kwargs={"cancel": cancel_hook})
+        thread = threading.Thread(target=stitcher.run, kwargs={"cancel": cancel_hook})
         thread.start()
         # Sleep long enough for at least 1 log.
         sleep(0.5)
@@ -273,4 +270,4 @@ def test_final_stitching_command_error(caplog, mocker):
         # than echo.
         stitcher._extra_args = ["ERROR"]
         with pytest.raises(ChildProcessError):
-            stitcher.start(cancel=lt.deps.CancelHook(id=uuid.uuid4()))
+            stitcher.run(cancel=lt.deps.CancelHook(id=uuid.uuid4()))

From 7727d0a51d2328aba9d0b900c8364ee0bc0632d9 Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Thu, 7 Aug 2025 12:06:58 +0100
Subject: [PATCH 16/18] Update language in BaseStitcher to clarify that the
 stitching uses subprocess not threading

---
 src/openflexure_microscope_server/stitching.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/openflexure_microscope_server/stitching.py b/src/openflexure_microscope_server/stitching.py
index 54b367f9..46c3e9c0 100644
--- a/src/openflexure_microscope_server/stitching.py
+++ b/src/openflexure_microscope_server/stitching.py
@@ -47,8 +47,10 @@ class BaseStitcher:
     """A base stitching class for all stitchers. Don't initialise this directly.
 
     The base class has no way to run the command. Child classes should either implement
-    a ``start``, ``running``, and ``wait`` methods if they stitch in a thread, or ``run``
-    if they stitch in this thread and return once complete.
+    a ``start``, ``running``, and ``wait`` methods if return after starting the
+    subprocess and can be polled or waited on like a thread; or ``run`` if the
+    the function blocks while the stitching subprocess is ongoing and return once
+    complete.
     """
 
     def __init__(self, images_dir: str, *, overlap: float, correlation_resize: float):

From 5caae613dc1ad23e7a2d8f1f4c9baee5a36ff921 Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Thu, 7 Aug 2025 13:55:47 +0000
Subject: [PATCH 17/18] Apply suggestions from code review of branch
 yet-another-smart-scan-refactor

Co-authored-by: Beth Probert <beth_probert@outlook.com>
---
 src/openflexure_microscope_server/scan_directories.py | 8 +++-----
 src/openflexure_microscope_server/stitching.py        | 8 ++++----
 tests/test_scan_data.py                               | 4 ++--
 tests/test_scan_directories.py                        | 3 +++
 tests/test_stitching.py                               | 2 +-
 5 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/src/openflexure_microscope_server/scan_directories.py b/src/openflexure_microscope_server/scan_directories.py
index 2f4091ba..de4539cc 100644
--- a/src/openflexure_microscope_server/scan_directories.py
+++ b/src/openflexure_microscope_server/scan_directories.py
@@ -43,7 +43,7 @@ class ScanData(BaseModel):
 
     This serialises into a human readable format where possible with
 
-    timestamps in %Y-%m-%s_%H:%M:%S format
+    timestamps in %Y-%m-%d_%H:%M:%S format
     timedeltas in %H:%M:%S format
 
     Properties that are not known until the end have ``None`` serialised as "Unknown"
@@ -154,9 +154,7 @@ class ScanData(BaseModel):
     @classmethod
     def parse_unknown_as_none(cls, value: Optional[str | int]) -> Optional[str | int]:
         """Validate the string "Unknown" as None."""
-        if value == "Unknown":
-            return None
-        return value
+        return None if value == "Unknown" else value
 
     @field_serializer("scan_result")
     def serialize_none_as_unknown(self, value: Optional[str | int]) -> str | int:
@@ -258,7 +256,7 @@ class ScanDirectoryManager:
     def get_scan_data_dict(self, scan_name: str) -> Optional[dict[str, Any]]:
         """Return the scan data read from a JSON file as a dict.
 
-        This is a dictionary not a base models as the data format has changed
+        This is a dictionary not a base model as the data format has changed
         somewhat over time.
         """
         json_fpath = self.get_scan_data_path(scan_name)
diff --git a/src/openflexure_microscope_server/stitching.py b/src/openflexure_microscope_server/stitching.py
index 46c3e9c0..c9a9e460 100644
--- a/src/openflexure_microscope_server/stitching.py
+++ b/src/openflexure_microscope_server/stitching.py
@@ -3,7 +3,7 @@
 This includes both live stitching and final stitching. This is done via subprocess
 to call openflexure-stitching over CLI. This cannot be done via Threading due to the
 CPU intensity of stitching causing scanning problems due to the Python Global
-Interpreter Lock (GIL). May be possible to shift to multiprocessing int the future.
+Interpreter Lock (GIL). May be possible to shift to multiprocessing in the future.
 """
 
 from typing import Optional, Any
@@ -99,7 +99,7 @@ class BaseStitcher:
         validate_command(full_cmd)
         return full_cmd
 
-    def validate_path(self):
+    def validate_path(self) -> None:
         """Check path is safe before making a command to run with subprocess.
 
         This is essential for stopping arbitrary code execution.
@@ -159,7 +159,7 @@ class PreviewStitcher(BaseStitcher):
                 return True
             return False
 
-    def wait(self):
+    def wait(self) -> None:
         """Wait for this preview stitch to return."""
         if self.running:
             with self._popen_lock:
@@ -167,7 +167,7 @@ class PreviewStitcher(BaseStitcher):
 
 
 class FinalStitcher(BaseStitcher):
-    """A class to handle the final stich for a scan."""
+    """A class to handle the final stitch for a scan."""
 
     def __init__(
         self,
diff --git a/tests/test_scan_data.py b/tests/test_scan_data.py
index df6dfe25..a1ea4f1c 100644
--- a/tests/test_scan_data.py
+++ b/tests/test_scan_data.py
@@ -63,7 +63,7 @@ def test_set_final_data():
 
     # Quickly take 123 images!
     scan_data.image_count += 123
-    # Should set duration based of finishing at datetime.now()
+    # Should set duration based on finishing at datetime.now()
     scan_data.set_final_data(result="Success")
     expected_duration = datetime.now() - MOCK_START_TIME
     expected_duration_s = expected_duration.total_seconds()
@@ -86,7 +86,7 @@ def test_custom_serialisation():
 
     scan_data.image_count += 123
     scan_data.set_final_data(result="Success")
-    # Can't mock datetime.now as datetime is immutable. So just replace duraction
+    # Can't mock datetime.now as datetime is immutable. So just replace duration
     scan_data.duration = MOCK_END_TIME - scan_data.start_time
     scan_data_dict = json.loads(scan_data.model_dump_json())
     assert scan_data_dict["image_count"] == 123
diff --git a/tests/test_scan_directories.py b/tests/test_scan_directories.py
index 04c4b2d1..f589c8a7 100644
--- a/tests/test_scan_directories.py
+++ b/tests/test_scan_directories.py
@@ -309,6 +309,9 @@ def test_get_scan_data_path():
 
     # Remove the images directory.
     shutil.rmtree(scan_dir.images_dir)
+    # When the images directory is deleted, internally `scan_dir.images_dir`
+    # will return `None`. Check that `get_scan_data_path` copes with the `None`
+    # return from `scan_dir.images_dir`.
     assert scan_dir_manager.get_scan_data_path(scan_name) is None
 
 
diff --git a/tests/test_stitching.py b/tests/test_stitching.py
index 75d5ad98..ae2e2abd 100644
--- a/tests/test_stitching.py
+++ b/tests/test_stitching.py
@@ -169,7 +169,7 @@ def _validation_error_tester(scan_path, **kwargs):
 
 
 def test_validation_error():
-    """Test a number of way to try to inject malicious arguments into the stitcher.
+    """Test a number of ways to try to inject malicious arguments into the stitcher.
 
     The stitcher should throw a validation error each attempt.
     """

From dbdf254154721ffc10440fafe057f7ec912d4c7b Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Thu, 7 Aug 2025 15:19:14 +0100
Subject: [PATCH 18/18] Fix CLI arg for FinalStitcher, and a docstring typo.

---
 src/openflexure_microscope_server/stitching.py | 4 ++--
 tests/test_stitching.py                        | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/openflexure_microscope_server/stitching.py b/src/openflexure_microscope_server/stitching.py
index c9a9e460..b88c4036 100644
--- a/src/openflexure_microscope_server/stitching.py
+++ b/src/openflexure_microscope_server/stitching.py
@@ -47,7 +47,7 @@ class BaseStitcher:
     """A base stitching class for all stitchers. Don't initialise this directly.
 
     The base class has no way to run the command. Child classes should either implement
-    a ``start``, ``running``, and ``wait`` methods if return after starting the
+    ``start``, ``running``, and ``wait`` methods if return after starting the
     subprocess and can be polled or waited on like a thread; or ``run`` if the
     the function blocks while the stitching subprocess is ongoing and return once
     complete.
@@ -207,7 +207,7 @@ class FinalStitcher(BaseStitcher):
         self._mode = "all"
 
         tiff_arg = "--stitch_tiff" if stitch_tiff else "--no-stitch_tiff"
-        self._extra_args = ["--stitch-dzi", tiff_arg]
+        self._extra_args = ["--stitch_dzi", tiff_arg]
 
     def _process_inputs(
         self,
diff --git a/tests/test_stitching.py b/tests/test_stitching.py
index ae2e2abd..ef248b82 100644
--- a/tests/test_stitching.py
+++ b/tests/test_stitching.py
@@ -76,7 +76,7 @@ FINAL_EXPECTED_COMMAND = [
     "openflexure-stitch",
     "--stitching_mode",
     "all",
-    "--stitch-dzi",
+    "--stitch_dzi",
     "--no-stitch_tiff",
     "--minimum_overlap",
     "0.09",