fix: address 10 full-codebase audit findings (v0.5.19)

Correctness:
- fetch_face_data: only fall back to faces[0] when person_id is absent;
  previously a missing person match injected a different person's bbox
- upload_tracker: change PK from (asset_id, status) to
  (asset_id, person_name, status); old PK allowed INSERT OR REPLACE to
  silently overwrite person_name when the same photo appeared in two
  people's jobs, breaking quality-replacement JOINs; auto-migrates DBs
- filter_recent_assets: treat years=0 as "no age filter" instead of
  falling through to Config.YEARS_FILTER via falsy `or`
- _is_module_available: return find_spec(...) is not None; find_spec
  returns None (not raises) for absent top-level modules, so the
  previous code always returned True
- execute_jobs error handler: use asset.get("id", "<unknown>") to avoid
  a secondary KeyError propagating out of execute_jobs on malformed dicts
- upload_to_frigate: also mark_rejected on HTTP 422, not only HTTP 400
  with "face" in body; other permanent errors left assets untracked and
  retried forever
- reconcile_frigate_mappings: sort key lambda f: (_ts(f), f) makes order
  deterministic when timestamps are equal or 0.0; set iteration order is
  hash-randomised, stable sort preserves it

Reuse / cleanup:
- config.py: add _getenv_optional_int delegating to _getenv_num(name, None, int)
- jobs.py: _resolve_strategy uses _getenv_optional_int("LIMIT") instead
  of inline os.environ.get + int() + warning duplicate of _getenv_num
- frigate_api.py: add _get_frigate_url() helper; eliminates 4× copy of
  os.environ.get("FRIGATE_URL", "").rstrip("/")
- quality.py: extract blur_score_from_image(img, max_dim=1440) helper;
  executor.py time-spread blur fallback now uses it instead of inlining
  the resize+RGB+assess_quality sequence, keeping scale logic in one place

CHANGELOG.md

@@ -7,6 +7,32 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [0.5.19] - 2026-06-15
+
+### Fixed
+
+- **`fetch_face_data` no longer falls back to an arbitrary person's face**: when `person_id` is provided but not found in the Immich `/api/faces` response, the function now returns `None` instead of falling back to `faces[0]`. Previously a Frigate group photo where the target person's face entry was missing would inject a different person's bounding box, causing the wrong face crop to be uploaded as training data.
+
+- **`tracked_assets` PRIMARY KEY now includes `person_name`**: the old `PRIMARY KEY (asset_id, status)` meant that `INSERT OR REPLACE` for person B on an asset already tracked for person A silently overwrote `person_name`, destroying the JOIN between `tracked_assets` and `frigate_files` for person A and breaking quality replacement. The key is now `(asset_id, person_name, status)`, giving each person their own row per asset. Existing databases are migrated automatically on first open.
+
+- **`filter_recent_assets` treats `years=0` as "no age filter"**: previously `years = years or Config.YEARS_FILTER` evaluated `0` as falsy and fell through to the default (10 years), silently discarding all older assets when the user explicitly set `YEARS_FILTER=0`. The check is now `if years is None: years = Config.YEARS_FILTER` followed by an early return for `years=0`.
+
+- **`_is_module_available` now correctly returns False for absent modules**: `importlib.util.find_spec` returns `None` (not raises) for missing top-level modules, so the previous `try: find_spec(); return True` always reported modules as installed. Fixed to `return find_spec(...) is not None`, ensuring `is_embedding_available()` returns False when InsightFace or onnxruntime are not installed.
+
+- **Error handler in `execute_jobs` uses `asset.get("id")` instead of `asset["id"]`**: a malformed asset dict missing the `"id"` key would cause a secondary `KeyError` inside the `except` block, propagating uncaught out of `execute_jobs()` and aborting the run mid-job. Changed to `asset.get("id", "<unknown>")`.
+
+- **HTTP 422 now triggers `mark_rejected`**: only `HTTP 400` with `"face"` in the body triggered permanent rejection; `HTTP 422` (Unprocessable Entity) left the asset untracked and caused it to be re-selected and re-attempted on every future run. Both codes are now treated as permanent rejections.
+
+- **Frigate filename reconciliation sort is now deterministic**: `sorted(new_files, key=_ts)` sorted a `set` — when `_ts()` returns `0.0` for non-matching filenames, Python's stable sort preserves the set's hash-randomised input order, producing non-deterministic `asset_id → frigate_filename` mappings. Changed the key to `lambda f: (_ts(f), f)` so equal-timestamp files sort alphabetically.
+
+### Changed
+
+- **`_resolve_strategy` uses `_getenv_optional_int("LIMIT")`**: the inline `os.environ.get("LIMIT", "").strip()` + `int()` + `logger.warning` block in `jobs.py` re-implemented the logic already in `_getenv_num`. A new `_getenv_optional_int` helper (delegating to `_getenv_num(name, None, int)`) replaces the duplicate, consolidating LIMIT parse warnings with the rest of the env-var helpers.
+
+- **`frigate_api.py` uses a shared `_get_frigate_url()` accessor**: `os.environ.get("FRIGATE_URL", "").rstrip("/")` was copy-pasted into all four public functions. A private helper eliminates the duplication so URL normalization is defined once.
+
+- **`blur_score_from_image()` extracted to `quality.py`**: the time-spread blur-score fallback in `execute_jobs` (resize to 1440px, RGB convert, `assess_quality`) is now a shared `blur_score_from_image(img, max_dim=1440)` helper. Both the executor and any future callers use the same cap and error handling so the score scale can't silently diverge between code paths.
+
 ## [0.5.18] - 2026-06-15
 
 ### Fixed

