Plugin: rework matching policy #35

hermes · 2026-08-31T12:15:45Z

hermes commented

2026-08-31 12:15:45 +00:00

Closes #29

Problem

The matcher collapsed duplicate candidates with setdefault, stripped arbitrary punctuation, ignored duration, and did not honor or create apple_music_id associations. Sync therefore could not persist an explicit ambiguous state.

Changes

implement precedence for existing apple_music_id, unique ISRC, unique strong metadata, then unresolved
preserve duplicate candidates as ambiguous, persist resolution state and current playlist memberships in appleplaylists.db
implement NFKC/case-fold/whitespace normalization with typographic apostrophe and dash folding while preserving other punctuation
apply the inclusive two-second duration tolerance, absorbing floating-point roundoff without accepting genuinely over-limit durations; ignore album/missing durations
settle associations across all selected playlists before rendering occurrences, keeping duplicates and repeated syncs consistent
write apple_music_id on association, fill only an empty local ISRC, and never write audio-file tags
trust existing associations during normal sync and preserve local metadata/ISRC conflicts

Verification

pytest -q — 80 passed
Ruff check and format check passed for the changed matcher/sync/test files
git diff --check and Python compileall passed
regression sabotage confirmed the ambiguity test fails when first-candidate selection is restored
real beets Library/Item smoke test confirmed apple_music_id and empty-ISRC persistence
regression tests use real beets items to cover repeated occurrences within/across playlists, unchanged repeat-sync contents, and fractional duration boundaries in both directions
both review regressions were reproduced with failing tests before the fixes

Scope

The interactive/explicit match command remains #31. Broader named/full-sync pruning and incomplete-view semantics remain #30.

Closes #29 ## Problem The matcher collapsed duplicate candidates with `setdefault`, stripped arbitrary punctuation, ignored duration, and did not honor or create `apple_music_id` associations. Sync therefore could not persist an explicit ambiguous state. ## Changes - implement precedence for existing `apple_music_id`, unique ISRC, unique strong metadata, then unresolved - preserve duplicate candidates as `ambiguous`, persist resolution state and current playlist memberships in `appleplaylists.db` - implement NFKC/case-fold/whitespace normalization with typographic apostrophe and dash folding while preserving other punctuation - apply the inclusive two-second duration tolerance, absorbing floating-point roundoff without accepting genuinely over-limit durations; ignore album/missing durations - settle associations across all selected playlists before rendering occurrences, keeping duplicates and repeated syncs consistent - write `apple_music_id` on association, fill only an empty local ISRC, and never write audio-file tags - trust existing associations during normal sync and preserve local metadata/ISRC conflicts ## Verification - `pytest -q` — 80 passed - Ruff check and format check passed for the changed matcher/sync/test files - `git diff --check` and Python compileall passed - regression sabotage confirmed the ambiguity test fails when first-candidate selection is restored - real beets `Library`/`Item` smoke test confirmed `apple_music_id` and empty-ISRC persistence - regression tests use real beets items to cover repeated occurrences within/across playlists, unchanged repeat-sync contents, and fractional duration boundaries in both directions - both review regressions were reproduced with failing tests before the fixes ## Scope The interactive/explicit `match` command remains #31. Broader named/full-sync pruning and incomplete-view semantics remain #30.

hermes added 1 commit

2026-08-31 12:15:45 +00:00

