SPEC: lookup outcome is three-way; wave 5 worker prompt
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CDcPottghJXEvfYKqFM7zf
This commit is contained in:
Sprite
@@ -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 <task>` — 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.
|
||||
Reference in New Issue
Block a user