test(e24): close the review's coverage gaps, and correct a comment that was untrue

`test_only_a_show_row_pays_for_a_resolution` seeded no watch state, so every rail query
inner-joined its way to zero rows and the loop body — with it the assertion — never ran. It
now seeds a video, a movie and an episode, and asserts all three kinds are present before
the loop, so it cannot go vacuous again.

The Plex half of the surprise rail had no test for either of its promises: every test that
reached `plex_surprise` passed budget="any" against a library with no state rows, so the
duration bounds and the already-started exclusion could both be deleted with the suite
green. The YouTube half has had tests for the same two rules since S1 — the asymmetric half
of one principle.

`plex_item` and `plex_show` — two of the five pinnable kinds — were pinned by no test at
all, leaving the rating_key → internal-id translation and both shelf arms uncovered,
including the self-reference cycle that once 500'd the shelf. And `GET /api/discover/pick`
was exercised only through its domain function, so the route's own query-string translation
could regress silently; the new test excludes all three candidates, which is what proves the
whole list parsed rather than one unsplit string.

`PIN_COLUMNS`/`PIN_KINDS` claimed in a comment to be the one place the pin vocabulary is
spoken and had no readers anywhere: `resolve_ref` re-spelled the column names and the route
re-typed the kind list. Both now drive off the constants.

The `_NO_TS` sentinel was introduced as a crash fix and is not one. The key it replaced,
`(ts is not None, ts)`, cannot raise — tuple comparison finds the first UNEQUAL element via
`==`, so two null keys compare equal and `None < None` is never reached; three independent
runs confirmed it over every permutation. The rationale now says what the sentinel actually
buys, lives once in `plex.rollup`, and the third rail that was still on the older spelling
has been swept to match. Its test is renamed and re-scoped to the mutation it does catch.

Frontend: the reroll's second press asserted `expect.any(Array)`, which cannot tell an
accumulating exclusion window from a replacing one — both that and SEEN_WINDOW = 1 (each
reinstating the measured nine-card cycle) left the suite green. And `onInfo`'s jump type was
untested while `onPlay`'s was pinned twice, so pointing the ⓘ affordance at the player
typechecked and shipped green.

All eight fixes mutation-checked; every mutant died on its intended test.

backend/app/discover/pins.py

