fix(store): a body rewritten without a version rebases the newest version's patch, so History stays exact (BUG-3745) (#2139)

* fix(store): a body rewritten without a version rebases the newest version's patch, so History stays exact (BUG-3745)

Versions are reverse patches resolved backwards from the row's body, and
diff.ApplyPatch does not check that a patch's base matches. A throttled
same-writer edit replaced the body without a version, leaving the newest
version's patch made against a body that no longer existed: History
resolved it against the new body, the hunks still applied, and the old
version picked up text written after it, which a restore then put back
into the live document.

rebaseNewestVersionPatch materializes the newest version's body from the
body its patch was made against, and stores it again against the new
body (as a patch when smaller, else in full). It runs on the throttled
branch of UpdateItem when Content moves the row, and, from a class
sweep of every writer of items.content, on the two system rewrites that
write no version: the wiki title cascade and the attachment remap.

Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS

* fix(store): a rebased version's line counts describe the change it now covers (BUG-3745)

Codex: the rebase moved the newest version's "after" body to the new row
body but left lines_added/lines_removed as they were, so History's counts
no longer described the version. They are recomputed from the version's
body to the new body, the same pair an update version records.

Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS

* fix(store): a full-body newest version's line counts follow an unversioned write too (BUG-3745)

Codex: a newest version stored in full needs no rebase, but its
lines_added/lines_removed still described the change before the
throttled write extended it. They are now recomputed for it as well.

Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS

internal/store/attachments.go

