feat(fix-trash): chain after stacking runs and harden the trash decisions#72
Merged
Conversation
--fix-trash-after-stacking / FIX_TRASH_AFTER_STACKING=true runs the fix-trash flow after each stacking run, in both once and cron modes. The per-key fix-trash body is extracted into fixTrashForAPIKey, called by the fix-trash command loop and by both stacker loops. The chained run creates its own client on purpose: fix-trash must not inherit the stacker's reset/replace/filter options (a reset-configured client would re-trigger stack deletion from FetchAllStacks). Closes #66
--trash-orphaned-raws=false and --fix-trash-after-stacking=false were silently overridden by TRASH_ORPHANED_RAWS=true / FIX_TRASH_AFTER_STACKING=true because the plain 'if !x' env fallback cannot tell an omitted flag from an explicit false. Both flags now use the existing Changed-tracking in PersistentPreRun, like replace-stacks and include-videos.
…estamps The replacement heuristic compared file-system timestamps of same-name assets, which failed in both directions: - a duplicate with identical timestamps was not detected, so trashing one copy cascaded the surviving copy and its RAW companions to trash - a recycled filename from an unrelated photo (DSC_0001.jpg every 10k shots) or a metadata touch bumping UpdatedAt looked like a replacement and silently suppressed a legitimate cascade forever A trashed asset is now skipped when an active asset has the same OriginalFileName and the same LocalDateTime: an active copy of the same photo exists, so the deletion applies to that duplicate only. Also make archived assets protective but untouchable: fix-trash now always fetches archived assets so an archived companion can protect its group, and never cascades an archived group member to trash.
The orphan pass treated any RAW absent from the stacker's groups as companionless. But assets the criteria cannot evaluate (no regex match, empty localDateTime under a time criterion) are skipped by StackBy before grouping, so their absence says nothing about companions — narrow criteria turned protected RAWs into false orphans. pkg/stacker gains MatchedAssetIDs, which reports the assets the criteria can key on the same path StackBy takes (legacy, expression, OR-groups). Ungrouped RAWs are now only orphan candidates when the criteria matched them; archived RAWs are never candidates and an archived developed companion now protects its group.
- Pass 1 describes the active-copy skip (same filename and capture time) instead of the removed timestamp heuristic - Archived assets: always fetched for protection, never trashed - Scheduled example no longer recommends --log-level warn: the summary of what was trashed is info-level and is the only record an unattended run leaves
Adversarial re-review of the previous fixes broke them in two spots, both reproduced by execution: - MatchedAssetIDs mirrored StackBy's keying but not the time merge: mergeTimeBasedGroups drops keyed assets whose time field does not parse, so under the default criteria a DNG with an empty localDateTime was matched, dropped from every group, and flagged as a false orphan despite its JPG companion. MatchedAssetIDs now also requires every time-based criterion to extract a parseable value. - hasActiveCopy compared capture times by string equality: sub-second precision drift between two uploads of the same photo (stripped SubSecTimeOriginal) defeated the copy detection while both copies still shared a grouping bucket, cascading the survivor. Capture times now compare with a one-second tolerance. Also refresh the docs: pass 2 describes the criteria-match and archived restrictions, the example output uses the current log line, and the archived-protection note mentions servers that ignore the archived option on /search/metadata.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in way to run fix-trash automatically after each stacking run (both once and cron modes) while hardening the fix-trash decision logic to avoid false cascades and false orphan-RAW detections.
Changes:
- Add
--fix-trash-after-stacking/FIX_TRASH_AFTER_STACKINGto chain a fullfix-trashrun after each stacking run. - Make
fix-trashsafer/correcter: detect surviving duplicates via (filename + capture time), ensure archived assets protect but are never trashed, and tighten orphan-RAW candidate selection to criteria-evaluable RAWs. - Add regression tests for the above behaviors and flag/env precedence.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Mentions chaining fix-trash after stacking. |
| pkg/stacker/stacker_matches.go | Adds MatchedAssetIDs helper to detect which assets criteria can truly evaluate. |
| pkg/stacker/stacker_matches_test.go | Unit tests for MatchedAssetIDs. |
| docs/commands/fix-trash.md | Updates fix-trash behavior docs + chaining instructions. |
| docs/api-reference/environment-variables.md | Documents FIX_TRASH_AFTER_STACKING. |
| cmd/stacker.go | Executes chained fix-trash after stacking; adds chained-mode diagnostics. |
| cmd/stacker_test.go | Verifies the new persistent flag exists and defaults correctly. |
| cmd/main.go | Adds --fix-trash-after-stacking and tracks explicit-flag precedence. |
| cmd/fixtrash.go | Refactors to shared fixTrashForAPIKey entrypoint; always fetch archived assets. |
| cmd/fixtrash_orphans.go | Tightens orphan RAW candidate selection; excludes archived RAWs; uses MatchedAssetIDs. |
| cmd/fixtrash_orphans_test.go | Adds regression tests for orphan RAW edge cases and archived behavior. |
| cmd/fixtrash_analysis.go | Replaces “newer timestamp” heuristic with capture-time-based copy detection; skips archived cascades. |
| cmd/fixtrash_analysis_test.go | Updates tests for new duplicate/cascade detection rules. |
| cmd/config.go | Adds new config vars + ensures explicit flags beat env vars. |
| cmd/config_test.go | Tests new env var + explicit-false flag precedence behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot review on #72: with --fix-trash-after-stacking, the run never goes through runFixTrash, so the existing filters-have-no-effect warning was bypassed — a user chaining fix-trash with FILTER_ALBUM_IDS could assume the pass is scoped to the album while it processes the whole library. The warning now also fires from runStacker when the chain is enabled. Also mention the CLI flag next to the env var in the README.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #66
What
FIX_TRASH_AFTER_STACKING/--fix-trash-after-stacking(default off): runs the full fix-trash flow after each stacking run, in bothonceandcronmodes — one container, one scheduler, as requested in #66. The chained run builds its own client on purpose: fix-trash must never inherit the stacker's reset/replace/filter options (a reset-configured client would re-trigger stack deletion fromFetchAllStacks).Safety fixes
A full-feature correctness review of fix-trash surfaced four issues, fixed here and pinned by tests:
--trash-orphaned-raws=falsewas silently overridden byTRASH_ORPHANED_RAWS=true; both fix-trash bools now use the existingChanged-tracking.pkg/stackergainsMatchedAssetIDs, which mirrors StackBy's matching on all three grouping paths and additionally requires every time-based criterion to be evaluable (the time merge silently drops unparseable-time assets).Docs updated accordingly; the scheduled example no longer recommends
--log-level warn(the info-level summary is the only record of what an unattended run trashed).Testing
go test -race ./...green; the two behavior fixes were additionally validated by adversarial review with executed reproductions.