172 lines
10 KiB
Plaintext
172 lines
10 KiB
Plaintext
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 "Sync design",
|
|
the PocketBase schema/rules, and the "setup"/"settings" screen lines. Do not
|
|
contradict it and do not edit it (the orchestrator updates SPEC after review).
|
|
2. 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-17 the user reported that every sync on their phone fails with
|
|
"Sync failed: HTTP 400". The orchestrator has fully diagnosed it from the live
|
|
server's request log. Do not repeat any of this:
|
|
|
|
- The phone reaches the server fine. Every request it sent carried NO valid auth
|
|
(PocketBase logged `auth: ""`). PocketBase does NOT reject an expired/invalid
|
|
`Authorization` token — it silently treats the request as anonymous. The
|
|
collection rules (`@request.auth.id != ""`) then turn writes away:
|
|
PATCH books/<id> -> 404 (the record exists; the rule hides it)
|
|
POST books -> 400 "Failed to create record." (create rule failure)
|
|
Reproduced over the internet with a garbage token: POST books -> 400.
|
|
- Cause: user auth tokens lasted PocketBase's default 5 days. The app logs in
|
|
once on the setup screen (`AuthRepository.login`) and NEVER refreshes the token,
|
|
and nothing handles an auth failure. Last good sync was 2026-09-13.
|
|
- Server side is ALREADY FIXED by the orchestrator (commit ab1d294): user tokens
|
|
now last 180 days. That is a backstop only; this wave is the app-side fix.
|
|
- Measured live against PocketBase v0.40.2 (you do not need to re-measure):
|
|
POST api/collections/users/auth-refresh with a VALID token
|
|
-> 200, body has the same shape as auth-with-password (`token`, `record`),
|
|
and the returned token is fresh (full 180 days).
|
|
POST api/collections/users/auth-refresh with a garbage/expired token, or none
|
|
-> 401 {"message":"The request requires valid record authorization token."}
|
|
|
|
### The data-loss consequence — this is the part that matters most
|
|
|
|
`SyncEngine.updateBook` (and `updateShelf`, `updateBookcase`) treat a 404 as "deleted
|
|
on the server" and HARD-DELETE the local row (SPEC: "On 404 for update/delete: drop
|
|
local record"). With the expired token, the 404 was the rule hiding a record that
|
|
still exists. So the user's failing sync hard-deleted FIVE books from the phone that
|
|
are intact on the server. They will not come back on their own: the pull cursor is
|
|
already past those rows' `updated`, so the incremental pull never sees them again.
|
|
|
|
The SPEC 404 rule is correct ONLY when the request was authenticated. This wave must
|
|
(a) make that true and (b) recover the rows already lost.
|
|
|
|
## What to build
|
|
|
|
### 1. Refresh the token at the start of every sync
|
|
|
|
- `data/remote/PocketBaseApi.kt`: add
|
|
`@POST("api/collections/users/auth-refresh") suspend fun authRefresh(): AuthResponse`.
|
|
`PbAuthInterceptor` already attaches the stored token; nothing to change there.
|
|
- `data/repo/SyncEngine.sync()`: after the existing `apiProvider.api()` null check and
|
|
BEFORE any push, call `authRefresh()`.
|
|
- 200 -> persist the new token (and user id) to `SettingsStore`, continue the sync.
|
|
The interceptor reads the store per request, so the rest of the sync uses it.
|
|
- HTTP 401 or 403 -> the session is gone. Clear the stored TOKEN ONLY (keep server
|
|
URL, user email and user id — see §3) and return a new
|
|
`SyncResult.AuthExpired`. Do NOT push or pull anything.
|
|
- IOException -> `SyncResult.Failure` as today. Offline is normal (SPEC:
|
|
offline-first); never clear the token because the network is down.
|
|
- anything else -> `SyncResult.Failure`, token untouched.
|
|
- Belt and braces: if ANY push/pull call later in the same sync gets HTTP 401 or 403,
|
|
stop and return `AuthExpired` the same way (clear token). Make sure such a
|
|
401/403 cannot be swallowed by the 404/409 handling in the create*/update* helpers.
|
|
- Put a KDoc comment on the 404 hard-delete helpers saying that the 404 is trusted
|
|
ONLY because `sync()` proved the token valid moments earlier, and why (the
|
|
2026-09-17 incident: an anonymous PATCH 404s on a record that exists). This codebase
|
|
comments with the evidence for a decision; match that.
|
|
- Coroutine cancellation: any catch you add or widen must rethrow
|
|
`CancellationException` first. Note the existing `catch (e: Exception)` in `sync()`
|
|
currently converts cancellation into a Failure — fix that too while you are there
|
|
(rethrow it), since you are restructuring that function.
|
|
|
|
### 2. Recover the rows already lost: a one-time full re-pull
|
|
|
|
- `SettingsStore`: a boolean flag (e.g. `full_repull_2026_09_17_done`), default false.
|
|
- `SyncEngine.sync()`: after a successful refresh, if the flag is false, clear the pull
|
|
cursors for bookcases, shelves and books before pulling, so this sync pulls
|
|
everything. Set the flag ONLY after the whole sync succeeds — a failed sync must
|
|
retry the full pull next time.
|
|
- ALSO: `AuthRepository.login` success clears all three pull cursors. Any fresh
|
|
sign-in (including the one the user is about to do after this bug) reconciles
|
|
fully. This is what protects against a recurrence we have not thought of.
|
|
- Confirm by reading `applyIncomingBook`/`shouldApplyRemote` that a full re-pull is
|
|
safe: a row missing locally is inserted; a local row with a pending local change is
|
|
NOT overwritten by an older remote. If it is not safe, STOP and say so in your
|
|
report rather than working around it.
|
|
|
|
### 3. The user is told, and can fix it in one tap
|
|
|
|
- `SettingsStore`: add a method that clears only the auth token. Keep the existing
|
|
`clearAuth()` (explicit sign-out) exactly as it is.
|
|
- Because the token is gone, `AuthRepository.isLoggedIn` becomes false and the app
|
|
already starts on the setup screen next launch (MainActivity). That is not enough —
|
|
the user is looking at the library when it happens:
|
|
- `LibraryViewModel.refreshSync` / `LibrarySyncPresenter`: a distinct outcome for
|
|
AuthExpired. The sync bar says something plain like
|
|
"Signed out — sign in again to sync", NOT "Sync failed". Tapping it (or an action
|
|
on it — match whatever the bar's existing affordances allow) navigates to setup.
|
|
- `SettingsViewModel.syncNow`: AuthExpired shows "Your session expired. Sign in
|
|
again." with a way to get to setup, not "Sync failed: HTTP 401".
|
|
- The setup screen PRE-FILLS the stored server URL and email when present, so
|
|
re-signing-in is typing the password only. Read `ui/setup/` and `ui/nav/` first;
|
|
reuse the existing route rather than inventing a second sign-in screen.
|
|
- Local data is NEVER touched by an auth expiry. Unsynced changes stay queued and go
|
|
up on the next sync after sign-in.
|
|
- Follow SPEC's "Design language". Add Paparazzi snapshots for the new library sync-bar
|
|
state and the settings message, light and dark, in the style of the existing ones,
|
|
and record them.
|
|
|
|
## 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. Everything here is doable with what is declared. If not, STOP and say so.
|
|
- DO NOT touch server/ or docs/. DO NOT restart or call the PocketBase service, and do
|
|
not read server/.dev-credentials. Unit tests use `FakePocketBaseApi` (extend it).
|
|
- DO NOT read, print, cat, grep or echo `app/local.properties`.
|
|
- DO NOT git commit, git add, or git push. Leave your work in the working tree.
|
|
- Build with `./tasks/gw <task>` — NEVER `./gradlew` directly.
|
|
- RUN BUILDS IN THE FOREGROUND and wait for them (1-3 minutes). Do not background a
|
|
build and end your turn.
|
|
- No emulator here (no KVM). You cannot run the app. Do not claim you did.
|
|
|
|
## Definition of done
|
|
|
|
First, BEFORE changing anything, run `./tasks/gw testDebugUnitTest` and record the
|
|
baseline test count summed from app/app/build/test-results/testDebugUnitTest/TEST-*.xml
|
|
(337 passed on 2026-09-17 before this wave; 2 skipped live tests). Then, after your changes:
|
|
|
|
./tasks/gw assembleDebug -> exit 0
|
|
./tasks/gw testDebugUnitTest -> exit 0, count UP from your baseline, no regressions
|
|
./tasks/gw recordPaparazziDebug -> exit 0
|
|
./tasks/gw verifyPaparazziDebug -> exit 0
|
|
|
|
Required new tests, with REAL assertions (assertion-free tests are a spec violation):
|
|
- sync calls authRefresh before any push/pull, and stores the refreshed token.
|
|
- refresh 401 -> `AuthExpired`; token cleared; server URL and email NOT cleared;
|
|
NO push or pull call made; a local PENDING_UPDATE book still exists afterwards.
|
|
THIS IS THE REGRESSION TEST FOR THE INCIDENT — make it look like the incident.
|
|
- refresh IOException -> `Failure`; token NOT cleared.
|
|
- a 401/403 from a push call mid-sync -> `AuthExpired`, and the book is NOT
|
|
hard-deleted.
|
|
- authenticated 404 on update still hard-deletes (SPEC behaviour preserved).
|
|
- one-time re-pull: a book present on the (fake) server with `updated` older than the
|
|
stored cursor, and absent locally, is restored by the first sync; the flag is set
|
|
after success; a sync whose pull fails leaves the flag unset.
|
|
- login success clears the pull cursors.
|
|
- `CancellationException` thrown inside sync propagates rather than becoming Failure.
|
|
- library and settings view models map AuthExpired to their new states.
|
|
- setup view model pre-fills stored URL and email.
|
|
|
|
Grep the build log for Kotlin warnings in files you touched. "Check for instance is
|
|
always 'false'" is NOT cosmetic on this project — it once hid a real bug.
|
|
|
|
## Report
|
|
|
|
End with a plain report covering:
|
|
- what you changed, file by file
|
|
- the verbatim tail of each gradle command above
|
|
- test count before and after, summed from the XML
|
|
- your own judgement: is there still ANY path where an unauthenticated request can
|
|
cause a local hard-delete or a lost pending change? Name it plainly if so.
|
|
- your reading of whether the full re-pull is safe (§2), with the line you relied on
|
|
- anything you did differently from these instructions, and why
|
|
- anything that looks wrong but was out of scope
|
|
|
|
Be honest. The orchestrator independently re-verifies everything; an inflated report
|
|
only wastes a round trip.
|