oficina / sheets Public

Whole-branch review — Claude Fable 5.1 (AGENTS.md §Review)

Read as one diff, origin/merge/native-document-windows...origin/fix/r1-native-version-gap (2e6c94e), in a scratch worktree, against the server write path (store/handle_report.go, handle_write_batch.go, handle_set.go, handle_open.go, handle.go, handlers_cell_batch.go, handlers_format.go, handlers_size.go, handlers_stream.go, live/hub.go) and the native accept/watch/format paths.

VERDICT: CHANGES-REQUIRED

What is right in the analysis: SetReport takes exactly one stamp per call (setContent and setBlank each do h.nextVersion++ once, including a clear of a cell that does not exist and a write of an unchanged value), the origin cell is always in the report and never derived, SetBatch rejects duplicate coordinates and marks Derived = !direct[coord], so an N-cell batch consumes N stamps and returns N non-derived cells with the last stamp. Verified with a probe against the real handlers: a 4-cell batch (two typed, two cleared, one of them also derived from another write in the same batch) returned 4 non-derived cells. swift test passes (16 + 4). make test green.

Findings:

1. [blocking] SheetModel.swift:121-122 — the invariant does not hold on the real server, so the fix does not stop the refetches it is meant to stop. store.Open consumes a stamp unconditionally (handle_open.go:157-158: recoveryVersion := h.nextVersion; h.nextVersion++, even with nothing stale), and handles are evicted the moment their refcount reaches zero (handle.go:138-145), so with sequential requests every request re-opens the handle and each write's first stamp is last + 2. Replaying real responses through the branch's rule (scratch Go test, one user, no peers): snapshot v2 → A1 write v2 (jump 0, apply) → A2 v4 (jump 2 > 1, refetch) → snapshot v6 → A3 v6 (apply) → 3-cell batch v10 (jump 4 > 3, refetch) → snapshot v12 → batch v14 (jump 2, apply) → batch v18 (jump 4 > 3, refetch). Every other write of any size still refetches, exactly as before the change (the old rule refetched on the same steps plus one more). The Swift tests pass because they feed hand-written events in which the arithmetic holds; none exercises a server response. This also means B12's diagnosis was incomplete: single-cell writes were already refetching every other time on this line.

1. [blocking, data visibility] format responses widen the accepted jump while consuming no stamp. formatResponse (handlers_format.go) returns one cell per coordinate in the rectangle, none flagged derived, with Version: h.Version(); native format() (SheetModel.swift:202) feeds that straight into accept(). So a bold over N cells makes the client accept a jump of N with zero content stamps behind it. When the model has missed writes — the real case is the stream's idle bye, which sets live = "Paused" and stops watch() reconnecting (SheetLive.swift:27,57) — a row or column format then applies over the missed peer edits instead of refetching; before this branch any jump over one refetched. singleStepAndFormatOnlyResponsesStillApply models a format response as cells: [], which the server never sends (sizesResponse is the one with no cells).

1. [nit, pre-existing] Snapshots and format/size responses report nextVersion (one above the last stamp), while write responses report the stamp itself, so the first write after a snapshot is a jump of 0 and one missed write immediately after a snapshot is undetectable under the old rule and this one. Worth closing while the contract is being made explicit, not a regression.

Suggested fix — make the version contract explicit instead of inferring it from cell counts, because the client cannot tell an Open bump from a missed write:

• Server: (a) in Open, take the recovery stamp only when len(stale) > 0; (b) add stamps (versions this request consumed) to gridResponse: len(writes) for /cells/batch, 1 for /cells, absent for format, size and GET; (c) optionally report snapshots/format/size at the last issued stamp rather than nextVersion — check grid.js applyBatch's < and the handle_version_* tests first, since Version()'s doc defines the current semantics.
• Client: refetch iff resync || response.version > version + (response.stamps ?? 0) (keep + max(…, 1) if (c) is skipped).
• Prove it against the server, not synthetic events: a Go test that replays real /cells, /cells/batch and /format responses through the rule (my probe above is ~40 lines), or drive the native model at a fixture server the way ResizeFixtureServer does.

"No server change" was a design choice in the discussion, not a constraint; the offline audit already named fromVersion as the alternative. Re-request review after the push.

Superseded by fix/r1-native-version-contract (new discussion). Cut from main after #34 landed. Its server half takes the Open stamp only when stale and adds an explicit stamps. The native rule is version > have + max(stamps, 1). It's proven with a Go test that replays real server responses. fix/r1-native-version-gap should not be merged.