Add test suite (T1)

First automated coverage. 38 tests, run against the built image (which has the
app deps); tests/ and requirements-dev.txt are not baked into the runtime image.

- tests/test_dedup_gating.py: the safety-critical one. Drives dedup-library.sh's
  process_group with --apply + --only-paths and asserts ONLY the confirmed path
  is deleted and the higher-ranked FLAC is kept.
- tests/test_pipeline_runner.py: lock skip, use_lock=False bypass, lock release,
  success/failure/timeout recording.
- tests/test_db.py: schema version + idempotent migrations, FK-safe job-run
  pruning, stale-run reconciliation.
- tests/test_credential_service.py: conf-field substitution incl. newline-
  injection safety, secret/non-secret classification, cookie-expiry parsing.
- tests/test_playlist_service.py: cron round-trips, name validation, render.
- tests/test_diagnostics.py: /health + /setup checks.
- conftest.py points every path setting at throwaway temp dirs and a temp DB, so
  tests never touch a real library, /config, or the network. See tests/README.md.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

.gitignore

@@ -1,5 +1,6 @@
 __pycache__/
 *.pyc
+.pytest_cache/
 *.db
 *.db-journal
 *.db-wal

requirements-dev.txt

@@ -0,0 +1,4 @@
+# Test-only dependencies. Not installed in the runtime image; used when running
+# the test suite. See tests/README.md.
+-r requirements.txt
+pytest==8.4.2

tests/README.md

