oficina / sheets Public

Design and failing regressions pushed at32ba449. Three observed failures: batch versions503/1004/4; single-cell versions3/5/2; blank-crash committed stamp3 followed by next edit2. Migration regression creates pre-watermark schema with encrypted content/name and version700, then requires additive migration and post-clear monotonic ordering. Implementation remains pending Fable spec review. Remote CI is expected red on this test-first prerequisite branch; atomic branch37c8055 remains unchanged and deployment-blocked.

Root provisioned fresh canary keys and bound named environment overnight-batch to overnight-sheets.bds.oncarlos.com. Bootstrap proposal guard now matches synthetic owner oficina-fixture@example.test. No bootstrap or candidate deployed. Browser regression script now includes native500-cell paste, cut/clear, then repaste without reload, which exercises the discovered monotonic-version failure.

Production deployment blocker: the platform reports Sheets production release 7212864, which is not an ancestor of origin/main 4aea8f4. Their common ancestor is 8ed3926. Seven deployed-side commits add the import page, workbook rename, and list/drop chrome. Main adds only suite-doctrine amendments to AGENTS.md, PRODUCT.md and the design spec. A main-based batch candidate would remove deployed routes and UI.

Root authorized isolated fix/reconcile-deployed-chrome to preserve both histories before any production deployment. A no-commit merge applies without textual conflicts; atomic and durable-version branches remain isolated. The existing discussion 15 explicitly records missing highest-capability review, deployed-canary evidence and human copy review, so deployment is not approval. Reconciliation requires Fable, current CI, copy approval for the carried strings, and exact-process deployed canary verification. No merge to main or deploy has occurred.

Fable spec review at 91e9991, actual model claude-fable-5-1: REQUEST CHANGES. No implementation approval or deployed-canary claim. Full result follows.

Verdict: REQUEST CHANGES (spec-level, small). The watermark design is correct and minimal. Four amendments are needed before implementation, none touching a settled decision.

Reviewed by Fable 5.1 (claude-fable-5-1), read-only, at 91e9991. All four new tests fail as described: 503→1004→4, 3→5→2, crash-after-blank 3→2, and the migration test on the missing column.

Why the fix is right

• buildHandle derives nextVersion only from surviving cells (handle_open.go:118-128), so deleting the newest rows destroys the ordering evidence. A tab-level column read at open is the minimal durable fix.
• The events table was the other candidate. It loses: O(n) at open and history.go names a retention horizon, so evidence would eventually be pruned.
• Every content mutation already funnels through SetReport or persistTouched. Table edit, import, restore and undo inherit the fix with no parallel logic. Format and size writes report h.Version() without allocating, and grid.js accepts equal versions, so leaving them alone is correct.
• Lock safety holds. The writer pool is one connection, and dbresolver pins every statement inside a transaction to that connection. The helper must take tx, never h.db. SetBatch's staged handle has db=tx, so its recovery and watermark writes roll back with the outer transaction.

Required changes

1. Specify the helper's SQL shape. "Never lowers", "allowlisted update" and "missing tab must fail" are only jointly satisfiable with SET content_version = MAX(content_version, ?) WHERE id = ?. A WHERE content_version < ? guard makes RowsAffected=0 mean either "no-op" or "missing", and the missing case is what must fail.
2. Open must fail on a missing tab row too, not only the advance path. Every fixture that opens a handle seeds a tabs row, so this is safe to require.
3. Add three regression cases: - Snapshot Version() handed out before a release or restart must not exceed the first edit version after reopen. grid.js discards paints strictly below its version, and this holds today only because open burns exactly one recovery stamp deterministically. Pin it so a later change to the burn cannot break the client contract silently. - Capped blank (dependents above MaxDirtyPerPass) runs transaction one only. Require that transaction one alone advances the watermark, and mutation-test it: remove the advance from setBlank's first transaction and the test must fail. Case 3's test as written passes if the advance is in the wrong transaction as long as it lands somewhere. - SetBatch on a retained handle whose in-memory nextVersion exceeds the persisted watermark, then a failing batch, then release and reopen. The next edit must exceed the last successful report.
4. Migration gating. Name rastrillo migration check in the deploy gate (Makefile:66 runs it already). Filter the pre-watermark set by migration ID rather than by the string "content_version", which will also strip any future migration mentioning the column. Current head is 0009; confirm main has not taken 0010 before merge.

Notes, not blockers

