From 13cb27b2ab0a36143ddd0be9a5468d29b3f27852 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Thu, 13 Aug 2026 14:46:07 +0200 Subject: [PATCH] docs: decide to build our own store and delete the vendored provider The vendored dmfs provider was kept on the grounds that it hands us the sync bookkeeping for free. The phase-1 sync audit measured that bookkeeping and found most of it broken, absent, or unusable: _DIRTY not set on delete, no home for a per-collection sync token, read-only collections inexpressible, ACCOUNT_TYPE write-once so enabling sync is a full migration, and cleanUpLists able to delete a user's lists after a backup restore. Sixteen findings are provider-imposed rather than platform- or protocol-imposed. Costing the alternative showed the swap is far smaller than assumed. TasksDataSource is already a 14-method, domain-shaped interface; exactly one file above the data layer references TasksContract. The work is a second implementation behind an interface built for it, not a rewrite. Against ~5 weeks to build, owning the store removes 2.5-4 weeks from the sync plan, and 8,200 of the vendored 14,555 lines are things we would never write - 23 migrations from a 2013 schema, 798 lines of full-text search the app has zero call sites for, and 1,581 lines of a type-safe layer over ContentValues that Room deletes. External mode (OpenTasks, tasks.org) is unaffected and keeps every file that describes somebody else's schema. STORAGE-DECISION.md is the reasoning; OWN-STORE.md is the architecture and the six-phase plan. :provider stays in-tree until phase 5 so recurrence parity can be tested against it before it goes. Also corrected here: the provider's JVM test count (51 -> 56, measured from the test-results XML) and a fourth site of the debunked "switching sync on is never a migration" claim, in StorageMode.kt. Co-Authored-By: Claude Opus 5 (1M context) --- .../agendula/data/tasks/StorageMode.kt | 11 +- .../agendula/domain/export/ICalendarWriter.kt | 23 +- docs/ARCHITECTURE.md | 18 +- docs/OWN-STORE.md | 439 ++++++ docs/README.md | 19 +- docs/ROADMAP.md | 20 +- docs/STORAGE-AND-SYNC.md | 58 +- docs/STORAGE-DECISION.md | 246 ++++ docs/SYNC.md | 1185 +++++++++++++++++ provider/PROVENANCE.md | 4 +- 10 files changed, 1984 insertions(+), 39 deletions(-) create mode 100644 docs/OWN-STORE.md create mode 100644 docs/STORAGE-DECISION.md create mode 100644 docs/SYNC.md diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/StorageMode.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/StorageMode.kt index 85789d7..7105f46 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/StorageMode.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/StorageMode.kt @@ -5,9 +5,14 @@ package de.jeanlucmakiola.agendula.data.tasks * * Only two values, though the document describes three modes. **Synced is not a * third store**: it is [LOCAL] with an account attached, so it is derived state - * (does an account of ours exist?) rather than something the user picks. That - * also means switching sync on is never a migration. Adding a `SYNCED` constant - * here would imply otherwise. + * (does an account of ours exist?) rather than something the user picks. Adding + * a `SYNCED` constant here would imply otherwise. + * + * ⚠️ This used to add "so switching sync on is never a migration". That is wrong. + * `TaskLists.ACCOUNT_TYPE` is write-once in the provider — `processors/lists/ + * Validating.java:68-76` throws `IllegalArgumentException` on any attempt to + * change it — so attaching an account to an existing local list means recreating + * every list and every task under the new account. See `docs/SYNC.md`. * * Nothing above the data layer reads this; it selects an authority for * [ProviderResolver] and stops there. diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/domain/export/ICalendarWriter.kt b/app/src/main/java/de/jeanlucmakiola/agendula/domain/export/ICalendarWriter.kt index e310bdf..520fa7a 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/domain/export/ICalendarWriter.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/domain/export/ICalendarWriter.kt @@ -57,12 +57,23 @@ object ICalendarWriter { /** * The UID to write for [task]. * - * Local tasks have none: the dmfs provider only permits a sync adapter to - * assign `_uid`, so in Local mode every task arrives here with `uid == null`. - * A VTODO without a UID is invalid and, worse, un-mergeable — re-importing a - * backup would duplicate every task instead of matching it. So we synthesise - * one from the row id, which is stable for as long as the row is, and tag it - * with our own domain so a synthesised UID is recognisable as such. + * Local tasks have none, because nothing assigns one: the provider never + * generates a `_uid` itself, and our write path does not set it either, so in + * Local mode every task arrives here with `uid == null`. + * + * Note this is a gap we leave open, not one the provider imposes. + * `processors/tasks/Validating.java:92-96` restricts `_uid` to sync adapters + * on *update* only; `insert` does not check it, so any caller may assign a UID + * at creation. Doing that would be strictly better than synthesising here — + * see `docs/SYNC.md`, where it is a phase-1 item, because a real UID minted at + * creation is what lets a local task later be pushed to CalDAV without + * duplicating. + * + * Until then: a VTODO without a UID is invalid and, worse, un-mergeable — + * re-importing a backup would duplicate every task instead of matching it. So + * we synthesise one from the row id, which is stable for as long as the row + * is, and tag it with our own domain so a synthesised UID is recognisable as + * such. */ fun uidFor(task: ExportTask): String = task.uid?.takeIf { it.isNotBlank() } ?: "agendula-${task.taskId}@jeanlucmakiola.de" diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 4fadb62..36d125e 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -22,7 +22,7 @@ the original "owns no database" thesis, settled in installed made someone else's roadmap a gate on the app working at all. The database is the vendored dmfs provider under *our* authority — our namespace, not a schema written from scratch — so every CalDAV engine still understands it. A -sync adapter of our own is the 1.x arc. +sync adapter of our own is the 1.x arc, designed in [`SYNC.md`](SYNC.md). The whole design hangs off one rule: @@ -143,9 +143,15 @@ The platform calls sit behind `ProviderEnvironment` so this decision is unit tested on the JVM (`ProviderResolverTest`) rather than only on a device. **Storage modes** are `LOCAL` and `EXTERNAL` only. `STORAGE-AND-SYNC.md` -describes three, but *Synced* is not a third store — it is Local with an account -attached, so it is derived state, and modelling it as a separate mode would imply -that turning sync on is a migration. It isn't. +describes three, but *Synced* is not a third store — it is the same store with an +account attached, so it stays derived state. + +⚠️ **What this used to say — "so turning sync on is not a migration" — is wrong.** +A task list's `ACCOUNT_NAME`/`ACCOUNT_TYPE` are write-once in the provider +(`processors/lists/Validating.java:68-76`), so local lists cannot be re-pointed at +an account; tasks have to be moved into new lists. Not modelling `SYNCED` as a +mode is still right, but the migration it implied away is real. See +[`SYNC.md`](SYNC.md). ### 4.2 `TasksContract` @@ -261,7 +267,9 @@ resolver, so vendoring an entire content provider **changed no UI, no ViewModel, no domain type, and not one line of `TasksRepository`.** Bundling the provider bundles **storage, not sync**. Our own sync adapter is a -separate, later piece of work — see `STORAGE-AND-SYNC.md`. +separate, later piece of work — designed in [`SYNC.md`](SYNC.md), and it lands +*underneath* this same seam: it writes through `TaskContract` with +`CALLER_IS_SYNCADAPTER`, so the layers above it stay untouched a second time. --- diff --git a/docs/OWN-STORE.md b/docs/OWN-STORE.md new file mode 100644 index 0000000..00ab459 --- /dev/null +++ b/docs/OWN-STORE.md @@ -0,0 +1,439 @@ +# Agendula's own task store — architecture and plan + +**Branch:** `feat/own-store` +**Decision:** taken. `docs/STORAGE-DECISION.md` costed it; this is the build. +**Supersedes:** the "keep `:provider`" position in `STORAGE-AND-SYNC.md` and the +"Settled" section of `SYNC.md`. + +--- + +## The decision, in one paragraph + +Agendula stops vendoring the dmfs OpenTasks provider. The `:provider` module — +14,555 lines of Java, 1.66× the size of the app itself — is **deleted**. In its +place the app gets its own Room database, designed for the two things Agendula +actually does: show tasks, and sync them over CalDAV. Support for *external* +providers (OpenTasks, tasks.org) **stays**, unchanged, as a user choice — so +anyone already syncing through DAVx5 keeps working exactly as they do today. + +Agendula becomes a normal Android app with a normal database, plus an optional +compatibility path into somebody else's ContentProvider. + +--- + +## What changes and what does not + +``` +BEFORE AFTER + + UI / ViewModels UI / ViewModels + │ │ + TasksRepository TasksRepository ← unchanged + │ │ + TasksDataSource (interface) TasksDataSource ← unchanged + │ ╱ ╲ + AndroidTasksDataSource RoomTasksDataSource AndroidTasksDataSource + │ │ │ + ContentResolver Room / SQLite ContentResolver + │ │ │ + ┌────┴─────┐ our tables ┌──────┴──────┐ + │ │ │ │ +:provider OpenTasks OpenTasks tasks.org +(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. + +**Deleted.** The `:provider` Gradle module, its manifest ``, its two +custom permissions, its 84 Java files, its 13 translated string resources, and +its three dmfs runtime dependencies from `:provider`'s own build file. + +**Kept for External mode.** `TasksContract.kt`, `ColumnReader.kt`, +`TaskMapper.kt`, `TaskWriteMapper.kt`, `AndroidTasksDataSource.kt`, +`ProviderResolver.kt`, `ProviderEnvironment.kt`, `TaskProjections.kt`. These +describe *somebody else's* schema and are exactly right for that job. + +--- + +## Storage modes after the change + +```kotlin +enum class StorageMode { + /** Agendula's own Room database. The default; always available. */ + OWN, + /** A tasks provider app already installed — OpenTasks, tasks.org. */ + EXTERNAL, +} +``` + +`LOCAL` (meaning "our bundled dmfs provider") is gone. `ProviderResolver.own` +and the `TaskProvider(isOwn = true)` case go with it: in `OWN` mode there is no +authority, no ContentResolver and no permission to grant. + +`ProviderResolver` narrows to what it was always really for — **discovering +external providers** — and `ProviderStatus.READY` becomes unconditional in `OWN` +mode. + +### Consequences worth stating plainly + +- **No runtime permission is needed for the default path.** Today's permission + prompt only ever applied to External mode; now that is visibly true. +- **Third-party apps can no longer read Agendula's tasks.** We publish no + ContentProvider. Users who need interop pick External mode, or wait for a + possible read-only facade (explicitly out of scope — see *Deliberately not + doing*). +- **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. + +--- + +## The schema + +Four tables. Designed from Agendula's actual reads and writes plus RFC 5545's +`VTODO`, not inherited from a 2013 schema. + +### `task_lists` + +| Column | Type | Notes | +|---|---|---| +| `id` | INTEGER PK | | +| `name` | TEXT NOT NULL | | +| `color` | INTEGER NOT NULL | ARGB | +| `account_id` | INTEGER NULL | FK → `accounts`, NULL = device-only | +| `is_visible` | INTEGER NOT NULL | default 1 | +| `is_synced` | INTEGER NOT NULL | default 1 | +| `owner` | TEXT NULL | CalDAV owner display name | +| `is_read_only` | INTEGER NOT NULL | **new** — the provider could not express this at all | +| `sort_order` | INTEGER NOT NULL | user ordering, which the provider also lacked | +| `href` | TEXT NULL | collection URL, relative to the account root | +| `ctag` | TEXT NULL | | +| `sync_token` | TEXT NULL | RFC 6578, **per collection** — not squatted into a shared slot | +| `is_dirty` | INTEGER NOT NULL | a real boolean, not dmfs's monotonic counter | + +> `account_id` being nullable is the single most important schema change. In the +> dmfs provider `ACCOUNT_TYPE` is write-once and throws on change, which made +> "turn sync on" a full data migration. Here, attaching a local list to an +> account is `UPDATE task_lists SET account_id = ?`. + +### `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. + +| Group | Columns | +|---|---| +| identity | `id`, `list_id`, `uid` (NOT NULL, minted at creation), `href`, `etag` | +| content | `title`, `description`, `location`, `url`, `color` | +| state | `status`, `percent_complete`, `completed_at`, `priority`, `classification` | +| time | `dtstart`, `due`, `duration`, `is_all_day`, `timezone` | +| recurrence | `rrule`, `rdate`, `exdate`, `recurrence_id`, `master_id` | +| hierarchy | `parent_id`, `sort_order` | +| audit | `created_at`, `last_modified`, `sequence` | +| sync | `is_dirty`, `is_deleted`, `unknown_properties` | + +Two entries deserve explanation. + +**`uid` is NOT NULL and assigned at creation.** Every task gets a real +RFC 4122 UUID the moment it is inserted, in every mode, synced or not. This +closes the gap `ICalendarWriter.uidFor` currently papers over by synthesising +`agendula-@…`, and it means any local task can later be pushed to a +server without duplicating. The provider never assigned one. + +**`unknown_properties`** holds the raw unfolded iCalendar lines of every +property we do not model — `ATTENDEE`, `CATEGORIES`, `X-*`, `GEO`, and anything +a future RFC adds. On write we re-emit them verbatim after the properties we do +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)`. + +### `task_alarms` + +| Column | Notes | +|---|---| +| `id`, `task_id` | FK, `ON DELETE CASCADE` | +| `minutes_before` | positive = before the reference | +| `reference` | `DUE` or `START` | +| `message` | optional | + +Replaces `AlarmHandler` (133 lines of Java) and the `dataN` slot convention. +Delete-and-reinsert stops being necessary — the provider's re-validate-everything +behaviour was the only reason `setAlarm` worked that way. + +### `accounts` + +| Column | Notes | +|---|---| +| `id`, `display_name`, `principal_url`, `home_set_url` | +| `username` | the app password is **not** here — Keystore only, per `SYNC.md` | +| `last_sync_at`, `last_sync_error` | + +Not populated until `SYNC.md` phase 2, but the FK exists from v1 so enabling +sync never requires a schema migration. + +--- + +## Recurrence: expand at read, not on write + +The provider maintained a materialised `instances` table, recomputed by +`Instantiating.java` on every write — and still only ever materialised **one** +upcoming occurrence. + +Agendula expands lazily instead: + +``` +tasks (masters + overrides) ──► RecurrenceExpander ──► List + rrule/rdate/exdate (lib-recur, in memory) occurrences +``` + +This is the right call here because **the repository already filters and sorts +in Kotlin, not SQL**. `TasksRepositoryImpl.loadTasks` reads the whole set, +applies `TaskFiltering.matches`, then `TaskSorting.DEFAULT`. Nothing depends on +the database being able to order by instance time, so nothing is lost — and a +materialised table's entire class of staleness bugs never exists. + +- Bounded window: expansion is capped (default: 1 year back, 2 years forward, + hard ceiling of N occurrences per series) so an unbounded `RRULE` cannot hang + the UI. +- `lib-recur` **pinned at 0.12.2** — 0.16.0 removed `RecurrenceSet`. We pin + because we chose to, and the pin is now ours to lift on our own schedule. +- Client-side expansion is required for CalDAV regardless: server-side + `CALDAV:expand` on `VTODO` is broken on every server `SYNC.md` targets. + +### Completing one occurrence of a recurring task + +The provider's `Detaching.java` implemented **model (d): detach the occurrence +as a brand-new task with its own UID**. We inherited that without ever choosing +it, and it is the model least compatible with CalDAV. + +**We implement model (a): a `RECURRENCE-ID` override.** Completing one +occurrence writes a second `tasks` row with the same `uid`, a `recurrence_id` +naming the occurrence, `master_id` pointing at the series, and the completed +state. This is what RFC 5545 specifies and what every other CalDAV client +expects to receive. + +This decision is now made explicitly, recorded here, and testable. + +--- + +## Reactivity + +`TasksDataSource.registerObserver(onChange: () -> Unit): AutoCloseable` **stays +as-is**. The Room implementation backs it with `InvalidationTracker.Observer` +over the four tables; the External implementation keeps its `ContentObserver`. +One interface, two mechanisms, `TasksRepositoryImpl.observing()` untouched. + +Going Flow-native in the DAOs is a later, optional refinement. Doing it now +would change the interface and therefore the External path, for no user-visible +gain. + +--- + +## Migrating existing users + +Anyone on v0.3.x has their tasks inside the bundled provider's SQLite file at +`/data/data/de.jeanlucmakiola.agendula/databases/tasks.db` (dmfs schema +version 23). Removing the Gradle module does **not** remove that file — an app +update leaves the data directory intact. + +So the migration reads the file directly, with no provider and no +ContentResolver involved: + +``` +OneShotImport + 1. does databases/tasks.db exist? no → nothing to do, mark done + 2. open SQLiteDatabase.OPEN_READONLY + 3. read tasklists → task_lists (account_type LOCAL → account_id NULL) + 4. read tasks → tasks (skip _deleted = 1; mint uid where NULL) + 5. read properties → task_alarms (mimetype = …/alarm only) + 6. verify counts, inside one Room transaction + 7. record completion in DataStore + 8. rename tasks.db → tasks.db.imported (kept one release, then deleted) +``` + +Rules that make this safe: + +- **Read-only, single transaction, verified counts.** Either the whole import + lands or none of it does. +- **The source file is renamed, never deleted**, for one release. If the import + is wrong we can still recover from a user's device. +- **Idempotent.** Guarded by a DataStore flag *and* by the rename, so a crash + mid-import cannot double-import. +- **Runs before first UI read**, gated the same way `StorageModeHolder.awaitReady()` + already gates the launch reminder re-sync. +- Tasks that were in an *external* account inside our bundled provider (only + possible if the user had pointed DAVx5 at our authority) are imported as + 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. + +--- + +## Effects on the sync plan + +`SYNC.md`'s phase list was written against the provider. Owning the store +deletes work from it outright: + +| `SYNC.md` item | Fate | +|---|---| +| "Assign UIDs at creation" (phase 0) | **gone** — `uid` is NOT NULL from v1 | +| Auto Backup / `cleanUpLists` data-loss guard (phase 0) | **gone** — no `cleanUpLists` | +| `lib-recur` pin rationale (phase 0) | reduced to a normal version choice | +| Local → Synced migration (phase 3) | **gone** — `account_id` is a nullable FK | +| ETag / href / CTag squats into `SYNC1`–`SYNC8` | **gone** — real columns | +| Per-collection sync token (phase 3) | **gone** — real column | +| `_DIRTY` set-on-delete workaround (phase 3) | **gone** — tombstones are ours | +| `CALLER_IS_SYNCADAPTER` ignored by instances URI | **gone** — no URIs | +| `Moving` dual-UID collision (phase 4) | **gone** | +| Read-only collections (phase 4) | now *possible* — `is_read_only` exists | +| Recurring-completion model | **decided here** — RECURRENCE-ID override | +| Byte-stable round-trip | improved — `unknown_properties` preserves the rest | + +Everything platform-level and protocol-level in `SYNC.md` is untouched: the +`targetSdk 34` sync-framework gate, the stub sync-adapter pattern, credential +storage, Play compliance, discovery, conditional `PUT`, conflict policy, and +every per-server quirk in the server-reality table. + +--- + +## Plan + +### 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`). + +**Done when:** the app still builds and behaves identically, with the provider +still present and still default. + +### Phase 1 — Schema and DAOs (1 wk) + +- The four entities above, plus DAOs, plus `schemas/` exported for migration + testing (`room.schemaLocation`, committed). +- `RoomTasksDataSource` implementing all 14 `TasksDataSource` methods except the + recurrence-dependent ones, which throw until phase 2. +- `DataModule` binds by `StorageMode`. + +**Done when:** a JVM test creates lists and non-recurring tasks through +`TasksDataSource` against an in-memory Room database and reads them back. + +### Phase 2 — Recurrence (1.5–2 wk) + +The hard phase. Budget accordingly. + +- `RecurrenceExpander` over `lib-recur`: `RRULE`, `RDATE`, `EXDATE`, overrides, + all-day handling, bounded window, `distanceFromCurrent`. +- `RECURRENCE-ID` override creation on single-occurrence edit and completion. +- A test suite that is the deliverable, not an afterthought: daily/weekly/ + monthly-by-day/yearly, `COUNT` and `UNTIL`, DST boundaries, all-day series, + 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. + +### 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. +- 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. + +**Done when:** `TaskWriteMapper`'s provider-quirk workarounds are demonstrably +unnecessary on the Room path (they stay for External). + +### Phase 4 — Import and cutover (1 wk) + +- `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. + +**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. + +### Phase 5 — Delete `:provider` (0.5 wk) + +- 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`. +- 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. + +**Done when:** `./gradlew build` is green with `:provider` absent, and the APK +declares no ContentProvider and no custom permissions. + +### Phase 6 — Harden (1 wk) + +- 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. +- 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. + +--- + +## Testing posture + +| Layer | How | +|---|---| +| Entities, DAOs, migrations | Room in-memory + `MigrationTestHelper`, JVM | +| `RecurrenceExpander` | pure JVM, no Android — the largest suite | +| Semantics (completion, hierarchy, validation) | JVM through `TasksDataSource` | +| `OneShotImport` | fixture `tasks.db` committed as a test resource | +| External mode | unchanged; existing `TaskMapper` / `TaskWriteMapper` tests stay | + +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. + +--- + +## Risks + +| 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. | +| **Room + KSP build cost** | KSP is already in the build for Hilt; Room adds one processor. | + +--- + +## Deliberately not doing + +- **An exported ContentProvider facade over Room.** Possible later (~1–1.5 wk), + not now. Shipping one would recreate the public-API surface whose validation + and URI plumbing is most of what we are deleting. +- **A domain-native schema.** The table shapes above stay recognisably close to + `TaskContract` where `TaskContract` was right, because it is a proven design + for `VTODO` and because it keeps a future facade cheap. +- **Flow-native DAOs.** Later refinement; changes the interface for no + user-visible gain today. +- **FTS / search.** The provider carried 798 lines of it. The app has never + called it. If search is wanted it is a feature request, designed on its own + terms. +- **Categories and attendees as first-class tables.** They round-trip through + `unknown_properties` until a feature actually needs them. diff --git a/docs/README.md b/docs/README.md index eb146c7..b672819 100644 --- a/docs/README.md +++ b/docs/README.md @@ -1,8 +1,10 @@ # Agendula — documentation -Agendula is a Material 3 Expressive **task** app for Android: a pure front-end over -the OpenTasks `TaskContract` provider (synced by DAVx5 / SmoothSync / DecSync -over CalDAV), with no own database or sync stack. Sibling to +Agendula is a Material 3 Expressive **task** app for Android. It **carries its +own task store** — the dmfs task provider vendored under our own authority — so +it is complete and local-first with nothing else installed; an external provider +(OpenTasks / tasks.org, synced by DAVx5 / SmoothSync / DecSync) is a user choice +rather than a requirement, and our own CalDAV sync is the 1.x arc. Sibling to [Calendula](https://codeberg.org/jlmakiola/calendula). See the top-level [`../README.md`](../README.md) for the project pitch. @@ -12,15 +14,24 @@ top-level [`../README.md`](../README.md) for the project pitch. |---|---| | [`ARCHITECTURE.md`](ARCHITECTURE.md) | How Agendula is built **today** — layers, the data seam, provider resolution, the reminder engine, DI, build/tooling, manifest. Start here to work on the code. | | [`ROADMAP.md`](ROADMAP.md) | **Status** and what's next — milestones (M0–M6 + Posture B), what's done, open decisions, how to build/verify. | +| [`STORAGE-AND-SYNC.md`](STORAGE-AND-SYNC.md) | **Where task data lives** — the decision to ship our own provider, the storage modes, permissions, distribution, and the dead ends. Supersedes `PLAN.md` on storage. | +| [`SYNC.md`](SYNC.md) | **How data reaches a server** — the CalDAV sync adapter: the VTODO ↔ `TaskContract` mapper, Nextcloud sign-in, the engine, libraries and their licenses. Step 5 of `STORAGE-AND-SYNC.md`. | +| [`STORAGE-DECISION.md`](STORAGE-DECISION.md) | **Keep the vendored provider, or build our own?** The measured cost of both. **Decided: build our own.** | +| [`OWN-STORE.md`](OWN-STORE.md) | **Agendula's own Room store** — the schema, recurrence design, migration off the vendored provider, and the six-phase plan that deletes `:provider`. Supersedes the "keep the provider" position in `STORAGE-AND-SYNC.md`. | | [`PLAN.md`](PLAN.md) | The original implementation plan and **design rationale** — the A-now-B-later thesis, what transfers from Calendula, the locked decisions. The "why". | | [`RELEASING.md`](RELEASING.md) | How to cut a release — the git-tag-as-source-of-truth flow, CI jobs, F-Droid repo, required secrets. | +| [`../provider/PROVENANCE.md`](../provider/PROVENANCE.md) | What the vendored `:provider` module is, where it came from, and **every** deviation from upstream dmfs. | Also: [`../CHANGELOG.md`](../CHANGELOG.md) (Keep a Changelog format; tag sections feed the release notes). ## How the docs relate -- **PLAN** is the design decisions (mostly stable; the "why"). +- **PLAN** is the original design decisions (the "why"), left as the historical + record. On storage it is **superseded by STORAGE-AND-SYNC**. +- **STORAGE-AND-SYNC** and **SYNC** are the standing decision documents: the + first settles where data lives, the second how it syncs. Both record rejected + alternatives on purpose, so decisions don't get relitigated. - **ARCHITECTURE** is the current shape of the code (kept in sync with the source as it grows). - **ROADMAP** is the moving status layer (update as milestones land). diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 81839d0..564f999 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -142,7 +142,7 @@ squatting `org.dmfs.tasks`, which is a dead end). prune account types we authenticate ourselves — the deletion is unsafe without that rework. Modernized to minSdk 29 / targetSdk 36 / Java 17. [`provider/PROVENANCE.md`](../provider/PROVENANCE.md) records every deviation, - each also marked `AGENDULA CHANGE` at the site. Upstream's 51 JVM tests pass. + each also marked `AGENDULA CHANGE` at the site. Upstream's 56 JVM tests pass. - ✅ Storage modes + the permission-gate bypass — `ProviderStatus.NEEDS_PERMISSION` can no longer fire in Local mode, and an upgrading Posture A user stays on the provider that holds their data (`ProviderResolver.autoMode`). @@ -152,9 +152,12 @@ squatting `org.dmfs.tasks`, which is a dead end). - ⬜ **Frontend surfaces for the above** — a storage-mode picker in Settings and an export screen. The backend is done and unused until these exist. - ⬜ File the DAVx5 issue (step 4) — non-blocking, cheap, serves F-Droid users. -- ⬜ Sync adapter (step 5) — the 1.x arc. Design discussion still open: protocol - coverage, account model, conflict resolution, and `ical4android`'s licence - against our MIT. +- ⬜ Sync adapter (step 5) — the 1.x arc. **Designed in [`SYNC.md`](SYNC.md)**, + not started: mapper → auth → engine → hardening, ~8–9 weeks. The account model + is settled (`AccountManager`) and `ical4android` is closed out (superseded by + `synctools`, GPLv3, so we write the mapper in-house); what's still open is + dav4jvm's JitPack-only distribution, conflict policy, and whether External mode + survives the milestone. - ⬜ Verify on a device: the local path with no account, and the vendored provider's timezone-change behaviour (change 3 in `PROVENANCE.md`). @@ -168,7 +171,9 @@ These carry over from [`PLAN.md`](PLAN.md) §9; resolved ones are struck through 2. ~~**tasks.org provider authority**~~ verified on device: `org.tasks.opentasks` + `org.tasks.permission.*`. 3. **jtx Board** — support its richer contract later, or stay OpenTasks-only? - (Not in the candidate list today.) + (Not in the candidate list today.) Note this is now downstream of + [`SYNC.md`](SYNC.md) open question 3: if External mode is retired once we sync + ourselves, the question disappears with it. 4. ~~**Posture B authority choice**~~ resolved: **our own** `de.jeanlucmakiola.agendula.tasks`. Squatting `org.dmfs.tasks` is a dead end, not merely a trade-off — two apps cannot declare the same authority or @@ -181,8 +186,9 @@ These carry over from [`PLAN.md`](PLAN.md) §9; resolved ones are struck through 6. **Resolver ordering / mode-selection UX** — `autoMode()` picks a sane default today (see [`ARCHITECTURE.md`](ARCHITECTURE.md) §4.1); the Settings override it assumes is not built yet. -7. **Sync protocol coverage**, account model, conflict resolution — the next - design discussion. +7. ~~**Sync protocol coverage**, account model, conflict resolution — the next + design discussion.~~ Taken up in [`SYNC.md`](SYNC.md); the remaining opens + live on that document's list. --- diff --git a/docs/STORAGE-AND-SYNC.md b/docs/STORAGE-AND-SYNC.md index aba0a6f..4cfb065 100644 --- a/docs/STORAGE-AND-SYNC.md +++ b/docs/STORAGE-AND-SYNC.md @@ -1,5 +1,15 @@ # Agendula — storage and sync +> ⚠️ **Partly superseded, 2026-08-13.** The core decision below — *vendor the +> dmfs provider in-tree as `:provider`* — has been **reversed**. Agendula builds +> its own Room store and deletes the vendored provider; External mode (OpenTasks, +> tasks.org) is unaffected and everything this document says about it still +> stands. See [`STORAGE-DECISION.md`](STORAGE-DECISION.md) for why and +> [`OWN-STORE.md`](OWN-STORE.md) for what replaces it. The permissions, +> distribution, storage-mode and dead-end sections below remain accurate; treat +> the "our own provider" sections as the historical record of a decision that was +> made, shipped, and then costed properly. + > Decided direction, captured 2026-08-01. Supersedes the earlier "Posture B = > bundle OpenTasks" working notes, which are withdrawn (see > [Dead ends](#dead-ends--do-not-revisit)). This is the detailed companion to @@ -34,7 +44,7 @@ | 2 | Vendor `:provider` under our own authority | the identity, done once — and it ships a complete local-first app | ✅ done | | 3 | Export / backup | our data now lives only in our app's private storage | ✅ backend done; no UI yet | | 4 | File the DAVx5 issue | cheap, non-blocking, serves F-Droid users | ⬜ | -| 5 | Sync adapter | the 1.x arc; design discussion pending | ⬜ | +| 5 | Sync adapter | the 1.x arc; designed in [`SYNC.md`](SYNC.md), not yet built | ⬜ | Everything below is the reasoning behind those choices, the alternatives that were rejected, and the constraints they have to survive. @@ -174,8 +184,13 @@ feature — see [Storage modes](#storage-modes--the-users-choice). | **Synced** | our bundled provider | our sync adapter | network + an account the user configures | | **External** | OpenTasks / tasks.org | whatever that provider's engine does (DAVx5 …) | that provider's `READ`/`WRITE_TASKS`, granted at runtime | -Local and Synced are the same store — Synced is Local with an account attached, -so switching on sync is not a migration. +Local and Synced are the same store — but ⚠️ **switching on sync *is* a +migration, contrary to what this document said until 2026-08-13.** The provider +enforces `ACCOUNT_NAME` and `ACCOUNT_TYPE` as **write-once** on a task list +(`processors/lists/Validating.java:68-76`, which throws), so a list created under +`org.dmfs.account.LOCAL` can never be re-pointed at a real account. Enabling sync +means creating new lists under the account and moving tasks into them. See +[`SYNC.md`](SYNC.md) — it is a costed deliverable there, not a free consequence. **Resolver ordering — decided, and it went both ways as expected.** `ProviderResolver` now takes an explicit `StorageMode` from Settings when there @@ -235,10 +250,17 @@ compliance and a small enum-shaped addition with near-zero ongoing maintenance for them. Say that explicitly. "Here's a change that can't break anything" lands very differently from "please support my app." -**Open — the next discussion.** Protocol coverage ("support as much as -possible"), the account model, conflict resolution, and where the DAV/iCalendar -work lives. One constraint to settle early: we're MIT; `dav4jvm` is Apache-2.0 -and fine, but **verify `ical4android`'s license** before assuming it's usable. +**The design is now written out in [`SYNC.md`](SYNC.md)** — protocol coverage, +the account model, conflict resolution, the VTODO ↔ `TaskContract` mapper, and +where the DAV/iCalendar work lives. Two corrections to what this section +originally said, both verified 2026-08-13: + +- `dav4jvm` is **MPL-2.0**, not Apache-2.0. Still fine against our MIT (file-level + copyleft), but it is ⚠️ **JitPack-only**, which collides with our + `FAIL_ON_PROJECT_REPOS` + `google()`/`mavenCentral()` policy and with the + JitPack dead end below. `SYNC.md` open question 1. +- `ical4android` is **superseded by `synctools`, which is GPLv3** — so it is + unusable, and the open question below is closed. We write the mapper in-house. --- @@ -336,6 +358,13 @@ Not on either yet; both are targets, so build *for* them rather than retrofittin throwaway spike* (a library string resource can be overridden from the app module, so the authority rename works), but not for anything we ship. + ⚠️ **This one comes back.** It is a dead end *for the provider*, where + vendoring was mandatory anyway. `dav4jvm` is JitPack-only, so the sync adapter + has to answer the same question on its own terms — and F-Droid turns out not to + be the obstacle (its inclusion policy trusts jitpack.io for freely-licensed + artifacts); our own trust-surface policy is. See [`SYNC.md`](SYNC.md) open + question 1. + --- ## Sequencing @@ -358,8 +387,11 @@ the roadmap should say so rather than inheriting the old estimate. ## Open questions -1. **Sync protocol coverage**, account model, conflict resolution — the next - discussion. Still open. +1. **Sync protocol coverage**, account model, conflict resolution — ✅ taken up in + [`SYNC.md`](SYNC.md). The account model is answered there (`AccountManager`, + which `PROVENANCE.md` change 1 already assumes); what stays open moves to that + document's own list — dav4jvm's distribution, conflict policy, External mode's + future, and recurring-task completion. 2. **Resolver ordering** — ✅ decided, see [Storage modes](#storage-modes--the-users-choice). The **mode-selection UX** is still open: `autoMode()` picks a default, but the Settings override it @@ -376,9 +408,11 @@ the roadmap should say so rather than inheriting the old estimate. ⚠️ **But that test is Robolectric, and it skips on ARM64**, where Robolectric has no SQLite backend in either mode. It runs on x86_64 CI. It is not a substitute for a device, and this remains on the device-verification list. -4. **`ical4android` licensing** vs our MIT. Still open — and note the export path - does *not* depend on it: `ICalendarWriter` is our own ~200 lines, no library. - The question is really about the sync adapter's iCalendar *parsing*. +4. ~~**`ical4android` licensing** vs our MIT.~~ ✅ Closed: `ical4android` is + superseded by `synctools`, which is **GPLv3**, so it is out — as is + `cert4android`. The export path never depended on it anyway (`ICalendarWriter` + is our own ~200 lines). The sync adapter's iCalendar parsing goes through an + in-house mapper over `ical4j`/`biweekly`; see [`SYNC.md`](SYNC.md). 5. **jtx Board** as an additional External-mode candidate — richer contract, later. (`PLAN.md` decision #3, still open.) 6. **The vendored provider's timezone-change behaviour** — upstream's receiver diff --git a/docs/STORAGE-DECISION.md b/docs/STORAGE-DECISION.md new file mode 100644 index 0000000..bf21c4d --- /dev/null +++ b/docs/STORAGE-DECISION.md @@ -0,0 +1,246 @@ +# Storage: keep the vendored provider, or build our own? + +**Status:** **decided — build our own.** See [`OWN-STORE.md`](OWN-STORE.md) for +the architecture and plan; this document is the reasoning that got there. + +The decision went further than the recommendation below: `:provider` is not kept +alongside a Room store, it is **deleted**. External mode (OpenTasks, tasks.org) +stays. The staged sequencing survives in a different form — the provider remains +in-tree until `OWN-STORE.md` phase 5 so recurrence parity can be tested against +it, then goes. + +This reopens a question `SYNC.md` marked settled. It is reopened on purpose: the +argument that settled it was *"the provider hands us the sync bookkeeping for +free"*, and the phase-1 audit found most of that bookkeeping broken, absent, or +unusable for our purposes. A conclusion is only as good as its premise. + +--- + +## The three options + +| | What it means | Store | Exported provider | +|---|---|---|---| +| **A** | Keep `:provider` as-is | dmfs `TaskProvider` | yes, ours today | +| **B** | Room, same `TaskContract` shape | our Room DB | dropped, or a later facade | +| **C** | Room, clean domain schema, `TaskContract` only as an export format | our Room DB | no | + +External mode (talking to OpenTasks / tasks.org) is orthogonal and survives all +three. It is the reason nothing below gets deleted. + +--- + +## What the swap actually touches — measured, not estimated + +### The seam is already there, and it is clean + +`TasksDataSource` (`data/tasks/TasksDataSource.kt`, 58 lines) is a **14-method, +domain-shaped interface**. It takes and returns `Task`, `TaskList`, `TaskForm` — +no `Cursor`, no `Uri`, no `ContentValues`. + +Above it, **17 files** import from `data.tasks`. What they import: + +``` +8 × TasksRepository 4 × TasksDataSource 3 × ProviderResolver +4 × recoveringFromProviderFailure 2 × ProviderStatus +1 each: StorageMode, StorageModeHolder, ProviderEnvironment, TaskQuery, … +``` + +**Exactly one file outside the data package touches `TasksContract` at all** — +`domain/Models.kt`, and only for four status integers, one priority constant and +one account-type string. Ten lines. Nothing else above the data layer knows a +ContentProvider exists. + +> The whole UI, all five milestones of it, is untouched by a storage swap. +> That is not luck — `AndroidTasksDataSource`'s own KDoc says the seam exists so +> that *"swapping the provider never reaches above this file."* It holds. + +### Nothing gets deleted + +| File | Lines | Under a Room store | +|---|---|---| +| `AndroidTasksDataSource.kt` | 220 | **kept** — External mode still needs it | +| `TasksContract.kt` | 178 | **kept** — External mode speaks it | +| `TasksRepositoryImpl.kt` | 151 | unchanged | +| `ProviderResolver.kt` | 143 | unchanged | +| `TaskWriteMapper.kt` | 118 | **kept** for External | +| `TaskMapper.kt` | 102 | **kept** for External | +| `TasksDataSource.kt` | 58 | unchanged — it is the interface | +| `TasksRepository.kt` | 53 | unchanged | +| `ProviderEnvironment.kt` | 51 | unchanged | +| `StorageModeHolder.kt` | 47 | unchanged | +| `ColumnReader.kt` | 40 | **kept** for External | +| `ProviderFlow.kt` | 31 | unchanged | +| `StorageMode.kt` | 30 | one new constant | +| `TaskProjections.kt` | 24 | **kept** for External | +| `Failures.kt` | 16 | unchanged | +| | **1,262** | **0 removed** | + +The work is **additive**: a second `TasksDataSource` implementation, a third +`StorageMode`, and one `@Binds` becoming a dispatcher. `DataModule.kt` has a +single binding to change. + +This reframes the question. It is not *rewrite vs. keep*. It is **write a second +backend behind an interface that exists for exactly this purpose, and run both +until one wins.** + +--- + +## What the new backend has to do + +Room entities and DAOs for the ~50 columns the app actually uses across four +tables are mechanical. The real work is the behaviour the provider's processors +perform. Measured against `:provider`'s Java: + +| Behaviour | Provider | Notes | +|---|---:|---| +| Instance expansion | ~1,070 | `Instantiating` + `instancedata` + iterables. **The hard one.** | +| Recurring-instance edit | 337 | `Detaching` — this *is* the recurrence-model decision | +| Completion coherence | 210 | `AutoCompleting`: status ↔ percent ↔ completed ↔ is_closed | +| Validation | 601 | three processors, mostly defending a *public* API | +| Parent / child | 269 | we use `parent_id` only | +| Alarm property rows | 133 | one Room entity | +| | **~2,620** | | + +And what we would **not** write, of the 14,555 vendored lines: + +| Not needed | Lines | Why | +|---|---:|---| +| `TaskDatabaseHelper` | 895 | 23 migrations from a 2013 schema. We start at v1. | +| `FTSDatabaseHelper` + ngrams | 798 | **the app never searches the provider** — verified, zero call sites | +| `model/adapters` | 1,581 | a type-safe layer over `ContentValues`. Room entities delete the problem. | +| `model` | 1,811 | cursor ↔ entity adaptation. Room's job. | +| `TaskProvider` + `SQLiteContentProvider` | 1,772 | URI matching, permissions, batch ops — for a public API | +| `CategoryHandler` + `RelationHandler` | 553 | unused | +| `utils` (most) | ~800 | dmfs jems idiom → Kotlin stdlib | +| **≈ 8,200 lines we would simply not have** | | | + +Two things make instance expansion less frightening than its line count: + +1. **We need client-side recurrence expansion regardless.** Server-side + `CALDAV:expand` on `VTODO` is broken on every server we target (`SYNC.md`), + so `lib-recur` is in the build either way. +2. **We would use the same eight `lib-recur` classes the provider does** — + `RecurrenceRule`, `RecurrenceSet`, `RecurrenceSetIterator`, `RecurrenceList`, + `RecurrenceRuleAdapter`, `DateTime`, `Duration`, + `InvalidRecurrenceRuleException`. The algorithm is in the library, not in the + provider. +3. And the provider's expansion **materialises only one upcoming occurrence + anyway** — it is not the complete implementation its size suggests. + +--- + +## The cost, both directions + +### Building it + +| | | +|---|---:| +| Schema, entities, DAOs | 1 wk | +| Instance expansion on `lib-recur`, with a real test suite | 1.5–2 wk | +| Completion / parent / validation semantics | 1 wk | +| Recurring-edit model — *shared cost, phase 1 either way* | (0.5–1 wk) | +| Migrating existing users' local data out of the provider | 0.5 wk | +| Tests to parity with the current 93 + 56 | 1 wk | +| **Net additional** | **4.5–6 wk** | + +### What it removes from the sync plan + +Roughly sixteen of the phase-1 audit's storage findings are **provider-imposed** +— they exist only because we run dmfs's implementation, and vanish when we own +the store: + +- `_DIRTY` not set on delete, and defaulting to `1` +- `TaskLists._DIRTY` as a monotonic counter, not a flag +- the instances URI ignoring `CALLER_IS_SYNCADAPTER` +- no home for a per-collection sync token, href, ETag or CTag — all four squat + into generic `SYNC1`–`SYNC8` slots +- **read-only collections cannot be represented at all** (`ACCESS_LEVEL` inert) +- sync-adapter delete ignoring the account parameters it forces you to supply +- `Moving` leaving a dual-UID collision +- `ACCOUNT_TYPE` write-once → enabling sync is a full data migration +- Auto Backup restore arming `cleanUpLists` → silent task loss +- `Detaching` deciding the recurring-completion model for us +- the `lib-recur` version trap (0.16.0 removed `RecurrenceSet`) — we pin because + the provider does, not because we want to + +Conservatively that is **2.5–4 weeks** off phases 0, 3 and 4 of the 11.5–15 week +sync plan, plus a class of bug that is currently *unfixable without patching +vendored Java*. + +### Net + +**≈ +1 to +3.5 weeks**, for a store we control, in exchange for two real losses. + +--- + +## The honest case for keeping it (Option A) + +Not nothing, and it should not be waved away: + +- **It works, and it has 56 passing JVM tests** over recurrence, reparenting, + instances and observers. A Room reimplementation is *new code with new bugs*, + in the layer that holds the user's only copy of their data. That risk is real + and it points at A. +- **Tombstones actually work.** Soft delete for account rows, hard delete for + sync adapters, hidden from normal queries, undelete refused. Of all the sync + bookkeeping, this is the piece that held up under audit. +- **The exported provider under our own authority** — third-party apps can read + Agendula's tasks, and asking DAVx5 to sync us stays possible. +- Eleven local modification sites, all marked `AGENDULA CHANGE`, all documented + in `provider/PROVENANCE.md`. The fork is under control today. + +And the case against keeping it: + +- **14,555 lines of Java — 1.66× the entire app** (8,783 lines of Kotlin). We + carry, build, lint, translate and ship all of it to use maybe a third. +- Upstream is effectively dormant; every future `targetSdk` bump and every + Android SQLite behaviour change lands on us, in someone else's code, in a + language the rest of the app does not use. +- It makes behavioural decisions on our behalf (`Detaching`, `AutoCompleting`) + that we then have to reverse-engineer before we can honour them over CalDAV. + +--- + +## Recommendation + +**Option B — build our own store on Room, keeping the `TaskContract` *shape* as +the internal model — and keep `:provider` in-tree while we do.** + +Three reasons, in order of weight: + +1. **The seam already exists and the work is additive.** Nothing is deleted, + nothing above `data/tasks` changes, and both backends can ship side by side + behind `StorageMode`. The "big rewrite" this decision was originally weighed + against does not exist. +2. **The premise that settled it is gone.** The provider was kept for sync + bookkeeping we have since measured as broken. Sixteen findings deep, keeping + it is now a *cost* to the sync plan, not a saving. +3. **The window is now.** After phase 1 the mapper and engine are written against + whichever store won, and this stops being a two-file change. + +Keeping the `TaskContract` *shape* rather than going domain-native (Option C) is +deliberate: it is a proven schema for exactly this problem, other engines +understand it, and it keeps a future exported facade cheap — without obliging us +to run a 2015 Java implementation of it. + +### Sequencing that keeps the risk low + +1. Add `StorageMode.OWN` and a Room `TasksDataSource`. Both backends live. +2. Ship it behind a setting; the vendored provider stays the default. +3. Run the sync engine against Room only. +4. Once Room has real production mileage, decide whether the exported + ContentProvider is worth re-implementing as a thin facade (~1–1.5 wk) or + whether External mode already covers everyone who wanted it. + +Step 4 is a genuinely open question and does not need answering now. That is the +point of sequencing it last. + +--- + +## Open + +- **Is an exported provider worth keeping at all?** It matters only if third + parties should read our tasks, or if we want DAVx5 to sync our store. External + mode arguably already serves the second. Undecided. +- **The Room estimate is mine, not measured.** Instance expansion is the item + that could overrun; everything else is well-bounded. diff --git a/docs/SYNC.md b/docs/SYNC.md new file mode 100644 index 0000000..4938dd8 --- /dev/null +++ b/docs/SYNC.md @@ -0,0 +1,1185 @@ +# Agendula — CalDAV sync + +> Design notes for Agendula's own sync adapter. Drafted 2026-08-13; **audited the +> same day** against the platform, the vendored provider source, and the current +> state of every library named. The audit refuted or corrected a substantial part +> of the first draft — the corrections are marked ⚠️ **inline and kept visible** +> rather than quietly rewritten, because most of them are things the next person +> would otherwise assume again. +> +> This is step 5 of [`STORAGE-AND-SYNC.md`](STORAGE-AND-SYNC.md)'s sequencing. +> That document decided **where task data lives**; this one decides **how it gets +> to a server**. +> +> Status: **draft / decision document.** Nothing here is built. Where a question +> is already answered by shipped code, it is marked ✅ and the code is named. +> +> ⚠️ **The storage question this document declared settled was reopened, and the +> answer changed.** Agendula is building its own Room store and deleting the +> vendored provider — see [`STORAGE-DECISION.md`](STORAGE-DECISION.md) and +> [`OWN-STORE.md`](OWN-STORE.md). Roughly sixteen of the findings below are +> **provider-imposed** and disappear with it; `OWN-STORE.md` § *Effects on the +> sync plan* lists them item by item. Everything platform-level (the targetSdk 34 +> sync gate, the stub adapter, credential storage, Play compliance) and everything +> protocol-level (discovery, RFC 6578, conditional PUT, conflict policy, the +> server-reality table) is unaffected and remains the plan. + +Scope: self-hosted CalDAV first (Nextcloud), F-Droid and Play, MIT license. + +--- + +## The plan, in short + +| Question | Direction | +|---|---| +| Where does task data live? | Our vendored `:provider` — **settled, shipped** | +| Who syncs it? | Agendula, via its own sync adapter | +| Account model | `AccountManager` **+ a real (stub) sync adapter** — ⚠️ the hybrid without one does not work | +| Scheduling | WorkManager, triggered *through* the sync framework | +| Protocol library | `dav4jvm` (MPL-2.0) — ⚠️ costs more than the first draft assumed | +| Self-signed certs | `cert4android` — ⚠️ **MPL-2.0, not GPLv3.** The first draft rejected it on a false premise | +| iCalendar | In-house mapper over `ical4j`; **no `synctools`** (GPLv3) | +| Recurrence | Read/write `RRULE`/`RDATE`/`EXDATE` directly — ⚠️ **not** the Instances table | +| Primary read path | ⚠️ `REPORT calendar-query` (VTODO filter, no time-range); `sync-collection` is the optimisation | +| Recurring completion | ⚠️ Accept all four models; `:provider` has **already chosen model (d)** for us | +| Sign-in | Nextcloud Login Flow v2 + generic CalDAV discovery + Digest | +| Conflict policy | `If-Match`; on 412 the server wins, local copy preserved | +| Reference implementations | jtx Board and DAVx5 — read, never link against (GPLv3) | + +| # | Phase | Deliverable | Effort | +|---|---|---|---| +| 0 | **Groundwork** | UIDs at creation, backup/prune safety, licence-attribution screen, Java-21 decision | 1 week | +| 1 | Mapper | VTODO ↔ `TaskContract`, unknown-property round-trip, fixture corpus | 2–3 weeks | +| 2 | Auth | Discovery, Login Flow v2, Digest, credential storage, cert trust | 1.5–2 weeks | +| 3 | Engine | `calendar-query` baseline, `sync-collection` optimisation, full reconciliation, conflicts, scheduling, **Local→Synced migration** | 5–6 weeks | +| 4 | Hardening | Per-server trap matrix, error UX, re-auth, Play compliance | 2–3 weeks | + +⚠️ **Revised upward from the first draft's 8–9 weeks to 11.5–15.** Phase 0 is new; +the migration in phase 3 was previously believed not to exist at all; and the +engine grew a second sync path plus a permanent reconciliation pass. + +**Calibration, for sanity:** Evolution shipped RFC 6578 in **June 2026** against +a request open since 2019. vdirsyncer has declined to implement it for twelve +years. Thunderbird still carries unlanded patches for one of its error paths. +This is not a phase that gets shortened by trying harder. + +--- + +## Settled — the storage question is not reopened here + +A later working draft re-argued storage as a fresh choice between *bundle the +dmfs provider* and *a Room-native store with a `TaskContract` facade*, and +recommended Room. Not adopted; this is the audit trail so it does not come back. + +The provider shipped in `f978c37`. Three of the arguments against it do not +survive contact with the repository: + +1. **"`synctools` is GPLv3 and propagates to the app."** Misattributed. + `:provider` depends on `jems`, `rfc5545-datetime` and `lib-recur` — all + Apache-2.0. `synctools` is a *mapper* choice, equally avoidable either way. +2. **"Cursor access everywhere; the widget pays the Calendula cost."** Calendula + queries `CalendarProvider` **cross-process**; ours is same-package, same-uid, + so `ContentResolver` returns the local provider instance — no Binder hop, no + `CursorWindow` marshalling. The observer→Flow seam already exists. +3. **"The dmfs schema can't hold what we need."** The `Properties` table takes + arbitrary mimetypes — `PropertyHandlerFactory.get` falls through to + `DEFAULT_PROPERTY_HANDLER`, and `PropertyHandler.insert` is a bare + `db.insert` with no validation, over sixteen real `data0`–`data15` TEXT + columns. That is the mechanism for unknown-property round-tripping, this + document's single most important correctness requirement. + +**But the audit also weakened the positive case.** The first draft claimed the +provider hands us the sync bookkeeping for free. It hands us *some* of it, with +sharp edges — see the next section. Room would have been the wrong trade anyway +(it buys Flow ergonomics we already have and costs the tombstone/instance +machinery we already run), but "for free" was too generous. + +--- + +## What the provider actually gives us + +⚠️ **This section is almost entirely rewritten.** Every line is verified against +`provider/src/main/java/`. + +| Mechanism | Reality | +|---|---| +| `CALLER_IS_SYNCADAPTER` | Works on `tasks` and `tasklists`. ⚠️ **Ignored on the `instances` URI** — `processors/instances/TaskValueDelegate` hardcodes `false` on every delegation | +| `_DIRTY` on user writes | ⚠️ **Set on insert/update only, never on delete.** `AutoCompleting.delete` skips `updateFields`; `TaskCommitProcessor.delete` sets only `_DELETED` | +| `_DIRTY` default | ⚠️ **Defaults to `1`** (`TaskDatabaseHelper:456`). Every downstream insert must write `_dirty=0` explicitly or it uploads straight back | +| `TaskLists._DIRTY` | ⚠️ **A monotonic counter, not a flag** — a trigger does `_dirty = _dirty + new._dirty + new._deleted` and nothing ever decrements it | +| `_DELETED` tombstones | ✅ Real. Soft-delete for account-backed tasks, hard delete for sync adapters *and* for the local account. Hidden from non-sync queries | +| Instances | ⚠️ **Materialises exactly one upcoming occurrence** (`UPCOMING_INSTANCE_COUNT_LIMIT = 1`), on write only, never re-expanded as time passes. Useless as a recurrence representation for sync | +| `_UID` | ⚠️ **Settable by anyone on insert** — restricted to sync adapters on *update* only. Never generated by the provider | +| `SYNC1`–`SYNC8`, `_SYNC_ID`, `SYNC_VERSION` | Exist and are free — but ⚠️ **`Moving` nulls all of them on a list move while keeping `_UID`**, leaving a live row and a tombstone sharing one UID | +| Per-collection sync state | ⚠️ **Does not exist.** The `SyncState` table is one blob **per account**, `db.replace`d | +| Read-only collections | ⚠️ **Cannot be represented.** `ACCESS_LEVEL` is inert — the contract says "not used yet", and `Validating.java:60` still carries upstream's `// TODO: ensure that the list is writable` | +| Account scoping on delete | ⚠️ The provider **requires** account params on a sync-adapter task delete and then **ignores them** (`TaskProvider.java:793` — upstream `// TODO`) | + +### The consequences, as rules + +1. **The upload query is `_dirty = 1 OR _deleted = 1`.** Deleting is not dirtying. +2. **Every downstream insert sets `_dirty = 0` explicitly.** +3. **Name the squats now**, because none of these columns exist: CTag → + `TaskLists.SYNC_VERSION`; per-task ETag → `Tasks.SYNC_VERSION`; href → + `Tasks._SYNC_ID`; per-collection sync-token → a `TaskLists.SYNC*` slot (the + `SyncState` blob is per-account and cannot hold it). +4. **Never write through the `instances` URI.** Read and write `RRULE`, `RDATE` + and `EXDATE` on `tasks` — they are raw TEXT and round-trip cleanly. +5. **Scope every adapter delete by `list_id` yourself.** +6. **Sequence DELETE before PUT on a list move**, or the two rows sharing a + `_UID` collide on the server. +7. **Read-only collections are enforced in the app layer**, not the provider. +8. **Soft-deleted subtasks lose their `Relation` rows** before the adapter sees + the tombstone (`Reparenting.unlinkParent` runs regardless of `isSyncAdapter`). + Either cache the relation or accept that `RELATED-TO` is unrecoverable on + delete. + +### Two collisions with the shipped app layer + +- **`AndroidTasksDataSource.setAlarm` deletes every alarm property row on the + task** before re-inserting. Once the adapter round-trips `VALARM`s, one local + reminder edit destroys all server-side alarms on that task. Decide ownership: + either the adapter owns `VALARM`s and the app stops bulk-deleting, or reminders + stay local-only and are never serialised. +- **`updateInstance` routes through the instances URI**, which per the table + above is permanently non-sync-adapter and forks override rows. Those overrides + arrive `_dirty=1` with no `_uid`/`_sync_id` and must be uploaded as + `RECURRENCE-ID` components. + +--- + +## ⚠️ The migration that was believed not to exist + +The first draft, `STORAGE-AND-SYNC.md` and `ARCHITECTURE.md` all asserted: +*"Synced is Local with an account attached, so switching on sync is not a +migration."* **That is false.** + +`processors/lists/Validating.java:68-76` throws on any attempt to change a task +list's `ACCOUNT_NAME` or `ACCOUNT_TYPE` — both are write-once, and the contract +documents them as such. Local lists live under `org.dmfs.account.LOCAL` and can +never be re-pointed at a real account. + +Turning on sync therefore means, as a real deliverable with its own tests: + +1. Create new lists under the account (sync-adapter insert; account params come + from the **URI**, never the values, and are frozen thereafter). +2. `UPDATE list_id` on every task — this *is* permitted, and it is the one path + the provider gives us. +3. Assign `_UID`s (or better: have them already, see phase 0). +4. Delete the old local lists (sync-adapter only). +5. Re-point `DEFAULT_LIST_ID`, per-list reminder overrides in DataStore, and + every scheduled alarm. + +Not modelling `SYNCED` as a third `StorageMode` is still right — it is derived +state. But the migration it implied away is real, and step 2's `Moving` processor +nulls `_sync_id`/`sync_version`/`SYNC1`–`SYNC8` and clones a tombstone as it goes. + +**Alternative worth considering:** don't migrate. Synced lists are always *new* +lists, and moving local tasks into them is an explicit user action with a visible +UI. Cheaper, more honest, and it never silently rewrites the user's data. + +--- + +## Account model — and the trap under it + +**Decision: `AccountManager` with our own account type, plus a registered +`` service whose entire job is to enqueue a WorkManager job.** + +⚠️ **The first draft's justification was circular** and its architecture did not +work. Both corrected: + +### The justification + +The draft argued AccountManager was *required* because the provider prunes lists +whose `ACCOUNT_TYPE` has no authenticator in this package. That is backwards — +`PROVENANCE.md` change 1 made the prunable set **empty by construction** +precisely so that shipping no authenticator prunes nothing. The provider imposes +no requirement at all. tasks.org proves the alternative: no AccountManager, Room +accounts, pure WorkManager, `GET_ACCOUNTS` stripped with `tools:node="remove"`. + +The real reasons, which are still good ones: a stable account identity that +third-party engines can address (the DAVx5 ask, open question 5), presence in +system Settings, and the sync framework as a change-trigger. + +### The trap ⚠️ + +`ContentService.hasAuthorityAccess()` gates `requestSync`, `setSyncAutomatically`, +`addPeriodicSync`, `setIsSyncable`, `getSyncStatus` and seven more behind a +compat change `@EnabledAfter(TIRAMISU)` — **on for targetSdk ≥ 34**, which we +are. With no package registering a sync adapter for our authority, every one of +those calls **returns silently**: no exception, no log. It is documented on no +Android behaviour-changes page. + +So "AccountManager accounts for visibility + WorkManager for scheduling + no sync +adapter" — exactly what the first draft described — yields: + +- every `ContentResolver` sync API a no-op that passes on a Robolectric shadow, +- an account in system Settings permanently reading **"Sync off for all items"**, +- a **greyed-out "Sync now"**, because `enabledSyncNowMenu()` needs at least one + checked authority switch. + +**Fix:** register a real `AbstractThreadedSyncAdapter` whose `onPerformSync` +enqueues a WorkManager job and waits — DAVx5's own comment: *"We use the sync +adapter framework only for the trigger, actual syncing is implemented with +WorkManager."* Declare `READ_SYNC_SETTINGS` / `WRITE_SYNC_SETTINGS`. Ship an +in-app sync button too, since "Sync now" stays greyed out under +`userVisible="false"`. + +**Free trigger already running:** `TaskProvider.syncToNetwork()` returns `true` +unconditionally, so `SQLiteContentProvider` already fires +`notifyChange(uri, null, syncToNetwork=true)` on every user write. The system is +*already* requesting a sync for our authority on each edit — there is simply +nothing registered to receive it. + +### ⚠️ Auto Backup will arm `cleanUpLists` into a data-loss path + +`backup_rules.xml` and `data_extraction_rules.xml` are both **empty rule sets**, +and `allowBackup="true"`. An empty set means Auto Backup's default: databases +included. So the provider's `tasks.db` is backed up and restored — while +AccountManager accounts, which live in `/data/system_ce/`, are not. + +`TaskProvider.onCreate` registers `addOnAccountsUpdatedListener(…, updateImmediately=true)`. +On the first callback after a restore, `cleanUpLists` sees lists carrying our +`ACCOUNT_TYPE` with no matching account and **deletes them, and their tasks, +silently** — cascading through `task_list_cleanup_trigger`. Exactly the failure +`PROVENANCE.md` change 1 exists to prevent, reintroduced from behind. + +Latent today (no authenticator ⇒ nothing prunable); live the day phase 3 ships. + +**Do not fix this by excluding the database from backup.** That was the audit's +suggestion and it is wrong for us: Local-mode data lives in exactly one place, +and Auto Backup is currently its only automatic safety net — removing it to +protect *synced* lists would trade a latent bug for a live one. Fix it at the +cause instead: + +1. Make pruning **event-driven** — react to `AccountManager`'s account-removed + broadcast, not to "absent from the visible set". +2. Add a post-restore reconciliation that offers to **re-attach the account** + rather than deleting. +3. Exclude only the **Keystore-encrypted credential blob** from backup — a + restored ciphertext is permanently undecryptable, since Keystore keys are + non-exportable. +4. Device-verify: restore a backup onto a fresh device, confirm nothing is pruned. + +--- + +## VTODO ↔ `TaskContract` + +In-house, behind an interface, so the iCalendar library stays swappable. + +### Unknown properties: the one non-negotiable + +Any `X-` property, unrecognised component or parameter written by another client +**must survive a read-modify-write cycle unchanged.** Failing this silently +destroys other people's data and is invisible in our own UI. + +Storage exists: a `Properties` row with our own `unknown-property` mimetype and +the serialised property in `DATA0`. Verified: no validation rejects an unknown +mimetype, the only index is non-unique so repeats are fine, and there is no FTS +interaction (`updateFTSEntry` is called only from `CategoryHandler`). Note +`MIMETYPE` is *declared* `INTEGER` while holding strings — harmless under SQLite +affinity, but don't be alarmed by it. `Tasks.HAS_PROPERTIES` is never set by +anything; do not filter on it. + +⚠️ **But "byte-stable" is unachievable as the draft stated it, and stating it that +way is dangerous** — the corpus would fail on day one and then be normalised +until the only thing that matters, *no unknown property is dropped*, is no longer +tested. Six independent reasons a faithful implementation cannot be byte-identical: +`PRODID` **must** change (emitting another product's is a lie); fold position +carries no information and a line may split between any two characters; parameter +quoting is optional (`TZID=Europe/Berlin` ≡ `TZID="Europe/Berlin"`); property +order within a component is unconstrained, and storing known fields as columns +destroys the original interleaving **by construction**; `DTSTAMP` is regenerated +and `VTIMEZONE` re-emitted; and the server will not return what we sent anyway. + +**Restate it as a semantic round-trip with byte-stable property values.** Re-parse +both sides into a canonical multiset of +`(component path, property name, params as a sorted map, unfolded unescaped value)` +and assert equality **modulo an explicitly enumerated allowlist** — `PRODID`, +`DTSTAMP`, `LAST-MODIFIED`, `SEQUENCE`, VTIMEZONE bodies, fold positions, +parameter quoting. Nothing else may differ. Byte-equality is then asserted where +it means something: the **unfolded, unescaped value octets** of every untouched +property, plus full parameter preservation including unknown parameters. RFC 5545 +§3.1 is the requirement being encoded: *"Applications MUST preserve the value data +for x-name and iana-token values that they don't recognize."* + +⚠️ **And the requirement is necessary but not sufficient.** A client that +round-trips perfectly, passing every fixture, still destroys the owner's data if +it PUTs back a body Nextcloud filtered on the way out (see +[Server reality](#️-server-reality--the-matrix-audited)). Never write back a body +whose ETag does not match the hash of what we downloaded. + +**Three gaps in the storage sketch.** A flat property-per-row model does not +represent unknown properties **nested inside a known sub-component** (an `X-` prop +on a `VALARM`), or entirely unknown components, or RFC 9074's alarm properties +(`ACKNOWLEDGED`, `PROXIMITY`) which are what other clients now write. Decide +between an opaque sub-component blob and reconstructing nesting from +`DATA0`–`DATA15`. And **set a size cap** — DAVx5 drops unknown properties above +~25 kB — for two reasons: Android's `CursorWindow` row limit, and +`CALDAV:max-resource-size`, whose violation is a failed PUT. + +**Test-first.** The corpus comes before the mapper, and the fixtures that catch +bugs are the adversarial ones, not a clean server-generated VTODO: a +`CLASS:CONFIDENTIAL` task fetched from a **shared** Nextcloud calendar; a resource +carrying a master plus `RECURRENCE-ID` overrides; unknown properties nested inside +a `VALARM`; a `TZID` the device's tzdb does not know; a UID containing `/` and +`@`; and one exceeding `max-resource-size`. + +### The rest of the minefield + +- `DUE` vs `DTSTART`; `VALUE=DATE` vs `DATE-TIME`; floating times and `TZID`. + Match the all-day/UTC convention `fix/provider-interaction-review` established. +- **`STATUS` / `PERCENT-COMPLETE` / `COMPLETED` disagree across clients.** Pick a + canonical reading, normalise on write only. Local convention to reconcile + against: the edit form writes `PERCENT_COMPLETE` clamped 0–100 and leaves + `STATUS` to the complete toggle. +- `RELATED-TO` for subtask trees, including orphans in another collection. The UI + nests one level; the *data* must not assume it. +- `VALARM` — see the `setAlarm` collision above before writing a line of this. +- `CATEGORIES`, `PRIORITY` (**0 = undefined, 1 = highest**). +- ⚠️ **`SEQUENCE` is preserve-verbatim, not ours to bump.** It is the *Organizer's* + revision counter (§3.8.7.4); a client that increments it on every save confuses + scheduling-aware peers. Related: **`DTSTAMP` is regenerated per serialisation, + `LAST-MODIFIED` changes only when the data actually did.** Conflating them makes + every sync look like an edit. +- ⚠️ **`COMPLETED` MUST be UTC** (§3.8.2.1) — no TZID, no floating, no DATE. +- ⚠️ **A `VALARM` with `TRIGGER;RELATED=END` requires `DUE`, or `DTSTART` plus + `DURATION`** (§3.8.6.3). A user clearing the due date on a task that has an + end-relative reminder produces an invalid resource, permanently rejected. Validate + before PUT — this is reachable from ordinary UI actions. +- ⚠️ **`RELATED-TO;RELTYPE` reads backwards to most implementers.** §3.8.4.5: + `PARENT` means *the referencing component is subordinate to the referenced + one*. Both Nextcloud Tasks and tasks.org put `RELTYPE=PARENT` on the child + pointing up — that is the correct reading. Also: the RFC explicitly disclaims + cascade semantics, so "completing a parent completes its subtasks" is a local + UI convention peers will not reproduce. +- ⚠️ **Round-trip `X-MOZ-LASTACK` / `X-MOZ-SNOOZE-TIME` unmodified.** Dropping + them causes documented **alarm storms** on Thunderbird. Emit RFC 9074 + `ACKNOWLEDGED` for our own writes rather than minting `X-MOZ-*`. Preserve + `X-APPLE-SORT-ORDER`; never interpret it. + +### ⚠️ Recurring completion — and the model our provider already chose + +The draft said "no standard; choose one". That is **confirmed and understated**, +and the choice is less free than it looked. + +**How settled the non-standard is.** `draft-ietf-calext-ical-tasks-17` — the +active IETF work item whose entire purpose is extending VTODO, `Updates: RFC5545` +— contains the substring **"recur" zero times** in 1,904 lines. It adds +`SUBSTATE`, `REASON`, `TASK-MODE` and a `VSTATUS` component and leaves this +untouched. RFC 8984 §5.2.6 (JSCalendar) is the only RFC that addresses it at all, +and it **blesses two mutually incompatible approaches and declines to pick**. +RFC 5545 permits `COMPLETED` on a recurring master with no interaction rule — +undefined, which is worse than forbidden, because every client picks differently +and all stay conformant. + +The four models in the wild: + +| | Model | Who | +|---|---|---| +| **a** | Write a `RECURRENCE-ID` override; master stays open | jtx Board, Thunderbird, eM Client | +| **b** | Advance the master's `DUE`/`DTSTART` in place, clear completion | tasks.org, Evolution, Nextcloud Tasks *in practice* | +| **c** | `STATUS:COMPLETED` on the master — kills the series | Nextcloud Tasks ≤ 0.17. **Always a bug** | +| **d** | Detach the completed occurrence as a **new task with a new UID**, and advance the master | **OpenTasks — i.e. our `:provider`** | + +**The finding that matters to us: `:provider` has already chosen model (d).** +`processors/instances/Detaching.java` nulls `_UID`, `_SYNC_ID` and every +`ORIGINAL_INSTANCE_*` on the detached row, and `detachAll` advances the master +and decrements `RRULE;COUNT`. dmfs did this deliberately — on the record, *"the +primary reason is to support Apple clients; they don't support overrides"*. So +our storage layer emits a UID-less orphan plus a moved master, and our sync +adapter has to either honour that, bypass the processor, or reconcile after it. +**That is a design constraint we inherited without deciding it**, and it is the +strongest single argument for treating this as a phase-1 decision rather than a +phase-4 detail. + +**Rules the audit establishes, regardless of which model we write:** + +- **Never write model (c).** Every instance found was filed as a defect; + Thunderbird fixed it fifteen years ago. +- **Never do (a) and (b) together.** `RECURRENCE-ID` is *defined* as the + instance's original `DTSTART`, so advancing the master orphans your own + override. Nextcloud Tasks 0.18 attempts exactly this — and is saved only by an + accident: its override write dispatches a Vuex action that **does not exist**, + which Vuex 4 swallows without throwing. Shipped behaviour is therefore silent + model (b) with no record the instance was ever completed. +- **Accept all four models on read, unconditionally**, including an inbound + master whose `DUE` moved and whose `COMPLETED` vanished. That is not corruption. +- **Never abort a sync batch on a multi-VTODO resource.** tasks.org returns from + its whole sync function on one — so a single Thunderbird-completed recurring + task **stops that entire collection from syncing**, ctag never advances, and + every other change in the batch is silently lost. Degrade to the master and + continue. +- **Keep overrides in the same calendar object resource** (RFC 4791 §4.1). +- ⚠️ **`RRULE` + `DUE` with no `DTSTART` has no well-defined `RECURRENCE-ID` + value** — undefined in RFC 5545, ubiquitous in the wild. Synthesising + `DTSTART := DUE` is the common workaround and is itself the source of visible + DTSTART/DUE desync between clients. Handle it explicitly. +- ⚠️ **Repeat-from-completion has no interoperable encoding at all.** Either drop + it or document that peers read it as repeat-from-due. +- Consider tasks.org's escape hatch: a per-account **"let the server schedule + recurring tasks"** switch. + +Open question 4 — but now with a default: **write (a) if we own the whole path; +accept that `:provider`'s `Detaching` pushes us toward (d) unless we bypass it.** + +> **Process note.** During this research a summarising fetch **fabricated a +> verbatim RFC 5545 sentence** ("A 'to-do' calendar component without the +> 'dtstart' property MUST NOT be part of a recurring set") that appears nowhere +> in the RFC — grep confirms zero hits — along with an invented DTSTART/DUE +> exclusivity rule. Normative text gets read from the raw RFC, never from a +> summary. Both fabrications would have inverted a design decision here. + +--- + +## Libraries + +⚠️ **The first draft's table had one outright licence error and understated two +libraries by roughly an order of magnitude.** Re-verified 2026-08-13 against +published POMs, Gradle module metadata and extracted jars. + +### Take + +| Library | Licence | Real cost | +|---|---|---| +| **dav4jvm** 4.0.1 | MPL-2.0 | ⚠️ **Ktor-only** (the OkHttp package was deleted in 3.0.0); ⚠️ **requires Java 21 bytecode** — we target 17 everywhere, including all seven floret-kit modules; pulls Ktor (~2.45 MB), `guava-jre` (wrong flavour — force `-android`), and `xpp3` (371 KB, duplicates framework `org.xmlpull.v1`). The library itself is only 433 KB / 246 classes | +| **cert4android** | ⚠️ **MPL-2.0 — not GPLv3** | Same org, same licence, same JitPack question as dav4jvm. See below | +| **ical4j** 4.3.0 | BSD-3-Clause | ⚠️ Not "needs desugaring" — `java.time` is native at minSdk 26 and we are 29. Real costs: **2.2 MB of duplicated tz data** in the jar (`zoneinfo/` *and* `zoneinfo-global/`, ~596 `.ics` each), a `ZoneRulesProvider` that pre-allocates 1500 synthetic zone IDs **and exhausts in production**, and a mandatory `ical4j.properties` + `MapTimeZoneCache` + registry shim | +| **lib-recur** | Apache-2.0 | ⚠️ **Version trap, see below** | + +### ⚠️ cert4android was rejected on a false premise + +The first draft listed it as GPLv3 and budgeted a hand-rolled trust-on-first-use +dialog instead. **It is MPL-2.0** — verbatim MPL text in `LICENSE`, SPDX +boilerplate in the README, GitHub agrees. The same licence as dav4jvm, which the +same document accepts two rows above. Two independent audit tracks caught this. + +That matters because the hand-rolled version is not a dialog: + +- **A background sync has no UI to show a dialog in.** cert4android's bound + service + notification approval *is* the library, not an accessory to it. +- **Network Security Config cannot express runtime trust** — it is a static + manifest resource. And since API 24, user-installed CAs aren't trusted without + an NSC entry, so "tell the user to install their CA" fails too. +- **The 3-arg `checkServerTrusted(chain, authType, host)` is mandatory**; + 2-arg-only TrustManagers have repeatedly broken on OkHttp. +- **Hostname verification is a second override** and the other thing Play flags. +- **Play has blocked publishing on unsafe `X509TrustManager` since 2016**, and + the guidance explicitly names "buggy or incomplete custom verification". + +This roughly dissolves the self-signed-cert line item in phase 4. + +### ⚠️ lib-recur is a version trap, not a free dependency + +The first draft said "already in the build at 0.12.2 — no new dependency". Both +halves are wrong. `provider/build.gradle.kts:56` declares it `implementation`, not +`api`, so it is **not** on `:app`'s compile classpath. And lib-recur **0.16.0 +removed `RecurrenceSet`**, which the vendored provider uses in +`TaskInstanceIterable`/`TaskInstanceIterator`. The moment `:app` adds a current +lib-recur, Gradle's highest-wins resolution upgrades the graph and **the provider +stops compiling.** + +Decide explicitly: pin `strictly = "0.12.2"` and accept the known fixes we forgo +(0.15.1 `FastForwarded`, 0.15.2 empty-`ByDay`), or budget the iterator rewrite +onto the post-0.16 API. Note upstream is dormant — last release 0.17.1, last +commit 2024-04. + +### Consider + +**biweekly** (BSD-2) is stronger than the first draft credited: 639 KB / 432 +classes, **no bundled tz database at all**, legacy `java.util.Date` so no +`ZoneRulesProvider` hazard — against ical4j's 2.2 MB of zone data and its +registry shim. Caveats: last release 0.6.8 (2024-01), still 0.x, mandatory +`jackson-core` for jCal (excludable), and the missing tz database means it relies +on `VTIMEZONE` components being present rather than resolving `TZID`s itself — +a real gap for CalDAV round-tripping. + +### Do not take + +| Library | Why not | +|---|---| +| **synctools** | **GPLv3.** Does exactly the mapping we need against exactly the schema we run, which makes it the sharpest temptation here. Still a one-way door. (Repo now archived and folded into `davx5-ose` as a module — the standalone coordinate is stale.) | +| **Android-SingleSignOn** | ⚠️ **GPL-3.0.** The first draft carried it as a harmless optional extra; it is the actual one-way door. It also proxies through the Files app and supports only OCS plus a few WebDAV verbs — not a CalDAV transport | +| **caldav4j** | Apache-2.0, but server-oriented, last release 2022-01 | +| **sardine** | Needs JAXB — a non-starter on Android | + +**Still true:** there is no mature Kotlin-native iCalendar library, and no +Android-suitable CalDAV client on Maven Central at all. That is *why* the JitPack +question is unavoidable rather than optional. + +### On GPL and Play + +The rejection reasoning is right in effect but was imprecise. MIT **is** +GPL-compatible; the constraint is on the terms of the distributed binary, not on +our source headers. And ⚠️ **GPLv3 is not a Play problem** — DAVx5 is GPLv3 and +ships on Play with 287k+ installs; §6 Installation Information is a hardware +provision. If the real reason is wanting Agendula to stay permissively +relicensable, say that, because that is the reason that holds. + +### ⚠️ Licence obligations we cannot currently discharge + +Settings exposes only our own MIT `LICENSE`. There is no third-party attribution +surface and no AboutLibraries in the build. But MPL-2.0 §3.2(a) requires telling +recipients how to obtain source; §3.4 requires retaining file headers; **BSD-3 +requires reproducing the copyright notice in binary distributions** (that is +ical4j, and it is not optional); Apache-2.0 §4(d) propagates NOTICE. Shipping any +of these without an attribution screen is a plain violation, independent of +copyleft. **Phase 0 work item.** Bonus trap: ical4j's POM declares a non-SPDX +licence name and a `LICENSE` URL that 404s, so generators produce empty output. + +### The JitPack question, restated + +`dav4jvm` and `cert4android` are both `com.github.bitfireAT:*` — JitPack only. +F-Droid's inclusion policy does trust jitpack.io for freely-licensed artifacts, +so **F-Droid is not the obstacle; our own `FAIL_ON_PROJECT_REPOS` policy is.** +But F-Droid's own writing is lukewarm — JitPack "hosts whatever is built from +GitHub, without checking the license" — and concretely: JitPack **does not sign +artifacts** (`.asc` 404s; Maven Central's does not), and rebuilds on demand, so a +coordinate is not immutable. We have no `verification-metadata.xml` today. + +Weigh against that: **dav4jvm shipped two breaking majors nineteen days apart** +(3.0.0 OkHttp→Ktor 2026-07-08; 4.0.0 callbacks→coroutines 2026-07-27), which +argues for vendoring a known-good tree rather than a floating pin — and at 433 KB +vendoring is far cheaper than depending, once the Ktor/guava/xpp3 tail is counted. +Open question 1. + +--- + +## Authentication and discovery + +### Nextcloud Login Flow v2 + +The protocol description survives audit against the server source: the endpoint, +the `{poll:{token,endpoint},login}` shape, 404-until-approval, the 20-minute +lifetime (`lifetime = 1200` in `LoginFlowV2Mapper.php`), and "the 200 is returned +exactly once" (the mapper deletes the row inside `poll()` before returning). It is +not deprecated, there is no v3, and OAuth2 is a worse fit. Corrections: + +- ⚠️ **Poll with `POST`, form-encoded.** A `GET` gets 405. The draft didn't say. +- ⚠️ **Set an explicit `User-Agent`.** `init()` passes it to `createTokens()`, + where it becomes the app password's **name** in Settings → Security → Devices & + sessions. With OkHttp's default the user sees `okhttp/4.12.0` and cannot tell + what to revoke — defeating the entire point of the flow. (`OCS-APIRequest` is + *not* needed here; v2 is a Frontpage route.) +- ⚠️ **404 only means pending.** "Treat anything that isn't a 200 as pending" + swallows 429 (brute-force protection), 503 (maintenance), Cloudflare challenge + pages (200 with HTML), and DNS/TLS failure — turning a diagnosable error into a + 20-minute spinner. Require `Content-Type: application/json` before parsing. + Stop polling on anything that is neither 404 nor 200. Note 404 is *also* + returned for expired/consumed, so keep tracking the deadline locally. +- ⚠️ **Validate the `endpoint` origin.** Verbatim is right for the *path*, wrong + as a blanket rule: it is generated from `overwrite.cli.url` / `overwriteprotocol` + / `trusted_proxies`, misconfigured on a large fraction of self-hosted installs. + Refuse a scheme downgrade to `http` outright — the poll token is exchanged for a + long-lived app password, so this is a credential-grade secret. If the host + differs from the one the user typed, confirm explicitly and say *"your server's + `overwrite.cli.url` is wrong"*, which saves a support round-trip. (The draft's + "some deployments return 302" is a proxy symptom, not a Nextcloud variant.) +- ⚠️ **`loginName` is not the uid.** It is what the user typed — possibly an + email, an LDAP-derived value, or the right name in the wrong case. Use it + **only** as the Basic auth username; never interpolate + `remote.php/dav/calendars//`. Discover via `current-user-principal` + → `calendar-home-set`, exactly as the generic path already does. This is the + classic "logged in but no calendars" bug. +- ⚠️ **Custom Tabs needs four things the draft omitted:** a `` entry for + `android.support.customtabs.action.CustomTabsService` (or provider detection + silently fails on API 30+); a try/catch with an `ACTION_VIEW` fallback + (`launchUrl` throws `ActivityNotFoundException` with no Custom Tabs browser — + realistic on GrapheneOS/CalyxOS/AOSP, i.e. disproportionately our users); + persistence of `{token, endpoint, deadline}` to disk immediately, so process + death mid-flow is resumable; and an explicit "I finished / Cancel" affordance, + since Custom Tabs return **no result** when dismissed and Nextcloud's flow ends + on a "you can close this window" page that never returns to the app. + +### Generic CalDAV discovery + +⚠️ **The draft's five steps were the happy path of a much longer pipeline, and +one of its two filters was inverted.** Corrected version, with live probes run +2026-08-13: + +``` +1. Input: email / mailto: / http(s) URL +2. Base URL typed → PROPFIND Depth:0 on it first (principal, home-set and + collection can all come back in one response) +3. Else: + a. SRV _caldavs._tcp. — honour RFC 2782 priority/weight, + honour non-443 ports, target "." = none + b. TXT _caldavs._tcp. — parse path= ⚠️ MISSING FROM DRAFT + c. ladder: [TXT path] → /.well-known/caldav → / ⚠️ "/" MISSING +4. PROPFIND Depth:0 for DAV:current-user-principal + - follow 301/302/303/307/308, re-sending PROPFIND and its body + - relative Location; reject HTTPS→HTTP; cap at 5; PERSIST 301/308 + - 401 → authenticate and retry. NOT a failure ⚠️ MISSING + - reject ⚠️ MISSING + - OPTIONS gate: DAV: header must contain calendar-access ⚠️ MISSING +5. PROPFIND Depth:0 on the principal for calendar-home-set + → iterate ALL hrefs (0..n); cross-host is normative ⚠️ DRAFT ASSUMED ONE +6. PROPFIND Depth:1 per home set, requesting properties BY NAME + → optionally recurse one level into plain {DAV:}collection members +7. Classify: + a. resourcetype as a SET; require CALDAV:calendar ⚠️ MISSING + b. VTODO test: property ABSENT ⇒ INCLUDE ⚠️ DRAFT INVERTED IT + c. privilege-set absent ⇒ assume writable; handle 403 on write +8. sync path: supported-report-set → RFC 6578 keyed on DAV:sync-token +9. Creation: OPTIONS feature-detect → MKCALENDAR / extended MKCOL / disable UI +``` + +**The two filter corrections, which are the important part:** + +- ⚠️ **`supported-calendar-component-set` absent means "supports everything", + not "supports nothing".** The draft kept only collections whose set *includes* + VTODO, which silently drops every server that doesn't advertise it. RFC 4791 + §5.2.3 also says the property SHOULD NOT come back from an allprop request — + so **request properties by name**, or you get none of them. Its grammar is + `(comp+)`; an empty element is non-conformant, and `dav4jvm`'s parser starts + all-`false` and would classify it as supporting nothing. Treat empty as all. +- ⚠️ **Classify on `resourcetype`, with a positive test.** The draft had no + resourcetype check at all, so a Depth:1 listing on Nextcloud yields inboxes, + outboxes, notification collections, trash bins and subscriptions as "task + lists". The test must be **`CALDAV:calendar` is present in the set** — *not* + exclusion by `schedule-outbox`, because **SOGo's main personal calendar reports + `collection` + `calendar` + `schedule-outbox` simultaneously** for every + non-Apple client, which is exactly what we are. The positive rule also keeps + shared calendars (which add `CS:shared` alongside `CALDAV:calendar`) and drops + Nextcloud's `nc:deleted-calendar`, which deliberately strips `caldav:calendar`. + Treat resourcetype as an **unordered set**, never by position. + +**Discovery traps, live-probed:** + +| Trap | Detail | +|---|---| +| SRV TXT `path=` | Live at **Posteo** (`path=/`), **GMX** and **Web.de** (`path=/begenda/dav/users/`). Skip it and GMX/Web.de land on the wrong path | +| Posteo | **SRV-only**, on port **8443**; its `/.well-known/caldav` 404s. Hardcoding 443 fails | +| Null SRV target | `_caldav._tcp.fastmail.com` and `runbox.com` return `0 0 0 .` — "explicitly unavailable" | +| Google SRV | Returns a valid record pointing at `calendar.google.com`, which **is not a DAV server** (PROPFIND → 405). A strict RFC 6764 client follows it into a dead end for every `@gmail.com` | +| well-known 401 | **iCloud and Zoho** answer 401 — the endpoint *is* the DAV root and wants auth. RFC-legal; must not be read as failure | +| Redirect downgrade | `dav.runbox.com` redirects **HTTPS→HTTP** in production today | +| Method preservation | A generic HTTP stack may legally downgrade 301/302 to GET, silently breaking PROPFIND | +| `` | RFC 5397 §3 — a **200** whose body means auth failed. Without this check a failed login looks like a successful discovery that found nothing | +| Cross-host home set | Normative (RFC 4791 §6.2.1's own example), and iCloud depends on it: principal on `caldav.icloud.com`, home set on `pNN-caldav.icloud.com`. Allow it, require HTTPS, surface the host change, never send credentials into an unvalidated redirect chain | +| `caldav.fastmail.com` | `d.fastmail.com` is dead — cert mismatch | + +⚠️ **Two `dav4jvm` defects we would inherit:** it handles 301/302/307/308 but +**not 303**, which RFC 6764 §5 names explicitly; and issue #209 — `location` is +mutated in place, so **permanent redirects never reach the caller**. DAVx5 never +rewrites its stored collection URL after a 301 and re-follows on every sync. +Persist the new URL ourselves on 301/308. + +**Read-only detection is softer than the draft assumed.** RFC 3744 §3.7 defines +a `DAV:read-current-user-privilege-set` privilege, so a server may legally return +`current-user-privilege-set` in a **403 propstat**. Default to writable when it is +absent (as DAVx5 does), expand aggregates yourself (`DAV:write` and `DAV:all` +imply content-write; some servers don't expand), and prefer sabre's +`{DAV:}share-access` where offered as the cleanest signal. + +**Creating lists is not always possible.** MKCALENDAR is only *RECOMMENDED* by +RFC 4791 §5.3.1. **iCloud is MKCOL-only** (and MKCOL under `…/calendars//` +returns 412 while `…///` returns 201); **Google has neither**; +**Posteo disables it** despite running sabre. Feature-detect via OPTIONS and +**disable the "new task list" UI** where neither is available. Set +`supported-calendar-component-set` **at creation — it is protected afterwards**, +and follow up with an explicit PROPPATCH for `displayname`, which most servers +ignore in the MKCALENDAR body. + +**Two more VTODO viability landmines:** Zimbra and OX/mailbox.org restrict tasks +to dedicated task lists (MKCALENDAR with `[VTODO]`), and **OX rejects recurring +VTODOs with 400**. And **server-side `CALDAV:expand` on VTODO is broken on +Nextcloud, Baïkal, SOGo, Radicale and Posteo alike** — expand client-side, which +`lib-recur` already gives us. Request `max-resource-size` too: violating it is a +failed PUT, and long `DESCRIPTION`/`ATTACH` payloads reach it. + +⚠️ **`getctag` is not a cheap pre-check** — `caldav-ctag-03` deprecated it in +2015 in favour of RFC 6578, and `DAV:sync-token` is itself PROPFIND-able, so the +same pre-check comes back in the Depth:1 listing we already make. On Nextcloud +they are literally the same value. Keep `getctag` only as a legacy fallback where +`supported-report-set` omits `sync-collection`. + +⚠️ **Auth is not just Basic:** + +- **Baïkal defaults to Digest** (`dav_auth_type`), and **OkHttp has no Digest + support** — square/okhttp#205 has been open for years. DAVx5 carries a + hand-written `BasicDigestAuthHandler` in dav4jvm precisely for this. Take it + (MPL-2.0, same decision as above) or detect the `WWW-Authenticate: Digest` + challenge and emit a real error instead of "wrong password". Baïkal is squarely + in our target audience. +- **Send Basic preemptively via an `Interceptor`**, gated to HTTPS and the + account's own origin. OkHttp's `Authenticator` is reactive-only — an extra round + trip on every request of a PROPFIND-heavy sync, and it never fires at all on + servers that answer 403/404 without a challenge. Note OkHttp strips + `Authorization` on cross-host redirects (correct, but it breaks `.well-known` + discovery across hosts — re-attach only after validating the target). +- **Fastmail requires an app password** and its Basic plan has no CalDAV at all. + **iCloud requires an app-specific password** and 2FA to mint one. **Google is + OAuth2-only** — refuse it with an explanation rather than a 401. Detect these + by domain at account-add time; "wrong password" that is actually "you used your + account password" is the single most common support ticket any CalDAV client + inherits. + +### Credential storage, rotation, revocation + +⚠️ `androidx.security:security-crypto` is not "effectively stalled" — it is +**formally deprecated and terminal**: deprecated at 1.1.0-alpha07 (2025-04), +shipped deprecated in stable 1.1.0 (2025-07), with release notes saying there +will be no subsequent releases. Its successor `datastore-tink` is alpha only. + +⚠️ And the alternative is weaker than implied: **AccountManager stores passwords +as plain `TEXT`** — no encryption or hashing anywhere in AOSP. FBE plus a +same-signature check is the whole boundary. That is DAVx5's actual posture and is +defensible, but state it rather than implying it is secure storage. + +**Decision:** Keystore `AES/GCM/NoPadding`, blob in DataStore. +`setUserAuthenticationRequired(false)` is the default — don't call it. Do **not** +set `setUnlockedDeviceRequired` (breaks background sync). Handle +`AEADBadTagException` / `KeyPermanentlyInvalidatedException` as *re-authenticate*, +not as a crash. Note `getUserData` returns null while the device is locked, so a +boot-triggered sync must wait for unlock. + +⚠️ **Revocation is bidirectional and the draft had neither direction:** + +- **On 401: stop syncing that account immediately**, mark `NEEDS_REAUTH`, notify + with a deep link into the login flow, and **do not retry on a timer**. + Nextcloud's brute-force protection throttles then 429s **per source IP** — a + retry loop on a dead app password takes down the user's *other* Nextcloud + clients on that network and looks like we broke their server. App passwords do + die in the wild (password change, admin revocation, server bug #39615). + Distinguish 401 (re-auth) from 403 (forbidden, do not re-auth) from 429/503 + (back off, honour `Retry-After`). Nextcloud returns 401 with + `PasswordLoginForbidden` when 2FA is on and a real password was used — worth + detecting for a precise message. +- **On account removal: call `DELETE /ocs/v2.php/core/apppassword`** + (this one *does* need `OCS-APIRequest: true`), best-effort. Otherwise + uninstalling never revokes access, and orphaned entries accumulate that the user + cannot identify — see the User-Agent point above. + +--- + +## The sync engine + +- Collection discovery and refresh; per-collection sync state (in a `TaskLists` + `SYNC*` slot, per the squat table). +- ⚠️ **Baseline is `REPORT calendar-query` with a VTODO comp-filter and *no* + time-range**, matching DAVx5 — which deliberately does not use RFC 6578 for + tasks, and omits the time-range *"because some servers don't return tasks + without time at all"*. `sync-collection` is the **optimisation on top**, not + the primary path. The draft had this the wrong way round. +- CTag / sync-token loop (RFC 6578 `sync-collection`) where it works. ⚠️ **The + full-reconciliation path is not a fallback for weak servers — it is a permanent + safety net on every server**, because a pruned change log behind a still-valid + token is undetectable (below). +- Local change detection: `_dirty = 1 OR _deleted = 1`, scoped by `list_id`. +- `If-Match` conditional PUT. +- Backoff and partial-failure recovery. A failed collection must not fail the + account. + +### ⚠️ RFC 6578, and where every shipping client has bugs + +The audit read RFC 6578 in full (no errata, fourteen years on) plus the +w3c-dist-auth threads that are its only authoritative gloss. Calibration first: +**Evolution shipped `sync-collection` in June 2026** against a request open since +2019; **vdirsyncer has declined it for twelve years**; **Thunderbird still has +unlanded patches** for one of the cases below. The library covers about a third. + +**⚠️ Note DAVx5 does not use RFC 6578 for tasks at all** — its own documentation +says *CalDAV tasks: use `REPORT calendar-query`*, because a collection +advertising both VEVENT and VTODO would stream every event change and force a +fetch to discover it isn't a task. That is a real argument for making +`calendar-query` our primary path and `sync-collection` the optimisation. + +1. **Invalidation has no status code.** §3.2 defines the `DAV:valid-sync-token` + precondition and never assigns an HTTP status. Observed: **403** (sabre ⇒ + Nextcloud, ownCloud, Baïkal, and Radicale 3.1.8), **400** (Google, CalDAV and + CardDAV), **409** (Radicale, per its maintainer), **412** (accepted by + Evolution). One server family, two codes across versions. + **Rule: ignore the status; match `` anywhere in the body + on any 4xx.** Thunderbird's CardDAV code accepts only 400 and therefore never + recovers from the 403 that most of the self-hosted world emits. +2. **Initial sync must not report deletions** (§3.4), so a forced full resync + cannot learn what was deleted. **Mark-and-sweep is mandatory** — and the + `initialIncomplete` flag must be persisted *alongside* the token, or a resumed + partial sync sweeps against an incomplete "present remotely" set and **deletes + live data**. +3. **⚠️ Persist the token only after the bodies are applied.** The RFC's own + Appendix B gets this backwards — it associates the new token with the + collection *first*, then fetches. Death in between loses those changes + permanently. Under WorkManager, process death mid-sync is routine, not + exotic. Persist per page, after step 5, atomically with `initialIncomplete`. +4. **Worse than invalidation: a token the server still accepts over a change log + it already pruned.** Returns 207, zero changes, "you're current" — no error, + no recovery, and **RFC 6578 provides no signal for it.** Nextcloud's + `totalNumberOfSyncTokensToKeep` defaults to 10,000 and its own admin manual + warns this "will lead to premature data deletion and synchronization + problems"; Baïkal #1140 has shipped an empty change set *forever* since 2022. + **The only mitigation is periodic full reconciliation** (PROPFIND `Depth: 1` + + ETag diff) on a slow cadence regardless of the token. +5. **Truncation: detect the 507 on the SELF href, not the error element.** + §3.6's `DAV:number-of-matches-within-limits` is a SHOULD and sabre omits it + entirely. Distinguish it from a **507 as the outer HTTP status**, which means + your `DAV:limit` could not be honoured — retry without the limit, don't page. + iCloud emits a SELF response with status **200**; ignore that one. + **Do not send `DAV:limit`** — Nextcloud regressed it to a localised HTML error + page in 28.0.10/29.0.7/30.0.0. Cap by bytes client-side instead, and still + implement 507 handling. Add an iteration cap **and** a no-progress guard: the + RFC never requires the token to advance, and an unchanged token spins forever. +6. **`supported-report-set` is a hint, not a contract.** Radicale advertised + `sync-collection` for years without implementing it; Cyrus 3.8 advertises it + and rejects the mandated empty token. A 207 with no `` must + **degrade to PROPFIND, not throw**. +7. **Tokens are opaque.** §3.2 says they MUST be URIs; Google, iCloud, fruux + (`0`), and grommunio all violate it. Never parse or validate. And **never + reuse a token across collections** — sabre validates only the prefix, then + returns a wrong-but-plausible delta with no error. Key by + `(accountId, collectionUrl)`. +8. **Deletion is `404` at *``* level.** A 404 + inside a `` is a missing *property* on a resource that exists. + Confusing the two nesting levels deletes live data. +9. **Three membership edge cases** (§3.5): create-then-delete between syncs is + reported as removed, so the delete handler must no-op on an href it has never + seen; delete-then-recreate at the same URI is reported as **changed**, so href + identity is not UID identity — re-read the UID from the body; and **ACL churn + may be reported as removal**, so toggling a share can look like mass deletion. + Apply a sanity threshold before acting on a large delete batch. +10. **`sync-collection` never reports collection property changes.** §3.5.1 keys + "changed" on an entity tag, and a calendar collection has no entity body. So + displayname, colour and **read-only status can only be refreshed by PROPFIND + on the home set** — and on sabre, `CalendarHome` does not implement + `ISyncCollection` at all, so there is no sync-collection there to use. + DAVx5 has this exact gap open as its own bug. +11. **`calendar-data` inside `sync-collection` is sanctioned by neither RFC.** + RFC 4791 §9.6 says it "is not a WebDAV property"; it works on sabre only + because that codebase exposes it as one by explicit accident. Request + `getetag` + `resourcetype`, then batch `calendar-multiget` — **and match the + returned hrefs against what you asked for**, because real servers reply with + responses for unrelated URLs. +12. **`getctag` is formally deprecated** by `caldav-ctag-03` in favour of this + REPORT — and every shipping client still keeps it as a fallback. Do the same, + but never compare a ctag to a sync-token. + +**What `dav4jvm` actually gives us:** spec-correct serialisation, `Depth: 0`, +`"infinite"` spelled right, a streaming `Flow` with the token arriving as an +`ExtraProperty`, and typed exceptions. **What it does not:** the truncation loop, +507 detection, any `valid-sync-token` handling (its `Error.kt` says outright +*"there is no logic for subclassing errors"*), mark-and-sweep, `initialIncomplete` +persistence, or multiget orchestration. And its error extraction only parses XML +content-types within a 20 KB excerpt at depth 1 — so a `` served as +`text/html`, or buried behind a PHP stack trace (exactly what ownCloud and Baïkal +emit), yields no recovery. Add a raw-body substring fallback, as Evolution does. + +### ⚠️ Scheduling has a ceiling the draft didn't price + +"WorkManager with network constraints" was the entire treatment. Reality: + +- An ordinary worker is documented for **< 10 minutes**. An initial full sync of a + large collection over a slow homelab link will exceed it. DAVx5's own + `workerWaitTimeout` is 10 minutes. +- Escalating to `setForeground` pulls in `FOREGROUND_SERVICE` + + `FOREGROUND_SERVICE_DATA_SYNC` (missing ⇒ `SecurityException` at targetSdk 34+), + a `tools:node="merge"` override on WorkManager's own service, and the + **Android 15 six-hours-per-24 `dataSync` budget** whose failure mode is a fatal + `RemoteServiceException`. Android 15 also **forbids starting a `dataSync` FGS + from `BOOT_COMPLETED`** — and we have a boot receiver. +- **Android 16 removed the shield**: jobs running alongside a foreground service + now obey the job runtime quota, and the `active` bucket is capped at 20 min / + rolling 60 min. + +**Therefore:** make sync **chunked and resumable** — persist the sync-token/ETag +cursor per collection so a killed worker resumes rather than restarts. Hard socket +and wall-clock timeouts. Periodic sync is a plain `PeriodicWorkRequest`, no FGS. +"Sync now" from a visible screen uses `setExpedited(RUN_AS_NON_EXPEDITED_WORK_REQUEST)` +— and implement `getForegroundInfo` unconditionally, since omitting it crashes +below API 31 and we support 29. FGS only for user-initiated full syncs, with +`Service.onTimeout → stopSelf()` as a backstop. Play requires a video demo per +declared FGS type. + +⚠️ **Be honest about cadence.** `PeriodicWorkRequest`'s 15-minute floor is +nominal. In the `rare` and `restricted` buckets network access is disabled +outright; Doze allows idle apps network roughly **once a day**. Combined with +unmetered-only, worst case is genuinely "once overnight". Promise eventual +consistency, and sync hard on app open and on connectivity-regained. + +### ⚠️ Server reality — the matrix, audited + +The draft listed six servers as a test matrix. What they actually do: + +| Server | VTODO | The thing that will bite | +|---|---|---| +| **Nextcloud** (sabre) | ✅ | ⚠️ **Never round-trip a body fetched from a shared calendar** — see below. Never send ``. Per-calendar UID uniqueness ⇒ **409 `no-uid-conflict`**. Trashbin renames the href to `-deleted.ics` ⇒ 403 on delete-then-recreate. MKCALENDAR is rate-limited 10/hour ⇒ 429, max 30 calendars ⇒ 403 | +| **Baïkal** (sabre) | ✅ | Handles `` and 507 **correctly** — the reference implementation for that path. Never prunes its change log, so tokens stay valid forever. Defaults to **Digest** auth | +| **Radicale** | ✅ | `supported-calendar-component-set` is **never enforced** — a VEVENT PUT into a VTODO-only collection is accepted. Advertises three reports it does not implement. Its VTODO time-range now implements all eight RFC 4791 §9.9 rows — **the widespread "Radicale doesn't do time ranges" claim is stale** | +| **SOGo** | ✅ | ⚠️ **Never invalidates a sync token** (`valid |= …` makes the check always pass) and tokens are **second-granularity**, so you re-receive up to a second of changes every sync. ⚠️ **The ETag is a row-version counter, and the body is regenerated per-principal** — same ETag, different bytes. Cannot create a VTODO-only collection at all | +| **Fastmail** | ✅ | ⚠️ **Reframe as supported.** The backend does VTODO fine; their own UI hides task-only calendars by design. Create collections as **mixed `VEVENT,VTODO`** so the list doesn't vanish from Fastmail's UI. Requires an app password; Basic plan has no CalDAV | +| **iCloud** | ⚠️ | **Reminders left CalDAV at iOS 13.** A new VTODO collection syncs bidirectionally but is **invisible in Reminders.app forever**. Market it as "store tasks in iCloud", never as "sync with Apple Reminders". Cheap detection: no VTODO-capable collection in the home set ⇒ upgraded account | +| **Google** | ❌ | ⚠️ **Drop it.** First-party docs: *"Doesn't support VTODO or VJOURNAL data"* and no MKCALENDAR. Refuse with an explanation, don't fail with a 401 | + +**Two data-destruction landmines, both confirmed from server source:** + +1. **Nextcloud rewrites task bodies on GET from a shared calendar.** + `CalendarObject::get()` strips `VALARM` on read-only shares, and for + `CLASS:CONFIDENTIAL` reduces the object to a VEVENT-shaped whitelist that + **deletes `DUE`, `STATUS`, `COMPLETED`, `PERCENT-COMPLETE`, `PRIORITY` and + `RELATED-TO`** — every property that makes it a task. **The ETag is left + untouched**, so `ETag ≠ md5(body)` and re-PUTting what you downloaded destroys + the task. Baïkal never does this. +2. **sabre runs vobject `REPAIR` on every PUT** unless you send + `Prefer: handling=strict` — adding UID/DTSTAMP/PRODID/VERSION — and when it + modifies the object it **suppresses the ETag response header**, so you must + re-GET rather than assume. It also 415s on **`DUE` < `DTSTART`**, value-type + mismatch between them, multiple UIDs, mixed component types in one resource, + and a present `METHOD`. + +**Three consequences for the design:** + +- **`supported-calendar-component-set` is not an invariant.** Unenforced on + Radicale, discarded by SOGo, immutable on sabre (403 if you try to change it). + Filter on it, but never rely on it. +- **VTODO scheduling exists nowhere.** sabre's own docs: *"We don't do VTODO + scheduling yet, and only support VEVENT."* Treat `ORGANIZER`/`ATTENDEE` on a + task as inert text to round-trip — which is a mercy, since it also means + scheduling never rewrites our objects or suppresses our ETags. +- **Never trust an ETag as a content hash** (SOGo, and Nextcloud shares). + +### ⚠️ Writing: conditional PUT, and the conflict policy that had to change + +**`If-None-Match: *` on create. `If-Match` on update and DELETE.** The draft said +"`If-Match` on every PUT", which omits the creation case entirely — RFC 4791 +§5.3.2 asks for `If-None-Match: *` there, and all three reference clients send it. +Without it, a filename collision (two devices minting the same UID, or a sanitiser +folding two UIDs onto one name) makes the second PUT **silently destroy the +first**, with no ETag to protect it because we have never seen the resource. + +⚠️ **412 means three different things** and the draft's single rule conflated them: + +| On | Means | Do | +|---|---|---| +| create (`If-None-Match`) | the filename is taken | re-fetch that href; adopt if the UID matches, else regenerate the name as a UUID | +| update (`If-Match`) | the server has a newer version | conflict resolution, below | +| update, resource gone | no selected representation, so the condition is false — **spec-correct** (Radicale and DAViCal do this) | `HEAD` to disambiguate; 404 ⇒ delete-vs-edit, not a conflict | + +⚠️ **The proposed conflict policy is unimplementable and is withdrawn.** The draft +said the local version would be *"preserved rather than discarded — a duplicate +task, marked, in the same list."* RFC 4791 §4.1 requires a **UID to be unique +within a collection**, and every target server enforces it: Nextcloud, Radicale +and SOGo all answer **409 `CALDAV:no-uid-conflict`**. So the preserved duplicate +can never be uploaded — same UID fails forever, a new UID forks a task that never +reconciles. The "visible clutter" the design wanted is either a permanently +failing row or a permanent fork. + +**Pick one and write the consequence down** (open question 2, now with real +options): **server-wins and discard the local edit** — DAVx5's stated policy — +or **server-wins and fork under a new UID**, marked in the UI, with the new UID +persisted so the fork is first-class from that moment. Prompting is unavailable; +a background sync has nobody to ask. + +⚠️ **The ETag may be weak or absent, and then `If-Match` can never succeed.** +RFC 4791 §5.3.4: when the server does not store your bytes verbatim, *"a strong +entity tag MUST NOT be returned"*. RFC 9110 §13.1.1: *"A weak entity-tag cannot be +used with If-Match."* On sabre this is the **default path** — `validateICalendar` +runs vobject `REPAIR` unless you send `Prefer: handling=strict`, and +`Server::createFile` then deliberately withholds the ETag. Worse, **weak ETags +also arrive from the user's reverse proxy**: any gzip-compressing nginx, +Cloudflare or Traefik in front of Nextcloud produces them, so the risk tracks the +user's deployment rather than their server software. + +Therefore: send **`Prefer: handling=strict`** to sabre-based servers — the +cheapest single fix in this whole audit, since it preserves both our bytes and +the ETag. Request `Accept-Encoding: identity`. Strip `W/` and keep a weak flag. +**If a PUT returns no ETag or a weak one, discard it and re-fetch** for the strong +validator *and* the server's canonical body. Bound every 412 retry loop. + +⚠️ **Errors are not one status.** RFC 4791 §5.3.2.1 defines **eleven** +preconditions, and the one we will hit most is not in the draft at all: **sabre +returns 415** for a VTODO whose `DUE` precedes `DTSTART`, whose `DUE`/`DTSTART` +value types disagree, or which carries a `METHOD`. The first two are reachable +from ordinary UI actions and must be validated client-side. Also: **507 MUST NOT +be auto-retried** (RFC 4918 §11.5 — quota exhaustion is common on hosted +Nextcloud, and a generic backoff loop violates the spec), and **5xx is not safely +retryable either** — a contradictory `RRULE`/`EXDATE` pair returns 500 from +Nextcloud and will do so forever. + +So line-for-line with "a failed collection must not fail the account", add its +twin: ⚠️ **a failed resource must not fail the collection.** Per-resource +quarantine with a failure counter, not backoff. A single HTTP 400 has halted all +calendar sync in DAVx5 for weeks. + +**DELETE needs the same care:** conditional on `If-Match`; **404/410 count as +success**; a resource deleted locally that was never uploaded is never DELETEd. +And Nextcloud's trashbin renames the href to `-deleted.ics`, so +delete → recreate → delete the same href returns **403** — which task apps hit +constantly, because they reuse hrefs. + +**href and UID are unrelated.** RFC 4791 §5.3.2 opens by saying the URL *"is +entirely arbitrary and does not need to bear a specific relationship"* to the +content, and `.ics` is MAY. Sanitise filenames following vdirsyncer's rule — +`a–zA–Z0–9_.-+`, **excluding `@`**, because some servers percent-encode it in the +path and then reject or "repair" the URL, and RFC 4791's own example UID is +`…@example.com`. Cap the basename around 200 bytes; fall back to a UUID. + +### Manifest and permissions for phase 3 + +⚠️ The draft named only `INTERNET`. Actually needed: `INTERNET`, +`READ_SYNC_SETTINGS`, `WRITE_SYNC_SETTINGS`, `ACCESS_NETWORK_STATE` (merged in by +`work-runtime`, but it shows in F-Droid's permission diff), plus +`FOREGROUND_SERVICE` + `FOREGROUND_SERVICE_DATA_SYNC` if the FGS route is taken. +The authenticator `` must be `android:exported="true"` guarded by +`android:permission="android.permission.ACCOUNT_MANAGER"` — note +`android.permission.ACCOUNT_AUTHENTICATOR` **does not exist**. + +Also missing from `libs.versions.toml` entirely: `androidx.work`, +`androidx.hilt:hilt-work`, `androidx.browser`. With Hilt that means a +`HiltWorkerFactory`, removing the default `WorkManagerInitializer`, and an +`@EarlyEntryPoint` for the authenticator service. + +### Two network facts for homelab users + +- ⚠️ **Ship a `network-security-config` with ``.** Since + Android 7, a user who correctly installs their private CA into Android's store + is *still* not trusted by apps. Cleartext `http://` is blocked by default since + API 28; any escape hatch must be a narrow, warned, per-account opt-in — Play's + User Data policy requires modern cryptography in transit. +- ⚠️ **Android 17 / targetSdk 37 breaks LAN CalDAV.** Local network protections + become mandatory: TCP to a local address and `.local` resolution require the + runtime `ACCESS_LOCAL_NETWORK` permission. The failure mode is a **connection + timeout, not a `SecurityException`** — "my Nextcloud at 192.168.1.50 just + hangs", the worst bug-report shape there is. We are safe at targetSdk 36 (which + gets an implicit grant) and must **not** request it before targeting 37 — but + `compileSdk` is already 37 and Play's floor rises annually, so this is a + scheduled break aimed precisely at the self-hosting demographic. + +--- + +## External mode's future + +`STORAGE-AND-SYNC.md` keeps Posture A as a user choice; a later draft proposed +replacing it with a one-time importer. Premature — it argues against something +shipped and working — but it is the right question one phase early. + +Once we sync ourselves, External mode's only job is reading tasks in someone +else's app. Costs are real and permanent: capability divergence (tasks.org's fork +is DB 22 and lacks `is_recurring`, which is why `TaskMapper.task` derives +recurrence from `rrule`/`rdate`), per-backend UI degradation forever, two +dangerous permissions in a static manifest, a doubled device matrix. + +**Decide before phase 1** — it determines whether the mapper and UI stay +dual-capable. Open question 3. + +If retired, the importer spec is sound: read-only, one-time, idempotent; identify +local lists by `ACCOUNT_TYPE`, treating unknown types as synced; **import the +`Properties` table, not just `Tasks`** (categories, alarms, `RELATED-TO`, `X-` +props — the commonly forgotten half); preserve `_UID`s; persist +`(authority, _ID, uid)` for idempotency; and while scanning, read the *names* of +synced lists so the UI can say *"these 3 lists come from cloud.example.de — add +that account to bring them back"*. + +--- + +## Play compliance + +⚠️ Absent from the first draft entirely. + +- **A privacy policy is mandatory regardless of collection**, linked both in Play + Console and **inside the app**. +- **"Not collected" is not defensible.** Play defines collection as transmitting + data off the device *irrespective of recipient*. Neither the on-device nor the + ephemeral exemption applies, and the E2EE exemption doesn't survive TLS to a + server that reads plaintext. File **Collected, not Shared**, encrypted in + transit. There is no credentials category, but "authentication information" is + explicitly named as personal and sensitive data. +- **Account Deletion policy does not apply** (offline-created accounts are out of + scope), but ship a "Remove account and delete local data" action anyway — + cheap insurance against a reviewer pattern-matching. +- **Do not ship `REQUEST_IGNORE_BATTERY_OPTIMIZATIONS` in the Play build.** + Generic server sync is not on the acceptable-use list. Use + `ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS`. DAVx5 declares the former and is + F-Droid-safe on it; we would not be. + +--- + +## What lands in floret-kit + +| Candidate | Kit module | Note | +|---|---|---| +| DAV client + iCalendar parse/serialise | new, e.g. `core-dav` | **the big one.** Calendula needs the same primitives. Design for two consumers from the start | +| Sync-adapter + authenticator scaffolding | new, e.g. `core-sync` | including the stub-adapter→WorkManager bridge, which is pure mechanics | +| Nextcloud Login Flow v2 | with `core-dav` | pure protocol, zero task knowledge | +| Credential storage | kit | Keystore mechanics, not domain | +| Third-party licence screen | kit | Calendula needs it the moment it takes any of the above | + +**Stays app-local:** the VTODO ↔ `TaskContract` mapper (domain, and where all the +judgement calls live), `ICalendarWriter`, conflict policy and its UI, and +everything about storage modes. + +--- + +## Test strategy + +- **Round-trip corpus first** — VTODO fixtures from each target server; assert + `parse → store → serialise` is byte-stable for untouched properties. The + mapper's specification, not its regression net. +- **Server matrix:** Nextcloud, Radicale, Baïkal (**on Digest**), SOGo, Fastmail, + iCloud — with the per-server traps above as named test cases. Google is out. + Two that must be explicit tests, because both silently destroy data: + **round-tripping a task from a shared Nextcloud calendar**, and **an + ETag-unchanged body change on SOGo**. +- **Conflict scenarios:** concurrent edit, delete-vs-edit, collection removed + server-side, credentials revoked mid-sync. +- **Interop:** edit the same task from Nextcloud web and from tasks.org + DAVx5. +- **Device, not Robolectric,** for account paths — `ProviderAccountCleanupTest` + skips on ARM64. Two specific device cases the audit added: **restore a cloud + backup onto a fresh device** and confirm nothing is pruned; **remove the + account** and confirm only our lists go. +- ⚠️ **Minified-build tests.** `:app` runs `isMinifyEnabled` + `isShrinkResources` + in release, and `proguard-rules.pro` already documents two R8-pruning outages. + ical4j resolves through seven `META-INF/services` files and instantiates its + cache from a class-name string — it needs `-keep class net.fortuna.ical4j.** { *; }` + plus ~8 `-dontwarn` lines, and is effectively unshrinkable. Without them this + fails in `release` only. + +--- + +## Open questions + +1. **dav4jvm + cert4android distribution *and* Java target.** Scoped JitPack, + vendor, or in-house verbs? Now compounded: 4.x requires **Java 21** and we + target 17 across `:app` and all of floret-kit. Vendoring recompiles at our own + target and freezes the API churn — it looks better than it did. Before phase 2. +2. **Conflict policy** — preserve-local-on-412, or documented LWW? +3. **External mode** — survives, or becomes an importer? Before phase 1. +4. **Canonical recurring-completion behaviour** — and specifically, ⚠️ **do we + honour `:provider`'s `Detaching` processor (model d), or bypass it and write + `RECURRENCE-ID` overrides (model a)?** No longer an open-ended taste question: + our storage layer already answered it and we have to ratify or override that. + Moved up to **phase 1**. +5. **The DAVx5 enum ask** — worth filing, and what compatibility we owe if it + lands. +6. ⚠️ **New: does Local→Synced migrate, or do synced lists start empty?** See + [the migration section](#-the-migration-that-was-believed-not-to-exist). +7. ⚠️ **New: lib-recur — pin at 0.12.2, or rewrite the provider's iterators?** + +Answered elsewhere and **not** open: the account model (`AccountManager` **plus a +stub sync adapter**), `ical4android` (superseded by `synctools`, GPLv3), and the +storage question. + +--- + +## Dead ends — do not revisit + +- **Depending on DAVx5 for sync.** Settled in `STORAGE-AND-SYNC.md`. +- **`synctools` / `ical4android`.** GPLv3. The temptation recurs because it does + exactly the right mapping against exactly our schema. +- **Rewriting storage to Room before sync exists.** [See above](#settled--the-storage-question-is-not-reopened-here). +- ⚠️ **AccountManager + WorkManager with no registered sync adapter.** Not a + design choice — a silent no-op at targetSdk ≥ 34. +- ⚠️ **Writing through the `instances` URI as a sync adapter.** The flag is + ignored there; every such write dirties the row and forks an override. +- ⚠️ **Hand-rolled TrustManager to avoid a GPL licence cert4android does not + have.** + +--- + +## Related + +- [`STORAGE-AND-SYNC.md`](STORAGE-AND-SYNC.md) — where task data lives. This is + its step 5. +- [`../provider/PROVENANCE.md`](../provider/PROVENANCE.md) — every deviation from + upstream dmfs. Change 1 is load-bearing for the account model here. +- [`ARCHITECTURE.md`](ARCHITECTURE.md) §4 — the data seam the adapter writes + underneath. diff --git a/provider/PROVENANCE.md b/provider/PROVENANCE.md index f911df4..6cfcefe 100644 --- a/provider/PROVENANCE.md +++ b/provider/PROVENANCE.md @@ -159,7 +159,7 @@ drift apart. Keep that convention. `contenttestpal`, which are JitPack-only; adding JitPack would widen the dependency trust surface for test-only code. ⚠️ This is the one place vendoring lost coverage — those were the provider's *integration* tests - (recurrence, reparenting, instances, observers). The 51 JVM tests in + (recurrence, reparenting, instances, observers). The 56 JVM tests in `src/test` all pass and are retained. 15. **`agendula_provider_changed_receivers` emptied.** Upstream notifies `org.andstatus.todoagenda`, which listens for changes to the *dmfs* authority @@ -175,7 +175,7 @@ drift apart. Keep that convention. Unlikely to ever be worth it — upstream 1.4.2 is from 2021 — but if it is: the `AGENDULA CHANGE` markers are the complete list of what to reapply, `git log` on -this directory is the audit trail, and the 51 JVM tests are the safety net. +this directory is the audit trail, and the 56 JVM tests are the safety net. Re-read change 1 before touching anything account-related. ## Known-unverified