pyproject.toml

@@ -1,6 +1,6 @@
 [project]
 name = "winnow"
-version = "0.5.18"
+version = "0.5.19"
 description = "Selects diverse, high-quality photos from Immich as training data for Frigate face recognition."
 license = "AGPL-3.0-or-later"
 requires-python = ">=3.13"

winnow/config.py

@@ -45,6 +45,11 @@ def _getenv_optional_float(name: str) -> float | None:
         return None
 
 
+def _getenv_optional_int(name: str) -> int | None:
+    """Return int value of env var, or None if unset/empty. Warns and returns None on invalid."""
+    return _getenv_num(name, None, int)
+
+
 def _getenv_bool(name: str, default: bool) -> bool:
     raw = os.getenv(name)
     if raw is None:

winnow/embeddings.py

@@ -235,8 +235,7 @@ def get_embedding(
 def _is_module_available(module_name: str) -> bool:
     """Check if a Python module is importable without importing it fully."""
     try:
-        importlib.util.find_spec(module_name)
+        return importlib.util.find_spec(module_name) is not None
-        return True
     except (ModuleNotFoundError, ValueError):
         return False
 

winnow/executor.py

@@ -22,7 +22,7 @@ from .frigate_api import (
 from .image_processing import process_face_mode
 from .immich_api import fetch_full_image
 from .log_config import console
-from .quality import assess_quality
+from .quality import blur_score_from_image
 from .reconcile import enrich_asset_with_face_data, reconcile_frigate_mappings
 from .upload_tracker import (
     get_lowest_quality_mapped_file,
@@ -176,20 +176,12 @@ def execute_jobs(jobs: list[dict]) -> None:
                                 if isinstance(saved, tuple):
                                     dims_map[filename] = saved
                                 # Time-spread path: compute blur score from the downloaded
-                                # image. Cap at 1440px so the scale matches the preview
+                                # image. Capped at 1440px via blur_score_from_image() so the
-                                # thumbnails the embedding path uses for scoring — Laplacian
+                                # scale matches the preview thumbnails the embedding path uses
-                                # variance grows with resolution, making full-res and
+                                # — Laplacian variance grows with resolution, making full-res
-                                # thumbnail scores incomparable if left uncapped.
+                                # and thumbnail scores incomparable if left uncapped.
                                 if score_map[filename] is None:
-                                    try:
+                                    score_map[filename] = blur_score_from_image(img)
-                                        score_img = img.convert("RGB") if img.mode != "RGB" else img
-                                        if score_img.width > 1440 or score_img.height > 1440:
-                                            score_img = score_img.copy()
-                                            score_img.thumbnail((1440, 1440), Image.LANCZOS)
-                                        score_map[filename] = assess_quality(score_img).blur_score
-                                    except Exception as exc:
-                                        logger.debug("Quality score fallback for %s: %s", asset["id"], exc)
-                                        score_map[filename] = 0.0  # unknown quality — treat as lowest
 
                                 count += 1
                             else:
@@ -197,7 +189,7 @@ def execute_jobs(jobs: list[dict]) -> None:
                                     f"[yellow]Skipped {asset['id']} (no usable face data)[/yellow]"
                                 )
                     except Exception as e:
-                        logger.error("Failed to process asset %s: %s", asset["id"], e)
+                        logger.error("Failed to process asset %s: %s", asset.get("id", "<unknown>"), e)
 
                     progress.advance(job_task)
                     progress.advance(overall_task)
@@ -534,7 +526,11 @@ def upload_to_frigate(jobs: list[dict]) -> None:
                                 progress.console.print(f"      [dim]{error_detail}[/dim]")
                             else:
                                 logger.debug("%s HTTP %s: %s", fname, resp.status_code, error_detail)
-                            if resp.status_code == 400 and "face" in full_body.lower():
+                            _is_permanent = (
+                                (resp.status_code == 400 and "face" in full_body.lower())
+                                or resp.status_code == 422
+                            )
+                            if _is_permanent:
                                 asset_id = asset_map.get(fname)
                                 if asset_id:
                                     mark_rejected(asset_id, person_name=name)

winnow/frigate_api.py

@@ -8,13 +8,18 @@ import requests
 logger = logging.getLogger(__name__)
 
 
+def _get_frigate_url() -> str:
+    """Return normalized FRIGATE_URL with trailing slash stripped, or '' if unset."""
+    return os.environ.get("FRIGATE_URL", "").rstrip("/")
+
+
 def get_frigate_version() -> str | None:
     """Fetch Frigate's version string from GET /api/version.
 
     Returns the version string (e.g. "0.16.0-beta4") or None if FRIGATE_URL
     is unset, the endpoint is unreachable, or the response is not parseable.
     """
-    frigate_url = os.environ.get("FRIGATE_URL", "").rstrip("/")
+    frigate_url = _get_frigate_url()
     if not frigate_url:
         return None
     try:
@@ -28,7 +33,7 @@ def get_frigate_version() -> str | None:
 
 def _get_faces_data() -> dict | None:
     """Fetch raw GET /api/faces response. Returns None if unavailable."""
-    frigate_url = os.environ.get("FRIGATE_URL", "").rstrip("/")
+    frigate_url = _get_frigate_url()
     if not frigate_url:
         return None
     try:
@@ -113,7 +118,7 @@ def recognize_face(file_path: str) -> tuple[str | None, float] | None:
     replace mean-comparison with nearest-neighbour distance across individual
     training embeddings for accurate coverage detection.
     """
-    frigate_url = os.environ.get("FRIGATE_URL", "").rstrip("/")
+    frigate_url = _get_frigate_url()
     if not frigate_url:
         return None
     try:
@@ -140,7 +145,7 @@ def delete_frigate_person_files(person_name: str, filenames: list[str]) -> bool:
     Uses POST /api/faces/{name}/delete with body {"ids": [filename, ...]}.
     Returns True on success, False if unreachable or the request fails.
     """
-    frigate_url = os.environ.get("FRIGATE_URL", "").rstrip("/")
+    frigate_url = _get_frigate_url()
     if not frigate_url or not filenames:
         return False
     from urllib.parse import quote

winnow/immich_api.py

@@ -196,14 +196,14 @@ def fetch_face_data(asset_id: str, person_id: str | None = None) -> FaceData | N
         if not isinstance(faces, list) or not faces:
             return None
 
-        # Match the target person if specified
+        # Match the target person if specified; never fall back to a different person's face.
         face = None
         if person_id:
             face = next(
                 (f for f in faces if isinstance(f, dict) and (f.get("person") or {}).get("id") == person_id),
                 None,
             )
-        if face is None:
+        else:
             face = faces[0] if isinstance(faces[0], dict) else None
         if face is None:
             return None
@@ -268,8 +268,11 @@ def fetch_full_image(asset_id: str, timeout: int = 60) -> Image.Image | None:
 
 
 def filter_recent_assets(assets: list[dict], years: int | None = None) -> list[dict]:
-    """Filter assets to keep only those from the last N years."""
+    """Filter assets to keep only those from the last N years. Pass years=0 to include all."""
-    years = years or Config.YEARS_FILTER
+    if years is None:
+        years = Config.YEARS_FILTER
+    if not years:
+        return list(assets)
     cutoff = datetime.now(timezone.utc) - timedelta(days=365 * years)
 
     logger.debug("Filtering assets older than %s years (%s)", years, cutoff)

winnow/jobs.py

@@ -8,7 +8,7 @@ from rich.progress import BarColumn, Progress, SpinnerColumn, TaskProgressColumn
 from rich.prompt import Confirm, IntPrompt, Prompt
 from rich.table import Table
 
-from .config import Config, _getenv_bool, _getenv_int
+from .config import Config, _getenv_bool, _getenv_int, _getenv_optional_int
 from .diversity import select_diverse_assets
 from .embeddings import is_embedding_available, load_embedding_model
 from .frigate_api import get_frigate_face_counts
@@ -68,13 +68,9 @@ def _resolve_strategy(strategy: str, has_embedding: bool) -> tuple[int | str, st
     if not has_embedding:
         return _getenv_int("LIMIT", 30), "time"
 
-    custom_limit = os.environ.get("LIMIT", "").strip()
+    custom_limit = _getenv_optional_int("LIMIT")
-    if custom_limit:
+    if custom_limit is not None:
-        try:
+        return custom_limit, "smart"
-            return int(custom_limit), "smart"
-        except ValueError:
-            logger.warning("LIMIT=%r is not a valid integer — using adaptive strategy", custom_limit)
-            # fall through to strategy_map
 
     strategy_map = {
         "adaptive": ("auto", "smart"),

winnow/quality.py

@@ -138,3 +138,23 @@ def assess_quality(
 
     return QualityResult(passed=len(reasons) == 0, reasons=reasons, blur_score=blur_score)
 
+
+def blur_score_from_image(img: Image.Image, max_dim: int = 1440) -> float:
+    """Compute Laplacian-variance blur score, capped at max_dim px to normalise scale.
+
+    Caps resolution so full-res and thumbnail scores are comparable — Laplacian
+    variance grows with pixel count, making uncapped full-res scores much larger
+    than thumbnail scores for the same perceived sharpness.
+
+    Returns 0.0 on any error so callers can treat the result as lowest quality.
+    """
+    try:
+        score_img = img.convert("RGB") if img.mode != "RGB" else img
+        if score_img.width > max_dim or score_img.height > max_dim:
+            score_img = score_img.copy()
+            score_img.thumbnail((max_dim, max_dim), Image.LANCZOS)
+        return float(assess_quality(score_img).blur_score)
+    except Exception as exc:
+        logger.debug("blur_score_from_image failed: %s", exc)
+        return 0.0
+

winnow/reconcile.py

@@ -71,7 +71,7 @@ def reconcile_frigate_mappings(
         )
         mappings = {
             frigate_file: asset_id
-            for (_, asset_id), frigate_file in zip(uploaded, sorted(new_files, key=_ts))
+            for (_, asset_id), frigate_file in zip(uploaded, sorted(new_files, key=lambda f: (_ts(f), f)))
             if asset_id
         }
         record_frigate_files_batch(person_name, mappings)

winnow/upload_tracker.py

@@ -38,7 +38,7 @@ CREATE TABLE IF NOT EXISTS tracked_assets (
     crop_width    INTEGER,
     crop_height   INTEGER,
     frigate_score REAL,
-    PRIMARY KEY (asset_id, status)
+    PRIMARY KEY (asset_id, person_name, status)
 );
 
 CREATE TABLE IF NOT EXISTS frigate_files (
@@ -58,6 +58,44 @@ _conn: sqlite3.Connection | None = None
 _conn_path: str | None = None
 
 
+def _migrate_schema_v2(conn: sqlite3.Connection) -> None:
+    """Migrate tracked_assets from PRIMARY KEY (asset_id, status) to (asset_id, person_name, status).
+
+    The old PK meant INSERT OR REPLACE for person B on an asset already tracked for
+    person A would silently overwrite person_name, breaking quality-replacement JOINs
+    for person A. The new PK gives each (asset, person) pair its own row.
+
+    SQLite does not support ALTER TABLE to change a primary key; we recreate the table.
+    """
+    pk_cols = {
+        r[1]
+        for r in conn.execute("PRAGMA table_info(tracked_assets)").fetchall()
+        if r[5] > 0  # column index 5 = pk position (0 = not in PK)
+    }
+    if "person_name" in pk_cols:
+        return  # Already at new schema
+
+    logger.info("Migrating tracked_assets: adding person_name to primary key")
+    conn.executescript("""
+        CREATE TABLE tracked_assets_new (
+            asset_id      TEXT NOT NULL,
+            person_name   TEXT,
+            status        TEXT NOT NULL CHECK(status IN ('uploaded', 'rejected')),
+            blur_score    REAL,
+            crop_width    INTEGER,
+            crop_height   INTEGER,
+            frigate_score REAL,
+            PRIMARY KEY (asset_id, person_name, status)
+        );
+        INSERT OR IGNORE INTO tracked_assets_new
+            SELECT asset_id, person_name, status, blur_score, crop_width, crop_height, frigate_score
+            FROM tracked_assets;
+        DROP TABLE tracked_assets;
+        ALTER TABLE tracked_assets_new RENAME TO tracked_assets;
+    """)
+    logger.info("tracked_assets schema migration complete")
+
+
 def _get_conn() -> sqlite3.Connection:
     """Return (or create) the module-level SQLite connection.
 
@@ -87,6 +125,7 @@ def _get_conn() -> sqlite3.Connection:
         _conn.executescript(_DDL)
         _conn.commit()
         _conn_path = db_path
+        _migrate_schema_v2(_conn)
         _maybe_migrate(data_dir, _conn)
 
     return _conn