From 2182c87c4072e911c51ad2b597ff2575fc833ce8 Mon Sep 17 00:00:00 2001 From: Holden Date: Wed, 17 Jun 2026 00:02:49 +0000 Subject: [PATCH] fix: address 2 code review findings (round 11) - upload_tracker: revert data[flat_key] = [] from round 8; clearing the entire shared legacy flat list on a corrupt value wipes all persons' IDs, not just the one being reset; since a corrupt non-list value is already unreadable by load_uploaded_ids, leaving it in place is safer than a mass-wipe; update warning message to note the field is unaffected but unreadable so the corruption is still observable - cli: use p.get("id") instead of p["id"] in both people-list fallback returns (_handle_duplicate_people lines 144 and 157) for consistency with the success path at line 149; bare subscript crashes on malformed unnamed persons that bypass _smaller_duplicate_ids --- winnow/cli.py | 4 ++-- winnow/upload_tracker.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/winnow/cli.py b/winnow/cli.py index 55391a0..f568f91 100644 --- a/winnow/cli.py +++ b/winnow/cli.py @@ -109,7 +109,7 @@ def _handle_duplicate_people(people: list[dict]) -> list[dict]: ) # Return deduplicated list — keep only the largest per name so that # downstream job creation never runs two jobs for the same Frigate folder. - return [p for p in people if p["id"] not in skip_ids] + return [p for p in people if p.get("id") not in skip_ids] # Auto-merge: survivor = largest asset count, rest merge into it inside Immich merged_any = False @@ -141,7 +141,7 @@ def _handle_duplicate_people(people: list[dict]) -> list[dict]: " — possible transient error or expired API key;" " proceeding with pre-merge list. Check IMMICH_API_KEY if this recurs." ) - return [p for p in people if p["id"] not in skip_ids] + return [p for p in people if p.get("id") not in skip_ids] # Filter out the smaller duplicate from any group whose merge failed — those # IDs still exist in Immich and would produce two jobs for the same folder. # IDs from groups that merged successfully are already gone from Immich, so diff --git a/winnow/upload_tracker.py b/winnow/upload_tracker.py index 39e57a3..04ead4c 100644 --- a/winnow/upload_tracker.py +++ b/winnow/upload_tracker.py @@ -444,10 +444,10 @@ def reset_person(person_name: str) -> None: person_ids = set(_get_ids(tracker_entry)) if person_ids and flat_key in data and not isinstance(data[flat_key], list): logger.warning( - "reset_person: %s has unexpected type for %s (%s) — clearing corrupt flat-list", + "reset_person: %s has unexpected type for %s (%s) — skipping flat-list cleanup;" + " all persons' legacy IDs in this field are unaffected but unreadable", filename, flat_key, type(data[flat_key]).__name__, ) - data[flat_key] = [] elif person_ids and flat_key in data: data[flat_key] = sorted(set(data[flat_key]) - person_ids) _save(filename, data)