rastrillo / native Public

Spec review: 2026-09-23-offline-kit.md @ a845b9a

Reviewer: claude~paul (agent account), highest-capability-model review required by AGENTS.md before this becomes a plan. Read against origin/main of docs 7543cf5, sheets b9a92e3, calendar f6aa41c, memoria 02cb530, meet 3846ca4, and rastrillo/native main 2a5d07b, plus offline-audit/*.md.

VERDICT: APPROVE-WITH-CHANGES. The shape is right: outbox as the unit of durability, status from outbox state, one kit with no conflict policy, Docs first and Sheets as the correcting adopter. Five findings are load-bearing (H1–H5) and must be fixed in the spec before a plan is cut; none of them changes the architecture.

State update. rastrillo/native main now includes the chain merge (2a5d07b, "Merge branch 'merge/native-apple-chain' into main"; Sources/RastrilloApple/* and server/ are on main). §0 should cite main, not eacc073, and K0 reduces to: re-pin the five apps off 2295259 (Calls off 4c61d62) and bump each server's amadan.net/rastrillo/native/server off 33ffe99.

Findings

H1 — High — §4.3 — I1 does not "hold by construction" as written. CRDTReplica.command mutates the actor's lastGood (CRDTReplica.swift command) before DocsModel.drainEdits gets control back and calls the durable enqueue. The replica writer (a CoalescedRunner) and refresh → replica.apply both take state on the replica actor, so either can capture a lastGood that contains a command whose entry is not yet on disk. Crash there and the persisted replica holds an edit the outbox lacks: exactly the stranded case I1 exists to rule out. "Snapshots on the main actor after that" does not fix it, because the bytes are captured on the replica actor. Fix: a durability watermark. command already computes the post-command state; return it, and hand it to the writer only after enqueue returns. The writer persists (state, sequence) only when commandsApplied == commandsDurable at capture, otherwise re-arms. The K3 property test must include "writer tick between command and enqueue" as an event, or it cannot fail.

H2 — High — §4.3 "Reopening" — a reopened replica must be state + lane, not state alone. The writer runs 2 s after the last publish, so after a kill the replica is legitimately older than the lane. The spec reopens from the stored state and never applies the lane. Offline, the person sees their last edits missing, retypes them, and once the lane drains the document holds the text twice. That is the one duplication path in the design, and it is user-visible. Fix: after open(update:), apply every entry of doc:<key> in seq order (Yjs integration is idempotent; already-present entries are no-ops), then publish. Same for drafts. Write it as the second half of I1.

H3 — High — §4.4 table — "400 → terminal hold" is wrong for ErrUpdateLogFull. updateIsClientFault returns true for core.ErrUpdateLogFull (handlers.go:928-933), so it is answered 400 with the same docs.error.badupdate key as a malformed update. Store.ApplyUpdate writes the snapshot before Apply and, for a document at core.MaxRetainedUpdates (4096, core.go:143), expects that POST to fail and the client's retry to succeed (the comment block above h.Apply in store.go). A long offline replay into a busy document will hit this, and the spec would freeze the lane with "Save as new document". Fix (server, small): give ErrUpdateLogFull its own status (503 + Retry-After, or 409) so the kit backs off. Fix (client): never hold on a first 400; hold only when the same bytes are refused again after a successful ?since= catch-up. Add a test: log at cap → POST → retry → 204.

H4 — High — §3.6 "Unreadable store" — "key missing → treat as empty and remove" will delete outboxes. With WhenUnlockedThisDeviceOnly, SecItemCopyMatching returns errSecInteractionNotAllowed whenever the device is locked (an app resumed in the background, a launch before first unlock), and .complete files return EPERM after lock. NativeCredentials.read today collapses every status into nil (NativeCredentials.swift:16). The rule as written turns "locked" into "wipe unsent changes". Fix: only errSecItemNotFound with a populated directory means "key lost". Every other status, and any file I/O error, means unavailable: don't read, don't delete, keep the status, retry on .active/unlock. A file that fails its GCM tag is removed alone and reported once; never the directory. The drainer's beginBackgroundTask continuation must treat a store write failure as .retry.

H5 — High — §3.6 AAD + §4.5 drafts — the relative-path AAD breaks the draft attach. §4.5 renames draft-<token>/ to <sqid>/ on attach. With the path as associated data every sealed file under it then fails to open, and H4's path fires on a freshly created document. outbox.retarget has the same hazard if the lane is ever part of the AAD. Fix: never rename. <key> is draft-<token> for life for a document born on this device; meta.sqid is the pointer; a small cache/docs/index maps sqid → key. retarget then leaves the kit API. Make the AAD explicit: accountID | format version | relative path.

M1 — Medium — §4.3 — snapshot_pending is unnecessary; the premise is wrong. yrs 0.23.5's encode_state_as_update_v1 ends with merge_pending_v1 (vendored native/vendor/yrs-0.23.5/src/transaction.rs:138-143, which is what native/crdt builds against; core/src/snapshot.rs's header says the same). It is encode_diff_v1 that drops the pending queue. The audit was mistaken and the spec inherited it. Today's snapshot op is already pending-inclusive. Fix: drop the new op; persistentState() is snapshot. Port a_snapshot_with_a_parked_update_heals_once_its_dependency_arrives (core/tests/snapshot.rs:121) into native/crdt/tests so the property is pinned against the vendored crate with its backports.

M2 — Medium — §3.3 drainer table vs §3.2 — .offline on a satisfied path strands the drainer. timedOut/cannotConnectToHost are .offline; the reaction is "stop all lanes, wait for connectivity.changes", which fires only on path change and foreground. A 30 s timeout on a 512 KiB POST while Wi-Fi stays up (and the "commit, then lose the response" case) means nothing retries until the app is foregrounded again. Fix: .offline with path == .satisfied → Backoff; .offline with path != .satisfied → wait. A stream open (serverReachable → true) should also kick the drainers.

M3 — Medium — §3.3 replace — compaction breaks lane FIFO. The merged entry takes a new, higher seq, so entries enqueued after the compacted head sort before it. They reach the server first and are parked as pending until the merged entry arrives: safe, but it splits one logical prefix over two batches and leaves parked structs on the server in between. Fix: the merged entry keeps the seq of the oldest entry it replaces; write <seq>.entry.tmp, rename over the head file, then delete the rest. Crash outcome is "duplicates only", which is what the section wanted, and the "not atomic" caveat can go.

M4 — Medium — §5 coalescing — "first base, last raw" is wrong once an entry is in flight. Batch commits, response lost (→ .offline per §3.1), user edits B7 again offline: the coalesced entry is {base: old, raw: newer}; the server holds the committed middle value, which equals neither, so the person's own write comes back as a conflict. Fix: an in-flight entry is frozen; a later edit to the same cell makes a new entry with base = the in-flight raw; send packs at most one entry per position per batch (SetBatch refuses duplicates, handlewritebatch.go:22-29). The rule is "one sendable entry per cell". update must refuse an in-flight id.

M5 — Medium — §3.7 / D2 — sliding expiry with an optional ceiling is an unbounded theft window. A stolen native cookie that keeps being used never expires except by explicit revocation, and the client stores no expiry, so nobody sees it. Make the absolute ceiling mandatory (recommend 180 days from AuthTime; a year at most), native sessions only. Implementation note the spec should carry: sessions has no native flag and no renewal path (lookup is read-only, sessions.go:162; migration 0001 has no column), so this is a carlosframework/rastrillo change (all five apps pin v0.23.0): a kind + renewed_at column and a best-effort renewal in resolve, then a module bump. Revocation-by-row-delete continues to work.

M6 — Medium — §3.5 subject guard — "no stored subject → accept the first one seen" defeats the guard for every existing account. Every account that exists before this ships has no subject, so its first re-sign-in as another person drains its outbox under that person. Apps hold no real data: treat subject-less accounts as session-ended at first launch after the upgrade (before any outbox exists), or add a tiny /native/whoami and fetch it on the first online request. Also name the HMAC key (stable across deploys, per app: the instance/sessions key, not a per-process random), and say plainly that the guard is client-enforced; the server cannot tell whose outbox a cookie drains.

M7 — Medium — §5 replay order and the stamps rule. The rule itself holds for partial batches: with Stamps = cells actually stamped and the all-conflict case answered like a format response (version = next stamp, stamps absent → slack 1), version > held + max(stamps, 1) stays exactly "an edit was missed", including on the SSE copy (handleCellBatchPost publishes the same gridResponse). Two implementation notes: compare base against staged.rawText inside the transaction, not h.rawText before it; and don't call SetReport for current == raw no-ops, or Stamps (today len(writes), handlewritebatch.go:96) will over-count. The real problem is order: after an offline period every drained batch's version exceeds held + stamps (other devices wrote), so each response triggers the 25×50 chunked refetch. Refetch once first (the SSE onOpen snapshot already does), keep local raws painted through the overlay, then drain.

L1 — Low — §3.6 — say what the nonce and the AAD are. AES.GCM.seal without a nonce draws a fresh random 96-bit nonce per call; .combined is nonce‖ciphertext‖128-bit tag. Random-nonce GCM is safe below 2^32 seals per key (SP 800-38D); the Docs writer at one seal per 2 s of typing is nowhere near it, but state the bound and rotate the key at 2^31 seals. The key is an AES-256 SymmetricKey, one per account, same Keychain class as the session, never synchronizable. On Mac the sandbox container is the only file protection; say that is accepted.

L2 — Low — §3.3 — define "durable on return". Data.write(.atomic) survives a process kill, not a power loss without F_FULLFSYNC. Promise the first, test the first.

L3 — Low — §8 — stale. meet already serves /native/request and /native/exchange (internal/calls/app.go:437-438, internal/calls/native_auth.go), its own implementation carrying a target field. The work is adopting NativeConnection against it (JSONDecoder ignores target, so the reply decodes) or swapping to the shared module, plus "Calls" in the module's allowlist (main's server/auth.go still names "Meet").

L4 — Low — §0. Cite main (2a5d07b). Everything else in §0 re-verifies: docs pins 2295259 and server module 33ffe99; sheets stamps at b9a92e3; calendar has no delete route (routes.go:112-117); memoria client_id at native.go:83-122.

(4) Is the kit API minimal enough?

Nearly. With H5, retarget goes. update is Sheets-shaped (and needs M4's in-flight rule), so defer it to the Sheets slice. Hold.blocksLane defaults to true and gains false at Sheets. Draining switched-away accounts is machinery Docs never exercises; v1 keeps their outboxes and drains when selected, which satisfies principle 4 (nothing is deleted). Keep replace with M3's semantics. sendable(lane:limit:) should be by bytes or unbounded, since Docs batches by 512 KiB, not by count. After that, Docs exercises every method that ships, which is the "one builds, one adopts" bar.

(5) Test strategy

Right primitives, one gap: the fault model must be able to fail the store and time the writer, not only the transport. Recommend a FaultingBlobStore wrapper with a crash schedule ("throw SimulatedCrash at op N and freeze the committed view") from which the test rebuilds Outbox + DocsModel, and a KeyProvider protocol so H4's unavailable-vs-missing paths are unit tests. The K3 property test's event alphabet needs: edit, writer tick (including between command and enqueue), ack, lose-response, crash, reopen-and-apply-lane, reconnect; assertions: I1, server text == local text after drain, no acked entry the server lacks. "Commit, then lose the response" needs a fake that integrates the update, so convergence can be asserted in-process; keep native/scripts/test-sync-latency.py and DocumentSyncIntegrationTests for the real kill -9. Add the Sheets partial-batch cases to version_contract_test.go as the spec says, plus M4's lost-response-then-edit sequence.

(6) Decisions

• D1 encrypt: agree, with H4, H5 and L1 applied. Uniform is the right call; the cost is one store, the benefit is no per-app argument.
• D2 sliding: agree, ceiling mandatory (M5). Native-only, 30 d idle, 180 d absolute. Note it as rastrillo work, not kit work.
• D3 no background sync: agree. It is also what makes H4 matter: the locked case is now the common one.
• D4 warn then wipe: agree. Skip the 10 s send attempt when offline or session-ended, as written.
• D5 surface Sheets conflicts: agree, with M4 and M7.
• D6 Calendar device notifications: cut from this spec. It is not offline work, it is L-sized (64-slot planning, re-plan on every change, preview policy), and it is the only item here that adds a new product surface. Keep email; give it its own spec.
• D7 no prefetch: agree.
• d1 kit in rastrillo/native from the first cut: agree. Pins make it opt-in; mark in-progress until Sheets.
• d2 partial apply on 409: agree.
• d3 Docs cap: raise maxUpdateBytes for authenticated writers to maxLogBytes (8 MiB). A state the client is allowed to GET should be POSTable. It removes the "only when it fits" clause from Save-as-new and the 413 hold for a big paste, with one constant and no splitting code.

Framing is right throughout: each decision has a default the text is written to, and the defaults are the ones I would pick apart from D6.

Revised at 0d5c0c4 on spec/offline-kit, in response to the review above. Everything is cited to rastrillo/native main 2a5d07b. I checked each claim against the code before adopting it. Two of them needed correcting; see M5 and L3 below. Nothing has been implemented.

High

• H1. Confirmed: CRDTReplica is an actor, and command sets lastGood (CRDTReplica.swift:5, :149) before drainEdits enqueues. §4.3 now splits I1 into two parts. I1a adds a durability watermark: persistentState() returns (state, sequence, commandsApplied), and the writer persists only when commandsApplied <= commandsDurable; otherwise it drops the write and re-arms. The property test now requires a writer tick between command and enqueue.
• H2. New I1b: reopening means the replica plus the whole doc:<key> lane applied in seq order, then publish. Drafts reopen the same way. There is a no-duplicate-text assertion.
• H3. Confirmed: updateIsClientFault counts ErrUpdateLogFull (handlers.go:928-933). ApplyUpdate expects the retry, per the store.go comment around lines 801-816, with MaxRetainedUpdates at 4096 (core.go:141-146). Changes: - server: ErrUpdateLogFull now returns 503 with Retry-After: 1, plus a Go test (log at cap → POST → retry → 204); - client: a first 400 is .retry after a ?since= catch-up; the lane is held only if the same bytes are refused again (§4.4, K6).
• H4. §3.6 now separates unavailable from lost. The key counts as lost only on errSecItemNotFound with a populated directory. Every other status, and any file I/O error, means don't read, don't delete, and retry on .active or unlock. A single file that fails its GCM tag is removed on its own. BlobStoreError has three cases. The drainer treats a store failure during ack/hold as .retry. NativeCredentials.read (currently :15-16) gets the same split.
• H5. Nothing sealed is ever renamed. A draft's <key> stays draft-<token> for life, meta.sqid points to the server document, and cache/docs/index maps sqid → key. retarget is removed. The AAD is now accountID | format version | relative path.

Medium

• M1. Confirmed: the vendored yrs encode_state_as_update_v1 ends with merge_pending_v1 (transaction.rs:138-143, and Cargo.toml:17 points at the vendor copy). snapshot_pending is dropped; persistentState() is snapshot. The core parked-update test (core/tests/snapshot.rs:121) is ported to native/crdt.
• M2. In the drainer table, .offline on a satisfied path now backs off, and it waits only when the path is down. A stream opening fires connectivity.changes.
• M3. replace keeps the oldest seq by overwriting <oldest>.entry with a single atomic write sealed for that path, then deleting the rest. I did not use your ".tmp then rename" approach: a GCM file sealed for .tmp would not open after being renamed. The "not atomic" caveat is gone.
• M4. In §5, an entry that is in flight is frozen. A later edit creates a new entry with base set to the in-flight raw. update refuses in-flight ids. Each batch holds at most one entry per position (handlewritebatch.go:23-29).
• M5. The ceiling is now mandatory: auth_time + 180 days (§3.7, D2). Confirmed that lookup is read-only (sessions.go:162) and the table has no kind or renewed columns. The spec now describes this as a carlosframework/rastrillo change adding kind and renewed_at. Correction: only the four data apps pin v0.23.0. Calls uses amadan.net/rastrillo/rastrillo.
• M6. At first launch after upgrade, accounts without a subject are marked session-ended; this happens before any outbox exists. /native/whoami is noted as the alternative I didn't choose. The key is named as Auth.SubjectKey, taken from a persistent per-app instance secret and never per-process. The spec now states plainly that the guard is client-enforced.
• M7. Two changes: - the server compares base against staged.rawText inside the transaction and skips SetReport for no-ops. stamps becomes a count of SetReport calls, because it is len(writes) today (handlewritebatch.go:95, not :96); - an all-no-op batch is answered like a format response. Replay order is: refetch once, keep the overlay, then drain.

Low

• L1. The spec now spells out the nonce and .combined layout. It states the 2^32 random-nonce bound (SP 800-38D), a key rotation at 2^31 seals, and a KeyProvider. It also states that the Mac sandbox is accepted as the only file protection.
• L2. "Durable on return" is defined as surviving a process kill (.atomic), not power loss (F_FULLFSYNC). The tests prove the kill case.
• L3. Confirmed that Calls serves its own endpoints (app.go:437-438), and its exchange reply decodes (nativeauth.go:173). But its /native/request requires a target that passes nativeTarget (nativeauth.go:67-80, 110). NativeConnection sends no target, so adopting it unchanged would get a 400. §8 now offers two routes: an optional target: "/" request field, or moving to the shared module. Either way, add "Calls" to the allowlist (server/auth.go:254).
• L4. §0 cites main 2a5d07b. Its tree is identical to eacc073. K0 is now: re-pin the apps and bump the server module.

API minimalism

• retarget is dropped.
• update is deferred to the Sheets slice, so Memoria adopts after Sheets.
• Hold.blocksLane defaults to true.
• sendable(lane:maxBytes:) is limited by bytes.
• v1 drains only the active account, and transport(for:) is gone. A switched-away account keeps its outbox and drains when it is selected again.

Test strategy. New §3.8:

• ScriptedTransport;
• FaultingBlobStore, with a crash schedule, a frozen committed view and on-demand .unavailable;
• a KeyProvider fake;
• an integrating fake server for Docs (yrs) and Sheets (stamps).

The property-test alphabet and assertions are as you listed them. The real kill -9 scripts stay, and M4's sequence joins the Sheets tests.

Decisions. The section is still "Decisions for Paul", and each row now names the default the spec is written to.

• Adopted your positions on D2 (the 180-day ceiling is mandatory), D6 (device reminders cut into their own spec; email stays) and d3 (authenticated maxUpdateBytes raised to 8 MiB, which removes the "only when it fits" condition and the 413 hold).
• D1, D3, D4, D5, D7, d1 and d2 are unchanged. D4 now says explicitly that the 10 s send is skipped when offline or when the session has ended.

Follow-up review of 0d5c0c4 (delta a845b9a..0d5c0c4 against H1–H5, M1–M7, L1–L4, the API list and §3.8). Same reviewer, session 21a32889.

VERDICT: APPROVE-WITH-CHANGES — one sentence-sized change; no further review round needed once it is in.

M3 ruling: the author's version is correct, and better than mine. Sealing with the .tmp path as associated data and then renaming would indeed fail to open; my fix was wrong. Overwriting <oldest seq>.entry with one Data.write(options: .atomic) is right: Foundation writes a temporary file in the same directory and rename(2)s it over the target, which is atomic on APFS, and the temp name never matters because the AAD is the final relative path, bound in memory before the write. FIFO is kept (the merged entry sits at the oldest seq; nothing enqueued later can sort before it). Crash after step 1 and before step 2: on reload the lane holds the merged entry at the oldest seq plus the surviving originals at higher seqs; the merged entry drains first and the originals arrive as duplicates of ops the server already integrated, so nothing is lost and nothing is reordered. That is a double-send of duplicates, which is exactly the tolerance the spec already demands of any lane that may use replace; no journal is needed.

M5 and L3 corrections: both confirmed. docs, sheets, calendar and memoria go.mod require github.com/carlosframework/rastrillo v0.23.0; meet requires only amadan.net/rastrillo/rastrillo. Calls' nativeRequest (internal/calls/native_auth.go) does !nativeTarget(input.Target) → 400, and nativeTarget("") is false, so NativeConnection.signIn as it stands would be refused. Both routes in §8 are fine.

H1, H2, H3, H4, H5, M1, M2, M4, M6, M7, L1, L2, L4, the API list (retarget gone, update deferred, blocksLane default, sendable by bytes, active-account drainer) and §3.8: resolved as written. The watermark in I1a is correct because drainEdits enqueues in application order and commandsDurable only grows, so commandsApplied <= commandsDurable at write time means every command in the capture is on disk.

Remaining findings that would cause a wrong implementation

1. §3.3 replace must refuse in-flight ids (the same rule §5 already gives update). Compaction triggers at 256 entries / 1 MiB while a batch may be in flight, and batches are cut by bytes, so the in-flight set and the set being merged need not coincide. If replace overwrites <oldest>.entry while that entry is in flight and the batch is a proper subset of the merged ids, the later ack removes <oldest>.entry, and with it the content of entries that were never sent (step 2 has already deleted their own files). That is a loss, not a duplicate. Rule: replace throws on any in-flight id; the ids must be a seq-contiguous run of one lane; the merged entry gets a new UUID and inherits the lane and kind. Compaction runs only over sendable, not-in-flight entries.

1. §3.8 alphabet needs a compact event. Without it the property test can never crash between replace step 1 and step 2, and can never exercise finding 1. Add compact (over a random contiguous run, sometimes overlapping the in-flight batch) so the generator can reach both.

Not blocking, for the plan: with .atomic, every write is a new inode, so set isExcludedFromBackup on accounts/<id>/ rather than per file, and pass .completeFileProtection in the same write options so the temp file carries it before the rename. And M6's "before any outbox exists" holds only if K5 ships in the same build as K3; if K3/K4 go to TestFlight first, subject-less accounts with outboxes exist and the first-launch rule should still mark them session-ended (the outbox survives that state, so nothing is lost).

6049830 applies the delta review's two remaining findings and both notes.

1. §3.3: replace throws on any in-flight id. The ids must be a seq-contiguous run within one lane, the merged entry gets a new UUID and inherits the lane and kind, and compaction runs only over sendable entries that aren't in flight.
2. §3.8: the event alphabet adds compact (sometimes overlapping the in-flight batch, and it must throw) and a crash between replace steps 1 and 2.

Notes: isExcludedFromBackup is set on accounts/<id>/, and .completeFileProtection is passed in the same write options. M6 now holds even if K5 ships after K3.

The spec is approved for planning. Paul's D1–D7 and d1–d3 stand at the written defaults unless he says otherwise.