From 1969b74cc98d3ff9904ae5c122a60053923eb4cd Mon Sep 17 00:00:00 2001 From: Sprite Date: Wed, 9 Sep 2026 10:42:32 +0000 Subject: [PATCH] SPEC: lookup outcome is three-way; wave 5 worker prompt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SPEC now states that a source which could not be reached must never be reported as a book that does not exist: Found / NotFound / Unavailable, where NotFound requires every source to have answered authoritatively. Also records that a rejected barcode must not be silent, and that the by-ISBN cover URL is not evidence a cover exists. run-task.sh exports CLAUDE_CODE_PRINT_BG_WAIT_CEILING_MS=0, per the wave-4 post-mortem: `claude -p` otherwise kills background tasks at 600s, so a worker that backgrounds a Gradle build can never report on it. Safe to edit now — no workers are running (hazard: never edit it while they are). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CDcPottghJXEvfYKqFM7zf --- docs/SPEC.md | 20 ++++- tasks/G-diagnostics.txt | 186 ++++++++++++++++++++++++++++++++++++++++ tasks/run-task.sh | 4 + 3 files changed, 208 insertions(+), 2 deletions(-) create mode 100644 tasks/G-diagnostics.txt diff --git a/docs/SPEC.md b/docs/SPEC.md index 774fc2e..dc0e7ec 100644 --- a/docs/SPEC.md +++ b/docs/SPEC.md @@ -91,12 +91,28 @@ Never let sync failure surface as a crash or a blocking dialog — a quiet statu Primary Open Library: https://openlibrary.org/api/books?bibkeys=ISBN:{isbn}&format=json&jscmd=data Fallback Google Books: https://www.googleapis.com/books/v1/volumes?q=isbn:{isbn} (no key) Cover: https://covers.openlibrary.org/b/isbn/{isbn}-L.jpg else GB imageLinks (force https, zoom=2) -Merge: prefer whichever has a title; fill blanks from the other. Return null if both miss, -and the UI must then offer manual entry pre-filled with the scanned ISBN. +Merge: prefer whichever has a title; fill blanks from the other. +Cover URL comes from a source that REPORTS one. Never synthesize the by-ISBN cover +URL as if it were evidence: for an edition with no art that endpoint returns 200 + +a 43-byte 1x1 transparent GIF, which loads "successfully" and paints nothing. As a +last resort it may be used only with `?default=false`, which makes a miss a 404. + +Lookup outcome is THREE-WAY, never a bare null. A source that could not be reached +must never be reported to the user as a book that does not exist: + Found(metadata) - at least one source returned a record + NotFound - EVERY source answered authoritatively and none had it + Unavailable - no source could be reached (non-2xx, timeout, transport error) + and none of the reachable ones had it +UI: Found -> the save sheet. NotFound -> manual entry pre-filled with the scanned +ISBN. Unavailable -> a retry affordance, with manual entry as the escape hatch; +it must NOT claim the book is unknown. ## Barcode scanning CameraX Preview + ImageAnalysis -> ML Kit BarcodeScanning (EAN_13, EAN_8, UPC_A). Validate ISBN-13 checksum before lookup; ignore non-book barcodes. Debounce repeats. +A rejected barcode is NOT silent: the camera screen must say a code was read and +was not a book ISBN, or the user cannot tell a non-book barcode from a dead camera. +Throttle that message — a non-book barcode sits in frame emitting continuously. Continuous mode: after a save, stay on camera for the next book (shelving a box of books is the real use case). Show a running "added this session" count. Handle: camera permission denial, torch toggle, and a manual-ISBN-entry escape hatch. diff --git a/tasks/G-diagnostics.txt b/tasks/G-diagnostics.txt new file mode 100644 index 0000000..42ffdbf --- /dev/null +++ b/tasks/G-diagnostics.txt @@ -0,0 +1,186 @@ +You are implementing ONE feature 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" were just rewritten for this task. They are the + spec you are implementing. Do not contradict them and do not edit SPEC.md. + 2. ~/bookshelf/docs/METADATA-SOURCES.md — why this work exists, section + "What actually failed". + +## The problem + +The app cannot tell three different things apart, and reports all of them +identically or not at all: + + (1) A barcode was decoded but is not a valid ISBN-13. + ScanCodeFilter.accept() returns null. NOTHING happens. No sheet, no message. + To the user this is indistinguishable from a camera that isn't working. + + (2) A lookup request FAILED — non-2xx (Google Books returns HTTP 429 to keyless + callers), timeout, or transport error. + Both OpenLibraryClient.fetchBody and GoogleBooksClient.fetchBody collapse + this to null, MetadataRepository collapses that to null, and the UI writes + "No match found — enter the details by hand." The app tells the user a book + does not exist when in truth it never managed to ask. + + (3) The lookup genuinely succeeded and neither source has the book. + This is the ONLY case where "No match found" is honest. + +This is not hypothetical. Two real books off the user's shelf (9781883937386 +"The Hittite Warrior", 9781883937676 "Shadow Hawk") failed on the phone, and both +are fully present in Open Library — there are regression tests proving the app's +own parser handles their real API responses. The bug is in how failure is +classified and shown, NOT in coverage. Do not "fix" this by adding a data source. + +## What to build + +### 1. Per-source classification (data.metadata) + +Give each client a three-way outcome instead of `BookMetadata?`: + + sealed interface SourceResult { + data class Found(val metadata: BookMetadata) : SourceResult + data object NotFound : SourceResult // answered 2xx, no record + data class Failed(val reason: String) : SourceResult // non-2xx/timeout/IO/parse + } + +`reason` is a short diagnostic string ("http 429", "timeout", "malformed json") — +it is for the log and for us, NOT prose to show the user verbatim. + +CRITICAL for testability: there is no MockWebServer in this project and you must +not add one. Put the classification in a PURE function that takes what an HTTP +response gives you and returns a SourceResult, e.g. + + internal fun classify(httpCode: Int, body: String?, isbn13: String): SourceResult + +and have fetch/lookup call it. Then it can be unit-tested exhaustively offline, +the way parseResponse already is. Keep the existing internal parseResponse +functions and their tests working. + +### 2. Combination (MetadataRepository) + + sealed interface LookupResult { + data class Found(val metadata: BookMetadata) : LookupResult + data object NotFound : LookupResult + data class Unavailable(val reason: String) : LookupResult + } + +Rules, exactly as SPEC states them: + - any source Found -> Found (merge as today via MetadataMerger) + - all sources NotFound -> NotFound + - otherwise (>=1 Failed, none Found) -> Unavailable + +That last rule is the whole point: a single reachable source saying "no" is NOT +authoritative while the other source could not be reached. Unit-test the full +matrix — 3x3 of (OL outcome, GB outcome) — with real assertions. + +Keep MetadataRepository.byIsbnCoverUrl and the existing last-resort cover +behaviour intact. An invalid ISBN (IsbnUtils.toIsbn13 returns null) is a +programming error at this layer, not a lookup outcome — keep it out of +LookupResult; the scan layer must never send one. + +### 3. Give the metadata HTTP client a call timeout (AppContainer) + +`metadataHttpClient` is a bare `OkHttpClient()` with no call timeout, so a stalled +connection hangs on default socket timeouts. Give it an explicit callTimeout +(10-15s is right — the user is standing at a bookshelf) plus connect/read +timeouts. Leave the PocketBase okHttpClient alone; it is a different client for a +reason (it carries the auth token) and sync is not in scope. + +### 4. Scanner feedback for a rejected barcode (ui.scan) + +ScanCodeFilter.accept() currently returns String?. Make the rejection visible: + + sealed interface ScanOutcome { + data class Isbn(val isbn13: String) : ScanOutcome + data class NotAnIsbn(val rawValue: String) : ScanOutcome + data object Ignored : ScanOutcome // debounced repeat: emit NO ui at all + } + +Route NotAnIsbn through ScannerController to the ScanScreen, which shows a +transient message near the reticle, e.g. "Read 012345678905 — not a book barcode". + +THROTTLING IS MANDATORY AND IS THE EASY THING TO GET WRONG. A non-book barcode +sitting in frame decodes on almost every analyzed frame. The message must not +flicker or re-trigger per frame: debounce the same rejected code the way repeats +of a valid code are already debounced, and let the message auto-clear after a +few seconds. Unit-test the throttle with an injected clock — ScanCodeFilter +already takes `nowMillis: () -> Long` for exactly this; use it, do not use real +time in tests. + +### 5. UI states (ui.scan) + +ScanSheetState gains a failure case alongside the existing ones: + + data class LookupFailed(val isbn13: String, val reason: String) : ScanSheetState + + - Found -> existing FoundBookSheet, unchanged + - NotFound -> existing ManualEntrySheet. Reword its copy so it reads as an + authoritative negative ("Not in Open Library or Google Books"), + not as a generic failure. + - LookupFailed -> a NEW sheet that says the lookup could not be completed and + offers: Retry (re-runs the lookup for that same ISBN), Enter by + hand (falls through to the manual-entry form, ISBN pre-filled), + and Skip. It must NOT say or imply the book is unknown. + +Wire Retry properly: it re-enters the loading state and re-runs the lookup. Do not +leave a sheet that can strand the user — note that ScanViewModel.onScanned +early-returns while any sheet is showing, so a sheet that cannot be dismissed +blocks every subsequent scan. + +## Constraints — these are hard + +- Kotlin, Jetpack Compose, Material 3. Match the surrounding code's style, naming + and comment density. Read neighbouring files before writing. +- Design language is in SPEC.md "Design language". Reuse existing components + (PrimaryButton, SecondaryButton, EmptyState, BookCover...). Do not invent new + colours or typography. +- DO NOT touch: app/build.gradle.kts, gradle/libs.versions.toml, any file under + data/local, data/remote, data/repo, or ui/settings, ui/locations, ui/detail. + Every dependency you need is already declared. If you believe you need a new + one, STOP and say so in your report instead. +- DO NOT edit docs/SPEC.md, docs/HANDOFF.md or docs/METADATA-SOURCES.md. +- 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 later — a previous worker did exactly that and could + never report. A full build here takes 1-3 minutes; just wait for it. +- There is no emulator on this box (no KVM). You cannot run the app. Verify with + assembleDebug, unit tests, and Paparazzi. + +## Definition of done + +All of these must pass, and you must run them yourself and paste the real output: + + ./tasks/gw assembleDebug -> exit 0 + ./tasks/gw testDebugUnitTest -> exit 0, and the pre-existing 107 tests still + pass (only LiveSyncTest may be skipped) + ./tasks/gw recordPaparazziDebug -> exit 0 + +Also required: +- New unit tests with REAL assertions for: the classify() function per source + (2xx-with-record, 2xx-without-record, 404, 429, 500, malformed body), the 3x3 + LookupResult combination matrix, and the ScanOutcome throttle with an injected + clock. Assertion-free tests are a spec violation. +- A Paparazzi snapshot for the new LookupFailed sheet, and one for the camera + overlay showing a rejected-barcode message, in BOTH light and dark. Follow the + existing pattern in src/test/java/org/modg/bookshelf/ui/screens/ScanScreenPaparazziTest.kt. +- Check the build log for Kotlin warnings on 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. If you see it, you + have written dead code; fix it. + +## Report + +End with a plain report covering: + - what you changed, file by file + - the verbatim tail of each of the three gradle commands + - the test count before and after + - 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. A worker on this project has over-claimed success before, and the +orchestrator independently re-verifies everything, so an inflated report only +wastes a round trip. diff --git a/tasks/run-task.sh b/tasks/run-task.sh index 20dbf4e..7ed8280 100755 --- a/tasks/run-task.sh +++ b/tasks/run-task.sh @@ -16,6 +16,10 @@ export ANDROID_HOME="$HOME/toolchain/android-sdk" export ANDROID_SDK_ROOT="$ANDROID_HOME" export PATH="$JAVA_HOME/bin:$ANDROID_HOME/platform-tools:$PATH" export GRADLE_USER_HOME="$HOME/.gradle" +# Wave-4 post-mortem: `claude -p` terminates background tasks after 600s and ends +# the turn, so a worker that backgrounds a 10-minute Gradle build can never report +# on it. 0 = wait indefinitely. Worker prompts ALSO require foreground builds. +export CLAUDE_CODE_PRINT_BG_WAIT_CEILING_MS=0 cd "$CWD" || exit 1 # Stable session id so a killed run can be resumed rather than restarted.