diff --git a/docs/HANDOFF.md b/docs/HANDOFF.md index 5cc0f17..70873e5 100644 --- a/docs/HANDOFF.md +++ b/docs/HANDOFF.md @@ -627,3 +627,58 @@ because either alone would have prevented this: Regression-checked against the real `logs/I-gbkey.json` that caused this: the old logic matches the quota pattern, the new logic yields SUCCESS and feeds the detector an empty blob. + +## Wave 8 — J-crashsafe: IN FLIGHT, launched 2026-09-12 17:50Z +Prompt: `tasks/J-crashsafe.txt`. Session id in `logs/J-crashsafe.sid`. Lease +`bookshelf-wave` held by `tasks/wave-guard.sh`, sentinel `logs/WAVE8-DONE`. + +**Why this wave exists.** The user installed the wave-7 build and the FIRST barcode +scanned crashed the app: ISBN decoded and displayed, spinner ran a few seconds, process +died. Never reproduced, on that book or any other. Their theory — that the scan fell +through to Google Books and crashed there — is structurally the best fit: keyless GB +429'd every caller on earth, so `GoogleBooksClient.classify`'s 2xx branch, +`toBookMetadata()`, `normalizeCoverUrl()` and the two-source merge had NEVER EXECUTED +in production before 486f6eb. A first crash belongs in a path's first real exercise. + +**It does not reproduce off-device, and that is now evidence rather than a guess.** +`app/app/src/test/java/org/modg/bookshelf/livemetadata/LiveMetadataLookupTest.kt` +(commit 93546ed, opt-in on `LIVE_METADATA=1`) drives the real `MetadataRepository.lookup` +against both real APIs with the real key. Nine lookups — both of the user's +previously-failing ISBNs plus a control, three times each — all returned Found with +cover art, 254ms to 4.7s, nothing thrown. Static review found no unsafe operation in +that path either. **Do not let a future worker "fix" this by rewriting the parse/merge +code; that code was exercised live and is fine.** + +**What the wave actually fixes** is the defect found while looking: nothing on that path +is exception-safe. Both clients' `fetch` catch only `IOException`, `classify` catches +only `SerializationException`/`IllegalArgumentException`, and `ScanViewModel.runLookup` +sits inside `viewModelScope.launch { ...collect { } }` with no try/catch at all. So any +throwable that is not an `IOException` — platform TLS, an OkHttp internal, memory +pressure, an API-level difference, none of which this JVM reproduces — kills the +process instead of surfacing as "couldn't be reached". Both clients' KDoc claims "Never +throws"; that claim is false today. Plus: the app has NO crash capture whatsoever, +which is why a one-time crash left nothing to work from. + +Scope, per the user's decision: (1) both clients catch `Throwable` -> new +`FailureKind.UNEXPECTED`, non-retryable, reason names the exception class only (never +its message — that can carry the key); (2) `ScanViewModel.runLookup` guards the same +way, which also covers the Room call in it; (3) a `CrashReporter` that persists the +stack trace and chains to the previous handler; (4) a Diagnostics section in settings to +read and SHARE it off the phone. **`CancellationException` must be rethrown, not +converted, in every one of those catches** — it is normal control flow here (dismissing +the sheet cancels the lookup) and is the easiest thing in this wave to get wrong. + +**Verify before accepting** (workers self-report optimistically; several waves have +over-claimed): + cd ~/bookshelf && ./tasks/gw assembleDebug && ./tasks/gw testDebugUnitTest \ + && ./tasks/gw verifyPaparazziDebug && git status --porcelain +**190 tests, 2 skipped today** (LiveSyncTest + LiveMetadataLookupTest, both opt-in +live). Count from the `TEST-*.xml` files, not the console. The count must go UP. +Grep the build log for `always 'false'`. Then eyeball the new settings Paparazzi PNGs. +Check specifically that `CancellationException` is rethrown in every new catch, and +that `CrashReporter` calls the previously-installed handler — a handler that does not +chain leaves the process hung instead of dying. + +**Not in scope, deliberately:** finding the original throwing line. Nobody knows what it +was and the prompt says not to hunt for it. If the guard lands and the user ever sees +"unexpected: SomeException" on the scan sheet, THAT is when we learn the answer. diff --git a/tasks/J-crashsafe.txt b/tasks/J-crashsafe.txt new file mode 100644 index 0000000..2179405 --- /dev/null +++ b/tasks/J-crashsafe.txt @@ -0,0 +1,217 @@ +You are implementing ONE change in the Bookshelf Android app at ~/bookshelf. + +READ FIRST, in this order: + 1. ~/bookshelf/docs/SPEC.md — the authoritative contract. Sections "Book metadata + lookup" and "Barcode scanning". Do not contradict it and do not edit it. + 2. ~/bookshelf/docs/METADATA-SOURCES.md — section "The key landed" and the + "Measured again 2026-09-09" section above it. This is the evidence base. + 3. The files you will change (listed in "What to build"). + +## The bug, and what is already known — do NOT redo this investigation + +On 2026-09-11 the Google Books API key landed (commit 486f6eb). On the user's phone, +the FIRST barcode scanned after installing that build **crashed the app**. The ISBN +was decoded and displayed, the spinner ran for a few seconds, then the process died. +It has never reproduced, on that book or any other. + +The orchestrator has already investigated. Do not repeat any of this: + +- **The structural suspicion is sound.** Keyless Google Books returned HTTP 429 to + every caller on the internet (a shared, exhausted daily quota — METADATA-SOURCES.md), + so before that commit `GoogleBooksClient.classify`'s 2xx branch, `toBookMetadata()`, + `normalizeCoverUrl()` and the two-source merge in `MetadataMerger` had NEVER ONCE + EXECUTED in production. A first crash belongs in a path's first real exercise. +- **It does not reproduce off-device.** There is now an opt-in live test, + `app/app/src/test/java/org/modg/bookshelf/livemetadata/LiveMetadataLookupTest.kt`, + which drives the real `MetadataRepository.lookup` against both real APIs with the + real key. Nine lookups — both of the user's previously-failing ISBNs plus a control, + three times each — all returned Found with cover art and nothing threw. Run it + yourself if you like: `LIVE_METADATA=1 GOOGLE_BOOKS_API_KEY= ./tasks/gw + testDebugUnitTest --tests '*LiveMetadataLookupTest*'`. (Get the key from the + environment if it is set; do NOT read app/local.properties.) +- Static review found no unsafe operation in that path: no `!!` outside a guarded + branch, no date parsing, no unsafe indexing, and `ui/components/BookCover.kt` + handles every `AsyncImagePainter.State`. + +**So do not go hunting for the throwing line. Nobody knows what it is, and you will +not find it either.** Your job is to make the crash impossible whatever it was, and to +make sure the next one leaves evidence. That is the whole wave. + +## The actual defect you ARE fixing + +The lookup path has no exception safety anywhere along it: + +- `GoogleBooksClient.fetch` catches **only `IOException`**. `OpenLibraryClient.fetch` + does the same. `classify` catches only `SerializationException` / + `IllegalArgumentException`. +- `ScanViewModel.runLookup` is called from inside + `viewModelScope.launch { scannerController.scanResults.collect { ... } }` with **no + try/catch at any level**. + +Both clients' KDoc claims "Never throws: every outcome, including transport failure, +comes back as a SourceResult rather than a swallowed null." **That claim is currently +false** — it is true only for `IOException`. Anything else that escapes on a real +device (the platform TLS stack, an OkHttp internal, memory pressure, an Android API +level difference — none of which the JVM here reproduces) takes the whole process +down instead of surfacing as "couldn't be reached". + +That is a real, provable defect regardless of whether it is what bit the user, and it +is exactly the failure class this project already decided to eliminate in wave 5: a +failure the user cannot distinguish from anything else. + +## What to build + +### 1. The clients genuinely never throw + +In BOTH `data/metadata/GoogleBooksClient.kt` and `data/metadata/OpenLibraryClient.kt`, +widen the catch in `fetch` from `IOException` to any `Throwable`, mapping the +unexpected case to a `SourceResult.Failed`. + +- Add a new `FailureKind` for it — `UNEXPECTED` — rather than reusing `TRANSPORT`. + It is NOT retryable (`RetryPolicy.isRetryable` must return false for it): we have no + idea what went wrong, so repeating it is as likely to repeat the damage as to help. +- The `reason` must name the exception class, e.g. `"unexpected: IllegalStateException"`. + That string is rendered on the scan sheet and is our ONLY diagnostic channel from a + real phone. Include the class name; do NOT include the message (it can contain a URL, + and therefore the API key). `GoogleBooksClient.redact` still applies on top. +- **`CancellationException` MUST be rethrown, not converted.** This is the one way to + get this wrong. Coroutine cancellation is normal control flow — the user dismissing + the sheet or leaving the screen cancels the lookup — and swallowing it turns a + cancelled coroutine into a "failed lookup" and breaks structured concurrency. + `kotlin.coroutines.cancellation.CancellationException`. Rethrow it first, before any + other handling. +- Keep the existing `IOException` arm and its specific classification + (`SourceResult.fromException`) exactly as it is. `UNEXPECTED` is the fallthrough for + what that function never sees, not a replacement for it. + +### 2. The ViewModel survives anything the layer below does + +In `ui/scan/ScanViewModel.kt`, `runLookup` must not be able to kill the process. Wrap +its body so that any `Throwable` becomes the existing "lookup failed" sheet state +(`ScanSheetState.LookupFailed` — read `ScanModels.kt`; it already carries a reason +string and already offers Retry and "Enter by hand", which is exactly the right +affordance here). + +- Again: rethrow `CancellationException` first. +- Note that `bookRepository.findByIsbn13(isbn13)` is inside this function too, so this + guard covers a Room/disk failure as well, not just the network. +- The reason string should mark it as unexpected and name the exception class, so it is + distinguishable on the sheet from an ordinary source failure. +- This is deliberately belt-and-braces with §1. Keep both: §1 keeps one source's + problem from destroying the other source's answer, §2 catches anything that escapes + anywhere else in the function. + +### 3. A crash leaves evidence + +New package `org.modg.bookshelf.diagnostics`. A `CrashReporter` that: + +- Installs a `Thread.setDefaultUncaughtExceptionHandler` from + `BookshelfApplication.onCreate`. +- **Chains to the previously-installed handler at the end, always.** If you do not, + the process hangs instead of dying and Android never shows its dialog or records the + crash. Capture the previous handler before installing, and call it in a `finally`. +- Writes ONE file per crash into the app's `filesDir`, containing: an ISO-8601 + timestamp, the app `versionName`/`versionCode`, `Build.MODEL`, `Build.VERSION.SDK_INT`, + the crashing thread's name, and the full stack trace INCLUDING the cause chain + (`Throwable.printStackTrace` on a `PrintWriter` over a `StringWriter` gives you the + causes for free — do not hand-roll the walk). +- Keeps only the most recent 5 and deletes older ones, so this cannot grow without + bound on a phone. +- Cannot itself throw: the whole body goes in `runCatching`, because an exception + inside an uncaught-exception handler is the worst possible place to have one. Must + not do I/O on the main thread at startup — writing happens only when a crash + happens, and reading happens only when the user opens the screen in §4. + +### 4. The user can actually read it + +Add a "Diagnostics" section to the settings screen (`ui/settings/`). It shows the most +recent crash — timestamp and the exception's first line — with a way to see the full +trace and get it off the phone. Getting it off the phone is the entire point: the user +is in another country from this machine and screenshots of a stack trace are painful. +Offer a share action (`Intent.ACTION_SEND`, `text/plain`) and a copy-to-clipboard. +When there are no crash files, say so plainly — an empty "Diagnostics" section that +looks broken is worse than none. + +Match the existing settings screen's structure and visual language exactly; read it +before writing. Follow SPEC's "Design language" section. This screen has Paparazzi +coverage — add snapshots for the new section in both light and dark, in the same style +as the existing settings snapshots, and record them. + +### 5. Update the comments you invalidate + +Both clients' class KDoc claims they never throw. After §1 that becomes true — say so +accurately, and say WHY the `UNEXPECTED` kind exists (the crash above, undiagnosed, +2026-09-12). This codebase comments with the evidence for a decision, not a restatement +of the code. Match that. + +## Constraints — these are hard + +- Kotlin. Match the surrounding code's style, naming and comment density. Read + neighbouring files before writing and follow their conventions. +- DO NOT touch: app/app/build.gradle.kts, gradle/libs.versions.toml, or any other + build file. If you believe you need a new dependency, STOP and say so in your report + instead — everything above is doable with what is already declared. +- DO NOT touch anything under data/local, data/remote, data/repo, data/prefs, or + server/. You may edit AppContainer.kt and BookshelfApplication.kt only as far as §3 + requires. +- DO NOT read, print, cat, grep or echo `app/local.properties`, and never paste a real + API key into a source file, a test, or your report. Tests use a fake like + "test-key-123". +- DO NOT git commit, git add, or git push. The orchestrator commits. Leave your work + in the working tree. +- Build with `./tasks/gw ` — NEVER `./gradlew` directly (tasks/gw is a + flock-serialized wrapper). +- RUN BUILDS IN THE FOREGROUND. Do not background a Gradle build and end your turn + saying you will report when it finishes. Three previous workers on this project did + exactly that and could never report. A full build takes 1-3 minutes; wait for it. +- There is no emulator on this box (no KVM). You cannot run the app. Do not claim you + did. + +## Definition of done + +Run these yourself and paste the real output: + + ./tasks/gw assembleDebug -> exit 0 + ./tasks/gw testDebugUnitTest -> exit 0. **190 tests pass today, 2 skipped** + (LiveSyncTest and LiveMetadataLookupTest, both + opt-in live tests). The count must go UP and + nothing may regress. Get the count by summing + the TEST-*.xml files, not by reading the console. + ./tasks/gw recordPaparazziDebug -> exit 0, for the new settings snapshots (§4) + ./tasks/gw verifyPaparazziDebug -> exit 0 against the re-recorded snapshots + +Required new tests, with REAL assertions (assertion-free tests are a spec violation +on this project): + - each client returns `Failed(kind = UNEXPECTED)` — not a crash — when the HTTP call + throws a non-IOException `RuntimeException`. An `Interceptor` that throws is the + straightforward way to provoke this with the real client. + - the reason names the exception class and does NOT contain the exception's message. + - `CancellationException` propagates out of a client rather than becoming a `Failed`. + Test this explicitly. It is the easiest thing here to get wrong. + - `RetryPolicy.isRetryable(UNEXPECTED)` is false, and `withRetry` therefore makes + exactly one attempt. + - `ScanViewModel.runLookup` puts the sheet into LookupFailed (and does not throw) + when the repository throws; and a `CancellationException` is not swallowed. + - `CrashReporter` writes a file containing the exception class and its cause's + message; retains only the newest 5; and DELEGATES to the previously-installed + handler (assert the previous handler was actually invoked). + +Also: grep the build log for Kotlin warnings on the files you touched. A warning +reading "Check for instance is always 'false'" is NOT cosmetic — that exact warning +hid a bug that blanked every book cover in this app for months. + +## Report + +End with a plain report covering: + - what you changed, file by file + - the verbatim tail of each gradle command above + - the test count before and after, summed from the XML + - **your own judgement call on this**: does §1+§2 actually make the lookup path + non-crashing, or is there still a way for a throwable to reach the top? Say so + plainly if there is. Naming a remaining hole is worth more than a clean report. + - anything you could NOT do, or did differently from these instructions, and why + - anything you noticed that looks wrong but was out of scope + +Be honest. Workers on this project have over-claimed success before, and the +orchestrator independently re-verifies everything, so an inflated report only wastes a +round trip.