fix: address 9 code review findings

- upload_tracker: partial migration now rolls back atomically on failure;
  JSON renamed only after successful commit so failed runs retry cleanly
- upload_tracker: allowlist score_col in _pick_mapped_file to close
  latent SQL injection surface
- config: move load_dotenv() from module import into _load() so no I/O
  at import time and reset() fully resets env loading
- config: use is None checks for IMMICH_URL/OUTPUT_DIR config-file
  fallback so explicitly empty env vars are not overridden by the file
- executor: skip reconcile when Frigate API is unreachable at upload
  start — tracker baseline is incomplete and would mis-trigger the
  external-upload guard, permanently losing file mappings
- reconcile: change polling break condition from >= to == target so
  transient overshoots don't prematurely exit the loop and trigger
  the external-upload guard
- jobs: apply capacity cap as the selection limit rather than truncating
  post-selection by position, so the diversity algorithm works within
  the right budget from the start
- Dockerfile: explicit gpu branch + exit 1 on unknown VARIANT instead
  of silent fallback

Dockerfile

@@ -48,8 +48,11 @@ RUN if [ "$VARIANT" = "cpu" ]; then \
         uv sync --frozen --no-dev --extra rocm; \
     elif [ "$VARIANT" = "intel" ]; then \
         uv sync --frozen --no-dev --extra intel; \
-    else \
+    elif [ "$VARIANT" = "gpu" ]; then \
         uv sync --frozen --no-dev --extra gpu; \
+    else \
+        echo "Unknown VARIANT: '$VARIANT'. Must be one of: cpu, rocm, intel, gpu" >&2; \
+        exit 1; \
     fi && \
     uv cache clean
 

winnow/config.py

@@ -9,8 +9,6 @@ from typing import ClassVar
 from dotenv import load_dotenv
 from rich.prompt import Prompt
 
-load_dotenv()
-
 CONFIG_FILE = Path(".immich_config.json")
 
 
@@ -81,6 +79,7 @@ class _Config:
 
     def _load(self) -> None:
         """Load configuration from environment and config file."""
+        load_dotenv()
         # Load from environment (highest priority)
         self.IMMICH_URL = os.getenv("IMMICH_URL")
         self.API_KEY = os.getenv("API_KEY")
@@ -102,12 +101,14 @@ class _Config:
         self.ENABLE_CACHE = os.getenv("ENABLE_CACHE", "true").lower() in ("true", "1", "yes")
         self.CACHE_DIR = os.getenv("CACHE_DIR", ".if_cache")
 
-        # Fall back to config file for non-sensitive values (API_KEY not stored here)
+        # Fall back to config file only when the env var is genuinely absent (None).
+        # An explicitly empty env var (IMMICH_URL="") takes priority over the file.
         if CONFIG_FILE.exists():
             try:
                 data = json.loads(CONFIG_FILE.read_text())
-                self.IMMICH_URL = self.IMMICH_URL or data.get("IMMICH_URL")
+                if self.IMMICH_URL is None:
-                if not os.getenv("OUTPUT_DIR"):
+                    self.IMMICH_URL = data.get("IMMICH_URL")
+                if os.getenv("OUTPUT_DIR") is None:
                     self.OUTPUT_DIR = data.get("OUTPUT_DIR", self.OUTPUT_DIR)
             except (json.JSONDecodeError, OSError) as e:
                 logging.warning("Failed to load config file: %s", e)

winnow/executor.py

@@ -299,16 +299,21 @@ def upload_to_frigate(jobs: list[dict]) -> None:
                 else get_frigate_person_files(name)
             )
             if _snapshot is None:
-                # Frigate GET is down; fall back to the tracker's mapped filenames
+                # Frigate GET is down. The tracker only knows files winnow mapped
-                # as the pre-upload baseline. reconciliation will still work unless
+                # previously — it is blind to manually-added Frigate files. Using
-                # there are concurrent manual uploads (handled by >target guard).
+                # the tracker as the baseline would make those unmapped files look
+                # like new uploads in reconcile, triggering the >target guard and
+                # silently dropping all mappings. Skip reconciliation entirely when
+                # we can't get a reliable live snapshot.
                 logger.warning(
-                    f"{name}: Frigate API unreachable at upload start"
+                    "%s: Frigate API unreachable at upload start"
-                    " — using tracker baseline for post-upload reconciliation"
+                    " — file mapping will be skipped for this batch", name
                 )
-                known_frigate_files_at_start: set[str] = get_tracked_frigate_filenames(name)
+                known_frigate_files_at_start: set[str] = set()
+                _skip_reconcile = True
             else:
                 known_frigate_files_at_start: set[str] = set(_snapshot)