Plugin: rework matching policy (#29 ) 4d96ad6625

hermes reviewed

2026-09-05 14:17:12 +00:00

hermes left a comment

Review: changes recommended before merge

Reviewed 4d96ad6625 against main. One correctness issue: the same Apple track can receive different matching decisions within a single sync, dropping earlier duplicate occurrences and making an unchanged second sync produce different contents. See inline reproduction.

Validation: 74 tests passed; Ruff check passed for matching.py, sync.py and their test files; compileall and git diff --check passed. The additional reproduction used real beets Library/Item objects and temporary SQLite databases with a fake playlistmanager store. No source changes made.

This is a comment review because the reviewing account is also the PR author.

## Review: changes recommended before merge Reviewed 4d96ad6625151f0c6e161fbc0916d9b7eec07b10 against main. One correctness issue: the same Apple track can receive different matching decisions within a single sync, dropping earlier duplicate occurrences and making an unchanged second sync produce different contents. See inline reproduction. Validation: 74 tests passed; Ruff check passed for matching.py, sync.py and their test files; compileall and git diff --check passed. The additional reproduction used real beets Library/Item objects and temporary SQLite databases with a fake playlistmanager store. No source changes made. This is a comment review because the reviewing account is also the PR author.

beetsplug/appleplaylists/sync.py Outdated

Lines 65-75

					
				@ -67,0 +62,14 @@

				                    state_store.replace_memberships(apple_playlist.id, tracks)

				                    item_ids: list[int] = []

				                    for track in tracks:

				                        resolution = index.resolve(track)

				                        if resolution.item is None:

				                            state_store.set_resolution_state(

				                                track.library_track_id, resolution.state

				                            )

				                            result.missing_tracks.append(track)

				                        else:

				                            index.associate(resolution.item, track)

				                            state_store.set_resolution_state(

				                                track.library_track_id, resolution.state

				                            )

P2: Use a consistent resolution for every occurrence of an Apple track. index.associate() changes which candidates later calls to resolve() consider, but earlier item_ids/missing_tracks entries are never reconciled. Reproduced with two real beets items having identical artist/title/duration and different ISRCs, and Apple occurrences [A (no ISRC), B (ISRC uniquely matching item 1), A]: the first A is ambiguous; B associates item 1; the second A now uniquely matches item 2. The first sync writes (1, 2) and reports A missing even though its persisted state is matched. An unchanged second sync writes (2, 1, 2). This loses one duplicate occurrence and makes the first result inconsistent with the saved association. Compute/reconcile decisions at Apple-track identity level before rendering occurrence lists, so all occurrences use the same final decision; add this regression case (and the equivalent across playlists).

**P2: Use a consistent resolution for every occurrence of an Apple track.** `index.associate()` changes which candidates later calls to `resolve()` consider, but earlier `item_ids`/`missing_tracks` entries are never reconciled. Reproduced with two real beets items having identical artist/title/duration and different ISRCs, and Apple occurrences `[A (no ISRC), B (ISRC uniquely matching item 1), A]`: the first A is ambiguous; B associates item 1; the second A now uniquely matches item 2. The first sync writes `(1, 2)` and reports A missing even though its persisted state is matched. An unchanged second sync writes `(2, 1, 2)`. This loses one duplicate occurrence and makes the first result inconsistent with the saved association. Compute/reconcile decisions at Apple-track identity level before rendering occurrence lists, so all occurrences use the same final decision; add this regression case (and the equivalent across playlists).

hermes reviewed

2026-09-05 14:18:20 +00:00

hermes left a comment

Additional independently identified and locally verified finding: the inclusive two-second duration boundary rejects some fractional-second values. This brings the review to two P2 correctness findings; changes recommended before merge.

beetsplug/appleplaylists/matching.py Outdated

					
				@ -20,0 +40,4 @@

				    item_duration = getattr(item, "length", None)

				    if item_duration is None or track.duration_ms is None:

				        return True

				    return abs(float(item_duration) * 1000 - track.duration_ms) <= 2000

P2: Allow floating-point rounding at the inclusive duration boundary. Verified with a real beets Item(length=128.003) and an otherwise identical Apple track with duration_ms=130003: abs(item.length * 1000 - track.duration_ms) is 2000.0000000000146, and the unique metadata candidate resolves as unmatched. The intended difference is exactly two seconds, so this violates the inclusive tolerance and unnecessarily omits the track. Use a narrowly bounded numerical tolerance (without accepting genuinely over-limit durations) and test a fractional-second boundary as well as the existing whole-second case.

**P2: Allow floating-point rounding at the inclusive duration boundary.** Verified with a real beets `Item(length=128.003)` and an otherwise identical Apple track with `duration_ms=130003`: `abs(item.length * 1000 - track.duration_ms)` is `2000.0000000000146`, and the unique metadata candidate resolves as `unmatched`. The intended difference is exactly two seconds, so this violates the inclusive tolerance and unnecessarily omits the track. Use a narrowly bounded numerical tolerance (without accepting genuinely over-limit durations) and test a fractional-second boundary as well as the existing whole-second case.

hermes added 1 commit

2026-09-05 15:06:38 +00:00

Plugin: fix matching consistency and duration boundary (#29 ) ba04e3ca19

hermes commented

2026-09-05 15:07:14 +00:00

Fixed both review findings in ba04e3ca19 (added to this PR without rewriting the original commit).

Matching now settles unique Apple-track associations across all selected playlists before rendering occurrences. Unresolved tracks are revisited after other associations narrow the candidate pool; every occurrence uses the final decision. The [A, B, A] reproduction now yields (2, 1, 2) on both the first and second sync, with no contradictory missing report. Regression tests cover both one playlist and occurrences split across playlists, including persisted states and unchanged repeat-sync contents.
Duration comparison now absorbs only floating-point roundoff at the inclusive two-second boundary. Real-beets tests cover both boundary directions and confirm durations one microsecond beyond the limit remain rejected.

Validation: 80 tests passed; focused Ruff lint/format checks, compileall and git diff --check passed. Both regressions were demonstrated failing before implementation. Independent review found no introduced correctness/security issues and independently ran all 80 tests. PR description updated with the fixes and current test count.

Fixed both review findings in ba04e3ca19eb963de88c65b59903a4bd50c6197c (added to this PR without rewriting the original commit). - Matching now settles unique Apple-track associations across all selected playlists before rendering occurrences. Unresolved tracks are revisited after other associations narrow the candidate pool; every occurrence uses the final decision. The `[A, B, A]` reproduction now yields `(2, 1, 2)` on both the first and second sync, with no contradictory missing report. Regression tests cover both one playlist and occurrences split across playlists, including persisted states and unchanged repeat-sync contents. - Duration comparison now absorbs only floating-point roundoff at the inclusive two-second boundary. Real-beets tests cover both boundary directions and confirm durations one microsecond beyond the limit remain rejected. Validation: 80 tests passed; focused Ruff lint/format checks, compileall and git diff --check passed. Both regressions were demonstrated failing before implementation. Independent review found no introduced correctness/security issues and independently ran all 80 tests. PR description updated with the fixes and current test count.

tophattedcat merged commit cf7f001c05 into main

2026-09-05 15:20:40 +00:00

tophattedcat referenced this pull request from a commit

2026-09-05 15:20:42 +00:00

Merge pull request 'Plugin: rework matching policy' (#35) from issue-29-matching-policy into main

Sign in to join this conversation.

No reviewers