From 8c3cbcf928762f25304e59aa51cce24bd99d71a3 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Thu, 13 Aug 2026 15:30:37 +0200 Subject: [PATCH] docs: fix seven defects in the own-store plan Reviewed the plan against the code it describes. Two design holes and five errors. Instance identity was the real one. The plan deleted the materialised instances table without saying what replaces the instance row id, which TasksRepositoryImpl.updateTask passes to updateInstance and which ListsScreen keys a lazy list by. Two occurrences of one series can appear in the same list, so taskId alone is not unique and a hash of (taskId, start) can collide - as a Compose key that is a visible bug. Task.id is dropped for occurrenceStart, updateInstance takes (taskId, occurrenceStart, form), and External mode maps back to a real instance row with one query. This is the single seam change, and the plan's "TasksDataSource unchanged" claim was wrong. Local lists had no account name. TaskList.accountName is non-null, ListsViewModel groups by it and ListsScreen renders it as a section header, so a null account_id must still report "Local". The unique index was wrong: overrides share their master's UID, so unique (list_id, uid) would reject the rows the recurrence design depends on. It needs recurrence_id in the key. Phase 0 broke background reminders. It dropped our authority from ProviderChangeReceiver's manifest filter while the provider was still the store, and renamed StorageMode.LOCAL to OWN four phases before OWN meant Room. Both moved to phase 5. Parity against the provider was overclaimed: the provider materialises one occurrence, so multi-occurrence expansion has nothing to compare against and is tested against RFC 5545 directly. The phases sum to 6.5-7 weeks, not the 6-6.5 stated, and the difference from STORAGE-DECISION.md's 4.5-6 is now explained rather than left as a contradiction. Gaps closed: WAL vs Auto Backup (checkpoint on ON_STOP, sidecars in the backup rules, tested in phase 6), cascade rules for master_id and parent_id, Instant type converters, a rollback path that re-runs the import from tasks.db.imported, the release note for dropping the authority and its permissions, and ICalendarWriter.uidFor's synthesis branch becoming External-only. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 108 +++++++++++++++++ docs/OWN-STORE.md | 254 +++++++++++++++++++++++++++++++++------ docs/STORAGE-DECISION.md | 6 + 3 files changed, 332 insertions(+), 36 deletions(-) create mode 100644 CLAUDE.md diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..cb898b3 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,108 @@ +## On-device work (USB-connected phone) + +A physical phone is connected over USB. Rules: + +- **Install when asked** — if the user says to install/deploy on device, do it + (build + `adb install`). That's the one action you may take on your own. +- **Do nothing else on the device unprompted.** Do not launch the app, take + screenshots, dump/read logcat, poke UI, or otherwise "test" or verify on the + device on your own initiative — even to confirm a change works. +- Read logs, capture screenshots, and inspect on-device behaviour **only when the + user explicitly asks for it, each time.** The user drives when it's time to + look; wait for that instruction. + +## UI / design conventions + +- Material 3 Expressive throughout. Consult the `material-3` skill before + designing anything new; prefer M3 tokens/components over hardcoded colours. +- **Selection pickers are full-screen** — browse-style "choose one" surfaces + (visibility, reminder, recurrence rule, colour, calendar, add-field, plus the + Settings pickers) use floret-kit's `FullScreenPicker` / `OptionPicker` (a + full-bleed sheet with a pinned title bar and connected grouped rows); a picker + that needs a commit/extra action passes it via the picker's `actions` slot. + The exception is the **recurring scope choosers** — the "this / this & following + / all" prompts shown when you *save an edit to*, *drag* or *delete* a recurring + event — which stay compact `OptionCard`-in-`AlertDialog` popups (a quick 2–3 option + decision reads better as a popup than a near-empty full screen). `AlertDialog` + is otherwise only for plain confirmations. Radio/text-list dialogs are banned. + +## Releases + +The committed `versionCode` / `versionName` in `app/build.gradle.kts` **are the +release trigger**: merging a bumped `versionName` into `main` runs +`.gitea/workflows/release.yaml`, which builds, signs, publishes to the +self-hosted F-Droid repo, then mints the `vX.Y.Z` tag + release. `versionCode` is +pinned to `MAJOR*10000 + MINOR*100 + PATCH` (e.g. 2.13.0 → 21300). Full process +in `docs/RELEASING.md`. **Never tag/release UI changes before on-device review +and explicit go-ahead.** + +Release builds are kept **F-Droid reproducible** — `vcsInfo`, `dependenciesInfo`, +and the AGP metadata block are deliberately disabled in `build.gradle.kts`; don't +re-enable them. Use the `releaseTest` build type (R8-shrunk twin, debug-signed, +own applicationId suffix) to smoke-test a release candidate on-device. + +### Per-version changelogs are written by hand, in every locale + +`fastlane/metadata/android//changelogs/.txt` is the +"What's New" both F-Droid and Play publish. **There is no auto-translation +layer** — Weblate owns `values-*/strings.xml` only, not the fastlane tree — so +when cutting a release you write these files yourself, one per locale, each a +short summary under **500 characters** (Play's hard cap, applied per locale; +F-Droid truncates in-client). + +Write one for **every language the app ships** (`app/src/main/res/values-*`), +using store locale codes: `en-US`, `en-GB`, `de-DE`, `es-ES`, `fr-FR`, `it-IT`, +`pl-PL`, `pt-PT`, `ru-RU`, `zh-CN`, `ar`. Missing locales aren't fatal — both +stores fall back — but see the `en-GB` trap below. + +**`en-GB` is the Play Console's default language.** Play's fallback is the +*default locale*, not `en-US`, so a release with only an `en-US` changelog ships +with **no "What's New" at all** — this is why the latest release had none. +`en-GB` must exist. + +The rest of the plumbing is already locale-agnostic: `sync_changelog_to_fastlane.sh` +seeds `en-US` only and never overwrites a committed file, while +`fastlane_to_fdroid_localized.sh` and `supply` pick up every locale that has a +`changelogs/` dir. So the files are the whole job. + +## Forge / `tea` CLI + +**Codeberg is canonical** (`codeberg.org/jlmakiola/calendula`) for git, issues, +PRs, tags and releases — including the `floret-kit` submodule. The self-hosted +Gitea instance is **build infrastructure only**: signing key, F-Droid publishing, +release pipeline. + +Use the **`tea` CLI** for forge interaction — not raw API calls. Note the flag is +a *subcommand* flag, not global: `tea pulls list --login codeberg`, never +`tea --login codeberg pulls list`. Two accounts, neither default: + +- **Everything → `codeberg`** (user `jlmakiola`). PRs, issues, releases, repo + settings. `tea pulls create --login codeberg ...` +- **Build infra only → `jeanluc`** (`gitea.jeanlucmakiola.de`, user `makiolaj`). + Release-pipeline runs, Actions secrets. `tea ... --login jeanluc` + +Workflows are split by directory and this is load-bearing — Forgejo's lookup is +first-match-wins across `.forgejo/` → `.gitea/` → `.github/`: + +- `.forgejo/workflows/` runs on **Codeberg** (`ci.yaml`, `translations.yaml`) and + must reference **no secrets** — that's what makes fork PRs safe. +- `.gitea/workflows/` runs on **Gitea** (`release.yaml`, `renovate.yml`) and is + where every secret lives. + +Don't add a workflow without deciding which side it belongs on. + +## Translations + +Community translations are managed on a self-hosted **Weblate**, which owns all +`values-*` files (including German — API only, never hand-edit). Partial +translations are expected (`MissingTranslation` is informational, not fatal); +extra/stale keys stay fatal. + + +## Git Operations + +Use a commit format which references the issues, dont add any Co-Authered by Claude lines, and don't write extensive commit and merge messages, simple human ones suffice, for prs, stuff like testing etc, isnt interesting write what has chnaged, and if deviated from the underling issues pls explain why, add a closes issue line at the end of all prs. + +## Comments + +Don't add extensive code comments, methode discription, so as a Java Doc is fine, but no extensive explanbanitory conmments diff --git a/docs/OWN-STORE.md b/docs/OWN-STORE.md index 00ab459..48a9db9 100644 --- a/docs/OWN-STORE.md +++ b/docs/OWN-STORE.md @@ -30,8 +30,8 @@ BEFORE AFTER │ │ TasksRepository TasksRepository ← unchanged │ │ - TasksDataSource (interface) TasksDataSource ← unchanged - │ ╱ ╲ + TasksDataSource (interface) TasksDataSource ← one method + │ ╱ ╲ changes AndroidTasksDataSource RoomTasksDataSource AndroidTasksDataSource │ │ │ ContentResolver Room / SQLite ContentResolver @@ -42,10 +42,18 @@ BEFORE AFTER (deleted) tasks.org (external, unchanged) ``` -**Unchanged above the data layer.** `TasksRepository`, every ViewModel, every -screen. Verified: exactly one file outside `data/tasks` references -`TasksContract` (`domain/Models.kt`, for five constants), and it stops doing so -in phase 0. +**Unchanged above the data layer.** Every ViewModel, every screen. Verified: +exactly one file outside `data/tasks` references `TasksContract` +(`domain/Models.kt`, for five constants), and it stops doing so in phase 0. +Navigation addresses tasks by `taskId` throughout (`Destinations.kt:59`, +`TaskDetailScreen.kt:202,384`) — never by the instance id — so the change below +does not reach the UI. + +**One seam method changes.** `TasksDataSource.updateInstance(instanceId, form)` +becomes `updateInstance(taskId, occurrenceStart, form)`, and `Task` gains +`occurrenceStart: Instant?`. See *Instance identity* — this is the one place the +"nothing above the data layer changes" claim needed qualifying, and +`TasksRepositoryImpl.updateTask` is the only caller. **Deleted.** The `:provider` Gradle module, its manifest ``, its two custom permissions, its 84 Java files, its 13 translated string resources, and @@ -73,6 +81,12 @@ enum class StorageMode { and the `TaskProvider(isOwn = true)` case go with it: in `OWN` mode there is no authority, no ContentResolver and no permission to grant. +> **This rename happens in phase 5, not phase 0.** Between phases 1 and 4 both +> stores exist, so the enum carries `LOCAL` (dmfs), `OWN` (Room) and `EXTERNAL` +> simultaneously. Renaming `LOCAL` → `OWN` up front would make `OWN` mean *dmfs* +> for four phases and *Room* afterwards, which is exactly the kind of thing that +> gets misread six weeks later. + `ProviderResolver` narrows to what it was always really for — **discovering external providers** — and `ProviderStatus.READY` becomes unconditional in `OWN` mode. @@ -85,10 +99,30 @@ mode. ContentProvider. Users who need interop pick External mode, or wait for a possible read-only facade (explicitly out of scope — see *Deliberately not doing*). +- **Local lists still report an account name.** `TaskList.accountName` is a + non-null String that `ListsViewModel.kt:98` groups by and + `ListsScreen.kt:222` renders as a section header. `RoomTasksDataSource` maps + `account_id IS NULL` to `accountName = "Local"`, `accountType = + "local"`, so the existing grouping and `TaskList.isLocal` keep working with no + UI change. (`isLocal` moves off `TasksContract.LOCAL_ACCOUNT_TYPE` in phase 0 + and compares against a `domain` constant instead.) + + Knock-on, benign: `TaskEditViewModel.kt:101` picks the first *non*-local list + as the edit form's default. In `OWN` mode with no account configured every + list is local, so it falls through to `firstOrNull()`. Same practical result, + worth knowing before someone reports it as a bug. - **Auto Backup gets simpler and safer.** One Room file we control, with a documented restore path, instead of a provider database whose `cleanUpLists` routine could delete restored lists whose accounts no longer exist. + ⚠️ With one caveat that has to be handled, not assumed away: **Room enables + write-ahead logging by default**, and Auto Backup copies files without + checkpointing. A `-wal` sidecar can hold writes the backed-up `.db` does not. + We checkpoint (`PRAGMA wal_checkpoint(TRUNCATE)`) on `ON_STOP` and include + `.db`, `-wal` and `-shm` together in the backup rules, so a restore is + consistent either way. Phase 6 tests this, because "our backup is safer" is + the kind of claim that is worth exactly as much as its test. + --- ## The schema @@ -121,8 +155,14 @@ Four tables. Designed from Agendula's actual reads and writes plus RFC 5545's ### `tasks` -Master rows *and* recurrence overrides live here; an override is a row with -`recurrence_id` set and `parent_task_id` pointing at its master. +Master rows *and* recurrence overrides live here. An override is a row with +`recurrence_id` set and `master_id` pointing at its series master. + +> `master_id` and `parent_id` are different things and must not be conflated. +> **`parent_id`** is task hierarchy — a subtask's parent, the thing +> `RELATED-TO;RELTYPE=PARENT` carries. **`master_id`** is recurrence — which +> series an override belongs to. A row can have both: a subtask can itself +> recur. | Group | Columns | |---|---| @@ -150,8 +190,26 @@ own. This is what makes an honest round-trip possible, and it replaces the provider's `data0`–`data15` bag with something that cannot silently lose a field it has no column for. -Indices: `(list_id, is_deleted)`, `(parent_id)`, `(uid)` unique per list, -`(master_id, recurrence_id)`, `(is_dirty)`. +Indices: `(list_id, is_deleted)`, `(parent_id)`, `(master_id, recurrence_id)`, +`(is_dirty)`, and **unique on `(list_id, uid, recurrence_id)`**. + +> The unique index deliberately includes `recurrence_id`. An override **shares +> its master's UID** — that is what makes it an override rather than a separate +> task — so a unique index on `(list_id, uid)` alone would reject the very rows +> the recurrence design depends on. With `recurrence_id` NULL on the master and +> set on each override, the constraint says the right thing: one master and at +> most one override per occurrence, per UID, per list. + +**Cascades.** `master_id` is `ON DELETE CASCADE` — deleting a series deletes its +overrides, which would otherwise become unreachable rows that still sync. +`parent_id` is `ON DELETE SET NULL`: deleting a parent promotes its subtasks to +top level rather than destroying work the user did not ask to lose. `task_id` on +`task_alarms` cascades. + +**Type converters.** Every time column is `kotlin.time.Instant` in the entity +and INTEGER epoch-millis in SQLite, via one `@TypeConverter` pair. `status` and +`priority` convert through the existing `domain` enums, so `statusFromInt` / +`toInt()` keep their single home. ### `task_alarms` @@ -206,6 +264,43 @@ materialised table's entire class of staleness bugs never exists. - Client-side expansion is required for CalDAV regardless: server-side `CALDAV:expand` on `VTODO` is broken on every server `SYNC.md` targets. +### Instance identity + +Deleting the materialised `instances` table deletes the instance **row id**, and +two things use it today: + +- `TasksRepositoryImpl.updateTask` → `dataSource.updateInstance(current.id, …)` +- `ListsScreen.kt:422` → `items(results, key = { it.id })` + +The Compose key is the constraint that decides the design. Two occurrences of +one series can appear in the same list, so `taskId` alone is not unique, and a +hash of `(taskId, start)` folded into a `Long` can collide — which as a Compose +key is a visible bug, not a theoretical one. + +So we address occurrences by what they actually are: + +```kotlin +data class Task( + val taskId: Long, // the master row — unchanged, what navigation uses + val occurrenceStart: Instant?, // null for a non-recurring task + … +) + +fun updateInstance(taskId: Long, occurrenceStart: Instant, form: TaskForm) +``` + +`Task.id` is dropped; the Compose key becomes `"$taskId@${occurrenceStart}"`, +which is unique by construction and stable across reloads. + +**External mode absorbs this without loss.** `AndroidTasksDataSource` maps +`(taskId, occurrenceStart)` back to a real instance row with one query — +`WHERE task_id = ? AND instance_start = ?` — before writing through the +instances URI. One extra query on an operation the user performs by hand, in +exchange for a seam that does not depend on a foreign table's row ids. + +This is the **only** change to `TasksDataSource`, and +`TasksRepositoryImpl.updateTask` is its only caller. + ### Completing one occurrence of a recurring task The provider's `Detaching.java` implemented **model (d): detach the occurrence @@ -272,8 +367,25 @@ Rules that make this safe: local lists, with their `uid` preserved. Rare, but preserving the UID is what lets them be re-attached to an account later. -`tasks.db.imported` is excluded from Auto Backup; the new Room database is -included, which is the whole point of owning it. +`tasks.db.imported` is excluded from Auto Backup; the new Room database (with +its `-wal` and `-shm` sidecars) is included, which is the whole point of owning +it. + +### If the import goes wrong in production + +The rename is not just tidiness — it is the rollback. `tasks.db.imported` is a +complete, untouched dmfs database, so recovery does not need `:provider` to +still exist: + +1. `OneShotImport` can be re-run against `tasks.db.imported` as well as + `tasks.db`; the DataStore flag is clearable by a targeted fix release. +2. Re-import truncates the Room tables first and re-runs in one transaction, so + a second attempt is not a merge and cannot duplicate. +3. Only after a release with no import defects reported does a subsequent + version delete `tasks.db.imported`. + +This is the reason phase 5 (deleting `:provider`) ships *after* phase 4 rather +than with it — and the reason the deletion is its own release. --- @@ -308,15 +420,28 @@ every per-server quirk in the server-reality table. ### Phase 0 — Untangle (0.5 wk) -- `domain/Models.kt` stops importing `TasksContract`; the status and priority - constants move into `domain`. This is the last contract reference above the - data layer. -- `StorageMode`: `LOCAL` → `OWN`; `ProviderResolver` loses `own` / `isOwn`. -- `ProviderChangeReceiver`'s manifest filter drops our own authority. -- Add Room + KSP to the version catalog (KSP is already applied to `:app`). +- `domain/Models.kt` stops importing `TasksContract`; the status, priority and + local-account constants move into `domain`. This is the last contract + reference above the data layer. +- `Task.id` → `Task.occurrenceStart`; `updateInstance(taskId, occurrenceStart, + form)`. `AndroidTasksDataSource` gains the lookup query, so the *existing* + provider path exercises the new signature before Room ever does. +- Add `StorageMode.OWN` as a **third** value alongside `LOCAL` and `EXTERNAL`. +- Add Room + `room.schemaLocation` to the version catalog (KSP is already + applied to `:app` for Hilt). -**Done when:** the app still builds and behaves identically, with the provider -still present and still default. +Deliberately **not** here — both were in an earlier draft and both were wrong: + +- *Renaming `LOCAL` → `OWN`.* The provider is still the store until phase 4; + renaming now makes `OWN` mean dmfs for four phases and Room afterwards. + Phase 5. +- *Dropping our authority from `ProviderChangeReceiver`'s manifest filter.* That + receiver is what re-syncs reminders while the app is backgrounded + (`ProviderChangeReceiver.kt:47`). Removing the filter while the provider is + still live would silently stop background reminder updates. Phase 5. + +**Done when:** the app builds and behaves identically, provider still present, +still default, and the seam change is proven on the provider path. ### Phase 1 — Schema and DAOs (1 wk) @@ -341,19 +466,38 @@ The hard phase. Budget accordingly. a series with an override, a series with an exception, and an unbounded rule hitting the window ceiling. -**Done when:** `updateInstance` and recurring reads pass parity tests written -against the current provider's observed behaviour, *except* where model (a) -deliberately differs from model (d) — those differences enumerated as tests. +**Done when:** the expansion suite is green and single-occurrence editing forks +correctly. + +⚠️ **Parity against the provider is only partly available, and the earlier draft +of this plan overclaimed it.** The provider materialises exactly one upcoming +occurrence, so there is no multi-occurrence behaviour to compare against. The +split: + +| Behaviour | Reference | +|---|---| +| Multi-occurrence expansion | RFC 5545 §3.8.5 and `lib-recur` directly — **no provider parity exists** | +| The single next occurrence | provider parity, while it is still in-tree | +| Editing one occurrence (forking) | provider parity — *except* model (a) vs (d), enumerated as explicit difference tests | +| All-day and DST handling | provider parity | + +That partial availability is still the reason deletion is phase 5 rather than +phase 0. It is just not the blanket safety net it was described as. ### Phase 3 — Semantics parity (1 wk) - Completion coherence: `status` ↔ `percent_complete` ↔ `completed_at` ↔ closed, replacing `AutoCompleting.java` and — importantly — the reopen asymmetry that `TaskWriteMapper` currently works around in the app. -- Parent/child integrity, orphan handling on delete. +- Parent/child integrity: `parent_id` is `ON DELETE SET NULL`, so deleting a + parent promotes its subtasks rather than destroying them. - Validation: `DUE` xor `DURATION`, `due >= dtstart`, all-day pinned to UTC midnight, list must exist. -- Delete semantics: hard delete when `account_id IS NULL`, tombstone when set. +- Delete semantics: hard delete when `account_id IS NULL`, tombstone when set; + `master_id` cascades so a deleted series takes its overrides with it. +- `ICalendarWriter.uidFor`'s synthesis branch becomes dead on the Room path + (`uid` is NOT NULL). It stays for External, where UIDs really can be absent — + the KDoc gets updated to say which path each branch now serves. **Done when:** `TaskWriteMapper`'s provider-quirk workarounds are demonstrably unnecessary on the Room path (they stay for External). @@ -363,8 +507,9 @@ unnecessary on the Room path (they stay for External). - `OneShotImport` per the rules above, with tests over a fixture `tasks.db` captured from a real v0.3.x install. - `OWN` becomes the default for new installs and for upgraders after import. -- Backup rules updated: include the Room database, exclude `tasks.db.imported` - and the Keystore blob. +- Backup rules updated: include the Room database **and its `-wal`/`-shm` + sidecars**, exclude `tasks.db.imported` and the Keystore blob. WAL checkpoint + on `ON_STOP`. **Done when:** an upgrade from a v0.3.2 APK with seeded data lands every task, list and reminder in Room, verified by count and by content. @@ -374,9 +519,22 @@ list and reminder in Room, verified by count and by content. - Remove the module, its `settings.gradle.kts` include, its `:app` dependency, the three dmfs deps it pulled in, `provider/PROVENANCE.md`. - Add `lib-recur` (and `rfc5545-datetime`) directly to `:app`. +- `StorageMode`: `LOCAL` is deleted, `OWN` is what remains beside `EXTERNAL`. + `ProviderResolver` loses `own` / `isOwn`. +- `ProviderChangeReceiver`'s manifest filter drops our own authority — safe + *now*, because nothing of ours broadcasts `ACTION_PROVIDER_CHANGED` any more. + In `OWN` mode the in-app `InvalidationTracker` observer covers foreground + changes, and until sync exists nothing outside the app can change our data. + **When `SYNC.md` phase 3 lands, the sync worker must call + `ReminderScheduler.sync()` itself** — that is the replacement for the + broadcast, and it belongs in the sync work, not here. - Attribution screen: dmfs code is gone, but `lib-recur` stays and is Apache-2.0. `PROVENANCE.md` is replaced by a short note in `STORAGE-DECISION.md` recording that the fork existed and why it ended. +- **Release note, user-facing:** dropping the `` also drops the + `de.jeanlucmakiola.agendula.tasks` authority and both custom permissions. + Anyone who pointed DAVx5 or another app at that authority loses it silently — + it has to be called out in the release, with External mode as the answer. **Done when:** `./gradlew build` is green with `:provider` absent, and the APK declares no ContentProvider and no custom permissions. @@ -385,11 +543,32 @@ declares no ContentProvider and no custom permissions. - Room migration test infrastructure (`MigrationTestHelper`) wired up, so v1 → v2 is cheap when sync adds columns. -- Restore-path test: Auto Backup restore into a fresh install. +- Restore-path test: Auto Backup restore into a fresh install, **including the + WAL case** — write, background, restore, verify the last write survived. - Performance check at 5,000 tasks with 20 recurring series. -**Total: 6–6.5 weeks** to a shipping app with its own store, before any CalDAV -work begins. `SYNC.md`'s own estimate drops by 2.5–4 weeks in exchange. +### Total + +| Phase | | | +|---|---|---:| +| 0 | Untangle | 0.5 | +| 1 | Schema and DAOs | 1 | +| 2 | Recurrence | 1.5–2 | +| 3 | Semantics parity | 1 | +| 4 | Import and cutover | 1 | +| 5 | Delete `:provider` | 0.5 | +| 6 | Harden | 1 | +| | | **6.5–7 wk** | + +Against which `SYNC.md`'s own estimate drops by 2.5–4 weeks, so the net cost of +owning the store is roughly **+2.5 to +4.5 weeks** — before counting the bugs +that stop being unfixable. + +> This does not contradict `STORAGE-DECISION.md`'s 4.5–6 week figure; it +> supersedes it. That estimate costed only the new store (schema, expansion, +> semantics, import, tests) on the assumption `:provider` would be *kept* +> alongside it. This plan deletes the provider, which adds phase 0's untangling +> and phase 5's removal — work the earlier figure never had to include. --- @@ -405,8 +584,10 @@ work begins. `SYNC.md`'s own estimate drops by 2.5–4 weeks in exchange. The 93 existing app tests must stay green throughout. The 56 provider tests leave with the module in phase 5 — replaced, not abandoned: phases 2 and 3 owe -equivalent coverage of the behaviour those tests protected, and phase 2's -parity suite is written against them. +equivalent coverage of the behaviour those tests protected. Note the limit +recorded in phase 2: parity covers the single next occurrence, forking and +all-day/DST handling. Multi-occurrence expansion has no provider behaviour to +compare against and is tested against RFC 5545 and `lib-recur` directly. --- @@ -414,10 +595,11 @@ parity suite is written against them. | Risk | Mitigation | |---|---| -| **Recurrence is subtler than estimated** | Phase 2 is isolated and pure-JVM; it can overrun without blocking phases 3–4. The provider stays in-tree until phase 5, so we can always compare against it. | -| **Import loses a user's data** | Read-only source, single transaction, count verification, source file renamed not deleted, fixture-based tests. | -| **Regression in a behaviour nobody documented** | Phase 2's parity tests are written *against the provider while it is still present*. That is why deletion is phase 5, not phase 0. | -| **Losing third-party interop** | External mode covers users who need it. A read-only facade stays possible later; nothing in this design forecloses it. | +| **Recurrence is subtler than estimated** | The likeliest overrun, and the least mitigated — provider parity does not cover multi-occurrence expansion, so the reference is the RFC. Phase 2 is isolated and pure-JVM, so it can overrun without blocking phases 3–4. | +| **Import loses a user's data** | Read-only source, single transaction, count verification, source renamed not deleted, re-runnable from `tasks.db.imported`, fixture-based tests. | +| **Regression in a behaviour nobody documented** | Partial: phase 2 and 3 parity tests run against the provider while it is still present, for the behaviours where parity exists at all. That is why deletion is phase 5. | +| **A restore silently loses recent writes** | WAL checkpoint on `ON_STOP`, sidecars included in the backup rules, and a phase 6 test that exercises exactly this. | +| **Losing third-party interop** | External mode covers users who need it. Called out in the phase 5 release note. A read-only facade stays possible later; nothing here forecloses it. | | **Room + KSP build cost** | KSP is already in the build for Hilt; Room adds one processor. | --- diff --git a/docs/STORAGE-DECISION.md b/docs/STORAGE-DECISION.md index bf21c4d..13491e0 100644 --- a/docs/STORAGE-DECISION.md +++ b/docs/STORAGE-DECISION.md @@ -143,6 +143,12 @@ Two things make instance expansion less frightening than its line count: | Tests to parity with the current 93 + 56 | 1 wk | | **Net additional** | **4.5–6 wk** | +> ⚠️ Superseded by [`OWN-STORE.md`](OWN-STORE.md)'s **6.5–7 wk**. The figure +> above costed the new store only, on the assumption `:provider` would be kept +> beside it. The decision taken was to delete the provider, which adds the +> untangling (phase 0) and the removal (phase 5) that this estimate never had to +> include. The costing logic stands; the total does not. + ### What it removes from the sync plan Roughly sixteen of the phase-1 audit's storage findings are **provider-imposed**