@@ -0,0 +1,44 @@
+# Tests
+
+Unit and safety tests for the control-plane app and the dedup file-deleter.
+They run against throwaway temp dirs and a temp SQLite database (set up in
+`conftest.py`); they never touch a real library, your `/config`, or the network.
+
+## Running them
+
+The runtime image doesn't ship `pytest`, so install it into a throwaway run of
+the built image (which already has the app's own dependencies). Run as root so
+pip can write into the image's venv, and keep pytest's cache off so it doesn't
+create files in your working tree:
+
+```bash
+docker run --rm --user root -v "$PWD":/src -w /src -e PYTHONPATH=/src \
+  --entrypoint bash alembic:latest \
+  -c "pip install -q pytest && pytest -q -p no:cacheprovider"
+```
+
+Tests set their own temp `ALEMBIC_CONFIG_DIR`, `MUSIC_DATA_DIR`, and encryption
+key (see `conftest.py`), so nothing here touches a real library or config.
+
+Or, if you have a matching Python environment locally:
+
+```bash
+pip install -r requirements-dev.txt
+PYTHONPATH=. pytest -q
+```
+
+## What's covered
+
+- **Dedup safety** (`test_dedup_gating.py`): `dedup-library.sh` deletes only the
+  paths confirmed via `--only-paths`, and keeps the higher-ranked (FLAC) copy.
+  This is the most important test: the script deletes files.
+- **Runner** (`test_pipeline_runner.py`): lock skip, `use_lock=False` bypass,
+  success/failure/timeout recording, lock release.
+- **Migrations** (`test_db.py`): schema version, idempotent re-run, FK-safe
+  job-run pruning, stale-run reconciliation.
+- **Credential rendering** (`test_credential_service.py`): config field
+  substitution (including newline-injection safety), secret vs non-secret
+  classification, cookie-expiry parsing.
+- **Playlists** (`test_playlist_service.py`): cron round-trips, name validation,
+  config render/delete.
+- **Diagnostics** (`test_diagnostics.py`): the `/health` and `/setup` checks.

tests/conftest.py

@@ -0,0 +1,67 @@
+"""Test setup.
+
+Every path-derived setting must point at throwaway dirs BEFORE any app module
+imports, because app.db builds its SQLAlchemy engine from settings at import
+time. So this runs at module load (conftest is imported before test modules).
+"""
+import os
+import tempfile
+from pathlib import Path
+
+from cryptography.fernet import Fernet
+
+_REPO = Path(__file__).resolve().parent.parent
+_TMP = Path(tempfile.mkdtemp(prefix="alembic-tests-"))
+(_TMP / "config" / "pipeline").mkdir(parents=True, exist_ok=True)
+(_TMP / "music").mkdir(parents=True, exist_ok=True)
+
+_KEY = _TMP / "master.key"
+_KEY.write_bytes(Fernet.generate_key())
+
+os.environ["ALEMBIC_CONFIG_DIR"] = str(_TMP / "config")
+os.environ["MUSIC_DATA_DIR"] = str(_TMP / "music")
+os.environ["PIPELINE_DIR"] = str(_REPO / "pipeline")
+os.environ["ENCRYPTION_KEY_FILE"] = str(_KEY)
+os.environ["SESSION_SECRET"] = "test-only-secret"
+# Make sure no stray OIDC env leaks in from the host shell.
+for _v in ("POCKETID_ISSUER", "POCKETID_CLIENT_ID", "POCKETID_CLIENT_SECRET", "ALLOWED_EMAIL"):
+    os.environ.pop(_v, None)
+
+import pytest  # noqa: E402
+
+_TABLES = [
+    "job_runs", "dedup_candidates", "dedup_runs", "genre_candidates", "genre_runs",
+    "manual_imports", "manual_fix_audit", "playlists", "secrets", "scheduled_jobs",
+    "users", "app_settings",
+]
+
+
+@pytest.fixture(autouse=True)
+def fresh_db():
+    """Each test gets an initialized, empty database (the engine is bound to the
+    one temp DB path, so clear rows between tests rather than rebinding)."""
+    from sqlalchemy import text
+
+    from app.db import engine, init_db
+
+    init_db()
+    with engine.begin() as conn:
+        for table in _TABLES:
+            conn.execute(text(f"DELETE FROM {table}"))
+    yield
+
+
+@pytest.fixture()
+def db():
+    from app.db import SessionLocal
+
+    session = SessionLocal()
+    try:
+        yield session
+    finally:
+        session.close()
+
+
+@pytest.fixture()
+def config_dir():
+    return _TMP / "config"

tests/test_credential_service.py

@@ -0,0 +1,59 @@
+import os
+import stat
+
+from app.services import credential_service as cs
+from app.services import playlist_service as pl
+
+
+def test_set_conf_field_replaces_existing():
+    text = "user = OLD\npass = OLD\n"
+    out = cs._set_conf_field(text, "user", "newuser")
+    assert "user = newuser" in out
+    assert "pass = OLD" in out  # other lines untouched
+
+
+def test_set_conf_field_appends_when_absent():
+    out = cs._set_conf_field("input = x\n", "user", "bob")
+    assert out.rstrip().endswith("user = bob")
+
+
+def test_set_conf_field_strips_newlines():
+    # a newline in a value must not inject a second config line
+    out = cs._set_conf_field("user = x\n", "user", "bad\nname = injected")
+    assert "\nname = injected" not in out
+    assert "user = badname = injected" in out
+
+
+def test_secret_keys_classification():
+    assert "password" in cs.SECRET_KEYS
+    assert "client_secret" in cs.SECRET_KEYS
+    # identifiers/urls/prefs are NOT secret (shown as text in the UI)
+    assert "username" not in cs.SECRET_KEYS
+    assert "base_url" not in cs.SECRET_KEYS
+    assert "region" not in cs.SECRET_KEYS
+
+
+def test_render_playlist_confs_injects_creds_safely(db, config_dir):
+    cs.set_credentials(db, "spotify", {"client_id": "CID", "client_secret": "CSECRET"})
+    # a password containing shell metacharacters must land literally
+    cs.set_credentials(db, "soulseek", {"username": "seek", "password": "p'a$(id)ss"})
+    pl.create(db, name="rtest", spotify_url="https://open.spotify.com/playlist/Z")
+
+    conf = config_dir / "pipeline" / "rtest.conf"
+    text = conf.read_text()
+    assert "user = seek" in text
+    assert "pass = p'a$(id)ss" in text
+    assert "spotify-id = CID" in text
+    assert "spotify-secret = CSECRET" in text
+    # rendered config must be owner-only (holds the Soulseek password)
+    assert stat.S_IMODE(os.stat(conf).st_mode) == 0o600
+
+
+def test_bandcamp_cookie_expiry_parsing():
+    jar = "\n".join([
+        "# Netscape HTTP Cookie File",
+        "\t".join([".bandcamp.com", "TRUE", "/", "TRUE", "1893456000", "identity", "abc"]),
+    ])
+    exp = cs._bandcamp_cookie_expiry(jar)
+    assert exp == 1893456000.0
+    assert cs._bandcamp_cookie_expiry("no identity cookie here") is None

tests/test_db.py

@@ -0,0 +1,61 @@
+import time
+
+from sqlalchemy import text
+
+from app import db as appdb
+
+
+def test_init_db_sets_version_and_is_idempotent():
+    with appdb.engine.begin() as conn:
+        version = conn.execute(text("SELECT version FROM schema_version")).fetchone()[0]
+    assert version >= 3  # all shipped migrations applied
+    # Re-running must not raise (e.g. duplicate-column on a re-applied ALTER).
+    appdb.init_db()
+    appdb.init_db()
+
+
+def test_prune_old_job_runs_fk_safe():
+    now = time.time()
+    old = now - 100 * 86400
+    with appdb.engine.begin() as conn:
+        conn.execute(
+            text(
+                "INSERT INTO job_runs (id, job_key, started_at, status, triggered_by) "
+                "VALUES (1,'old',:o,'success','schedule'),(2,'new',:n,'success','schedule')"
+            ),
+            {"o": old, "n": now},
+        )
+        conn.execute(
+            text(
+                "INSERT INTO manual_imports (id, source_path, imported_by, imported_at, status, job_run_id) "
+                "VALUES (1,'x','me',:o,'done',1)"
+            ),
+            {"o": old},
+        )
+
+    deleted = appdb.prune_old_job_runs(max_age_days=90)
+    assert deleted == 1
+    with appdb.engine.begin() as conn:
+        remaining = [r[0] for r in conn.execute(text("SELECT job_key FROM job_runs"))]
+        mi_ref = conn.execute(text("SELECT job_run_id FROM manual_imports WHERE id=1")).fetchone()[0]
+        mi_count = conn.execute(text("SELECT COUNT(*) FROM manual_imports")).fetchone()[0]
+    assert remaining == ["new"]
+    assert mi_ref is None  # reference nulled, not cascade-deleted
+    assert mi_count == 1  # the import record itself is kept
+
+
+def test_mark_interrupted_runs():
+    now = time.time()
+    with appdb.engine.begin() as conn:
+        conn.execute(
+            text(
+                "INSERT INTO job_runs (id, job_key, started_at, status, triggered_by) "
+                "VALUES (1,'stuck',:n,'running','schedule')"
+            ),
+            {"n": now},
+        )
+    appdb.mark_interrupted_runs()
+    with appdb.engine.begin() as conn:
+        row = conn.execute(text("SELECT status, finished_at FROM job_runs WHERE id=1")).fetchone()
+    assert row[0] == "interrupted"
+    assert row[1] is not None

tests/test_dedup_gating.py

@@ -0,0 +1,53 @@
+"""The safety-critical test: dedup-library.sh must delete ONLY the files a
+human confirmed via --only-paths, and must keep the higher-ranked copy.
+
+Rather than stand up a full beets fixture, this sources the script's setup +
+function definitions (everything before the Pass 1 top-level code) and drives
+process_group directly with "ghost" (bare-path) entries, which skip the beets
+path translation and are deleted with plain rm -- exactly the code path that
+enforces --only-paths gating and FLAC-over-MP3 ranking."""
+import subprocess
+from pathlib import Path
+
+REPO = Path(__file__).resolve().parent.parent
+SCRIPT = REPO / "pipeline" / "lib" / "dedup-library.sh"
+
+
+def _drive_process_group(work: Path) -> subprocess.CompletedProcess:
+    keep = work / "keep.flac"
+    dupe = work / "dupe.mp3"
+    other = work / "other.mp3"
+    for f, size in ((keep, 5000), (dupe, 1000), (other, 1000)):
+        f.write_bytes(b"\0" * size)
+    confirm = work / "confirm.txt"
+    confirm.write_text(str(dupe) + "\n")  # only dupe.mp3 is confirmed for deletion
+
+    script = r"""
+set -euo pipefail
+export ALEMBIC_CONFIG_DIR="{work}" MUSIC_DATA_DIR="{work}"
+mkdir -p "{work}/logs"
+# Source only setup + functions (stop before the Pass 1 top-level code).
+end=$(grep -n '^# Pass 1: numbered siblings' "{script}" | head -1 | cut -d: -f1)
+source <(head -n $((end - 2)) "{script}")
+APPLY=1
+ONLY_PATHS_FILE="{confirm}"
+declare -A ONLY_PATHS=(["{dupe}"]=1)
+# Group 1: keep.flac vs dupe.mp3 (dupe confirmed -> deleted)
+process_group "P2 case-insensitive: g1" "{keep}" "{dupe}"
+# Group 2: keep.flac vs other.mp3 (other NOT confirmed -> must survive)
+process_group "P2 case-insensitive: g2" "{keep}" "{other}"
+""".format(work=work, script=SCRIPT, confirm=confirm, dupe=dupe, keep=keep, other=other)
+    return subprocess.run(["bash", "-c", script], capture_output=True, text=True)
+
+
+def test_only_confirmed_paths_are_deleted(tmp_path):
+    proc = _drive_process_group(tmp_path)
+    assert proc.returncode == 0, proc.stderr
+
+    keep = tmp_path / "keep.flac"
+    dupe = tmp_path / "dupe.mp3"
+    other = tmp_path / "other.mp3"
+
+    assert keep.exists(), "FLAC (higher rank) must be kept"
+    assert not dupe.exists(), "confirmed duplicate must be deleted"
+    assert other.exists(), "an unconfirmed delete-candidate must NOT be deleted"

tests/test_diagnostics.py

@@ -0,0 +1,25 @@
+from app.services import diagnostics
+
+
+def test_master_key_present():
+    # conftest wrote a real key at ENCRYPTION_KEY_FILE
+    assert diagnostics.master_key_present() is True
+
+
+def test_oidc_not_configured_by_default():
+    assert diagnostics.oidc_configured() is False
+
+
+def test_summary_degraded_without_oidc():
+    result = diagnostics.summary()
+    assert result["status"] == "degraded"  # OIDC missing is critical
+    keys = {c["key"] for c in result["checks"]}
+    assert {"oidc", "master_key", "config_writable"} <= keys
+
+
+def test_config_writable_true():
+    checks = {c["key"]: c for c in diagnostics.checks()}
+    assert checks["config_writable"]["ok"] is True
+    # critical checks are flagged as such
+    assert checks["oidc"]["critical"] is True
+    assert checks["beets_library"]["critical"] is False

tests/test_pipeline_runner.py

@@ -0,0 +1,60 @@
+import asyncio
+
+from app.services import pipeline_runner as pr
+
+# One loop for the whole module: pipeline_runner._lock is a module-level
+# asyncio.Lock, and reusing it across different loops (as asyncio.run would
+# create) raises "bound to a different event loop".
+_loop = asyncio.new_event_loop()
+
+
+def _run(coro):
+    return _loop.run_until_complete(coro)
+
+
+def test_run_job_success_records_row_and_log():
+    run = _run(pr.run_job("test:ok", ["true"]))
+    assert run.status == "success"
+    assert run.exit_code == 0
+    assert run.log_path  # a log file path was recorded
+
+
+def test_run_job_failure():
+    run = _run(pr.run_job("test:fail", ["false"]))
+    assert run.status == "failed"
+    assert run.exit_code == 1
+
+
+def test_lock_skip_when_held():
+    async def scenario():
+        await pr._lock.acquire()
+        try:
+            return await pr.run_job("test:locked", ["true"], use_lock=True)
+        finally:
+            pr._lock.release()
+
+    run = _run(scenario())
+    assert run.status == "skipped_lock"
+
+
+def test_use_lock_false_bypasses_held_lock():
+    async def scenario():
+        await pr._lock.acquire()
+        try:
+            return await pr.run_job("test:bypass", ["true"], use_lock=False)
+        finally:
+            pr._lock.release()
+
+    run = _run(scenario())
+    assert run.status == "success"
+
+
+def test_lock_released_after_run():
+    _run(pr.run_job("test:release", ["true"]))
+    assert not pr._lock.locked()
+
+
+def test_timeout_kills_and_marks_failed():
+    run = _run(pr.run_job("test:timeout", ["sleep", "5"], timeout=0.3))
+    assert run.status == "failed"
+    assert run.exit_code == -1

tests/test_playlist_service.py

@@ -0,0 +1,51 @@
+import pytest
+
+from app.services import playlist_service as pl
+
+
+@pytest.mark.parametrize(
+    "time_str,cron",
+    [("01:30", "30 1 * * *"), ("00:00", "0 0 * * *"), ("23:59", "59 23 * * *")],
+)
+def test_cron_roundtrip(time_str, cron):
+    assert pl.time_to_cron(time_str) == cron
+    assert pl.cron_to_time(cron) == time_str
+
+
+def test_time_to_cron_invalid():
+    assert pl.time_to_cron("") is None
+    assert pl.time_to_cron("25:00") is None
+    assert pl.time_to_cron("nonsense") is None
+
+
+def test_cron_to_time_non_daily_is_blank():
+    # weekly/day-of-month crons aren't representable as a plain HH:MM
+    assert pl.cron_to_time("30 1 * * sun") == ""
+    assert pl.cron_to_time(None) == ""
+
+
+@pytest.mark.parametrize("name", ["techno", "my chill mix", "goldenera", "a-b_c 1"])
+def test_validate_name_accepts(name):
+    assert pl.validate_name(name) == name.strip()
+
+
+@pytest.mark.parametrize("name", ["", "../escape", "a/b", "x;rm -rf", "x|e", "ü", "x" * 65])
+def test_validate_name_rejects(name):
+    with pytest.raises(ValueError):
+        pl.validate_name(name)
+
+
+def test_create_renders_conf_and_delete_removes_it(db, config_dir):
+    p = pl.create(db, name="unittest", spotify_url="https://open.spotify.com/playlist/ABC")
+    conf = config_dir / "pipeline" / "unittest.conf"
+    assert conf.exists()
+    text = conf.read_text()
+    assert "input = https://open.spotify.com/playlist/ABC" in text
+    assert "path = " in text and "/unittest" in text
+    pl.delete(db, p.id)
+    assert not conf.exists()
+
+
+def test_create_rejects_bad_name(db):
+    with pytest.raises(ValueError):
+        pl.create(db, name="../evil", spotify_url="https://open.spotify.com/playlist/X")