From 62cd61e7078693159d3c215967a8e7f895c6ac12 Mon Sep 17 00:00:00 2001 From: Sprite Date: Thu, 17 Sep 2026 01:39:41 +0000 Subject: [PATCH] Plan wave 11 (M-authexpiry): refresh token each sync, recover lost rows wave-chain.sh: commit subject now comes from tasks/.subject instead of being hard-coded for waves 9/10; drop a stale session trailer. Co-Authored-By: Claude Opus 5 --- tasks/M-authexpiry.subject | 1 + tasks/M-authexpiry.txt | 171 +++++++++++++++++++++++++++++++++++++ tasks/wave-chain.sh | 3 +- 3 files changed, 173 insertions(+), 2 deletions(-) create mode 100644 tasks/M-authexpiry.subject create mode 100644 tasks/M-authexpiry.txt diff --git a/tasks/M-authexpiry.subject b/tasks/M-authexpiry.subject new file mode 100644 index 0000000..4616c6e --- /dev/null +++ b/tasks/M-authexpiry.subject @@ -0,0 +1 @@ +refresh auth token every sync; recover rows lost to the expired-token sync diff --git a/tasks/M-authexpiry.txt b/tasks/M-authexpiry.txt new file mode 100644 index 0000000..1866388 --- /dev/null +++ b/tasks/M-authexpiry.txt @@ -0,0 +1,171 @@ +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/ -> 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 ` — 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 +(about 308+ expected; 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. diff --git a/tasks/wave-chain.sh b/tasks/wave-chain.sh index 57d5efb..a1e575a 100755 --- a/tasks/wave-chain.sh +++ b/tasks/wave-chain.sh @@ -139,7 +139,7 @@ EOF COST="$(jq -r '"$" + ((.total_cost_usd//0)|tostring) + ", " + ((.num_turns//0)|tostring) + " turns"' "$L/$T.json" 2>/dev/null)" git add -A git commit -q -F - < -Claude-Session: https://claude.ai/code/session_01RWoivrRUEmJLFFwbsrGkqQ EOF SHA="$(git rev-parse --short HEAD)" if timeout 120 git push -q origin HEAD 2>>"$CLOG"; then PUSHED="pushed"; else PUSHED="PUSH FAILED (see wave-chain.log)"; fi