amadan
design: incorporate Fable watermark review and pin regression contracts
4 files changed,
+117
−8
docs/superpowers/specs/2026-09-07-durable-tab-versions.md+11 −6internal/sheets/store/handle_blank_cap_test.go+12 −1internal/sheets/store/handle_version_migration_test.go+1 −1internal/sheets/store/handle_version_watermark_test.go+93 −0
CI failed — run details
Intent
Human
READ-ONLY Fable spec review; no edits/deploys. Repo /home/paulca/amadan.net/oficina/sheets-core/.claude/worktrees/overnight-versions at91e9991. Read AGENTS.md and docs/superpowers/specs/2026-09-07-durable-tab-versions.md. Review against store/handle_open.go, handle_set.go, handle_report.go, handle_write_batch.go, models.go and new handle_version_watermark_test.go/handle_version_migration_test.go. Atomic batch candidate37c8055 is blocked by real version regression: paste500=503, clear500=1004, next edit=4 after handle reopen; single clear3/5/2 and crash-after-blank3/2 also fail. Proposed additive Tab.ContentVersion watermark persists with input/recalc tx and survives cell deletion; batch rollback must revert it. Assess minimal correct fix, migration preservation, every mutation/recovery path, lock safety and missing regression cases. APPROVE or REQUEST CHANGES. This is design review before implementation, not a deployed-canary claim. Ignore separate workbook-tabs/fixture-bootstrap proposals in this pass.
Attributed to this commit: 5.89 g CO₂e (217449 tokens). This is an attribution, not a measurement — usage counted from this commit's own marker forward to the next one on fix/durable-tab-versions, never backward from it. A turn that produced more than one commit books all of its tokens to the last one, and a concurrent session on this branch can add to the same figure. Some of it was priced using the shared default rate, because no model-specific factor row exists yet for at least one model involved. The Carbon view has the repo total, which is never derived from this attribution.
diff --git a/docs/superpowers/specs/2026-09-07-durable-tab-versions.md b/docs/superpowers/specs/2026-09-07-durable-tab-versions.md| index b8399c6..15871e1 100644 |
| --- a/docs/superpowers/specs/2026-09-07-durable-tab-versions.md |
| +++ b/docs/superpowers/specs/2026-09-07-durable-tab-versions.md |
| @@ -1,6 +1,6 @@ |
| # Durable tab version watermark |
| -Status: proposed prerequisite; independent Fable review pending. No implementation is approved yet. Branch fix/durable-tab-versions starts at atomic batch candidate 37c8055; Amadan discussion 19/task 3. Atomic production deployment is blocked until this defect is fixed and reviewed. |
| +Status: revision 2 proposed prerequisite; Fable 5.1 (claude-fable-5-1) requested four small amendments at 91e9991, now incorporated for focused re-review. No implementation is approved yet. Branch fix/durable-tab-versions starts at atomic batch candidate 37c8055; Amadan discussion 19/task 3. Atomic production deployment is blocked until this defect is fixed and reviewed. |
| ## Reproduction |
| @@ -31,9 +31,9 @@ SQLite integers are signed 64-bit. Refuse an exhausted/corrupt watermark before |
| | Row/column insert/delete/move | Not implemented in this repository; future structural operations must allocate and persist a tab stamp in the same mutation transaction. Do not delete a watermark with cell/axis rows | |
| | Tab add/rename/reorder | Future metadata events have separate ordering semantics; do not masquerade as cell history | |
| -Implement one small transaction-scoped helper for monotonic advancement using an allowlisted update of content_version on Tab by ID. It never lowers a stored value. Both input write paths and persistTouched call it. No outer db calls occur while a transaction holds the one writer connection. A missing tab row must fail rather than silently discarding the watermark. |
| +Implement one small transaction-scoped helper for monotonic advancement using an allowlisted update of content_version on Tab by ID. It never lowers a stored value. Both input write paths and persistTouched call it. No outer db calls occur while a transaction holds the one writer connection. Use SET content_version = MAX(content_version, ?) WHERE id = ? so RowsAffected=0 means the tab is missing, not that a lower stamp was safely ignored. A missing tab row must fail rather than silently discarding the watermark. |
| -Read the tab watermark while building a handle, before allocating nextVersion. A failed input/recalculation transaction must not commit its watermark; the existing dirty-marker behavior after a committed input transaction remains valid. A failed atomic batch must leave both the live handle and persisted watermark unchanged. |
| +Read the tab watermark while building a handle, before allocating nextVersion; a missing tab row fails Open. Preserve the existing deterministic recovery-stamp reservation, even when there are no stale cells, so a snapshot version handed out before release is no greater than the first edit stamp after reopen. Check exhaustion at Open and SetReport allocation (including staged SetReport calls inside SetBatch). Move the in-memory nextVersion increment above the blank crash seam after transaction one commits. A failed input/recalculation transaction must not commit its watermark; the existing dirty-marker behavior after a committed input transaction remains valid. A failed atomic batch must leave both the live handle and persisted watermark unchanged. |
| ## Regression coverage |
| @@ -42,10 +42,15 @@ Read the tab watermark while building a handle, before allocating nextVersion. A |
| 3. Blank crash seam after the first transaction: release/reopen and require the next edit to exceed the committed blank's allocated stamp. |
| 4. Overlapping formula/dependent recomputation, removal of the newest cached cells, and reopen preserve ordering and correct values. |
| 5. Existing failure injection into a late SetBatch event must leave Tab.ContentVersion unchanged alongside inputs, history and axes. A subsequent successful edit must remain usable. |
| -6. Migrate a database built with BootSchema through the pre-watermark migration set; seed encrypted names/content and history, then apply the new migration. Verify byte-for-byte ciphertext retention, default watermark, schema agreement and monotonic first post-upgrade edits. |
| +6. Migrate a database built with BootSchema through the pinned pre-watermark migration IDs (Sheets 0001 through 0009 plus framework schema); seed encrypted names/content and history, then apply the new migration. Verify byte-for-byte ciphertext retention, default watermark, schema agreement and monotonic first post-upgrade edits. |
| 7. A high watermark in an otherwise empty tab survives Open; the next write uses a higher stamp. A lower attempted advance cannot reduce it. Exhaustion refuses without a write. |
| -8. Run existing crash/recovery, plain SQLite/WAL leakage and race tests. The watermark contains no plaintext values and adds no logs. |
| +8. Snapshot Version() before release/restart must be no greater than the first edit stamp after reopening. Test both populated and cleared tabs. |
| +9. A capped blank with more than MaxDirtyPerPass dependents must advance the watermark in transaction one alone. Mutation-test by removing only that advancement; the regression must fail. |
| +10. A retained handle starts a batch with nextVersion above the persisted watermark, commits a batch, then rolls back a late-failing batch. After release/reopen the next edit must exceed the successful report, and the failed batch must leave its watermark unchanged. |
| +11. Run existing crash/recovery, plain SQLite/WAL leakage and race tests. The watermark contains no plaintext values and adds no logs. |
| ## Review and deploy |
| -Keep this prerequisite independently attributable in discussion 19. Fable reviews the design before implementation and the final code before merge. Integrate with atomic batch only after passing full make test, migration agreement, exact-head CI and deployed canary browser regression (paste, clear, new paste, reload, second observer). A migration-only deploy must not erase or replace any user data. Root coordinates production promotion and /api/version proof. |
| +Keep this prerequisite independently attributable in discussion 19. Fable reviews the design before implementation and the final code before merge. Integrate with atomic batch only after passing full make test, rastrillo migration check, exact-head CI and deployed canary browser regression (paste, clear, new paste, reload, second observer). A migration-only deploy must not erase or replace any user data. Root coordinates production promotion and /api/version proof. |
| + |
| +The current migration head is 0009_alter_formats_ink_intent. Confirm current main has not acquired 0010 before final integration; assign a fresh additive migration ID if it has. The old-database fixture must select known IDs, not filter SQL text, so future migrations mentioning the column cannot be silently omitted. |
diff --git a/internal/sheets/store/handle_blank_cap_test.go b/internal/sheets/store/handle_blank_cap_test.go| index 3074408..f06ce08 100644 |
| --- a/internal/sheets/store/handle_blank_cap_test.go |
| +++ b/internal/sheets/store/handle_blank_cap_test.go |
| @@ -137,9 +137,20 @@ func TestBlankFeederOverCapDependentsAreRecoverable(t *testing.T) { |
| t.Fatalf("test setup: only %d dependents wired, want more than MaxDirtyPerPass (%d)", len(touched), calc.MaxDirtyPerPass) |
| } |
| - if err := h.Set(feederRow, colA, "", 7); err != nil { |
| + rep, err := h.SetReport(feederRow, colA, "", 7) |
| + if err != nil { |
| t.Fatalf("blank feeder: %v", err) |
| } |
| + if !rep.Capped { |
| + t.Fatal("blank unexpectedly recalculated") |
| + } |
| + var watermark uint64 |
| + if err := db.Raw("SELECT content_version FROM tabs WHERE id = ?", localTab).Scan(&watermark).Error; err != nil { |
| + t.Fatal(err) |
| + } |
| + if watermark < rep.Version { |
| + t.Fatalf("capped blank stamp %d not retained by watermark %d", rep.Version, watermark) |
| + } |
| h.Release() |
| // The real proof: reopen and read the dependents' actual VALUE, not |
diff --git a/internal/sheets/store/handle_version_migration_test.go b/internal/sheets/store/handle_version_migration_test.go| index 24dacbb..e33bd8f 100644 |
| --- a/internal/sheets/store/handle_version_migration_test.go |
| +++ b/internal/sheets/store/handle_version_migration_test.go |
| @@ -23,7 +23,7 @@ func TestVersionWatermarkMigrationPreservesExistingContent(t *testing.T) { |
| defer d.Close() |
| old := &migrate.Set{} |
| for _, m := range sheets.BootSchema.All() { |
| - if !strings.Contains(m.SQL, "content_version") { |
| + if !strings.HasPrefix(m.ID, "sheets/") || m.ID <= "sheets/0009_alter_formats_ink_intent" { |
| old.Add(m) |
| } |
| } |
diff --git a/internal/sheets/store/handle_version_watermark_test.go b/internal/sheets/store/handle_version_watermark_test.go| index 54283be..012b082 100644 |
| --- a/internal/sheets/store/handle_version_watermark_test.go |
| +++ b/internal/sheets/store/handle_version_watermark_test.go |
| @@ -3,6 +3,7 @@ package store_test |
| import ( |
| "amadan.net/oficina/sheets/internal/sheets/store" |
| "errors" |
| + "gorm.io/gorm" |
| "testing" |
| ) |
| @@ -108,3 +109,95 @@ func TestBlankCrashRetainsCommittedVersion(t *testing.T) { |
| t.Fatalf("committed blank stamp=%d next=%d", allocated, next.Version) |
| } |
| } |
| + |
| +func TestSnapshotVersionSurvivesReopen(t *testing.T) { |
| + for _, clear := range []bool{false, true} { |
| + t.Run(map[bool]string{false: "populated", true: "cleared"}[clear], func(t *testing.T) { |
| + db, s, id := seeded(t) |
| + h, err := store.Open(db, s, id) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if clear { |
| + if err = h.Set(rowA, colA, "", actor); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + snapshot := h.Version() |
| + h.Release() |
| + h, err = store.Open(db, s, id) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + defer h.Release() |
| + rep, err := h.SetReport(rowA, colA, "9", actor) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if rep.Version < snapshot { |
| + t.Fatalf("snapshot %d exceeds new edit %d", snapshot, rep.Version) |
| + } |
| + }) |
| + } |
| +} |
| + |
| +func TestRetainedHandleBatchRollbackKeepsDurableVersion(t *testing.T) { |
| + db, s, id := seeded(t) |
| + h, err := store.Open(db, s, id) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if err = h.Set(rowA, colA, "", actor); err != nil { |
| + t.Fatal(err) |
| + } |
| + var before uint64 |
| + if err = db.Raw("SELECT content_version FROM tabs WHERE id = ?", id).Scan(&before).Error; err != nil { |
| + t.Fatal(err) |
| + } |
| + if h.Version() <= before { |
| + t.Fatal("fixture did not retain an unused stamp") |
| + } |
| + rep, err := h.SetBatch([]store.CellWrite{{Row: 0, Col: 0, Raw: "8"}}, actor) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if err = db.Raw("SELECT content_version FROM tabs WHERE id = ?", id).Scan(&before).Error; err != nil { |
| + t.Fatal(err) |
| + } |
| + calls := 0 |
| + if err = db.Callback().Create().Before("gorm:create").Register("watermark-failure", func(tx *gorm.DB) { |
| + if tx.Statement.Table == "events" { |
| + calls++ |
| + if calls == 3 { |
| + tx.AddError(errors.New("injected failure")) |
| + } |
| + } |
| + }); err != nil { |
| + t.Fatal(err) |
| + } |
| + _, err = h.SetBatch([]store.CellWrite{{Row: 0, Col: 0, Raw: ""}, {Row: 0, Col: 1, Raw: "9"}}, actor) |
| + db.Callback().Create().Remove("watermark-failure") |
| + if err == nil { |
| + t.Fatal("batch failure not injected") |
| + } |
| + var after uint64 |
| + if err = db.Raw("SELECT content_version FROM tabs WHERE id = ?", id).Scan(&after).Error; err != nil { |
| + t.Fatal(err) |
| + } |
| + if after != before { |
| + t.Fatalf("watermark survived rollback: %d -> %d", before, after) |
| + } |
| + h.Release() |
| + h, err = store.Open(db, s, id) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + defer h.Release() |
| + next, err := h.SetReport(rowA, colA, "10", actor) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if next.Version <= rep.Version { |
| + t.Fatalf("last successful %d >= reopened edit %d", rep.Version, next.Version) |
| + } |
| +} |