fix: symlink guard placement, fetch_all_assets raw count, config TOCTOU (#31)

executor.py:
- Move islink check into _safe_person_dir on the raw path, before realpath
  resolves it; the previous check at the rmtree site was unreachable dead code
  because realpath already followed any symlink

immich_api.py / jobs.py:
- fetch_all_assets now returns (assets, total_raw) where total_raw is the
  item count seen before non-dict filtering; callers use it for MIN_FACE_COUNT
  guard and display so transient non-dict API items can't incorrectly skip people
- Add WARNING when pagination stops because a page had items but all were non-dict

config.py:
- Cache _data_cfg.exists() in _data_cfg_exists so the dual-config warning
  and config_file selection always read from the same stat() result; previously
  two calls created a TOCTOU window where log and code could disagree

Bump version to 0.5.7

CHANGELOG.md

@@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [0.5.7] - 2026-06-15
+
+### Fixed
+
+- **Symlink guard moved before `realpath`**: `_safe_person_dir` now checks whether the raw (unresolved) path is a symlink before calling `os.path.realpath`. The previous check in `execute_jobs` ran after `realpath` had already resolved the link, making it unreachable dead code.
+
+- **`fetch_all_assets` returns raw item count**: the function now returns `(assets, total_raw)` where `total_raw` is the total items seen across all pages before non-dict filtering. `auto_configure` and `_configure_person` use `total_raw` for the `MIN_FACE_COUNT` guard and display, so transient non-dict API items cannot cause a person to be incorrectly skipped.
+
+- **Pagination stop on all-non-dict page now logs a warning**: when `valid_assets` is empty but the page was non-empty (all items were non-dict), a `WARNING` is emitted explaining why pagination stopped, distinguishing it from natural end-of-data.
+
+- **`_data_cfg.exists()` called once**: the result is cached in `_data_cfg_exists` so the dual-config warning check and the `config_file` selection always agree — previously two separate `stat()` calls created a TOCTOU window where the log could claim one file while the code loaded another.
+
 ## [0.5.6] - 2026-06-15
 
 ### Fixed

pyproject.toml

@@ -1,6 +1,6 @@
 [project]
 name = "winnow"
-version = "0.5.6"
+version = "0.5.7"
 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"

uv.lock

@@ -862,7 +862,7 @@ wheels = [
 
 [[package]]
 name = "winnow"
-version = "0.5.6"
+version = "0.5.7"
 source = { editable = "." }
 dependencies = [
     { name = "croniter" },

winnow/config.py

@@ -123,14 +123,15 @@ class _Config:
         # Prefer DATA_DIR/.immich_config.json (volume-safe in Docker) and fall back
         # to the legacy CWD path so existing installations continue to work.
         _data_cfg = Path(self.DATA_DIR) / ".immich_config.json"
-        if _data_cfg.exists() and _LEGACY_CONFIG_FILE.exists():
+        _data_cfg_exists = _data_cfg.exists()
+        if _data_cfg_exists and _LEGACY_CONFIG_FILE.exists():
             logging.warning(
                 "Two config files found: %s and %s — using %s. Remove the legacy file to silence this.",
                 _data_cfg,
                 _LEGACY_CONFIG_FILE,
                 _data_cfg,
             )
-        config_file = _data_cfg if _data_cfg.exists() else _LEGACY_CONFIG_FILE
+        config_file = _data_cfg if _data_cfg_exists else _LEGACY_CONFIG_FILE
         if config_file.exists():
             try:
                 data = json.loads(config_file.read_text())

winnow/executor.py

@@ -43,8 +43,13 @@ def _safe_person_dir(output_dir: str, person_name: str) -> str:
 
     os.path.join silently discards output_dir when person_name is absolute,
     and '../..' sequences resolve outside the tree. Both are rejected here.
+    Symlinks on the raw (unresolved) path are also rejected — checking after
+    realpath would be too late because realpath follows the link first.
     """
-    candidate = os.path.realpath(os.path.join(output_dir, person_name))
+    raw = os.path.join(output_dir, person_name)
+    if os.path.islink(raw):
+        raise ValueError(f"Person name {person_name!r} resolves to a symlink — skipping")
+    candidate = os.path.realpath(raw)
     base = os.path.realpath(output_dir)
     # Use the base path as its own prefix when it's the filesystem root ("/"),
     # otherwise append os.sep — avoids the false "//" double-slash when base == "/".
@@ -100,9 +105,6 @@ def execute_jobs(jobs: list[dict]) -> None:
                 logger.error(str(e))
                 continue
             # Face crops are transient (uploaded then discarded); wipe before each run.
-            if os.path.islink(person_dir):
-                logger.error("person_dir %s is a symlink — refusing to remove", person_dir)
-                continue
             if os.path.isdir(person_dir):
                 shutil.rmtree(person_dir)
             os.makedirs(person_dir, exist_ok=True)

winnow/immich_api.py

@@ -83,19 +83,25 @@ def merge_people(survivor_id: str, merge_ids: list[str]) -> bool:
         return False
 
 
-def fetch_all_assets(person: dict) -> list[dict]:
+def fetch_all_assets(person: dict) -> tuple[list[dict], int]:
-    """Fetch all assets for a person with pagination."""
+    """Fetch all assets for a person with pagination.
+
+    Returns (assets, raw_total) where assets is the list of valid dict items
+    and raw_total is the total item count seen before non-dict filtering.
+    raw_total may exceed len(assets) if Immich returned non-dict items.
+    """
     name = person.get("name", "Unknown")
     person_id = person.get("id")
     if not person_id:
         logger.error("Person dict missing 'id' field for %s — skipping asset fetch", name)
-        return []
+        return [], 0
     url = f"{Config.IMMICH_URL}/api/search/metadata"
     page_size = 1000
 
     logger.debug("Fetching assets for %s...", name)
 
-    assets = []
+    assets: list[dict] = []
+    total_raw = 0  # items seen across all pages before non-dict filtering
     for page in range(1, MAX_PAGES + 1):
         try:
             resp = requests.post(
@@ -116,6 +122,7 @@ def fetch_all_assets(person: dict) -> list[dict]:
                 page_assets = page_assets.get("items", [])
 
             page_count = len(page_assets)  # raw count for termination check before filtering
+            total_raw += page_count
 
             # Single pass: partition valid assets from unexpected non-dict items
             valid_assets, skipped_count = [], 0
@@ -128,6 +135,11 @@ def fetch_all_assets(person: dict) -> list[dict]:
                 logger.warning("%s: skipping %s non-dict item(s) in page %s", name, skipped_count, page)
 
             if not valid_assets:
+                if page_count > 0:
+                    logger.warning(
+                        "%s: page %s returned %s item(s) but none were valid dicts — stopping pagination",
+                        name, page, page_count,
+                    )
                 break
 
             assets.extend(valid_assets)
@@ -140,7 +152,7 @@ def fetch_all_assets(person: dict) -> list[dict]:
             logger.error("Exception fetching assets for %s (page %s): %s", name, page, e)
             break
 
-    return assets
+    return assets, total_raw
 
 
 def fetch_face_data(asset_id: str, person_id: str | None = None) -> FaceData | None:

winnow/jobs.py

@@ -162,10 +162,10 @@ def _configure_person(person: dict, people: list[dict]) -> dict | None:
 
     console.print(f"Scanning for {name}...")
     with console.status("[bold green]Fetching assets...[/bold green]"):
-        all_assets = fetch_all_assets(person)
+        all_assets, total_raw = fetch_all_assets(person)
         recent_assets = filter_recent_assets(all_assets, years=years)
 
-    rprint(f"  Found [bold]{len(all_assets)}[/bold] total, [bold]{len(recent_assets)}[/bold] in range ({years} years).")
+    rprint(f"  Found [bold]{total_raw}[/bold] total, [bold]{len(recent_assets)}[/bold] in range ({years} years).")
 
     # Ask before strategy so the post-dedup count can inform the choice
     retry_env = os.environ.get("RETRY_REJECTED", "false").lower() in ("true", "1", "yes")
@@ -262,16 +262,18 @@ def auto_configure(people: list[dict]) -> list[dict]:
     for person in valid_people:
         name = person["name"]
 
-        all_assets = fetch_all_assets(person)
+        all_assets, total_raw = fetch_all_assets(person)
         recent_assets = filter_recent_assets(all_assets, years=Config.YEARS_FILTER)
 
-        rprint(f"  {name}: {len(all_assets)} total, {len(recent_assets)} recent")
+        rprint(f"  {name}: {total_raw} total, {len(recent_assets)} recent")
 
         # MIN_FACE_COUNT guard: skip people with too few Immich assets.
+        # Uses total_raw (pre-filter count) so non-dict items from a transient
+        # Immich schema issue don't cause a person to be skipped incorrectly.
         # Done here (after fetch) rather than upfront because Immich v2.7.5+
         # dropped assetCount from the /api/people response.
-        if min_face_count > 0 and len(all_assets) < min_face_count:
+        if min_face_count > 0 and total_raw < min_face_count:
-            rprint(f"  [dim]Skipping {name} ({len(all_assets)} assets < MIN_FACE_COUNT={min_face_count}).[/dim]")
+            rprint(f"  [dim]Skipping {name} ({total_raw} assets < MIN_FACE_COUNT={min_face_count}).[/dim]")
             continue
 
         # Enforce MAX_AUTO_IMAGES against the tracked file count only.