• The overflow refusal belongs at both allocation sites (SetReport's v, staged in SetBatch) and at the read in buildHandle.
• After setBlank's first transaction commits, h.nextVersion++ runs after the crash seam. A seam error leaves an unreleased handle ready to reuse v. Unreachable in production, but moving the increment above the seam is free.
• assignFormat creates format-only cells with InputVersion 0. They skip the max calculation in buildHandle, which is correct.

Design approval at 9f6abe2 by actual model claude-fable-5-1. APPROVE for implementation only; branch review, tests, CI and deployed canary remain required.

Verdict: APPROVE the design at 9f6abe2 for implementation. All four amendments from my 91e9991 review are incorporated, and I verified each against the touched code rather than the spec text alone.

Amendment checks

1. MAX SQL. Spec now names SET content_version = MAX(content_version, ?) WHERE id = ?. SQLite's changes count includes matched rows whose value is unchanged, so RowsAffected=0 unambiguously means the tab row is missing. Sound.
2. Missing-tab Open. Spec states Open fails on a missing tab row. The seeded fixture creates the tab row before every Open, so no existing test breaks.
3. Three regression cases. All present and correctly shaped: - Snapshot test (case 8): with the seeded stale cell, first Open recovers at stamp 2 and hands out 3; reopen reads watermark 2, burns 3, edits at 4. Cleared variant: blank at 3, snapshot 4, reopen edits at 5. The rep.Version < snapshot assertion matches the spec's "no greater than". - Capped blank (case 9): setBlank returns after transaction one when over cap, so the watermark read directly after SetReport can only be satisfied by an advance inside transaction one. The mutation you plan is the right one, and the test reads the column, not cell stamps, so dependents' bumped InputVersion cannot mask the mutation. - Retained-batch rollback (case 10): the failing batch emits exactly three event inserts (blank input, content input, recalc marker), so calls == 3 fires inside the outer transaction. Watermark compared before and after, then reopen and edit above the last successful report. Correct.
4. Migration gating. BootSchema is a Merge, which qualifies IDs as sheets/NNNN_name and clears the namespace so All does not double-qualify. The filter !HasPrefix("sheets/") || ID <= "sheets/0009_alter_formats_ink_intent" therefore keeps sessions, auth, and sheets 0001 to 0009. Main head is 0009 with no 0010. Makefile test-migrations runs rastrillo migration check and the spec now names it.

Also incorporated from my notes: exhaustion check at Open and at both SetReport allocation sites, and the nextVersion increment moved above the blank crash seam.

Not blockers

• No regression case names the missing-tab refusal for either Open or the advance helper. Add one small test each at implementation time so the guard is mutation-testable, per the repo's own rule.
• The case-10 fixture precondition h.Version() <= before is trivially satisfied after any edit, since nextVersion always sits one above the watermark. Harmless, but it documents less than it appears to.

Durable version implementation is pushed at 1aca44c (code6ccb609) after Fable design approval at9f6abe2. Full local make test passed, including migration check, sealed-content gates and race tests. Focused clear/reopen, snapshot, retained batch rollback, formula recovery, missing-tab, exhaustion and old-database ciphertext/history preservation tests pass.

Required mutation proof: Go overlay removes only setBlank transaction-one watermark advance; TestBlankFeederOverCapDependentsAreRecoverable fails with capped blank stamp3 not retained by watermark0. Tracked source remains intact. Code review prompt is prepared; root allocates the Fable slot. Exact-head CI, Fable branch review, deployed-source reconciliation and canary session/browser proof remain gates. No production deployment.

Exact-head remote CI passed for 1aca44c2876be6c16dd6b03c45e0bb28f3024257: vet, fmt, build, migrations, go-test, race all green. Whole watermark implementation review is running with Fable. Atomic inherited base still requires its revised-spec/full code approval, which will be covered explicitly in final combined integration review; the watermark review does not substitute for that separate unresolved gate.

Fable branch review: APPROVE durable watermark code at 1aca44c2876be6c16dd6b03c45e0bb28f3024257. Actual model claude-fable-5-1. Exact-head CI had already passed all six steps when the reviewer finished; its prompt described CI as pending, now superseded by the recorded CI evidence. This approval covers the watermark change, not inherited atomic code, source reconciliation, copy, canary or deployment.

Verdict: APPROVE the code at 1aca44c for the durable tab version watermark. This is a branch review only, not canary or deployment approval. Exact-head CI is still pending and production source reconciliation with deployed 7212864 remains a hard separate prerequisite.

What I verified myself

• go vet, the store package under -race, the sheets package, and rastrillo migration check all pass on this head.
• Main at origin still tops out at migration 0009, so 0010 does not collide.
• The mutation log shows the capped-blank regression failing when only the transaction-one advance is removed, as the spec required.

Transaction semantics, per path

• setContent and setBlank call advanceContentVersion inside transaction one, ahead of the cell write and AppendInput, so a rolled-back input never commits its watermark (handle_set.go:55, handle_set.go:130).
• persistTouched advances in the same transaction as the cache stamps and AppendRecalc, which also covers recoverStale (handle_set.go:237).
• The blank crash seam now sits after the in-memory increment (handle_set.go:84), so a crash after transaction one leaves memory, watermark and disk agreeing.
• SetBatch builds the staged handle on tx, so every nested advance and the recovery stamp roll back with the outer transaction. The live handle only adopts staged.nextVersion after commit (handle_write_batch.go:41, handle_write_batch.go:94).

MAX helper and missing tab

• The update uses MAX(content_version, ?) with a Select allowlist. SQLite counts a matched but unchanged row in changes(), so RowsAffected != 1 means the tab row is absent and the transaction fails rather than silently dropping the stamp (handle_version.go:18).
• Open fails on a missing tab row before building anything (handle_open.go:47). Both guards have dedicated tests, closing the gap the design review flagged.
• No other code path writes Tab as a whole struct. The only non-store site is a Count, so a stale Save cannot lower the watermark.

Snapshot and overflow ordering

• Open checks exhaustion at maxVersion >= MaxInt64-1 before reserving the recovery stamp, so the recovery stamp is always below the helper's own refusal bound and can never be refused mid-recovery (handle_open.go:132).
• SetReport checks before allocating, and the staged handle inherits that check inside batches (handle_report.go:108).
• The recovery reservation is preserved even with no stale cells, so a pre-release snapshot equals at most the first post-reopen stamp. The client discards only strictly lower versions (grid.js:82), so equality is safe.

Migration

• 0010 is a single additive ALTER TABLE ... ADD ... NOT NULL DEFAULT 0. The upgrade test now selects migrations by pinned ID rather than SQL text, and additionally proves history rows survive byte for byte.

Non-blocking notes

• The spec status line was updated in this range to record design approval. It correctly says branch review, CI, reconciliation and canary remain outstanding. Update it again to record this code review with the head SHA.
• TestVersionExhaustionRefusesWithoutMutation sets content_version to -1 and only asserts Open fails. Whether that failure comes from a scan error or from wrap-to-MaxUint64 is driver behaviour under modernc sqlite. Either outcome is a refusal, so this is fine, but the test does not pin which.

Isolated deployment and authenticated browser gate completed 2026-09-07 ~21:26 UTC. Approved writer aca6668fa2765eba5c6b70b27f37d1071fbec973 was clean-built with fresh run ID r20260907-f0d51843ba611261, deployed only canary/overnight-batch at overnight-sheets.bds.oncarlos.com, environment overnight-batch. Actual GET /api/version returned aca6668-fixture-r20260907-f0d51843ba611261; /healthz 200, /signin and /sheets 404. Exact observed /data/overnight-sheets.bds.oncarlos.com.db and /run/carlos/overnight-sheets.bds.oncarlos.com.sock guards remained intact. Writer could serve only after guarded empty-database initialization succeeded.

Unset all three SHEETSFIXTURERUNID/TOKENHASH/EXPIRES_AT variables, then replaced channel with exact reviewed ordinary d35cf9494fe45bd729722ccb5ca0a52b44f7eb91, built via make release. Actual /api/version d35cf94 verified before successful committed tools/drive/canary-batch.mjs run. Real deployed browser evidence: native 500-cell paste one batch request, 442ms; second-page live update; clear then paste visible; invalid late formula rolls back whole batch; reload persistence; scriptless table; deliberately failed first cut chunk stops remaining queue (one request). No POST /signin or email delivery attempted.

Honest convergence caveat: both CLI 45s deploy watches timed out during adoption/wake. Scoped restart selected only the canary environment/host. Writer root deliberately returns 404; direct /healthz and process-version receipts established success. First browser invocation failed its version guard with transient 'instance failed to wake' and made no editor assertions. Retried only after direct /api/version returned d35cf94; full committed gate then passed. No guard or code changed.

Teardown: normal POST /signout returned 303 /signin; replaying original saved cookie on /sheets also returned 303 /signin. Fixture environment variables verified absent, private raw-token and browser-state files removed. Canary remains ordinary d35cf94; production process remains 7212864 and edge channel unchanged. Synthetic workbook remains isolated in canary. This certifies ordinary session validation and batch behavior, not email delivery.

Evidence: /tmp/sheets-canary-writer-receipt.json; /tmp/sheets-canary-batch-result.json; /tmp/sheets-canary-batch-browser.log; /tmp/sheets-canary-teardown-result.json. Existing independent Claude Fable 5.1 approvals and exact-head green CI remain unchanged. Production deployment remains blocked by source reconciliation and pending user copy review; no production merge/deploy performed.