@@ -1759,6 +1759,7 @@ func (s *Store) RemapAttachmentReferencesInWorkspace(workspaceID string, oldToNe
 		collectionID string
 		content      string
 		fields       string
+		oldContent   string
 	}
 	var updates []rowUpdate
 	for rows.Next() {
@@ -1770,7 +1771,7 @@ func (s *Store) RemapAttachmentReferencesInWorkspace(workspaceID string, oldToNe
 		newContent := remapAttachmentRefs(content, oldToNew)
 		newFields := remapAttachmentRefs(fields, oldToNew)
 		if newContent != content || newFields != fields {
-			updates = append(updates, rowUpdate{id: id, collectionID: collectionID, content: newContent, fields: newFields})
+			updates = append(updates, rowUpdate{id: id, collectionID: collectionID, content: newContent, fields: newFields, oldContent: content})
 		}
 	}
 	if err := rows.Err(); err != nil {
@@ -1873,6 +1874,13 @@ func (s *Store) RemapAttachmentReferencesInWorkspace(workspaceID string, oldToNe
 			u.content, u.fields, workspaceID, u.id); err != nil {
 			return fmt.Errorf("update item %s: %w", u.id, err)
 		}
+		// The body changed with no version, so the newest version's reverse
+		// patch is rebased onto it (BUG-3745).
+		if u.content != u.oldContent {
+			if err := s.rebaseNewestVersionPatch(tx, u.id, u.oldContent, u.content); err != nil {
+				return fmt.Errorf("rebase newest version of %s: %w", u.id, err)
+			}
+		}
 		// This rewrite maps `pad-attachment:` ids and cannot change a relation
 		// VALUE — but it does write the blob, and the hook is a pure function
 		// of (blob, schema), so calling it is a no-op while OMITTING it would

internal/store/bug3745_throttled_version_test.go

@@ -0,0 +1,241 @@
+package store
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/PerpetualSoftware/pad/internal/diff"
+	"github.com/PerpetualSoftware/pad/internal/models"
+)
+
+// BUG-3745: a throttled write (same actor and source within the window)
+// replaces the body without a new version. The newest version is a reverse
+// patch made against the body it replaced, so unless that patch is rebased
+// onto the new body, History resolves it against content it was never made
+// from, and the old version picks up text that did not exist yet.
+func TestBUG3745_ThrottledWriteKeepsTheNewestVersionExact(t *testing.T) {
+	s := testStore(t)
+	ws := createTestWorkspace(t, s, "Throttle")
+	col := createTestCollection(t, s, ws.ID, "Docs")
+
+	var paras []string
+	for i := 0; i < 40; i++ {
+		paras = append(paras, "Paragraph about the deployment pipeline and its many careful steps, number "+strings.Repeat("x", i%7)+".")
+	}
+	a := strings.Join(paras, "\n\n")
+	b := strings.Replace(a, "Paragraph about", "Notes about", 1) // a small edit early in a long doc
+	c := b + "\n\nA paragraph appended later, which version A never had."
+
+	item := createTestItem(t, s, ws.ID, col.ID, "Long doc", a)
+	write := func(body string) *models.Item {
+		t.Helper()
+		out, err := s.UpdateItem(item.ID, models.ItemUpdate{Content: &body, LastModifiedBy: "alice", Source: "web"})
+		if err != nil {
+			t.Fatal(err)
+		}
+		return out
+	}
+	first := write(b)
+	if first.BodyEditedWithoutVersion {
+		t.Fatal("instrument: the first edit by alice/web must write a version")
+	}
+	var isDiff bool
+	if err := s.db.QueryRow(s.q(`SELECT is_diff FROM item_versions WHERE item_id = ? ORDER BY created_at DESC, version_seq DESC LIMIT 1`), item.ID).Scan(&isDiff); err != nil || !isDiff {
+		t.Fatalf("instrument: the newest version must be a reverse patch (is_diff=%v, %v)", isDiff, err)
+	}
+	second := write(c)
+	if !second.BodyEditedWithoutVersion {
+		t.Fatal("instrument: the second edit inside the window must be throttled")
+	}
+
+	versions, err := s.ListItemVersionsResolved(item.ID, c)
+	if err != nil {
+		t.Fatal(err)
+	}
+	found := false
+	for _, v := range versions {
+		if strings.Contains(v.Content, "appended later") && v.Content != c {
+			t.Errorf("a version holds text written after it: %q...", v.Content[:60])
+		}
+		if v.Content == a {
+			found = true
+		}
+	}
+	if !found {
+		t.Fatalf("no version resolves to exactly the body before alice's edits; got %d versions", len(versions))
+	}
+	// Its line counts describe the change it now covers: a to c.
+	wantAdded, wantRemoved := diff.LineCounts(a, c)
+	var added, removed int
+	if err := s.db.QueryRow(s.q(`SELECT lines_added, lines_removed FROM item_versions WHERE item_id = ? ORDER BY created_at DESC, version_seq DESC LIMIT 1`), item.ID).Scan(&added, &removed); err != nil {
+		t.Fatal(err)
+	}
+	if added != wantAdded || removed != wantRemoved {
+		t.Errorf("newest version counts +%d -%d, want +%d -%d (a to c)", added, removed, wantAdded, wantRemoved)
+	}
+}
+
+// The property: after any run of throttled writes, every version in the
+// reverse chain still resolves to exactly the body it recorded.
+func TestBUG3745_EveryVersionResolvesExactlyAfterThrottledWrites(t *testing.T) {
+	s := testStore(t)
+	ws := createTestWorkspace(t, s, "Chain")
+	col := createTestCollection(t, s, ws.ID, "Docs")
+
+	base := make([]string, 30)
+	for i := range base {
+		base[i] = "Line " + strings.Repeat("ab", i%5) + " of a long shared document about release planning."
+	}
+	body := func(edit func([]string) []string) string {
+		cp := append([]string(nil), base...)
+		return strings.Join(edit(cp), "\n")
+	}
+	b0 := body(func(l []string) []string { return l })
+	b1 := body(func(l []string) []string { l[2] = "bob rewrote line two"; return l })
+	b2 := body(func(l []string) []string { l[2] = "bob rewrote line two"; l[5] = "alice edited line five"; return l })
+	b3 := b2 + "\nalice appended a line"
+	b4 := "alice prepended a line\n" + b3
+	b5 := strings.Replace(b4, "release planning", "launch planning", 1)
+
+	item := createTestItem(t, s, ws.ID, col.ID, "Chain doc", b0)
+	write := func(who, body string) bool {
+		t.Helper()
+		out, err := s.UpdateItem(item.ID, models.ItemUpdate{Content: &body, LastModifiedBy: who, Source: "web"})
+		if err != nil {
+			t.Fatal(err)
+		}
+		return out.BodyEditedWithoutVersion
+	}
+	if write("bob", b1) || write("alice", b2) {
+		t.Fatal("instrument: a change of writer must write a version")
+	}
+	for _, b := range []string{b3, b4, b5} {
+		if !write("alice", b) {
+			t.Fatal("instrument: alice's later edits inside the window must be throttled")
+		}
+	}
+
+	versions, err := s.ListItemVersionsResolved(item.ID, b5)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// Newest first: alice's version holds b1 (the body before her first
+	// edit), bob's holds b0, then the create.
+	want := []string{b1, b0}
+	if len(versions) < len(want) {
+		t.Fatalf("got %d versions, want at least %d", len(versions), len(want))
+	}
+	for i, w := range want {
+		if versions[i].Content != w {
+			t.Errorf("version %d resolves to %q..., want %q...", i, versions[i].Content[:50], w[:50])
+		}
+	}
+}
+
+// The two system rewrites of items.content that write no version, the wiki
+// title cascade and the attachment remap, also leave the newest reverse
+// patch made against a body that is gone (BUG-3745 class sweep). In both,
+// the rewritten text sits far from the patch's hunk, so an unrebased patch
+// still applies and carries the rewrite into the old version.
+func TestBUG3745_SystemRewritesKeepTheNewestVersionExact(t *testing.T) {
+	long := func(head, tail string) string {
+		lines := []string{head}
+		for i := 0; i < 30; i++ {
+			lines = append(lines, "Filler line "+strings.Repeat("q", i%6)+" about quarterly planning and staffing.")
+		}
+		return strings.Join(append(lines, tail), "\n")
+	}
+	setup := func(t *testing.T, head string) (*Store, *models.Workspace, *models.Collection, *models.Item, string) {
+		t.Helper()
+		s := testStore(t)
+		ws := createTestWorkspace(t, s, "Rewrites")
+		col := createTestCollection(t, s, ws.ID, "Docs")
+		before := long(head, "the tail as bob left it")
+		item := createTestItem(t, s, ws.ID, col.ID, "Linker", long(head, "the original tail"))
+		if _, err := s.UpdateItem(item.ID, models.ItemUpdate{Content: &before, LastModifiedBy: "bob", Source: "web"}); err != nil {
+			t.Fatal(err)
+		}
+		after := long(head, "the tail as alice left it")
+		if _, err := s.UpdateItem(item.ID, models.ItemUpdate{Content: &after, LastModifiedBy: "alice", Source: "web"}); err != nil {
+			t.Fatal(err)
+		}
+		var isDiff bool
+		if err := s.db.QueryRow(s.q(`SELECT is_diff FROM item_versions WHERE item_id = ? ORDER BY created_at DESC, version_seq DESC LIMIT 1`), item.ID).Scan(&isDiff); err != nil || !isDiff {
+			t.Fatalf("instrument: the newest version must be a reverse patch (is_diff=%v, %v)", isDiff, err)
+		}
+		return s, ws, col, item, before
+	}
+	newest := func(t *testing.T, s *Store, itemID string) string {
+		t.Helper()
+		cur, err := s.GetItem(itemID)
+		if err != nil {
+			t.Fatal(err)
+		}
+		versions, err := s.ListItemVersionsResolved(itemID, cur.Content)
+		if err != nil || len(versions) == 0 {
+			t.Fatalf("versions: %d, %v", len(versions), err)
+		}
+		return versions[0].Content
+	}
+
+	t.Run("wiki title cascade", func(t *testing.T) {
+		s, ws, col, item, before := setup(t, "See [[Old Target]] for the plan.")
+		target := createTestItem(t, s, ws.ID, col.ID, "Old Target", "the target")
+		title := "New Target"
+		if _, err := s.UpdateItem(target.ID, models.ItemUpdate{Title: &title}); err != nil {
+			t.Fatal(err)
+		}
+		if cur, _ := s.GetItem(item.ID); !strings.Contains(cur.Content, "[[New Target]]") {
+			t.Fatal("instrument: the cascade must rewrite the linking item's body")
+		}
+		if got := newest(t, s, item.ID); got != before {
+			t.Errorf("the newest version after the cascade resolves to %q..., want the body before alice's edit, with the old title", got[:40])
+		}
+	})
+
+	t.Run("attachment remap", func(t *testing.T) {
+		oldID, newID := "00000000-0000-0000-0000-0000000000a1", "00000000-0000-0000-0000-0000000000b2"
+		s, ws, _, item, before := setup(t, "![chart](pad-attachment:"+oldID+")")
+		if err := s.RemapAttachmentReferencesInWorkspace(ws.ID, map[string]string{oldID: newID}); err != nil {
+			t.Fatal(err)
+		}
+		if cur, _ := s.GetItem(item.ID); !strings.Contains(cur.Content, newID) {
+			t.Fatal("instrument: the remap must rewrite the item's body")
+		}
+		if got := newest(t, s, item.ID); got != before {
+			t.Errorf("the newest version after the remap resolves to %q..., want the body before alice's edit, with the old id", got[:40])
+		}
+	})
+}
+
+// A newest version stored in full needs no rebase, but its line counts
+// still describe the change it covers, which the throttled write extended.
+func TestBUG3745_FullBodyVersionCountsFollowTheThrottledWrite(t *testing.T) {
+	s := testStore(t)
+	ws := createTestWorkspace(t, s, "Counts")
+	col := createTestCollection(t, s, ws.ID, "Docs")
+	a := "one\ntwo"
+	b := "completely\ndifferent\nwords" // a patch is not smaller than a body this short
+	c := b + "\nand\nthree\nmore"
+	item := createTestItem(t, s, ws.ID, col.ID, "Short", a)
+	for i, body := range []string{b, c} {
+		out, err := s.UpdateItem(item.ID, models.ItemUpdate{Content: &body, LastModifiedBy: "alice", Source: "web"})
+		if err != nil {
+			t.Fatal(err)
+		}
+		if (i == 1) != out.BodyEditedWithoutVersion {
+			t.Fatalf("instrument: write %d throttled=%v", i, out.BodyEditedWithoutVersion)
+		}
+	}
+	var isDiff bool
+	var added, removed int
+	if err := s.db.QueryRow(s.q(`SELECT is_diff, lines_added, lines_removed FROM item_versions WHERE item_id = ? ORDER BY created_at DESC, version_seq DESC LIMIT 1`), item.ID).Scan(&isDiff, &added, &removed); err != nil {
+		t.Fatal(err)
+	}
+	if isDiff {
+		t.Fatal("instrument: the newest version must be stored in full")
+	}
+	if wa, wr := diff.LineCounts(a, c); added != wa || removed != wr {
+		t.Errorf("newest version counts +%d -%d, want +%d -%d (a to c)", added, removed, wa, wr)
+	}
+}

internal/store/items.go

@@ -3036,6 +3036,16 @@ func (s *Store) updateItemWithParentLinkOnce(
 			}
 		} else {
 			bodyEditedWithoutVersion = true
+			// The row's body moves on without a version of its own, so a
+			// reverse patch in the newest version, made against the body
+			// being replaced, would be resolved against one it was never made
+			// from (BUG-3745). Only Content moves the row; ExternalContent
+			// leaves it, and the patch with it, as they are.
+			if input.Content != nil {
+				if err := s.rebaseNewestVersionPatch(tx, id, existing.Content, *input.Content); err != nil {
+					return nil, fmt.Errorf("rebase newest version: %w", err)
+				}
+			}
 		}
 	}
 
