diff --git a/docs/ELECTRON-OFFLINE-ENGINE-DESIGN.md b/docs/ELECTRON-OFFLINE-ENGINE-DESIGN.md index 70c5590d..e84b5ece 100644 --- a/docs/ELECTRON-OFFLINE-ENGINE-DESIGN.md +++ b/docs/ELECTRON-OFFLINE-ENGINE-DESIGN.md @@ -1,7 +1,42 @@ +> # ⚠️ SUPERSEDED — this is not what was built +> +> This document designs a **full offline mail replica**: a persistent background sync engine with +> JMAP `Foo/changes` cursors, three state machines, a retry ladder, reconcile/sweep logic and an +> epoch-fenced multi-account registry. **That scope was dropped.** After the adversarial review +> (`ELECTRON-OFFLINE-ENGINE-REVIEW.md`), the human narrowed the requirement to *"a SQLite index we +> can prompt against"* — retrieval to feed an LLM, refreshed on each delivery/change event. +> +> **What was actually built:** `lib/mail-index/**` + `app/api/offline/{reindex,search}` — an +> encrypted SQLite/FTS5 index over mail, calendar, contacts and file *metadata*, written by an +> ordinary request-scoped API route that the renderer's existing live JMAP push connection calls +> when something changes. No background worker, no cursors, no resident credentials. Staleness +> between refreshes is acceptable by design. +> +> Most of the review's CRITICAL and HIGH findings **stopped existing** rather than being fixed: C2, +> C3, C4, H1 and H2 were all consequences of a long-lived worker holding credentials, and there is +> no worker. +> +> **Still accurate and still worth reading here:** +> - §3 — the SQLite/SQLCipher binding investigation. `@signalapp/sqlcipher` is what shipped, for the +> reasons given, and the `PRAGMA key` silent-no-op landmine is real (the shipped code asserts +> `cipher_version` returns a non-empty *string*, per the review's correction). +> - §6 — `safeStorage`, including the Linux `basic_text` hazard. Shipped as described, with +> `getSelectedStorageBackend()` correctly guarded to Linux only (a review finding). +> - §1 — the codebase survey (auth model, push pipeline, CSP, account model). All verified. +> - §2.4's hosted-deployment gate (`VNCMAIL_DESKTOP_STORE_DIR`) — shipped, and now covered by a test. +> - §14 — what was and was not empirically verified. +> +> **Wrong in hindsight, beyond the scope change:** §2.1's claim that Option A needs no new secret +> handling (the review's C2 is right — credentials are request-scoped, not resident); and §2.1's +> assumption that Next's output file tracing would carry the native module (it does not — the +> standalone build needs an explicit copy step, now in `scripts/assemble-standalone.mjs`). + # Electron Offline Engine — Design -Status: **design only, not implemented.** Nothing outside this file has been changed on this -branch. `electron/main.ts`, `electron/preload.ts` and `lib/jmap/client.ts` are untouched. +Status: **superseded design, never implemented.** See the note above. Nothing outside this file was +changed by the pass that wrote it; `electron/main.ts`, `electron/preload.ts` and +`lib/jmap/client.ts` were untouched *at that time* (`main.ts` has since gained the index's store-dir +and key-channel wiring, which is a small fraction of what this document describes). Repo: `brvncde-dotcom/vncmail-plus`, branch `claude/electron-offline-design`, worktree `~/worktrees/vncmail-electron-sqlite`. Based on `claude/electron-desktop` (the working desktop diff --git a/docs/ELECTRON-OFFLINE-ENGINE-REVIEW.md b/docs/ELECTRON-OFFLINE-ENGINE-REVIEW.md index 4f5bf7a6..e5460bb6 100644 --- a/docs/ELECTRON-OFFLINE-ENGINE-REVIEW.md +++ b/docs/ELECTRON-OFFLINE-ENGINE-REVIEW.md @@ -1,3 +1,30 @@ +> # ⚠️ SUPERSEDED — reviews a design that was not built +> +> This reviews `ELECTRON-OFFLINE-ENGINE-DESIGN.md`, which was **dropped**. Its findings were the +> direct cause: seeing them, the human narrowed the requirement from a full offline mail replica to +> *"a SQLite index we can prompt against"*, refreshed on each delivery/change event. What shipped is +> `lib/mail-index/**` + `app/api/offline/{reindex,search}` — see that doc's superseded note. +> +> **This review did its job.** Most of its severe findings were resolved by the scope change +> removing the thing they were about, which is the strongest outcome a review can have: +> +> | Finding | Outcome | +> |---|---| +> | **C1** — `@signalapp/sqlcipher` in `dependencies` breaks both Alpine `docker build`s | **FIXED as specified.** It is an `optionalDependencies` entry with a guarded runtime require (`lib/mail-index/binding.ts`). Both `docker build`s verified passing, and the require verified failing cleanly with MODULE_NOT_FOUND inside the musl image. | +> | **C2** — credentials are request-scoped, so no persistent worker can hold them | **MOOT.** There is no worker. Indexing is a normal API route using the request's own `jmap_stalwart_ctx` cookie, via the existing `lib/stalwart/credentials.ts`. | +> | **C3** — the OAuth-refresh mitigation is itself the bug | **MOOT, and avoided by construction.** The indexer never touches the refresh-token cookie; it only reads an already-minted auth header, so it cannot rotate a token into a response nobody reads. | +> | **C4** — shared `registry.json` breaks the multi-account safety premise | **MOOT.** No registry, no epochs, no concurrent workers. | +> | **H1** — a server-side engine can't read a renderer-only setting | **MOOT.** The renderer decides when to index. | +> | **H2** — key handoff sequencing, and a nonce via env is readable by same-user processes | **FIXED.** The key crosses on an **inherited file descriptor**, never env, and is fetched per job and zeroed after — not held. Sequencing is moot: the key is fetched when a job runs, not at spawn. | +> | **H3** — local unread-count arithmetic needs a coherence story | **MOOT.** A retrieval index does not need to stay coherent with live unread counts. | +> | **H4** — no cap on concurrent multi-account sync | **MOOT.** One request, one account. | +> | *medium/low:* `getSelectedStorageBackend()` is Linux-only and would crash elsewhere | **FIXED** — platform-guarded. | +> | *medium/low:* `cipher_version` check would pass vacuously on zero rows | **FIXED** — the shipped assertion requires a non-empty *string*, and a test reads the raw file bytes for a plaintext canary. | +> | *medium/low:* the two bindings are not "the same code either way" | **CONFIRMED true, the hard way.** `@signalapp/sqlcipher` rejects varargs params (`TypeError: Params must be either object or array`) where better-sqlite3 accepts them. Documented in `binding.ts`. | +> +> Reviewing this file's own accuracy: its two re-executed claims (the binding working in Electron 43, +> and `PRAGMA key` being a silent no-op) both held up and both shaped the shipped code. + # Adversarial review: `docs/ELECTRON-OFFLINE-ENGINE-DESIGN.md` Reviewer: independent agent, fresh context, no relation to the design's author. 2026-08-04.