diff options
| author | Peter Stone <thepeterstone@gmail.com> | 2026-11-05 08:42:28 +0000 |
|---|---|---|
| committer | Peter Stone <thepeterstone@gmail.com> | 2026-11-05 08:42:28 +0000 |
| commit | 32df466c5c851c1a3e88c973627a9c07d2634e2b (patch) | |
| tree | 3c4e7cee54f4a999d7e8122573b016ae818951b9 /CLAUDE.md | |
| parent | 9678aa3e980d1e44defb3808812291968565f869 (diff) | |
Say what happened in words, and stop streaming from publishers
Three things, from one red banner on Peter's phone reading "Playback failed:
ERROR_CODE_IO_BAD_HTTP_STATUS". His words were "arcane and sketchy". Both halves were fair, and
the second was pointing at a real bug rather than a wording problem.
**The app was streaming episodes from their publishers.** `toMediaItem` falls back to the
enclosure URL when there is no local file, and nothing stopped a play reaching it: press play on
an undownloaded episode and it opened the publisher's CDN directly. Three things wrong with that.
It is the automatic publisher fallback the download path had removed on purpose in October ("Do
not reintroduce an automatic fallback for a transient failure"), surviving where nobody looked.
It is an outbound request to a third party that nothing asked for -- the sketchy part, and Peter
was right to read it that way. And worst, nothing in playback checks `audioFromServer`, so the
breaks the server measured on *its* copy were being applied to a copy with different advertising
in different places: skips landing in the wrong spot with no way to tell why.
`PlaybackAdvancer.start` now returns NeedsFetch for any episode with no file, which routes through
the download path that already honours the server rule. One caller still reaches the fallback --
Android Auto resolves a browse id straight to a media item without the advancer, and a car that
silently plays nothing is worse than a car that streams. That needs fetching inside the session
callback; until then OutboundRequestsTest pins the fallback to exactly one line.
Ruled out first, with evidence rather than theory: the skip server (every request in its access
log is a 200), the user agent (both the app's and Media3's get 200 from the enclosure), and my own
deploy the day before.
**The messages.** Media3's constant is now a sentence (`PlaybackFailureText`), and it was not the
only one. The same pass found `Download failed: ${it.message}`, `Could not read that file:
permission denied`, and `FeedFetcher` putting the literal word `UnknownHostException` under a
podcast's name in the library -- that one reaches two surfaces, because the text is also stored as
the subscription's `lastError`. **Two existing tests asserted the machine text came through**,
which is how it survived review; they assert the opposite now.
Accuracy turned out to be the harder half. "It will be retried" reads better than "Try the
download button again" and is a lie: a failed download is deliberately left FAILED. A 404 and a
503 both mean "it didn't work" to a library and are different decisions for the person choosing
whether to unsubscribe, so they say different things. Where the cause cannot be narrowed the
sentence stays vague rather than guessing. Every cause goes to logcat -- which detekt caught me
failing to do in FeedFetcher, three swallowed exceptions after I removed their messages.
**The guardrails Peter asked for**, both watched failing before being trusted:
- `PlainLanguageTest` -- no listener-facing message may be built out of an exception or spell out
an error constant. Deliberately scoped to where errors surface rather than judging all prose.
- `OutboundRequestsTest` -- one OkHttpClient, built in AppGraph, carrying SecureSchemeInterceptor;
nothing may open a connection another way; and the publisher-stream fallback must stay at one
site. The same arrangement the server has in safefetch/audit_test.go and for the same reason:
this app fetches URLs chosen by podcast publishers.
**And the readout, which Peter asked for directly.** The per-show "Detected skips" lines read the
confirmation rows the on-device detector used to write, so since detection moved they could only
ever say "Nothing found yet" -- on every show, including ones the server was publishing breaks
for. Counted off the episodes now. Verified on a device against a staged state rather than argued:
three of five episodes carrying a detected intro, one carrying an ad break, reading "Found in 3 of
5 episodes" / "None found in 5 episodes" / "Found in 1 of 5 episodes". *Not checked* and *checked,
found nothing* are different answers again, which was the whole point of the readout.
1016 tests, detekt 0, lint clean. Coverage 94.58% -> 94.12%, COVERAGE_EXCEPTION for the same
reason as yesterday: the deleted confirmation-row code was covered.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018AwnALZL6bD4VBtATDVv3B
Diffstat (limited to 'CLAUDE.md')
| -rw-r--r-- | CLAUDE.md | 51 |
1 files changed, 43 insertions, 8 deletions
@@ -168,14 +168,17 @@ clip files on disk, and unsubscribing is the only thing that clears them. columns the on-device detector used to, which is why playback needed no changes when detection moved. No migration, no schema change, nothing for a real library to go wrong over. -Two things the deletion left standing that a future session should look at rather than assume: - -- **The per-show detection readout has nothing feeding it.** `ShowSettingsViewModel.loadDetection` - reads `ShowFingerprintDao.get` and `MidrollDao.getCandidates`, and nothing has written a - *candidate* row since detection moved: the server writes per-episode ranges and offsets, not the - confirmation state the readout describes. So "2 of 3 episodes agree" can never appear, and a - phone that never ran the old version shows "nothing found yet" for every show while the server - is finding and publishing breaks. Predates this deletion; it is the same migration's doing. +Two things the deletion left standing. The first is fixed; the second is not: + +- **The per-show detection readout had nothing feeding it — fixed 2026-11-05.** It read + `ShowFingerprintDao.get` and `MidrollDao.getCandidates`, the confirmation state the *on-device* + detector kept, and nothing had written a candidate row since detection moved. So every show + reported "Nothing found yet" for ever, including shows the server was actively publishing breaks + for — seen on Peter's phone. It is counted off the episodes now (`detectedIntroMs`, + `detectedOutroMs`, and the midroll ranges `ServerSkips` writes), worded by `SkipSummary.showState`, + and it keeps the distinction that was the point: *not checked* and *checked, nothing found* are + different answers. `show_midrolls` is consequently an entity nothing reads or writes; the table + stays because dropping it needs a migration and nothing is gained by one. - **`RetentionSweeper`'s pending-episode guard protects nothing.** It reads `getAllPendingEpisodeIds` and `getAllMidrollPendingEpisodeIds` so retention cannot reclaim a file mid-detection. Nothing sets a pending basis any more, so both are always empty — harmless, but a @@ -553,6 +556,38 @@ by default and correct; it is simply waiting for a publisher who marks breaks. ## Things a future session should know +- **Nothing the listener reads is written for a machine, and the build enforces it** + (`PlainLanguageTest`, added 2026-11-05). A red banner on Peter's phone read "Playback failed: + ERROR_CODE_IO_BAD_HTTP_STATUS"; his words were "arcane and sketchy" and both halves were fair. + The same pass found a downloader printing `${it.message}`, an OPML import printing + `permission denied`, and `FeedFetcher` putting the literal word `UnknownHostException` under a + podcast's name in the library — **and two tests that asserted the machine text came through**, + which is how it survived review. Causes go to logcat; the screen gets a sentence, from + `PlaybackFailureText`, `FeedFailureText` or `FailureText`. Accuracy is the harder half of the + rule: "it will be retried" would read better than "try the download button again" and would be + a lie, because a failed download is deliberately not retried. + +- **Every outbound request goes through one client, and the build enforces that too** + (`OutboundRequestsTest`). One `OkHttpClient`, built in `AppGraph`, carrying + `SecureSchemeInterceptor`; everything else is handed it, Coil's artwork fetches included. The + same arrangement the server has in `safefetch/audit_test.go`, for the same reason: this app + fetches URLs chosen by podcast publishers. + +- **Playback plays the app's own copy, never the publisher's** (changed 2026-11-05). + `toMediaItem` falls back to the enclosure URL when there is no local file, and that fallback was + reachable from every play: pressing play on an undownloaded episode streamed it straight from + the publisher's CDN. That is the automatic fallback the download path had removed on purpose; it + is an unasked-for request to a third party; and because nothing in playback checks + `audioFromServer`, the breaks the server measured on *its* copy were being applied to a copy with + different advertising in different places. `PlaybackAdvancer.start` now returns `NeedsFetch` for + any episode with no file, so the app fetches first. + + **One caller still reaches the fallback: Android Auto.** `onAddMediaItems` resolves a browse id + straight to a media item without going through the advancer, and a car that silently plays + nothing is worse than a car that streams. Fixing it properly means fetching inside the session + callback. `OutboundRequestsTest` pins the fallback to exactly one line so it cannot spread back. + + - **The episode screen is the player; there is no separate player screen** (merged 2026-10-19). Opening an episode puts you in its transport at the stored position, and pressing play changes nothing about the layout. Playback state still has one owner, `PlayerViewModel`; the screen |