@@ -5952,6 +5962,55 @@ func (s *Store) newestVersionIsUnflushedApplierRow(q rowQueryer, itemID, body st
 	return !parseTime(createdAt).Before(parseTime(flushedAt.String)), nil
 }
 
+// rebaseNewestVersionPatch keeps the newest version exact when an
+// unversioned write replaces the row's body (BUG-3745). Versions are reverse
+// patches resolved backwards from the row, and diff.ApplyPatch does not check
+// that a patch's base matches: a patch made against oldBody, resolved against
+// newBody, can apply and carry newBody's later text into the old version. So
+// the version's own body is materialized from oldBody, the base its patch was
+// made against, and stored again against newBody: as a patch when that is
+// smaller, otherwise in full. Either way its line counts are recomputed. A
+// patch that does not apply to oldBody was already broken before this write
+// (data written before this fix can be), and is left as it is: refusing the
+// write instead would block every edit to that item over history that is
+// already damaged.
+func (s *Store) rebaseNewestVersionPatch(tx *sql.Tx, itemID, oldBody, newBody string) error {
+	var vid, content string
+	var isDiff bool
+	err := tx.QueryRow(s.q(`
+		SELECT id, content, is_diff FROM item_versions
+		WHERE item_id = ?
+		ORDER BY created_at DESC, version_seq DESC
+		LIMIT 1
+	`), itemID).Scan(&vid, &content, &isDiff)
+	if err == sql.ErrNoRows {
+		return nil
+	}
+	if err != nil {
+		return err
+	}
+	// The version's recorded change is its body to the one that replaced it
+	// (PLAN-2348 U2), which is now newBody: the unversioned edits joined it.
+	// A full-body version needs only its counts brought up to date.
+	if !isDiff {
+		added, removed := diff.LineCounts(content, newBody)
+		_, err = tx.Exec(s.q(`UPDATE item_versions SET lines_added = ?, lines_removed = ? WHERE id = ?`), added, removed, vid)
+		return err
+	}
+	body, err := diff.ApplyPatch(oldBody, content)
+	if err != nil {
+		return nil
+	}
+	stored, storedDiff := body, false
+	if patch := diff.CreateReversePatch(body, newBody); diff.IsDiffSmaller(patch, body) {
+		stored, storedDiff = patch, true
+	}
+	added, removed := diff.LineCounts(body, newBody)
+	_, err = tx.Exec(s.q(`UPDATE item_versions SET content = ?, is_diff = ?, lines_added = ?, lines_removed = ? WHERE id = ?`),
+		stored, s.dialect.BoolToInt(storedDiff), added, removed, vid)
+	return err
+}
+
 func (s *Store) shouldCreateItemVersion(q rowQueryer, itemID, actor, source, viaApp string) (bool, error) {
 	var createdBy, src, createdAt, rowViaApp string
 	var imported int

internal/store/wiki_links.go

@@ -846,6 +846,11 @@ func (s *Store) cascadeTitleRename(tx *sql.Tx, renamedItemID, workspaceID, oldTi
 		`), newContent, ts, ts, work.workspaceID, work.id); err != nil {
 			return fmt.Errorf("cascade rename: update source %s: %w", work.id, err)
 		}
+		// The body changed with no version, so the newest version's reverse
+		// patch is rebased onto it (BUG-3745).
+		if err := s.rebaseNewestVersionPatch(tx, work.id, content, newContent); err != nil {
+			return fmt.Errorf("rebase newest version of %s: %w", work.id, err)
+		}
 		if err := s.replaceWikiLinks(tx, work.id, work.workspaceID, newContent); err != nil {
 			return fmt.Errorf("cascade rename: reparse %s: %w", work.id, err)
 		}