oficina / memoria Public

Whole-branch review — Fable 5.1 (dispatched as claude-fable-5-1), 2026-09-23

Diffed origin/merge/native-testflight...origin/fix/r1-native-note-idempotency (977e20c, one commit on cb534c3). Scratch worktree; nothing edited. Merge #13 first, as the discussion says.

VERDICT: APPROVE-WITH-NITS

The three questions asked of it

• Can a client_id collide across users or accounts? Is the store key scoped per owner? No, and no — and that is consistent with this app's model rather than a hole in it. Store.Ingest resolves (source, external_id) with external_id = Fingerprint(key), team-wide; there is no owner column on documents and AGENTS.md forbids scope.Owned here because documents are team-shared. The web paste path already keys team-wide on the title (handlers.go:883, Key: title), so any team member can already revise any other member's paste by pasting under the same title. A native note keyed on a 122-bit random UUID is strictly harder to collide with than that. Instances are one team per DB, so "across accounts" is the same set as "across team members". Nothing here widens what a member could already do through the browser.
• Can one user overwrite or probe another's note? Only by holding the other person's client_id, which exists on their device and is never listed by any route. With it, a different body revises that note (same as the web title path) and the response id confirms existence; without it there is nothing to enumerate — the 400 for a malformed id and the 200 for a fresh id are the only two answers, and a fresh id always succeeds. No oracle.
• Is malformed input rejected? Yes: uuid.Parse plus len == 36 admits only the canonical dashed form (braced, urn:uuid: and bare-hex are refused — I checked the length check is what refuses them, since uuid.Parse alone accepts all three), and parsed.String() canonicalises case so Swift's uppercase uuidString and a lowercase replay are one key. Empty means "old client", server mints as before. The 400 sentence carries no content.

Fix and test

• Root fix: same draft -> same (paste, native-<uuid>) -> Ingest's same-key-same-body path returns the first id and writes nothing; edited draft -> same row, revision+1. Swift side mints @State clientID = UUID() once per sheet and keeps it across a failed Save (the sheet stays up with saving = false), which is the retry case B10 names.
• Test green. Reviewer-chosen mutations: M3 (malformed id silently falls back to a server key instead of 400) — red. M1 (key also carries the title, so an edited-title replay would create a second note) — survived. M2 (client_id ignored for bodies over 100 KB) — survived, as expected for a single small fixture. M1 is the one that matters: see finding 1.
• Full go test ./..., go vet, gofmt -l clean on this head; amadan ci status passed at 977e20c.

Findings (none blocking)

1. [low] native_test.go — the "edited draft revises, never duplicates" claim is stated in the comment and the discussion but not tested. My M1 mutation (key = parsed.String() + "-" + title) leaves the suite green. Add one subtest: replay the same client_id with a changed title and a changed body, assert same id, document count unchanged, revision bumped. That is the second half of B10 (the phone that lost a response and the person who then fixed a typo).
2. [low] native.go:113 — uuid.Nil is a valid, predictable key. 00000000-0000-0000-0000-000000000000 parses and is 36 chars, so any client that ships a zero UUID (a struct default, a bad decode) converges every user's notes onto one document that revises forever. Refuse parsed == uuid.Nil (and optionally require parsed.Version() == 4). One line plus a row in the malformed loop.
3. [nit] Spec §"Native Apple clients" still says notes use "an opaque unique source key so two same-title notes never replace each other." Still true, but the key is now client-chosen per draft, which is the property the next reader needs. Per AGENTS.md the spec wins over code, so a one-sentence amendment belongs in this commit or the merge that squashes it.
4. [note, pre-existing] No unique index on documents(source, external_id) (schema.sql:22 is non-unique), so two concurrent identical replays could in principle both insert. SQLite's single writer and the Swift button disabling while saving make this theoretical; it predates this branch for every source.
5. [note, acknowledged] ErrIndexLag residual — a replay of a note whose first save hit index lag answers indexed:true. Pre-existing Ingest behaviour, rare, and the app says "after the server restarts" out loud on the other path. Fine to leave.

Mutations run and reverted: 3 (1 red, 2 survived; the survivor that matters is finding 1). Live defects found: 0.

I fixed Fable's findings 1–3 in 5054485, on top of a merge of #13's new head 4f37b93 (merge commit 3358baf).

1. Edited replay: a new subtest replays the same clientid with a changed title and body. It checks for the same id, no new document, and the revision bumped by one. It fails under your M1 mutation (`key = clientid + "-" + title`): the edited replay got id 5 when it should have got 1.
2. Zero UUID: uuid.Nil now gets a 400. I added it to the malformed-id loop, and that row fails when the check is removed.
3. Spec: I amended §"Native Apple clients". The key is now a client_id the app mints once per draft, and the section says what a retry and an edited retry do.

I did not add a UUIDv4-only rule. make ci passes, and so does the native fixture test with the real MemoriaReview binary. amadan ci status passed at 5054485.