+                _skip_reconcile = False
                 # Remove tracker mappings for files that no longer exist in Frigate
                 # (manually deleted, or cleaned up outside winnow). This corrects the
                 # effective_count so those slots are available for new uploads.
@@ -548,7 +553,7 @@ def upload_to_frigate(jobs: list[dict]) -> None:
                 )
 
             # Batch-map Frigate filenames to asset IDs now that all uploads are done.
-            if actually_uploaded:
+            if actually_uploaded and not _skip_reconcile:
                 reconcile_frigate_mappings(name, known_frigate_files_at_start, actually_uploaded)
 
             # Per-person summary

winnow/jobs.py

@@ -292,11 +292,13 @@ def auto_configure(people: list[dict]) -> list[dict]:
 
         # Cap selection to remaining capacity (no cap when replacement-only — executor
         # decides per-image whether to swap; any candidate could be an improvement).
-        auto_cap = None
         if not quality_replacement_only:
             if limit == "auto":
+                # Switch from open-ended auto to a fixed budget at remaining capacity
+                # so the diversity selector itself stops at the right count instead of
+                # selecting MAX_AUTO_IMAGES and then discarding the excess by position.
                 if already_uploaded > 0:
-                    auto_cap = capacity
+                    limit = capacity
             else:
                 limit = min(limit, capacity)
 
@@ -320,11 +322,6 @@ def auto_configure(people: list[dict]) -> list[dict]:
             rprint(f"  [dim]Skipping {name} (0 images selected).[/dim]")
             continue
 
-        # Apply auto_cap post-selection if needed
-        if auto_cap is not None and len(job["assets"]) > auto_cap:
-            job["assets"] = job["assets"][:auto_cap]
-            job["limit"] = len(job["assets"])
-
         rprint(f"  [green]Queued {job['limit']} images for {name}.[/green]")
         jobs.append(job)
 

winnow/reconcile.py

@@ -45,7 +45,7 @@ def reconcile_frigate_mappings(
             )
             return
         current_files = set(fresh)
-        if len(current_files - known_files_before) >= target:
+        if len(current_files - known_files_before) == target:
             break
 
     new_files = current_files - known_files_before

winnow/upload_tracker.py

@@ -112,22 +112,21 @@ def _maybe_migrate(cache_dir: str, conn: sqlite3.Connection) -> None:
 
     logger.info("Migrating JSON tracker files to SQLite in %s", cache_dir)
 
-    with conn:
+    try:
-        if upload_json.exists():
+        with conn:
-            try:
+            if upload_json.exists():
-                data = json.loads(upload_json.read_text())
+                _migrate_json_data(conn, json.loads(upload_json.read_text()), "uploaded")
-                _migrate_json_data(conn, data, "uploaded")
+            if reject_json.exists():
-                upload_json.rename(upload_json.with_suffix(".json.bak"))
+                _migrate_json_data(conn, json.loads(reject_json.read_text()), "rejected")
-            except Exception as exc:
+    except Exception as exc:
-                logger.warning("Migration of %s failed: %s", upload_json, exc)
+        logger.warning("JSON migration failed, will retry next run: %s", exc)
+        return
 
-        if reject_json.exists():
+    # Rename only after successful commit so a failed run retries cleanly next start.
-            try:
+    if upload_json.exists():
-                data = json.loads(reject_json.read_text())
+        upload_json.rename(upload_json.with_suffix(".json.bak"))
-                _migrate_json_data(conn, data, "rejected")
+    if reject_json.exists():
-                reject_json.rename(reject_json.with_suffix(".json.bak"))
+        reject_json.rename(reject_json.with_suffix(".json.bak"))
-            except Exception as exc:
-                logger.warning("Migration of %s failed: %s", reject_json, exc)
 
     logger.info("JSON → SQLite migration complete")
 
@@ -309,9 +308,14 @@ def has_frigate_scores(person_name: str) -> bool:
     return row[0] > 0
 
 
+_VALID_SCORE_COLS = frozenset({"blur_score", "frigate_score"})
+
+
 def _pick_mapped_file(
     person_name: str, score_col: str, *, highest: bool, exclude: set[str] | None = None
 ) -> tuple[str, str, float] | None:
+    if score_col not in _VALID_SCORE_COLS:
+        raise ValueError(f"Invalid score column: {score_col!r}")
     conn = _get_conn()
     order = "DESC" if highest else "ASC"
     rows = conn.execute(