@@ -41,28 +41,34 @@ def resolve_ref(db: Session, user: User, kind: str, ref: str) -> tuple[str, obje
 
     The client speaks the ids the rest of the API speaks: a video id, a channel id, a Plex
     `rating_key`, a playlist id. Plex's internal integer keys never leave the backend, so the
-    translation belongs here rather than in five call sites."""
+    translation belongs here rather than in five call sites.
+
+    The COLUMN half of every answer comes from `PIN_COLUMNS`; only the LOOKUP differs per kind
+    (a direct `db.get`, a rating-key translation, an int parse plus an ownership check). Before the
+    E24 review the branches spelled the column names out again and `PIN_COLUMNS` had no readers at
+    all — a constant that called itself the one vocabulary while nothing spoke it."""
+    col = PIN_COLUMNS.get(kind)
+    if col is None:
+        return None
     if kind == "video":
-        return ("video_id", ref) if db.get(Video, ref) is not None else None
+        return (col, ref) if db.get(Video, ref) is not None else None
     if kind == "channel":
-        return ("channel_id", ref) if db.get(Channel, ref) is not None else None
+        return (col, ref) if db.get(Channel, ref) is not None else None
     if kind == "plex_item":
         it = by_rating_key(db, PlexItem, ref)
-        return ("plex_item_id", it.id) if it is not None else None
+        return (col, it.id) if it is not None else None
     if kind == "plex_show":
         sh = by_rating_key(db, PlexShow, ref)
-        return ("plex_show_id", sh.id) if sh is not None else None
-    if kind == "playlist":
-        try:
-            pid = int(ref)
-        except ValueError:
-            return None
-        pl = db.get(Playlist, pid)
-        # A playlist is per-user, unlike the other four (which are shared catalog rows whose PRIVACY
-        # lives in the state tables). Someone else's list must read as "no such thing", not as a 403 —
-        # the same reason `owned_or_404` 404s rather than 403s.
-        return ("playlist_id", pl.id) if pl is not None and pl.user_id == user.id else None
-    return None
+        return (col, sh.id) if sh is not None else None
+    try:
+        pid = int(ref)
+    except ValueError:
+        return None
+    pl = db.get(Playlist, pid)
+    # A playlist is per-user, unlike the other four (which are shared catalog rows whose PRIVACY
+    # lives in the state tables). Someone else's list must read as "no such thing", not as a 403 —
+    # the same reason `owned_or_404` 404s rather than 403s.
+    return (col, pl.id) if pl is not None and pl.user_id == user.id else None
 
 
 def list_pins(db: Session, user: User) -> list[ComfortPin]:

backend/app/discover/rails.py

@@ -6,8 +6,6 @@ side-by-side rails would just move the choice the user could not make ("which ha
 level up, which is the opposite of what this epic is for.
 """
 
-from datetime import datetime, timezone
-
 from sqlalchemy.orm import Session
 
 from app import sysconfig
@@ -15,11 +13,7 @@ from app.discover import cards, queries
 from app.discover.queries import RAIL_LIMIT
 from app.models import PlexShow, User
 from app.plex.lookups import by_rating_key
-
-# Sort sentinel for a row whose timestamp is missing. A plain `(ts is not None, ts)` key looks like it
-# handles NULLs and does not: two NULL rows compare equal on the flag and then compare `None < None`,
-# which raises. A rail must not be able to 500 the landing page over missing metadata.
-_NO_TS = datetime.min.replace(tzinfo=timezone.utc)
+from app.plex.rollup import NO_TS
 
 
 def plex_available(db: Session) -> bool:
@@ -64,7 +58,7 @@ def continue_watching(db: Session, user: User, limit: int = RAIL_LIMIT) -> list[
     ]
     if plex_available(db):
         rows += [(ts, cards.from_plex(c)) for ts, c in queries.plex_in_progress(db, user.id, limit)]
-    rows.sort(key=lambda r: r[0] or _NO_TS, reverse=True)
+    rows.sort(key=lambda r: r[0] or NO_TS, reverse=True)
     out = [card for _, card in rows[:limit]]
     # Resolve AFTER the slice: only the rows that survive it are worth a query.
     for card in out:

backend/app/plex/rollup.py

@@ -3,11 +3,25 @@
 Drives the show/season grid badges and the Resume / Play-from-start / mark-all buttons.
 """
 
+from datetime import datetime, timezone
+
 from sqlalchemy import and_, case, func, or_, select
 from sqlalchemy.orm import Session, aliased
 
 from app.models import PlexItem, PlexState
 
+# Sort sentinel for a rail row whose timestamp is missing, so `key=lambda r: r[0] or NO_TS` reads the
+# same at every rail that merges two type-homogeneous lists in Python.
+#
+# It is NOT a crash fix, whatever the E24 commit that introduced it says. The `(ts is not None, ts)`
+# key it replaced cannot raise: tuple comparison finds the first UNEQUAL element via `==`, so two
+# `(False, None)` keys compare equal and `None < None` is never reached — verified by execution
+# during the E24 epic-end review, over every permutation of null/non-null rows. What the sentinel
+# buys is one idiom instead of two, and a key that says "missing sorts last" outright rather than
+# encoding it in a boolean flag. A BARE `r[0]` would genuinely raise, which is the mutation the
+# rails' tests do guard against.
+NO_TS = datetime.min.replace(tzinfo=timezone.utc)
+
 
 def show_status(total: int, watched: int, inprog: int) -> str:
     """Roll a show's episodes up into a single watch state: fully watched, partially started, or new."""

backend/app/routes/discover.py

@@ -29,7 +29,9 @@ router = APIRouter(prefix="/api/discover", tags=["discover"])
 
 
 class PinIn(BaseModel):
-    kind: Literal["video", "channel", "plex_item", "plex_show", "playlist"]
+    # From `pins.PIN_KINDS`, not a second hand-written list: the vocabulary, the schema columns and
+    # this request contract have to move together, and re-typing them here is how they would not.
+    kind: Literal[*pins.PIN_KINDS]
     # The id the rest of the API speaks for that kind: a video id, a channel id, a Plex rating_key,
     # or a playlist id. `pins.resolve_ref` does the translation to the row's internal key.
     ref: str

backend/app/routes/plex/discover.py

@@ -14,8 +14,6 @@ grid uses, applied to each rail's movie + show query (a rail may legitimately co
 the client hides). Genre-language matches the grid; duration is movie-only.
 """
 
-from datetime import datetime, timezone
-
 from fastapi import APIRouter, Depends, Query
 from sqlalchemy import and_, func, or_, select
 from sqlalchemy.orm import Session
@@ -43,12 +41,6 @@ RAIL_LIMIT = 20
 GENRE_TILES = 12
 SURPRISE_PER_KIND = 3  # Surprise-me: at most this many movies + this many shows (6 total)
 
-# Sort sentinel for a row with no timestamp. The `(ts is not None, ts)` key this replaced LOOKS
-# NULL-safe and is not: two null-timestamped rows compare equal on the flag and then compare
-# `None < None`, which raises — so a show whose rollup has no `resumed_at`, or a movie state written
-# before `progress_updated_at` existed, could 500 a rail rather than sort last (E24).
-_NO_TS = datetime.min.replace(tzinfo=timezone.utc)
-
 
 def _scope_libs(db: Session, scope: str) -> tuple[list[int], list[int]]:
     """The movie/show library ids visible for this scope (empty list = out of scope)."""
@@ -121,10 +113,11 @@ def _recently_added(
         else []
     )
     # Merge the two type-homogeneous lists by added_at, then serialize each side in one batch so the
-    # per-card state lookups stay batched (no N+1). The `(is not None, value)` key sorts a missing
-    # timestamp last under reverse.
+    # per-card state lookups stay batched (no N+1). Same `or NO_TS` key as the two rails below — the
+    # E24 review found this one still on the older `(is not None, value)` form, i.e. one rule in two
+    # spellings, with the two comments explaining it differently.
     tagged = [(m.added_at, "movie", m) for m in movies] + [(s.added_at, "show", s) for s in shows]
-    tagged.sort(key=lambda t: (t[0] is not None, t[0]), reverse=True)
+    tagged.sort(key=lambda t: t[0] or rollup.NO_TS, reverse=True)
     top = tagged[:RAIL_LIMIT]
     # Key the serialized cards back by rating_key (globally unique in Plex — a movie and a show never
     # share one), so the merge doesn't depend on object identity staying alive.
@@ -179,7 +172,7 @@ def _recently_watched(
         )
         for ep, st, sh_title, sh_rk in db.execute(eq).all():
             rows.append((st.watched_at, leaf_card(ep, st, show_title=sh_title, show_rk=sh_rk)))
-    rows.sort(key=lambda r: r[0] or _NO_TS, reverse=True)
+    rows.sort(key=lambda r: r[0] or rollup.NO_TS, reverse=True)
     return [card for _, card in rows[:RAIL_LIMIT]]
 
 
@@ -243,7 +236,7 @@ def _continue_watching(
         for sh, resumed_at in srows:
             rows.append((resumed_at, show_card(sh, "in_progress")))
 
-    rows.sort(key=lambda r: r[0] or _NO_TS, reverse=True)
+    rows.sort(key=lambda r: r[0] or rollup.NO_TS, reverse=True)
     return [card for _, card in rows[:RAIL_LIMIT]]
 
 

backend/tests/test_discover.py

@@ -236,10 +236,14 @@ def test_the_resume_rail_obeys_the_channel_blocklist(db):
     assert hero.pick(db, user, "any", 0) is None
 
 
-def test_a_null_timestamp_sorts_last_instead_of_raising(db):
-    """The `(ts is not None, ts)` key this rail's Plex sibling used compares `None < None` when two
-    rows both lack a timestamp, which raises. A missing progress timestamp must sort last, not 500
-    the landing page."""
+def test_a_null_timestamp_sorts_last(db):
+    """A missing progress timestamp sorts last rather than 500ing the landing page.
+
+    Narrower than it was written to be, and the epic-end review corrected it: the previous key,
+    `(ts is not None, ts)`, does NOT raise on two nulls — tuple comparison finds the first UNEQUAL
+    element via `==`, so `None < None` is never reached, and this test passes under that key too.
+    What it does discriminate is the naive `key=lambda r: r[0]`, which really does raise. See
+    `plex.rollup.NO_TS` for the corrected rationale."""
     user, _ = _seed_yt(db)
     db.add_all(
         [
@@ -539,6 +543,45 @@ def test_a_pinned_video_survives_leaving_the_feed_scope(db):
     assert card["next"]["id"] == "vmed"
 
 
+def test_the_plex_surprise_obeys_the_time_budget(db):
+    """The one control on the page has to mean what it says: a 100-minute film is not an answer to
+    "10 perc". The YouTube half of this rule has had its own test since S1; the Plex half's duration
+    bounds could be deleted with the whole suite still green, because every test that reached
+    `plex_surprise` passed budget="any"."""
+    user, _ = _seed_yt(db)
+    _seed_plex(db, user)
+    _enable_plex(db)
+    drawn = lambda budget: {  # noqa: E731
+        (c["source"], c["id"]) for s in range(6) for c in rails.surprise(db, user, budget, s, 5)
+    }
+    assert ("plex", "pm1") not in drawn("short")
+    # …and it IS offered where it fits, so this cannot pass by the rail being empty.
+    assert ("plex", "pm1") in drawn("long")
+
+
+def test_the_plex_surprise_never_offers_something_already_started(db):
+    """A half-watched film is not a surprise, and the resume tier has already had its chance at it.
+    The mirror of `test_surprise_never_offers_something_already_started` for the other source — the
+    asymmetric half of one principle, which the house rules say to fix everywhere."""
+    user, _ = _seed_yt(db)
+    movie, _, _ = _seed_plex(db, user)
+    _enable_plex(db)
+    db.add(
+        PlexState(
+            user_id=user.id,
+            item_id=movie.id,
+            status="new",
+            position_seconds=600,
+            progress_updated_at=NOW,
+        )
+    )
+    db.commit()
+    drawn = {
+        (c["source"], c["id"]) for s in range(6) for c in rails.surprise(db, user, "long", s, 5)
+    }
+    assert ("plex", "pm1") not in drawn
+
+
 def test_a_new_pin_lands_at_the_end_even_after_a_hole(db):
     """Positions are never renumbered on removal — `remove_pin` deletes the row, and every FK cascade
     silently removes a pin when its target disappears — so the append position has to come from
@@ -603,11 +646,46 @@ def test_the_resume_rail_says_what_would_actually_play(db):
 
 def test_only_a_show_row_pays_for_a_resolution(db):
     """`next` is the answer to "a show has no Play button". Attaching it to a movie or a video would
-    both cost a query for nothing and re-open the self-reference cycle the shelf already hit."""
+    both cost a query for nothing and re-open the self-reference cycle the shelf already hit.
+
+    The states are what make this test exist: without them every rail query inner-joins its way to
+    zero rows, the loop body never runs, and the assertion is vacuous — which is exactly how the
+    epic-end review found it."""
     user, _ = _seed_yt(db)
-    _seed_plex(db, user)
+    movie, _, ep1 = _seed_plex(db, user)
     _enable_plex(db)
-    for card in rails.continue_watching(db, user):
+    db.add_all(
+        [
+            VideoState(
+                user_id=user.id,
+                video_id="vmed",
+                status="new",
+                position_seconds=300,
+                progress_updated_at=NOW,
+            ),
+            PlexState(
+                user_id=user.id,
+                item_id=movie.id,
+                status="new",
+                position_seconds=600,
+                progress_updated_at=NOW,
+            ),
+            PlexState(
+                user_id=user.id,
+                item_id=ep1.id,
+                status="new",
+                position_seconds=120,
+                progress_updated_at=NOW,
+            ),
+        ]
+    )
+    db.commit()
+
+    rail = rails.continue_watching(db, user)
+    # A video, a movie and a show — one row of each kind that can appear here, so the assertion below
+    # actually exercises both sides of the equality.
+    assert {c["kind"] for c in rail} == {"video", "movie", "show"}
+    for card in rail:
         assert ("next" in card) == (card["kind"] == "show"), card["id"]
 
 
@@ -732,6 +810,63 @@ def test_pinning_twice_is_the_same_shelf(db, as_user):
     assert len(c.get("/api/discover/pins").json()["items"]) == 1
 
 
+def test_pinning_a_plex_show_resolves_to_the_episode_that_would_play(db, as_user):
+    """Two of the five pinnable kinds are Plex, and the rating_key → internal-id translation is
+    theirs alone — a wrong column or a wrong model in either arm pins the wrong row, or 404s every
+    Plex pin. Neither arm had a test before the epic-end review."""
+    user, _ = _seed_yt(db)
+    _, _, ep1 = _seed_plex(db, user)
+    _enable_plex(db)
+    db.add(PlexState(user_id=user.id, item_id=ep1.id, status="watched", watched_at=NOW))
+    db.commit()
+    c = as_user(user)
+    assert c.post("/api/discover/pins", json={"kind": "plex_show", "ref": "ps1"}).status_code == 200
+    (tile,) = c.get("/api/discover/pins").json()["items"]
+    assert (tile["kind"], tile["id"]) == ("show", "ps1")
+    assert tile["next"]["id"] == "ps1e2"  # ep1 is finished
+
+
+def test_pinning_a_plex_movie_plays_itself_without_a_cycle(db, as_user):
+    """The self-reference that once 500'd the whole shelf, on the other leaf kind: `next` has to be a
+    distinct dict, not the tile aliased into itself. Covered for a video since S1, never for a movie."""
+    user, _ = _seed_yt(db)
+    _seed_plex(db, user)
+    _enable_plex(db)
+    c = as_user(user)
+    assert c.post("/api/discover/pins", json={"kind": "plex_item", "ref": "pm1"}).status_code == 200
+    (tile,) = c.get("/api/discover/pins").json()["items"]
+    assert tile["id"] == "pm1" and tile["next"]["id"] == "pm1"
+
+
+def test_the_reroll_endpoint_parses_its_whole_exclusion_list_and_its_press_count(db, as_user):
+    """The reroll's behaviour is covered at the domain level; the ROUTE's query-string translation —
+    the comma split, the cap, the `start` passthrough — was not covered at all, so a change there
+    could hand back the same card on every press with the suite green. Excluding all three candidates
+    is what proves the whole list parsed: a delimiter change leaves one unsplit string in the set,
+    and a hero comes back."""
+    user, _ = _seed_yt(db)
+    db.add(
+        VideoState(
+            user_id=user.id,
+            video_id="vmed",
+            status="new",
+            position_seconds=300,
+            progress_updated_at=NOW,
+        )
+    )
+    db.add(ComfortPin(user_id=user.id, video_id="vlong", position=0))
+    db.commit()
+    c = as_user(user)
+
+    # `start` rotates which tier answers first: 0 leads with resume, 1 with the shelf.
+    assert c.get("/api/discover/pick?seed=1&start=0").json()["hero"]["tier"] == "resume"
+    assert c.get("/api/discover/pick?seed=1&start=1").json()["hero"]["tier"] == "comfort"
+
+    skip = ",".join(f"youtube:{v}" for v in ("vshort", "vmed", "vlong"))
+    assert c.get(f"/api/discover/pick?seed=2&exclude={skip}").json()["hero"] is None
+    assert c.get("/api/discover/pick?budget=whenever").status_code == 422
+
+
 def test_pinning_something_that_does_not_exist_is_a_404(db, as_user):
     user, _ = _seed_yt(db)
     r = as_user(user).post("/api/discover/pins", json={"kind": "video", "ref": "nope"})