From 728b84dc8ca46fc04b507e01d9872f1879f4fc34 Mon Sep 17 00:00:00 2001 From: Holden Date: Mon, 15 Jun 2026 02:56:51 +0000 Subject: [PATCH] fix: address 10 full-codebase audit findings (v0.5.19) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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", "") 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 | 26 +++++++++++++++++++++++++ pyproject.toml | 2 +- winnow/config.py | 5 +++++ winnow/embeddings.py | 3 +-- winnow/executor.py | 28 ++++++++++++--------------- winnow/frigate_api.py | 13 +++++++++---- winnow/immich_api.py | 11 +++++++---- winnow/jobs.py | 12 ++++-------- winnow/quality.py | 20 ++++++++++++++++++++ winnow/reconcile.py | 2 +- winnow/upload_tracker.py | 41 +++++++++++++++++++++++++++++++++++++++- 11 files changed, 126 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4540e2..670e6b6 100644 --- a/CHANGELOG.md +++ b/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", "")`. + +- **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 diff --git a/pyproject.toml b/pyproject.toml index 712b2cf..0e2488b 100644 --- a/pyproject.toml +++ b/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" diff --git a/winnow/config.py b/winnow/config.py index b516325..d164d83 100644 --- a/winnow/config.py +++ b/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: diff --git a/winnow/embeddings.py b/winnow/embeddings.py index 5f8f7b2..cab4294 100644 --- a/winnow/embeddings.py +++ b/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 True + return importlib.util.find_spec(module_name) is not None except (ModuleNotFoundError, ValueError): return False diff --git a/winnow/executor.py b/winnow/executor.py index 8c88990..3059122 100644 --- a/winnow/executor.py +++ b/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 - # thumbnails the embedding path uses for scoring — Laplacian - # variance grows with resolution, making full-res and - # thumbnail scores incomparable if left uncapped. + # image. Capped at 1440px via blur_score_from_image() so the + # scale matches the preview thumbnails the embedding path uses + # — Laplacian variance grows with resolution, making full-res + # and thumbnail scores incomparable if left uncapped. if score_map[filename] is None: - try: - 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 + score_map[filename] = blur_score_from_image(img) 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", ""), 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) diff --git a/winnow/frigate_api.py b/winnow/frigate_api.py index 32c4fa6..4f5cafc 100644 --- a/winnow/frigate_api.py +++ b/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 diff --git a/winnow/immich_api.py b/winnow/immich_api.py index 5f537d8..3bde08b 100644 --- a/winnow/immich_api.py +++ b/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.""" - years = years or Config.YEARS_FILTER + """Filter assets to keep only those from the last N years. Pass years=0 to include all.""" + 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) diff --git a/winnow/jobs.py b/winnow/jobs.py index e530c0f..8a87551 100644 --- a/winnow/jobs.py +++ b/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() - if custom_limit: - try: - 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 + custom_limit = _getenv_optional_int("LIMIT") + if custom_limit is not None: + return custom_limit, "smart" strategy_map = { "adaptive": ("auto", "smart"), diff --git a/winnow/quality.py b/winnow/quality.py index 8db3ac7..b4e1724 100644 --- a/winnow/quality.py +++ b/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 + diff --git a/winnow/reconcile.py b/winnow/reconcile.py index a4ffcc2..652097c 100644 --- a/winnow/reconcile.py +++ b/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) diff --git a/winnow/upload_tracker.py b/winnow/upload_tracker.py index be22cd5..655535b 100644 --- a/winnow/upload_tracker.py +++ b/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