diff --git a/CLAUDE.md b/CLAUDE.md index 5c5c86d..d380f12 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -40,7 +40,8 @@ Feature work goes through a PR (squash merge to main, see PR #7 for the template ## Sync state model (when touching sync.py / models) - `DiagnosisItem.reason` is **never overwritten** — it's diagnosis-time evidence. Sync-side detail (errors, threshold notes, missing video_ids) goes on `SyncAttempt.reason`. -- `DiagnosisItem.status` is terminal in two cases: `'applied'` (code-set when every API call for the action succeeded) and `'skipped'` (manual override for findings the user never wants to retry). Anything else (`'open'`) gets re-evaluated next run. +- `DiagnosisItem.status` is terminal in two cases: `'applied'` (code-set when every API call for the action succeeded, with one carve-out: `ytm_dedupe` is non-idempotent and flips to `'applied'` after any attempt regardless of outcome to prevent auto-retry over-removal) and `'skipped'` (manual override for findings the user never wants to retry). Anything else (`'open'`) gets re-evaluated next run. - Drift fixes are like-then-unlike (fail-safe order). Both halves must succeed before flipping to `'applied'`. - Continue-on-error is the dispatcher contract — `rate_video` / `like_song` wrap all exceptions as typed write errors so the loop never aborts mid-run. - Before a scaled `sync` run (>~50 actions), re-scan with `scan youtube-likes` + `compare-likes`. Region restrictions in particular flip between days — acting on a stale diagnosis can unlike videos that have since become available again (real example: 94 region-blocked ghosts un-blocked themselves overnight in testing). +- Duplicate dedupe (`ytm_dedupe`) is **one `rate_song(INDIFFERENT)` per finding, ytmusic-source + N=2 only**. `rate_song(LIKE)` is non-idempotent (every call appends to LM), so our 0.4 drift sync was the dup producer in the first place. Propagation is minute-scale; the hazard is running `scan ytmusic` → `compare-likes` → `sync` cycle before propagation settles (stale snapshot recreates the finding → over-remove). **Always re-scan ytmusic + compare-likes immediately before a dedupe sync** — a stale finding can also remove the only remaining LM entry if the dup was fixed manually or by propagation since the previous diagnosis. diff --git a/README.md b/README.md index 2aa2f3a..b3411c4 100644 --- a/README.md +++ b/README.md @@ -160,12 +160,13 @@ uv run likesurgeon sync --drift-min-confidence 1.0 # only apply 100%-confidence | `possible_pointer_drift` | YouTube like → unlike | `videos.rate("like")` then `videos.rate("none")` | — | `confidence >= --drift-min-confidence` (default 0.95) | Re-points the YouTube like at the YT Music track's video_id. Like first, then unlike — a partial failure leaves a duplicate like (cleaned up on the next run) instead of losing the original. | | `ytmusic_only` | — | — | — | never | Informational only. Reverse-direction sync (YT Music → YouTube) is out of scope for 0.4. | | `metadata_drift` | — | — | — | never | Informational only. Title/artist drift is signal for the user, not a write target. | -| `duplicate_in_source` | — | — | — | never | Informational. Acts on this finding deferred to 0.5 (dedupe command). | +| `duplicate_in_source` | YT Music unlike (-1 entry) | — | `rate_song("INDIFFERENT")` | ytmusic source, **N=2 only** | Removes one LM entry per finding. **Non-idempotent** (validated for N=2; N≥3 unvalidated → skip). Terminal after attempt — failures are NOT auto-retried within the same Diagnosis (avoids over-removing if the server processed but the client errored). Real failures self-correct via the next `compare-likes`. Propagation is minute-scale — wait a few minutes between a dedupe `sync` and the next `scan ytmusic` / `compare-likes`. | #### State model - Each HTTP call (or skip decision) writes one `SyncAttempt` row with `kind`, `status` (`applied`/`failed`/`skipped`), and a `reason`. The originating `DiagnosisItem.reason` (the diagnosis-time evidence) is **never overwritten** — sync detail lives on `SyncAttempt.reason` instead. -- `DiagnosisItem.status` only flips to `'applied'` when every API call for the action succeeded. For drift that means BOTH halves. Anything else (failure, low-confidence skip) leaves it at `'open'` so the next `sync` re-evaluates it. +- `DiagnosisItem.status` only flips to `'applied'` when every API call for the action succeeded — *except* for `ytm_dedupe`, which is non-idempotent and flips to `'applied'` after any attempt (success OR failure) to prevent auto-retry over-removal. For drift that means BOTH halves. For other failures and low-confidence skips, the status stays at `'open'` so the next `sync` re-evaluates. +- **Re-scan ytmusic + compare-likes immediately before a dedupe sync.** A stale `duplicate_in_source` finding can remove the only remaining LM entry if the dup was already fixed manually or by propagation between diagnose and sync. Eventual consistency also runs the other way: avoid re-scanning for a few minutes *after* a dedupe sync, since a stale snapshot can recreate the same finding and the next sync over-removes. - Re-running `sync` is idempotent: applied items are skipped; failures and previously-skipped findings are re-tried (so lowering `--drift-min-confidence` will pick up borderline drifts on the next run). - **Permanent manual skip.** If you want to tell sync to *never* attempt a particular finding again — e.g. a private/deleted YouTube ghost that `videos.rate` rejects with 403/404 — set its `DiagnosisItem.status` to `'skipped'` directly (currently via SQL on `~/.like-surgeon/like-surgeon.sqlite`). The planner treats `'applied'` and `'skipped'` identically as terminal at item level. - Continue-on-error: a failure (quota exhausted, transport error, revoked token) records the per-item `failed` row and moves on. The run exits non-zero if any action failed. diff --git a/src/likesurgeon/cli.py b/src/likesurgeon/cli.py index 269dc87..b837a07 100644 --- a/src/likesurgeon/cli.py +++ b/src/likesurgeon/cli.py @@ -800,10 +800,9 @@ def sync( if not yes and not typer.confirm("Proceed?", default=False): raise typer.Abort() - # Build the YouTube client only if the plan needs it. ytm-only - # runs (no yt_unlike / yt_relike actions) skip the scope check - # entirely so a user without the YouTube write scope can still - # apply the YT Music half. + # Always construct (cheap); only the write-scope check and write method calls are + # gated on needs_youtube. + # ytm_dedupe is ytmusic-only; not in needs_youtube needs_youtube = any(a.kind in {"yt_unlike", "yt_relike"} for a in actions) from .youtube_client import YouTubeClient diff --git a/src/likesurgeon/models.py b/src/likesurgeon/models.py index 709f655..f538d4a 100644 --- a/src/likesurgeon/models.py +++ b/src/likesurgeon/models.py @@ -162,7 +162,10 @@ class DiagnosisItem(Base): of one source with meaningfully different title or artists. - ``duplicate_in_source`` — the same ``video_id`` appears more than once in a single snapshot (ytmusic accumulates these over time). - Informational; actual dedup ships as a separate command in 0.5. + Actionable via ``ytm_dedupe`` (ytmusic source, N=2 only) as of 0.5. + Non-idempotent — ``status='applied'`` is set after attempt regardless + of outcome (carve-out from the standard 'applied = full success' + invariant; see ``sync.execute()`` docstring). """ __tablename__ = "diagnosis_items" @@ -192,7 +195,7 @@ class SyncAttempt(Base): """One row per ``sync`` API call (or skip decision). ``kind`` is one of ``yt_unlike``, ``ytm_like``, ``yt_relike_like``, - ``yt_relike_unlike`` — the two halves of a drift fix get separate rows + ``yt_relike_unlike``, ``ytm_dedupe`` — the two halves of a drift fix get separate rows so the audit trail stays atomic per HTTP call. ``status`` is one of ``applied``, ``failed``, ``skipped``. ``reason`` carries sync-side detail (error message, threshold note, missing video_id, etc.) — the diff --git a/src/likesurgeon/sync.py b/src/likesurgeon/sync.py index 5e7d6ad..711054c 100644 --- a/src/likesurgeon/sync.py +++ b/src/likesurgeon/sync.py @@ -12,7 +12,9 @@ * ``DiagnosisItem.status`` flips to ``"applied"`` only when every API call for the action succeeded (drift = both halves). Anything else (failure, skip) leaves it as ``"open"`` so the next ``sync`` run - re-evaluates it. + re-evaluates it. Exception: ``ytm_dedupe`` is non-idempotent and + flips to ``"applied"`` after any attempt (success or failure) to + prevent auto-retry over-removal. * Per-action commit cadence: a crash mid-run preserves prior actions' SyncAttempt rows AND any status updates already committed. Drift's two HTTP calls count as one action (one commit). @@ -20,6 +22,7 @@ from __future__ import annotations +import re from dataclasses import dataclass from typing import Literal @@ -35,10 +38,11 @@ ISSUE_YTMUSIC_ONLY, ) from .models import DiagnosisItem, SyncAttempt, Track +from .snapshot import YTMUSIC_LIKED_SONGS from .youtube_client import YouTubeClient, YouTubeWriteError from .ytmusic_client import YTMusicClient, YTMusicWriteError -ActionKind = Literal["yt_unlike", "ytm_like", "yt_relike"] +ActionKind = Literal["yt_unlike", "ytm_like", "yt_relike", "ytm_dedupe"] _TRACK_LOOKUP_BATCH_SIZE = 500 @@ -109,6 +113,40 @@ class ExecResult: skipped: int +@dataclass(frozen=True) +class _DuplicateReason: + count: int + source: str + positions: tuple[int, ...] + + +_DUPLICATE_REASON_RE = re.compile( + r"^appears (?P\d+) times in (?P\w+) snapshot " + r"\(positions: (?P[\d, ]+)\)$" +) + + +def _parse_duplicate_in_source_reason(reason: str) -> _DuplicateReason | None: + """Parse the diagnosis-time reason emitted by ``build_duplicate_in_source_items``. + + Returns ``None`` for any deviation: malformed input, multi-source + contamination, count-vs-positions mismatch (e.g. + ``"appears 2 times ... (positions: 0, 1, 5)"``). Callers MUST treat + ``None`` as "do not auto-act." + """ + m = _DUPLICATE_REASON_RE.fullmatch(reason) + if m is None: + return None + count = int(m.group("count")) + try: + positions = tuple(int(p.strip()) for p in m.group("positions").split(",")) + except ValueError: + return None + if len(positions) != count: + return None + return _DuplicateReason(count=count, source=m.group("source"), positions=positions) + + def plan( items: list[DiagnosisItem], video_ids: dict[int, str], @@ -124,9 +162,12 @@ def plan( for an explicit manual override ("I never want to act on this finding" — e.g. a private/deleted YouTube ghost that ``videos.rate`` can't unlike anyway, so retrying would just noise the audit log forever). - Findings of type ``ytmusic_only``, ``metadata_drift``, or - ``duplicate_in_source`` are also silently dropped (informational, not - actionable in 0.4 — within-source dedup is deferred to 0.5). + Findings of type ``ytmusic_only`` or ``metadata_drift`` are silently + dropped (informational, not actionable in 0.5). + ``duplicate_in_source`` maps to ``ytm_dedupe`` only when the parsed + reason validates as ytmusic-source + N=2 + matching position count. + All other shapes produce a ``SkipRecord`` — we never default to a + destructive YT Music write. """ actions: list[PlannedAction] = [] skips: list[SkipRecord] = [] @@ -205,14 +246,61 @@ def plan( ) ) - elif item.issue_type in { - ISSUE_YTMUSIC_ONLY, - ISSUE_METADATA_DRIFT, - ISSUE_DUPLICATE_IN_SOURCE, - }: + elif item.issue_type in {ISSUE_YTMUSIC_ONLY, ISSUE_METADATA_DRIFT}: # Informational findings — no record, no action. continue + elif item.issue_type == ISSUE_DUPLICATE_IN_SOURCE: + parsed = _parse_duplicate_in_source_reason(item.reason) + if parsed is None: + skips.append( + SkipRecord( + item_id=item.id, + kind="ytm_dedupe", + reason="unparseable duplicate_in_source reason", + ) + ) + continue + if parsed.source != YTMUSIC_LIKED_SONGS: + skips.append( + SkipRecord( + item_id=item.id, + kind="ytm_dedupe", + reason=f"duplicate in {parsed.source} source; not handled in 0.5", + ) + ) + continue + if parsed.count != 2: + skips.append( + SkipRecord( + item_id=item.id, + kind="ytm_dedupe", + reason=( + f"count={parsed.count}; only N=2 is auto-handled in 0.5. " + "File an issue for higher counts." + ), + ) + ) + continue + primary = _video_id_for(video_ids, item.source_track_id) + if not primary: + skips.append( + SkipRecord( + item_id=item.id, + kind="ytm_dedupe", + reason="no video_id available for source track", + ) + ) + continue + actions.append( + PlannedAction( + item_id=item.id, + kind="ytm_dedupe", + primary_video_id=primary, + secondary_video_id=None, + ) + ) + # Unknown future issue types fall through silently (no WARN channel # — sync.plan stays pure). @@ -225,12 +313,15 @@ def _video_id_for(video_ids: dict[int, str], track_id: int | None) -> str | None return video_ids.get(track_id) +_PLAN_ACTION_KINDS = ("yt_unlike", "ytm_like", "yt_relike", "ytm_dedupe") + # Quota cost per action kind (YouTube `videos.rate` = 50 units; ytm is free). # Drift's worst case = 100 (like 50 + unlike 50 if the like succeeds). _QUOTA_COST: dict[ActionKind, int] = { "yt_unlike": 50, "ytm_like": 0, "yt_relike": 100, + "ytm_dedupe": 0, } @@ -246,11 +337,11 @@ def summarize(actions: list[PlannedAction], skips: list[SkipRecord]) -> str: quota = sum(_QUOTA_COST[a.kind] for a in actions) lines = ["Sync plan:"] - for kind in ("yt_unlike", "ytm_like", "yt_relike"): + for kind in _PLAN_ACTION_KINDS: lines.append(f" {kind}: {by_action.get(kind, 0)}") lines.append(f" skipped: {len(skips)}") if by_skip: - for kind in ("yt_unlike", "ytm_like", "yt_relike"): + for kind in _PLAN_ACTION_KINDS: count = by_skip.get(kind, 0) if count: lines.append(f" {kind}: {count}") @@ -276,6 +367,12 @@ def execute( Commit cadence: per-action. A crash mid-run preserves all prior commits — the next ``sync`` re-evaluates anything not at ``status='applied'``. + + Carve-out: ``ytm_dedupe`` flips ``item.status`` to ``"applied"`` + regardless of success. The call is non-idempotent — auto-retrying + a failed unlike risks over-removal because client failure can't + distinguish "server processed, client errored" from "server didn't + process". Genuine failures self-correct via the next compare-likes. """ applied = 0 failed = 0 @@ -303,10 +400,16 @@ def execute( ok = _dispatch(action, ytm=ytm, yt=yt, session=session) if ok: - item.status = "applied" applied += 1 else: failed += 1 + # ytm_dedupe is non-idempotent — terminal-on-attempt regardless of success. + # Client failure doesn't distinguish "server processed, client errored" + # from "server didn't process", so auto-retry would risk over-removal. + # Genuine failures self-correct via the next compare-likes (lingering dup + # → new finding → new attempt). + if ok or action.kind == "ytm_dedupe": + item.status = "applied" session.commit() return ExecResult(applied=applied, failed=failed, skipped=skipped) @@ -323,6 +426,10 @@ def _dispatch( ``True`` iff every call for the action succeeded — i.e. the dispatcher is allowed to flip ``DiagnosisItem.status`` to ``'applied'``. + + Note: for ``ytm_dedupe``, the bool return is used only for + ``ExecResult`` counting. Terminality (``status='applied'`` regardless + of success) is handled by ``execute()``, not here. """ if action.kind == "yt_unlike": return _try_yt_rate( @@ -369,6 +476,14 @@ def _dispatch( rating="none", ) + if action.kind == "ytm_dedupe": + return _try_ytm_unlike( + session, + action.item_id, + ytm=ytm, + video_id=action.primary_video_id, + ) + # Unreachable for the closed ActionKind set, but keeps the function # total for static analyzers. return False @@ -434,3 +549,33 @@ def _try_ytm_like( ) ) return True + + +def _try_ytm_unlike( + session: Session, + item_id: int, + *, + ytm: YTMusicClient, + video_id: str, +) -> bool: + try: + ytm.unlike_song(video_id) + except YTMusicWriteError as exc: + session.add( + SyncAttempt( + diagnosis_item_id=item_id, + kind="ytm_dedupe", + status="failed", + reason=str(exc), + ) + ) + return False + session.add( + SyncAttempt( + diagnosis_item_id=item_id, + kind="ytm_dedupe", + status="applied", + reason="ok", + ) + ) + return True diff --git a/src/likesurgeon/ytmusic_client.py b/src/likesurgeon/ytmusic_client.py index 82cc2eb..0d8b529 100644 --- a/src/likesurgeon/ytmusic_client.py +++ b/src/likesurgeon/ytmusic_client.py @@ -219,6 +219,20 @@ def like_song(self, video_id: str) -> None: except Exception as exc: # noqa: BLE001 — system-boundary catch raise YTMusicWriteError(video_id, str(exc)) from exc + def unlike_song(self, video_id: str) -> None: + """Remove ONE LM-playlist entry for ``video_id`` via ``rate_song(..., "INDIFFERENT")``. + + This is the only viable dedupe path: ``LIKE`` is non-idempotent (every call + appends to LM), and ``setVideoId`` isn't returned by ``get_liked_songs`` so + ``remove_playlist_items`` can't target a specific occurrence. Propagation is + eventually consistent on the order of minutes — see callers' cooldown notes. + """ + client = self._build() + try: + client.rate_song(video_id, "INDIFFERENT") + except Exception as exc: # noqa: BLE001 — system-boundary catch + raise YTMusicWriteError(video_id, str(exc)) from exc + def fetch_liked_songs(self, limit: int = 5000) -> list[dict[str, Any]]: """Fetch up to ``limit`` liked songs. Returns the raw track dicts. diff --git a/tests/test_cli.py b/tests/test_cli.py index f599368..38423d1 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -328,13 +328,15 @@ def rate_video(self, video_id: str, rating: str) -> None: class _FakeYTMusicWrite: - """Stub for the YT Music half of `sync`. Records like_song calls.""" + """Stub for the YT Music half of `sync`. Records like_song and unlike_song calls.""" instances: list[_FakeYTMusicWrite] = [] raise_on: set[str] = set() + raise_on_unlike: set[str] = set() def __init__(self, **kwargs: Any) -> None: self.calls: list[str] = [] + self.unlike_calls: list[str] = [] type(self).instances.append(self) def like_song(self, video_id: str) -> None: @@ -344,6 +346,13 @@ def like_song(self, video_id: str) -> None: if video_id in type(self).raise_on: raise YTMusicWriteError(video_id, "boom") + def unlike_song(self, video_id: str) -> None: + from likesurgeon.ytmusic_client import YTMusicWriteError + + self.unlike_calls.append(video_id) + if video_id in type(self).raise_on_unlike: + raise YTMusicWriteError(video_id, "boom") + @pytest.fixture(autouse=True) def _reset_sync_fakes() -> Iterable[None]: @@ -353,6 +362,7 @@ def _reset_sync_fakes() -> Iterable[None]: _FakeYouTubeWrite.rate_raise_on = set() _FakeYTMusicWrite.instances = [] _FakeYTMusicWrite.raise_on = set() + _FakeYTMusicWrite.raise_on_unlike = set() yield @@ -777,6 +787,226 @@ def test_sync_limit_above_action_count_is_noop( assert "applied=2" in result.output +def _seed_dedupe_item(home: Path, *, video_id: str, source: str = "ytmusic_liked_songs") -> int: + """Seed a Diagnosis with one ISSUE_DUPLICATE_IN_SOURCE item. Returns item id.""" + from likesurgeon.config import DEFAULT_DB_FILENAME + from likesurgeon.db import init_db, make_engine, make_session_factory + from likesurgeon.diagnosis import ISSUE_DUPLICATE_IN_SOURCE + from likesurgeon.models import Diagnosis, DiagnosisItem, Track + + home.mkdir(parents=True, exist_ok=True) + db_path = home / DEFAULT_DB_FILENAME + engine = make_engine(db_path) + init_db(engine) + factory = make_session_factory(engine) + + s = factory() + try: + diag = Diagnosis(ytmusic_snapshot_id=None, youtube_snapshot_id=None) + s.add(diag) + s.flush() + + track = Track( + source=source, + video_id=video_id, + title="dup title", + artists="[]", + canonical_key="ck-dup", + dedupe_key="dk-dup", + ) + s.add(track) + s.flush() + + item = DiagnosisItem( + diagnosis_id=diag.id, + issue_type=ISSUE_DUPLICATE_IN_SOURCE, + confidence=1.0, + reason=f"appears 2 times in {source} snapshot (positions: 0, 1)", + source_track_id=track.id, + related_track_id=None, + status="open", + ) + s.add(item) + s.flush() + item_id = item.id + s.commit() + finally: + s.close() + return item_id + + +def test_sync_ytm_only_dedupe_success_no_youtube_method_calls( + fake_home: Path, + patch_sync_clients: None, +) -> None: + """ytm_dedupe-only plan: unlike_song is called, has_write_scope and rate_video are NOT.""" + from sqlalchemy import select + + from likesurgeon.cli import app + from likesurgeon.models import SyncAttempt + + _seed_dedupe_item(fake_home, video_id="dup_vid") + + runner = CliRunner() + result = runner.invoke(app, ["sync", "--yes"]) + + assert result.exit_code == 0, result.output + + yt = _FakeYouTubeWrite.instances[0] + assert yt.scope_calls == 0, "has_write_scope must not be called for ytm_dedupe-only runs" + assert yt.rate_calls == [], "rate_video must not be called for ytm_dedupe-only runs" + + ytm = _FakeYTMusicWrite.instances[0] + assert ytm.unlike_calls == ["dup_vid"] + + assert "ytm_dedupe: 1" in result.output + + s = _open_db(fake_home) + try: + attempts = list(s.scalars(select(SyncAttempt)).all()) + assert len(attempts) == 1 + assert attempts[0].kind == "ytm_dedupe" + assert attempts[0].status == "applied" + finally: + s.close() + + +def test_sync_ytm_only_dedupe_failure_exits_nonzero_but_marks_applied( + fake_home: Path, + patch_sync_clients: None, +) -> None: + """ytm_dedupe that raises: exit non-zero, no YouTube calls, item.status='applied' (terminal-on-attempt).""" + from sqlalchemy import select + + from likesurgeon.cli import app + from likesurgeon.models import DiagnosisItem, SyncAttempt + + item_id = _seed_dedupe_item(fake_home, video_id="dup_vid") + _FakeYTMusicWrite.raise_on_unlike = {"dup_vid"} + + runner = CliRunner() + result = runner.invoke(app, ["sync", "--yes"]) + + assert result.exit_code != 0 + + yt = _FakeYouTubeWrite.instances[0] + assert yt.scope_calls == 0 + assert yt.rate_calls == [] + + s = _open_db(fake_home) + try: + item = s.get(DiagnosisItem, item_id) + assert item.status == "applied", "ytm_dedupe is terminal-on-attempt even on failure" + attempts = list(s.scalars(select(SyncAttempt)).all()) + assert len(attempts) == 1 + assert attempts[0].kind == "ytm_dedupe" + assert attempts[0].status == "failed" + finally: + s.close() + + +def test_sync_limit_with_mixed_dedupe_and_unlike( + fake_home: Path, + patch_sync_clients: None, +) -> None: + """--limit 1 with one ytm_dedupe + one yt_unlike: exactly one action runs, other stays open.""" + from sqlalchemy import select + + from likesurgeon.cli import app + from likesurgeon.config import DEFAULT_DB_FILENAME + from likesurgeon.db import init_db, make_engine, make_session_factory + from likesurgeon.diagnosis import ISSUE_DUPLICATE_IN_SOURCE, ISSUE_UNAVAILABLE_VIDEO + from likesurgeon.models import Diagnosis, DiagnosisItem, SyncAttempt, Track + + # Seed a diagnosis with two items in one transaction so they share one Diagnosis row. + fake_home.mkdir(parents=True, exist_ok=True) + db_path = fake_home / DEFAULT_DB_FILENAME + engine = make_engine(db_path) + init_db(engine) + factory = make_session_factory(engine) + + s = factory() + try: + diag = Diagnosis(ytmusic_snapshot_id=None, youtube_snapshot_id=None) + s.add(diag) + s.flush() + + track_dup = Track( + source="ytmusic_liked_songs", + video_id="dup_vid", + title="dup", + artists="[]", + canonical_key="ck-dup", + dedupe_key="dk-dup", + ) + track_ghost = Track( + source="youtube_liked_videos", + video_id="ghost_vid", + title="ghost", + artists="[]", + canonical_key="ck-ghost", + dedupe_key="dk-ghost", + ) + s.add_all([track_dup, track_ghost]) + s.flush() + + item_dup = DiagnosisItem( + diagnosis_id=diag.id, + issue_type=ISSUE_DUPLICATE_IN_SOURCE, + confidence=1.0, + reason="appears 2 times in ytmusic_liked_songs snapshot (positions: 0, 1)", + source_track_id=track_dup.id, + related_track_id=None, + status="open", + ) + item_ghost = DiagnosisItem( + diagnosis_id=diag.id, + issue_type=ISSUE_UNAVAILABLE_VIDEO, + confidence=1.0, + reason="unavailable", + source_track_id=track_ghost.id, + related_track_id=None, + status="open", + ) + s.add_all([item_dup, item_ghost]) + s.flush() + dup_id = item_dup.id + ghost_id = item_ghost.id + s.commit() + finally: + s.close() + + runner = CliRunner() + result = runner.invoke(app, ["sync", "--yes", "--limit", "1"]) + + assert result.exit_code == 0, result.output + + ytm = _FakeYTMusicWrite.instances[0] + yt = _FakeYouTubeWrite.instances[0] + + dedupe_ran = len(ytm.unlike_calls) == 1 + unlike_ran = len(yt.rate_calls) == 1 + # Exactly one of the two actions ran. + assert dedupe_ran ^ unlike_ran, ( + f"Expected exactly one action; got unlike_calls={ytm.unlike_calls}, rate_calls={yt.rate_calls}" + ) + + s = _open_db(fake_home) + try: + items = {r.id: r for r in s.scalars(select(DiagnosisItem)).all()} + applied_statuses = [v.status for v in items.values() if v.status == "applied"] + open_statuses = [v.status for v in items.values() if v.status == "open"] + assert len(applied_statuses) == 1, "exactly one item should be applied" + assert len(open_statuses) == 1, "exactly one item should remain open" + + attempts = list(s.scalars(select(SyncAttempt)).all()) + assert len(attempts) == 1, "exactly one SyncAttempt row expected" + finally: + s.close() + + _ = dup_id, ghost_id # referenced above via items dict; kept for clarity + + # --------------------------------------------------------------------------- # duplicate_in_source — compare-likes pipeline + canonicalization. # diff --git a/tests/test_sync.py b/tests/test_sync.py index cafacb4..0c704d3 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -20,6 +20,8 @@ ExecResult, PlannedAction, SkipRecord, + _DuplicateReason, + _parse_duplicate_in_source_reason, execute, plan, resolve_video_ids, @@ -48,15 +50,26 @@ def rate_video(self, video_id: str, rating: str) -> None: class FakeYTMusic: - def __init__(self, raise_on: set[str] | None = None) -> None: + def __init__( + self, + raise_on: set[str] | None = None, + raise_on_unlike: set[str] | None = None, + ) -> None: self.calls: list[str] = [] self._raise_on = raise_on or set() + self.unlike_calls: list[str] = [] + self.raise_on_unlike: set[str] = raise_on_unlike or set() def like_song(self, video_id: str) -> None: self.calls.append(video_id) if video_id in self._raise_on: raise YTMusicWriteError(video_id, "boom") + def unlike_song(self, video_id: str) -> None: + self.unlike_calls.append(video_id) + if video_id in self.raise_on_unlike: + raise YTMusicWriteError(video_id, "boom") + # --------------------------------------------------------------------------- # Fixtures @@ -432,10 +445,11 @@ def test_plan_ignores_ytmusic_only_and_metadata_drift(session: Session) -> None: assert skips == [] -def test_plan_ignores_duplicate_in_source_alongside_actionable(session: Session) -> None: - """duplicate_in_source is informational (acted on by a future 0.5 - dedupe command). Planner must emit no action / no skip for it, - while still planning actionable findings in the same diagnosis.""" +def test_plan_duplicate_in_source_ytmusic_n2_emits_ytm_dedupe_alongside_actionable( + session: Session, +) -> None: + """A ytmusic-source N=2 duplicate emits a ytm_dedupe action, while an + actionable finding in the same diagnosis still produces its own action.""" diag = _make_diagnosis(session) dup_t = _make_track(session, "dup", suffix="dup") ghost_t = _make_track(session, "ghost", suffix="g") @@ -444,6 +458,7 @@ def test_plan_ignores_duplicate_in_source_alongside_actionable(session: Session) diag, issue_type=ISSUE_DUPLICATE_IN_SOURCE, source_track=dup_t, + reason="appears 2 times in ytmusic_liked_songs snapshot (positions: 0, 1)", ) ghost = _make_item( session, @@ -459,11 +474,145 @@ def test_plan_ignores_duplicate_in_source_alongside_actionable(session: Session) drift_min_confidence=0.95, ) - # The dup finding produces neither an action nor a skip. - assert all(a.item_id != dup.id for a in actions) - assert all(s.item_id != dup.id for s in skips) - # The ghost finding still becomes a yt_unlike action. - assert [a.item_id for a in actions] == [ghost.id] + assert skips == [] + assert len(actions) == 2 + assert actions[0].item_id == dup.id + assert actions[0].kind == "ytm_dedupe" + assert actions[0].primary_video_id == "dup" + assert actions[0].secondary_video_id is None + assert actions[1].item_id == ghost.id + assert actions[1].kind == "yt_unlike" + + +def test_plan_duplicate_in_source_ytmusic_n3_emits_skip(session: Session) -> None: + """N=3 duplicates are not auto-handled in 0.5 — planner emits a SkipRecord + with a reason containing 'count=3'.""" + diag = _make_diagnosis(session) + dup_t = _make_track(session, "dup3", suffix="dup3") + dup = _make_item( + session, + diag, + issue_type=ISSUE_DUPLICATE_IN_SOURCE, + source_track=dup_t, + reason="appears 3 times in ytmusic_liked_songs snapshot (positions: 0, 1, 5)", + ) + session.commit() + + actions, skips = plan( + [dup], + {dup_t.id: "dup3"}, + drift_min_confidence=0.95, + ) + + assert actions == [] + assert len(skips) == 1 + assert skips[0].item_id == dup.id + assert skips[0].kind == "ytm_dedupe" + assert "count=3" in skips[0].reason + + +def test_plan_duplicate_in_source_youtube_emits_skip(session: Session) -> None: + """Duplicates in the youtube_liked_videos source are not handled — planner + emits a SkipRecord with a reason containing 'youtube_liked_videos source'.""" + diag = _make_diagnosis(session) + dup_t = _make_track(session, "ytdup", suffix="ytdup") + dup = _make_item( + session, + diag, + issue_type=ISSUE_DUPLICATE_IN_SOURCE, + source_track=dup_t, + reason="appears 2 times in youtube_liked_videos snapshot (positions: 0, 5)", + ) + session.commit() + + actions, skips = plan( + [dup], + {dup_t.id: "ytdup"}, + drift_min_confidence=0.95, + ) + + assert actions == [] + assert len(skips) == 1 + assert skips[0].item_id == dup.id + assert skips[0].kind == "ytm_dedupe" + assert "youtube_liked_videos source" in skips[0].reason + + +def test_plan_duplicate_in_source_count_mismatch_emits_skip(session: Session) -> None: + """count=2 but 3 positions — the v3 regex hole. Parser returns None, + planner emits a SkipRecord with reason containing 'unparseable'.""" + diag = _make_diagnosis(session) + dup_t = _make_track(session, "mismatch", suffix="mismatch") + dup = _make_item( + session, + diag, + issue_type=ISSUE_DUPLICATE_IN_SOURCE, + source_track=dup_t, + reason="appears 2 times in ytmusic_liked_songs snapshot (positions: 0, 1, 5)", + ) + session.commit() + + actions, skips = plan( + [dup], + {dup_t.id: "mismatch"}, + drift_min_confidence=0.95, + ) + + assert actions == [] + assert len(skips) == 1 + assert skips[0].kind == "ytm_dedupe" + assert "unparseable" in skips[0].reason + + +def test_plan_duplicate_in_source_missing_video_id_emits_skip(session: Session) -> None: + """Valid ytmusic N=2 reason, but the source track has no video_id in the + lookup map — planner emits a SkipRecord with reason containing 'no video_id'.""" + diag = _make_diagnosis(session) + dup_t = _make_track(session, "dup_novid", suffix="novid") + dup = _make_item( + session, + diag, + issue_type=ISSUE_DUPLICATE_IN_SOURCE, + source_track=dup_t, + reason="appears 2 times in ytmusic_liked_songs snapshot (positions: 0, 1)", + ) + session.commit() + + # dup_t.id intentionally omitted from video_ids + actions, skips = plan( + [dup], + {}, + drift_min_confidence=0.95, + ) + + assert actions == [] + assert len(skips) == 1 + assert skips[0].kind == "ytm_dedupe" + assert "no video_id" in skips[0].reason + + +def test_plan_skips_failed_ytm_dedupe_item_in_same_diagnosis(session: Session) -> None: + """A DiagnosisItem with status='applied' is silently dropped regardless of + issue_type — pins the non-idempotent retry guard for ytm_dedupe.""" + diag = _make_diagnosis(session) + dup_t = _make_track(session, "dup_done", suffix="done") + dup = _make_item( + session, + diag, + issue_type=ISSUE_DUPLICATE_IN_SOURCE, + source_track=dup_t, + status="applied", + reason="appears 2 times in ytmusic_liked_songs snapshot (positions: 0, 1)", + ) + session.commit() + + actions, skips = plan( + [dup], + {dup_t.id: "dup_done"}, + drift_min_confidence=0.95, + ) + + assert actions == [] assert skips == [] @@ -493,6 +642,31 @@ def test_summarize_counts_and_quota() -> None: assert "200" in s +def test_summarize_includes_ytm_dedupe() -> None: + """ytm_dedupe action and skip both appear in summarize output. + ytm_dedupe has quota cost 0, so YouTube quota is unchanged.""" + actions = [ + PlannedAction(item_id=1, kind="ytm_dedupe", primary_video_id="d", secondary_video_id=None), + ] + skips = [ + SkipRecord(item_id=2, kind="ytm_dedupe", reason="count=3; only N=2 is auto-handled"), + ] + + s = summarize(actions, skips) + + # Action line for ytm_dedupe + assert "ytm_dedupe: 1" in s + # Skip total and per-kind breakdown + assert "skipped: 1" in s + assert "ytm_dedupe: 1" in s + # Quota: ytm_dedupe cost=0, so 0 units + assert "0" in s + # The existing non-ytm_dedupe action kinds show 0 + assert "yt_unlike: 0" in s + assert "ytm_like: 0" in s + assert "yt_relike: 0" in s + + # --------------------------------------------------------------------------- # execute — happy paths # --------------------------------------------------------------------------- @@ -860,3 +1034,187 @@ def rate_video(self, video_id: str, rating: str) -> None: assert attempts2 == [] finally: fresh.close() + + +# --------------------------------------------------------------------------- +# _parse_duplicate_in_source_reason +# --------------------------------------------------------------------------- + + +def test_parse_dup_reason_ytmusic_n2() -> None: + result = _parse_duplicate_in_source_reason( + "appears 2 times in ytmusic_liked_songs snapshot (positions: 0, 1)" + ) + assert result == _DuplicateReason(count=2, source="ytmusic_liked_songs", positions=(0, 1)) + + +def test_parse_dup_reason_youtube_n2() -> None: + result = _parse_duplicate_in_source_reason( + "appears 2 times in youtube_liked_videos snapshot (positions: 3, 7)" + ) + assert result == _DuplicateReason(count=2, source="youtube_liked_videos", positions=(3, 7)) + + +def test_parse_dup_reason_n3() -> None: + result = _parse_duplicate_in_source_reason( + "appears 3 times in ytmusic_liked_songs snapshot (positions: 0, 1, 5)" + ) + assert result == _DuplicateReason(count=3, source="ytmusic_liked_songs", positions=(0, 1, 5)) + + +def test_parse_dup_reason_count_mismatch_returns_none() -> None: + # count=2 but 3 positions — mismatch must return None + result = _parse_duplicate_in_source_reason( + "appears 2 times in ytmusic_liked_songs snapshot (positions: 0, 1, 5)" + ) + assert result is None + + +def test_parse_dup_reason_unknown_source_text_returns_none() -> None: + # surrounding text makes fullmatch fail + result = _parse_duplicate_in_source_reason("manually edited: appears 2 times in elsewhere") + assert result is None + + +def test_parse_dup_reason_random_text_returns_none() -> None: + result = _parse_duplicate_in_source_reason("random text") + assert result is None + + +# --------------------------------------------------------------------------- +# execute — ytm_dedupe terminal-on-attempt semantics +# --------------------------------------------------------------------------- + + +def test_execute_ytm_dedupe_success_marks_applied(session: Session) -> None: + diag = _make_diagnosis(session) + t = _make_track(session, "vid", suffix="dup") + item = _make_item( + session, + diag, + issue_type=ISSUE_DUPLICATE_IN_SOURCE, + source_track=t, + reason="appears 2 times in ytmusic_liked_songs snapshot (positions: 0, 1)", + ) + session.commit() + original_reason = item.reason + + ytm = FakeYTMusic() + yt = FakeYouTube() + actions = [ + PlannedAction( + item_id=item.id, kind="ytm_dedupe", primary_video_id="vid", secondary_video_id=None + ) + ] + res = execute(session, actions, [], ytm=ytm, yt=yt) + + assert res == ExecResult(applied=1, failed=0, skipped=0) + assert ytm.unlike_calls == ["vid"] + session.refresh(item) + assert item.status == "applied" + assert item.reason == original_reason + rows = _attempts_for(session, item.id) + assert len(rows) == 1 + assert rows[0].kind == "ytm_dedupe" + assert rows[0].status == "applied" + + +def test_execute_ytm_dedupe_failure_still_marks_applied(session: Session) -> None: + """ytm_dedupe is non-idempotent; execute() flips status='applied' regardless to prevent auto-retry within same Diagnosis.""" + diag = _make_diagnosis(session) + t = _make_track(session, "vid", suffix="dup") + item = _make_item( + session, + diag, + issue_type=ISSUE_DUPLICATE_IN_SOURCE, + source_track=t, + reason="appears 2 times in ytmusic_liked_songs snapshot (positions: 0, 1)", + ) + session.commit() + original_reason = item.reason + + ytm = FakeYTMusic(raise_on_unlike={"vid"}) + yt = FakeYouTube() + actions = [ + PlannedAction( + item_id=item.id, kind="ytm_dedupe", primary_video_id="vid", secondary_video_id=None + ) + ] + res = execute(session, actions, [], ytm=ytm, yt=yt) + + assert res == ExecResult(applied=0, failed=1, skipped=0) + session.refresh(item) + assert item.status == "applied" + assert item.reason == original_reason + rows = _attempts_for(session, item.id) + assert len(rows) == 1 + assert rows[0].kind == "ytm_dedupe" + assert rows[0].status == "failed" + assert "boom" in rows[0].reason + + +def test_execute_ytm_dedupe_skip_emits_attempt(session: Session) -> None: + diag = _make_diagnosis(session) + t = _make_track(session, "vid", suffix="dup") + item = _make_item( + session, + diag, + issue_type=ISSUE_DUPLICATE_IN_SOURCE, + source_track=t, + reason="appears 3 times in ytmusic_liked_songs snapshot (positions: 0, 1, 5)", + ) + session.commit() + + ytm = FakeYTMusic() + yt = FakeYouTube() + skips = [ + SkipRecord( + item_id=item.id, + kind="ytm_dedupe", + reason="count=3; only N=2 is auto-handled in 0.5", + ) + ] + res = execute(session, [], skips, ytm=ytm, yt=yt) + + assert res == ExecResult(applied=0, failed=0, skipped=1) + session.refresh(item) + assert item.status == "open" + rows = _attempts_for(session, item.id) + assert len(rows) == 1 + assert rows[0].kind == "ytm_dedupe" + assert rows[0].status == "skipped" + assert rows[0].reason == "count=3; only N=2 is auto-handled in 0.5" + + +def test_parse_dup_reason_roundtrip_with_builder() -> None: + from likesurgeon.diagnosis import build_duplicate_in_source_items + from likesurgeon.models import SnapshotItem + from likesurgeon.snapshot import YTMUSIC_LIKED_SONGS + + items = [ + SnapshotItem( + snapshot_id=1, + track_id=10, + position=0, + video_id="vid_abc", + title="Song", + artists="[]", + canonical_key="song", + ), + SnapshotItem( + snapshot_id=1, + track_id=11, + position=1, + video_id="vid_abc", + title="Song", + artists="[]", + canonical_key="song", + ), + ] + diagnosis_items = build_duplicate_in_source_items( + diagnosis_id=1, snapshot_items=items, source=YTMUSIC_LIKED_SONGS + ) + assert len(diagnosis_items) == 1 + reason = diagnosis_items[0].reason + result = _parse_duplicate_in_source_reason(reason) + assert result == _DuplicateReason(count=2, source="ytmusic_liked_songs", positions=(0, 1)) diff --git a/tests/test_ytmusic_client.py b/tests/test_ytmusic_client.py index fd1070f..db73fce 100644 --- a/tests/test_ytmusic_client.py +++ b/tests/test_ytmusic_client.py @@ -118,6 +118,34 @@ def test_like_song_wraps_arbitrary_failure_as_ytmusic_write_error() -> None: assert isinstance(exc_info.value.__cause__, RuntimeError) +def test_unlike_song_calls_rate_song_with_INDIFFERENT() -> None: + """``unlike_song`` always passes ``"INDIFFERENT"`` — no other rating flows through.""" + fake = _RatingFakeYTMusic() + client = _RatingClient(fake) + client.unlike_song("vid42") + assert fake.calls == [("vid42", "INDIFFERENT")] + + +def test_unlike_song_wraps_arbitrary_failure_as_ytmusic_write_error() -> None: + """ytmusicapi can raise a wide range of exception types from rate_song. + Surface them all as ``YTMusicWriteError`` so the dispatcher's failure + path doesn't have to fingerprint each one.""" + fake = _RatingFakeYTMusic(raise_with=RuntimeError("boom")) + client = _RatingClient(fake) + with pytest.raises(YTMusicWriteError) as exc_info: + client.unlike_song("vid99") + assert exc_info.value.video_id == "vid99" + assert isinstance(exc_info.value.__cause__, RuntimeError) + + +def test_unlike_song_does_not_wrap_build_failure() -> None: + """``_build()`` is called outside the try block, so auth failures propagate + as ``AuthFileMissingError`` rather than being wrapped as ``YTMusicWriteError``.""" + client = YTMusicClient(browser_path=Path("/nonexistent")) + with pytest.raises(AuthFileMissingError): + client.unlike_song("vid") + + def test_missing_auth_raises(): client = YTMusicClient(browser_path=Path("/nope/browser.json")) with pytest.raises(AuthFileMissingError):