From 09ecc5e02faa697fa42186606e01047272b50479 Mon Sep 17 00:00:00 2001 From: xarmian Date: Thu, 15 Oct 2026 10:24:24 -0400 Subject: [PATCH] fix(diff): version writers never diff text that is not valid UTF-8 (BUG-3839) (#2200) * fix(diff): version writers never diff text that is not valid UTF-8 (BUG-3839) go-diff works on runes and slices its input at byte offsets derived from them, so text that is not valid UTF-8 near an edit either panics in CreatePatch (slice bounds out of range) or builds a patch that does not reproduce the old text: "hello world" -> "hello \xff world" reverse-applied to "hello d", with no error. Any invalid byte does it, lone \xff and \x80 as well as truncated sequences. diff.ReversePatch returns (patch, ok), false when either side is not valid UTF-8, and every version writer stores the full body in that case: the item edit and the throttled rebase (items.go), the import's attachment remap (attachments.go) and the legacy document edit (documents.go). The full body is stored losslessly on SQLite; Postgres refuses invalid UTF-8 in the column, so there the body write fails first and nothing is versioned. No existing row is rewritten. Requests cannot deliver such text (encoding/json coerces it, artifact import rejects it, the materializer emits U+FFFD for lone surrogates), but SQLite rows from older releases or direct edits can hold it, and an item like that 500ed on every save. Tests: diff unit pins for both shapes, a census that production code calls only ReversePatch, and a store pin per caller on SQLite and Postgres. With the UTF-8 check removed every SQLite leg panics or stores a wrong version, and the census names the reintroduced call. Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS * test(diff): LineCounts, FormatDiffSummary and Unified never panic on invalid UTF-8 (BUG-3839) Pins the 200k-case fuzz measurement deterministically: the panic shapes as seeds plus 20k generated pairs from the same seed-1 generator, with an instrument showing the inputs reach go-diff's PatchApply panic. Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS * fix(diff): ApplyPatch returns an error where go-diff would panic (BUG-3839) ApplyPatch refuses a base or a decoded patch text that is not valid UTF-8, and turns a go-diff PatchApply panic into an error. The second covers valid text: a well-formed patch applied over a base it was not made from panics in about 1.5% of measured pairs, and the version walk does that after it skips a row. Every caller (the item and document walks, the throttled-edit rebase, the attachment remap, the tie diagnostic) already treats the error as a patch that failed, so the row reads as patch_failed instead of a 500. Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS * test(store): the remap pin asserts the row it leaves over an invalid body (BUG-3839) With ApplyPatch refusing an invalid base, the remap cannot resolve a diff row over the invalid current body: it remaps the patch text in place, and the row reads as patch_failed. The pin asserts exactly that; it is red when the base check is removed (the row is then re-diffed from a fuzzy body). Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS * refactor(diff): recover wraps only the go-diff call, and the remap pin is named for patch_failed (BUG-3839) Lead rulings on BUG-3839 checkpoint 11. Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS * test(diff): the unchecked-patch census resolves the import name (BUG-3839) Matches any selector on the file's local name for the diff package (alias, method value), bare identifiers under a dot import, and skips web, node_modules, .git and vendor only at the module root. A self-test runs the matcher over each form. Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS * test(store): History past a broken row does not panic (BUG-3839) The main-reproduction pin: a broken newest row, then an older row applied over a base it was not made from. Red on main (d95f90850, slice bounds panic), green here. Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS * fix(diff): ApplyPatch refuses a result that is not valid UTF-8 (BUG-3839) Over a base it was not made from, go-diff can succeed and cut a rune in half: valid text in, "\x80" out (#2200 review r2, hunter). The rebase would write that body (Postgres refuses it, failing the edit) and History would show it. Pinned in the diff fuzz leg and in a mangled_result leg of the item History, document History and throttled-rebase store tests, on both backends; red when the check is removed. The doc comment now says an empty patch returns its base unchecked, which is exact. Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS --- internal/diff/bug3839_invalid_utf8_test.go | 189 +++++++ internal/diff/bug3839_no_panic_test.go | 164 ++++++ internal/diff/diff.go | 82 ++- internal/store/attachments.go | 2 +- internal/store/bug3839_broken_chain_test.go | 60 +++ .../bug3839_invalid_utf8_version_test.go | 468 ++++++++++++++++++ internal/store/documents.go | 4 +- internal/store/items.go | 8 +- 8 files changed, 969 insertions(+), 8 deletions(-) create mode 100644 internal/diff/bug3839_invalid_utf8_test.go create mode 100644 internal/diff/bug3839_no_panic_test.go create mode 100644 internal/store/bug3839_broken_chain_test.go create mode 100644 internal/store/bug3839_invalid_utf8_version_test.go diff --git a/internal/diff/bug3839_invalid_utf8_test.go b/internal/diff/bug3839_invalid_utf8_test.go new file mode 100644 index 000000000..6f8ae9c54 --- /dev/null +++ b/internal/diff/bug3839_invalid_utf8_test.go @@ -0,0 +1,189 @@ +package diff + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "strings" + "testing" +) + +// BUG-3839: go-diff cannot patch text that is not valid UTF-8. Near an edit +// it panics or builds a patch that does not reproduce the old text; both +// shapes are pinned below through CreateReversePatch (unchecked), and +// ReversePatch refuses them. +var bug3839Cases = []struct { + name string + old, new string +}{ + // Panic shapes: an invalid byte, lone or a truncated sequence, next to + // an edit. + {"lone_ff", "a\xff", "a\xffb"}, + {"lone_80", "x\x80y", "\x80"}, + {"truncated_2", "a\xc3", "a\xc3b"}, + {"truncated_3", "\xe2\x82", "x\xe2\x82y"}, + // Wrong-patch shape: reverse-applies to "hello d", with no error. + {"wrong_patch", "hello world", "hello \xff world"}, +} + +func TestBUG3839_ReversePatchRefusesInvalidUTF8(t *testing.T) { + for _, tc := range bug3839Cases { + t.Run(tc.name, func(t *testing.T) { + if p, ok := ReversePatch(tc.old, tc.new); ok { + t.Fatalf("ReversePatch accepted text that is not valid UTF-8: %q", p) + } + // Instrument: unchecked, each case really does fail. + var p string + panicked := func() (r any) { + defer func() { r = recover() }() + p = CreateReversePatch(tc.old, tc.new) + return nil + }() + if panicked == nil { + if got, err := ApplyPatch(tc.new, p); err == nil && got == tc.old { + t.Fatalf("instrument: the unchecked patch round-trips, so this case shows nothing") + } + } + }) + } +} + +func TestBUG3839_ReversePatchRoundTripsValidText(t *testing.T) { + for _, tc := range [][2]string{ + {"hello world", "hello brave world"}, + {"café ☕ 😀 naïve", "café ☕ naïve 😀!"}, + {"", "new"}, + {"old", ""}, + } { + p, ok := ReversePatch(tc[0], tc[1]) + if !ok { + t.Fatalf("ReversePatch refused valid text %q -> %q", tc[0], tc[1]) + } + if got, err := ApplyPatch(tc[1], p); err != nil || got != tc[0] { + t.Fatalf("%q -> %q: reverse-applied to %q (%v)", tc[0], tc[1], got, err) + } + } +} + +// Production code calls ReversePatch, never the unchecked CreatePatch or +// CreateReversePatch, which tests still use to build fixtures from valid +// text. A census of every non-test Go file in the module. +func TestBUG3839_NoUncheckedPatchCallers(t *testing.T) { + root := filepath.Join("..", "..") + fset := token.NewFileSet() + var bad []string + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + // The web UI and the module's own tooling dirs, at the root only: + // a Go package named web elsewhere is still production code. + if filepath.Dir(path) == root { + switch d.Name() { + case "node_modules", ".git", "web", "vendor": + return filepath.SkipDir + } + } + return nil + } + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + f, err := parser.ParseFile(fset, path, nil, 0) + if err != nil { + return err + } + // Inside the package, ReversePatch and CreateReversePatch are the + // checked door and its unchecked twin. + if f.Name.Name == "diff" && filepath.Base(filepath.Dir(path)) == "diff" { + return nil + } + bad = append(bad, uncheckedPatchUses(fset, f)...) + return nil + }) + if err != nil { + t.Fatal(err) + } + if len(bad) > 0 { + t.Fatalf("production code uses an unchecked patch builder; use diff.ReversePatch (BUG-3839):\n%s", strings.Join(bad, "\n")) + } +} + +const diffImportPath = "github.com/PerpetualSoftware/pad/internal/diff" + +// uncheckedPatchUses lists every reference to CreatePatch or +// CreateReversePatch through this file's import of the package: a call, a +// method value, under any import name, or bare under a dot import. +func uncheckedPatchUses(fset *token.FileSet, f *ast.File) []string { + local, dot := "", false + for _, imp := range f.Imports { + if strings.Trim(imp.Path.Value, `"`) != diffImportPath { + continue + } + switch { + case imp.Name == nil: + local = "diff" + case imp.Name.Name == ".": + dot = true + case imp.Name.Name != "_": + local = imp.Name.Name + } + } + if local == "" && !dot { + return nil + } + unchecked := func(name string) bool { return name == "CreatePatch" || name == "CreateReversePatch" } + var out []string + // A selector's own name is never a dot-imported identifier. + sel := map[*ast.Ident]bool{} + ast.Inspect(f, func(n ast.Node) bool { + switch x := n.(type) { + case *ast.SelectorExpr: + sel[x.Sel] = true + // A selector on a local variable that shadows the import name is + // a false positive here; none exists, and a hit says which file. + if id, ok := x.X.(*ast.Ident); ok && local != "" && id.Name == local && unchecked(x.Sel.Name) { + out = append(out, fset.Position(x.Pos()).String()+": "+x.Sel.Name) + } + case *ast.Ident: + if dot && !sel[x] && unchecked(x.Name) { + out = append(out, fset.Position(x.Pos()).String()+": "+x.Name) + } + } + return true + }) + return out +} + +// The census's own instrument: each way a file can reach the unchecked +// builders is found, and the checked door and a same-named local are not. +func TestBUG3839_UncheckedPatchCensusFindsEveryForm(t *testing.T) { + for _, tc := range []struct { + name string + src string + hits int + }{ + {"call", `import "` + diffImportPath + `"; func f() { diff.CreatePatch("a", "b") }`, 1}, + {"method_value", `import "` + diffImportPath + `"; var g = diff.CreateReversePatch`, 1}, + {"alias", `import d "` + diffImportPath + `"; func f() { d.CreatePatch("a", "b") }`, 1}, + {"dot", `import . "` + diffImportPath + `"; func f() { CreateReversePatch("a", "b"); _ = CreatePatch }`, 2}, + {"dot_inside_a_selector", `import . "` + diffImportPath + `"; func f() { _ = T{CreatePatch("a", "b")}.x; _ = other.CreatePatch }`, 1}, + {"checked_door", `import "` + diffImportPath + `"; func f() { diff.ReversePatch("a", "b") }`, 0}, + {"other_package_named_diff", `import "example.com/diff"; func f() { diff.CreatePatch("a", "b") }`, 0}, + {"alias_frees_the_name", `import d "` + diffImportPath + `"; func f(diff T) { diff.CreatePatch() }`, 0}, + } { + t.Run(tc.name, func(t *testing.T) { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "x.go", "package x\n"+tc.src, 0) + if err != nil { + t.Fatal(err) + } + if got := uncheckedPatchUses(fset, f); len(got) != tc.hits { + t.Fatalf("found %d uses, want %d: %v", len(got), tc.hits, got) + } + }) + } +} diff --git a/internal/diff/bug3839_no_panic_test.go b/internal/diff/bug3839_no_panic_test.go new file mode 100644 index 000000000..2f1c820ee --- /dev/null +++ b/internal/diff/bug3839_no_panic_test.go @@ -0,0 +1,164 @@ +package diff + +import ( + "math/rand" + "strings" + "testing" + "unicode/utf8" +) + +// BUG-3839: LineCounts, FormatDiffSummary and Unified are reached by text +// that may not be valid UTF-8 and are not guarded the way ReversePatch is. A +// 200k-case fuzz (seed 1, the generator below) found no panic in them while +// it found go-diff's PatchApply panicking at about 4%. This pins that +// measurement: the panic shapes as seeds, then the same generator, with an +// instrument showing the generator does reach the class. + +var bug3839Pieces = []string{"\xff", "\x80", "\xc3", "\xe2\x82", "\xf0\x9f\x98", "\xed\xa0\x80", "é", "😀", "\n", "a", "b", " "} + +// bug3839Pairs yields the seed shapes, then n generated pairs. +func bug3839Pairs(n int) [][2]string { + pairs := [][2]string{ + {"b\xe2\x82b\xff", "b"}, + {"\xff", ""}, + {"", "\x80"}, + {"\xe2\x82", "\xe2\x82\xac"}, + {"\xf0\x9f\x98", "\xf0\x9f\x98\x80"}, + {"\xed\xa0\x80", "a\xed\xa0\x80b"}, + {"x\n\xff\ny\n", "x\n\xc3\ny\n"}, + } + for _, tc := range bug3839Cases { + pairs = append(pairs, [2]string{tc.old, tc.new}) + } + r := rand.New(rand.NewSource(1)) + gen := func() string { + var b strings.Builder + for i, k := 0, r.Intn(12); i < k; i++ { + b.WriteString(bug3839Pieces[r.Intn(len(bug3839Pieces))]) + } + return b.String() + } + for i := 0; i < n; i++ { + o, nw := gen(), gen() + if r.Intn(2) == 0 { + nw = o[:r.Intn(len(o)+1)] + gen() + o[r.Intn(len(o)+1):] + } + pairs = append(pairs, [2]string{o, nw}) + } + return pairs +} + +func bug3839Panics(f func()) (panicked bool) { + defer func() { panicked = recover() != nil }() + f() + return false +} + +func TestBUG3839_DiffNeverPanicsOnInvalidUTF8(t *testing.T) { + pairs := bug3839Pairs(20000) + + // Instrument: the same inputs reach go-diff's panic. PatchApply is what + // ApplyPatch wrapped before it refused invalid text. + reached := 0 + for _, p := range pairs { + patches, err := dmp.PatchFromText("@@ -1,3 +1,4 @@\n a" + p[1] + "\n+b\n") + if err != nil { + continue + } + if bug3839Panics(func() { dmp.PatchApply(patches, p[0]) }) { + reached++ + } + } + t.Logf("instrument: %d of %d inputs panic go-diff's PatchApply", reached, len(pairs)) + if reached == 0 { + t.Fatal("instrument: no input reached go-diff's PatchApply panic, so a pass below shows nothing") + } + + for _, p := range pairs { + for name, f := range map[string]func(){ + "LineCounts": func() { LineCounts(p[0], p[1]) }, + "FormatDiffSummary": func() { FormatDiffSummary(p[0], p[1]) }, + "Unified": func() { Unified(p[0], p[1], 3) }, + "ApplyPatch": func() { _, _ = ApplyPatch(p[0], "@@ -1,3 +1,4 @@\n a"+p[1]+"\n+b\n") }, + } { + if bug3839Panics(f) { + t.Fatalf("%s(%q, %q) panicked", name, p[0], p[1]) + } + } + } +} + +// ApplyPatch refuses a base or a patch text that is not valid UTF-8 with an +// error, which the version walk reads as a patch that failed (BUG-3839). +func TestBUG3839_ApplyPatchRefusesInvalidUTF8(t *testing.T) { + valid := CreatePatch("ab", "abb") + for _, tc := range []struct{ name, base, patch string }{ + {"base_fuzz_first", "b\xe2\x82b\xff", "@@ -1,3 +1,4 @@\n ab\n+b\n"}, + {"base_lone_ff", "ab\xff", valid}, + {"base_truncated", "ab\xe2\x82", valid}, + {"patch_raw_byte", "ab", "@@ -1,2 +1,3 @@\n ab\n+\xff\n"}, + {"patch_escaped_byte", "ab", "@@ -1,2 +1,3 @@\n ab\n+%FF\n"}, + } { + t.Run(tc.name, func(t *testing.T) { + var err error + if bug3839Panics(func() { _, err = ApplyPatch(tc.base, tc.patch) }) { + t.Fatal("ApplyPatch panicked") + } + if err == nil { + t.Fatal("ApplyPatch accepted text that is not valid UTF-8") + } + }) + } + if got, err := ApplyPatch("ab", valid); err != nil || got != "abb" { + t.Fatalf("valid patch: %q %v", got, err) + } + if got, err := ApplyPatch("a+b", CreatePatch("a+b", "a+b c%")); err != nil || got != "a+b c%" { + t.Fatalf("valid patch over + and %%: %q %v", got, err) + } +} + +// go-diff panics on VALID text too, when a well-formed patch is applied over +// a base it was not made from and the base holds multi-byte runes. The version +// walk does that after skipping a row; ApplyPatch reports it as an error. +func TestBUG3839_ApplyPatchOverAnotherValidBaseNeverPanicsOrMangles(t *testing.T) { + r := rand.New(rand.NewSource(1)) + pieces := []string{"é", "😀", "À", "\n", "a", "b", " ", "ab", "%", "+"} + gen := func() string { + var b strings.Builder + for i, k := 0, r.Intn(14); i < k; i++ { + b.WriteString(pieces[r.Intn(len(pieces))]) + } + return b.String() + } + reached, mangled := 0, 0 + for i := 0; i < 20000; i++ { + a, b, base := gen(), gen(), gen() + p := CreatePatch(a, b) + patches, err := dmp.PatchFromText(p) + if err != nil { + t.Fatal(err) + } + // Instrument: go-diff itself panics on this input. + if bug3839Panics(func() { dmp.PatchApply(patches, base) }) { + reached++ + } + // Instrument: go-diff returns text that is not valid UTF-8. + bug3839Panics(func() { + if out, _ := dmp.PatchApply(patches, base); !utf8.ValidString(out) { + mangled++ + } + }) + var got string + var applyErr error + if bug3839Panics(func() { got, applyErr = ApplyPatch(base, p) }) { + t.Fatalf("ApplyPatch(%q, CreatePatch(%q, %q)) panicked", base, a, b) + } + if applyErr == nil && !utf8.ValidString(got) { + t.Fatalf("ApplyPatch(%q, CreatePatch(%q, %q)) returned %q, which is not valid UTF-8", base, a, b, got) + } + } + t.Logf("instrument: of 20000 inputs, %d panic go-diff's PatchApply and %d make it return invalid UTF-8", reached, mangled) + if reached == 0 || mangled == 0 { + t.Fatal("instrument: the inputs do not reach both go-diff failures, so a pass shows nothing") + } +} diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 08b1d557e..047ea704a 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -4,7 +4,9 @@ package diff import ( "fmt" + "net/url" "strings" + "unicode/utf8" difflib "github.com/sergi/go-diff/diffmatchpatch" ) @@ -13,6 +15,7 @@ var dmp = difflib.New() // CreatePatch computes a compact patch that transforms oldText into newText. // The patch can be applied with ApplyPatch to recover newText from oldText. +// Both texts must be valid UTF-8 (BUG-3839; see ReversePatch). func CreatePatch(oldText, newText string) string { diffs := dmp.DiffMain(oldText, newText, true) patches := dmp.PatchMake(oldText, diffs) @@ -20,25 +23,82 @@ func CreatePatch(oldText, newText string) string { } // ApplyPatch applies a patch to baseText to produce the target text. -// Returns an error if the patch cannot be applied cleanly. +// Returns an error if the patch cannot be applied cleanly, or if the base or +// the patch's text is not valid UTF-8: go-diff slices such text at offsets it +// derives from runes, and panics or builds the wrong text (BUG-3839). +// +// go-diff also panics on valid text, when a patch is applied over a base it +// was not made from and the base holds multi-byte runes (about 1.5% of such +// pairs in the BUG-3839 measurement), or returns text that is not valid +// UTF-8. The version walk does exactly that after it skips a row, so both +// are errors here too. +// +// An empty patch returns baseText as it is, valid or not: it records a +// version byte-identical to its base, and go-diff is never reached. func ApplyPatch(baseText, patchText string) (string, error) { if patchText == "" { return baseText, nil } + if !utf8.ValidString(baseText) { + return "", fmt.Errorf("base text is not valid UTF-8") + } + if !patchTextValidUTF8(patchText) { + return "", fmt.Errorf("patch text is not valid UTF-8") + } patches, err := dmp.PatchFromText(patchText) if err != nil { return "", fmt.Errorf("parse patch: %w", err) } - result, applied := dmp.PatchApply(patches, baseText) + result, applied, err := patchApply(patches, baseText) + if err != nil { + return "", err + } // Check that all hunks applied for i, ok := range applied { if !ok { return "", fmt.Errorf("patch hunk %d failed to apply", i) } } + // Over a base it was not made from, go-diff can also succeed and cut a + // rune in half: valid text in, invalid text out. No version recorded that. + if !utf8.ValidString(result) { + return "", fmt.Errorf("applying the patch produced text that is not valid UTF-8") + } return result, nil } +// patchTextValidUTF8 reports whether every text line of a patch decodes to +// valid UTF-8. A line is escaped, so "%FF" is the byte 0xFF once parsed; this +// decodes each line the way PatchFromText does, whose result is unexported. +func patchTextValidUTF8(patchText string) bool { + for _, line := range strings.Split(patchText, "\n") { + if line == "" { + continue + } + switch line[0] { + case ' ', '-', '+': + // PatchFromText ignores the error too: a malformed line decodes + // to "", which is valid. + text, _ := url.QueryUnescape(strings.ReplaceAll(line[1:], "+", "%2b")) + if !utf8.ValidString(text) { + return false + } + } + } + return true +} + +// patchApply is go-diff's PatchApply with its panic returned as an error. +func patchApply(patches []difflib.Patch, base string) (result string, applied []bool, err error) { + defer func() { + if r := recover(); r != nil { + result, applied, err = "", nil, fmt.Errorf("go-diff panicked applying the patch: %v", r) + } + }() + result, applied = dmp.PatchApply(patches, base) + return result, applied, nil +} + // IsDiffSmaller returns true if storing a diff would be meaningfully smaller // than storing the full content. Returns false for near-complete rewrites // where the diff is >= 80% of the full content size. @@ -52,10 +112,28 @@ func IsDiffSmaller(patch, fullContent string) bool { // CreateReversePatch computes a patch that transforms newText back into oldText. // Used for reverse-diff version storage: current doc has latest content, // versions store patches to go backwards. +// +// Both texts must be valid UTF-8, as for CreatePatch; production code calls +// ReversePatch, which checks (BUG-3839, TestNoUncheckedPatchCallers). func CreateReversePatch(oldText, newText string) string { return CreatePatch(newText, oldText) } +// ReversePatch is CreateReversePatch for text that may not be valid UTF-8 +// (BUG-3839). go-diff works on runes and slices the input at byte offsets it +// derives from them, so invalid UTF-8 near an edit either panics (slice +// bounds out of range) or yields a patch that does not reproduce oldText: +// "hello world" -> "hello \xff world" reverse-applied to "hello d". ok is +// false when either side is not valid UTF-8, and the caller stores the full +// body instead. Never sanitize and diff: the version would then record a body +// it does not hold. +func ReversePatch(oldText, newText string) (patch string, ok bool) { + if !utf8.ValidString(oldText) || !utf8.ValidString(newText) { + return "", false + } + return CreatePatch(newText, oldText), true +} + // FormatDiffSummary returns a human-readable summary of changes. Its "lines" // are character-diff CHUNKS (a one-character edit reads +1/-1); LineCounts is // the line-level count the version rows store (PLAN-2348 U2). diff --git a/internal/store/attachments.go b/internal/store/attachments.go index 8457c0c86..7e814f26a 100644 --- a/internal/store/attachments.go +++ b/internal/store/attachments.go @@ -2047,7 +2047,7 @@ func (s *Store) planVersionChainRemapTx(tx *sql.Tx, workspaceID string, oldToNew if newBody != r.body || (r.isDiff && newSucc != oldSucc) { stored, isDiff := newBody, false if r.isDiff { - if patch := diff.CreateReversePatch(newBody, newSucc); diff.IsDiffSmaller(patch, newBody) { + if patch, ok := diff.ReversePatch(newBody, newSucc); ok && diff.IsDiffSmaller(patch, newBody) { stored, isDiff = patch, true } } diff --git a/internal/store/bug3839_broken_chain_test.go b/internal/store/bug3839_broken_chain_test.go new file mode 100644 index 000000000..13c945a51 --- /dev/null +++ b/internal/store/bug3839_broken_chain_test.go @@ -0,0 +1,60 @@ +package store_test + +import ( + "testing" + + "github.com/PerpetualSoftware/pad/internal/diff" + "github.com/PerpetualSoftware/pad/internal/models" +) + +// BUG-3839: when a version row fails to apply, History keeps the body it had +// and applies the next older row over it, a base that row was not made from. +// go-diff panics on that for some multi-byte text, and the request was a 500. +// This walk has done that since the initial release. All text here is valid +// UTF-8; the older row reads as patch_failed instead. +func TestBUG3839_HistoryPastABrokenRowDoesNotPanic(t *testing.T) { + recorded, older, current := "a\né+abb😀", "%a+a😀ab", "é😀ab😀+%" + for _, backend := range contentBackends() { + t.Run(backend.name, func(t *testing.T) { + s := backend.open(t) + ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "BUG-3839"}) + if err != nil { + t.Fatal(err) + } + col, err := s.CreateCollection(ws.ID, models.CollectionCreate{Name: "Docs", Schema: `{"fields":[]}`}) + if err != nil { + t.Fatal(err) + } + item, err := s.CreateItem(ws.ID, col.ID, models.ItemCreate{Title: "Doc", Content: current, Fields: `{}`}) + if err != nil { + t.Fatal(err) + } + if _, err := s.DB().Exec(`DELETE FROM item_versions WHERE item_id = $1`, item.ID); err != nil { + t.Fatal(err) + } + for _, r := range []struct { + id, content, at string + seq int + }{ + // The newest row's patch does not apply to the current body. + {"r1", "@@ -1,3 +1,3 @@\n-zzz\n+yyy\n", "2026-10-02T00:00:00Z", 2}, + // The older row was made from the body r1 recorded. + {"r0", diff.CreatePatch(recorded, older), "2026-10-01T00:00:00Z", 1}, + } { + if _, err := s.DB().Exec(`INSERT INTO item_versions (id, item_id, content, is_diff, created_at, version_seq) VALUES ($1, $2, $3, $4, $5, $6)`, + r.id, item.ID, r.content, true, r.at, r.seq); err != nil { + t.Fatal(err) + } + } + vs, err := s.ListItemVersionsResolved(item.ID, current) + if err != nil || len(vs) != 2 { + t.Fatalf("versions: %d %v", len(vs), err) + } + for _, v := range vs { + if !v.Unreconstructable { + t.Fatalf("a row resolved over a base it was not made from: %q", v.Content) + } + } + }) + } +} diff --git a/internal/store/bug3839_invalid_utf8_version_test.go b/internal/store/bug3839_invalid_utf8_version_test.go new file mode 100644 index 000000000..5ed2f5c56 --- /dev/null +++ b/internal/store/bug3839_invalid_utf8_version_test.go @@ -0,0 +1,468 @@ +package store_test + +import ( + "strings" + "testing" + "unicode/utf8" + + "github.com/PerpetualSoftware/pad/internal/diff" + "github.com/PerpetualSoftware/pad/internal/models" + "github.com/PerpetualSoftware/pad/internal/store" +) + +// BUG-3839: go-diff cannot patch text that is not valid UTF-8. Near an edit it +// either panics (slice bounds out of range) or builds a patch that does not +// reproduce the version: "hello world" -> "hello \xff world" reverse-applied +// to "hello d". Every version writer now stores such a version as its full +// body. Requests cannot deliver invalid UTF-8 (encoding/json coerces it), but +// SQLite keeps it in rows an older release or a direct edit wrote, and the +// store writes whatever string it is given. +// +// Postgres refuses invalid UTF-8 in a text column, so there the body write +// itself fails, before any version is built: the PG legs pin that the store +// returns that error, does not panic, and leaves the item as it was. + +// bug3839Long is a body long enough that a small edit is stored as a patch. +func bug3839Long(tail string) string { + var b strings.Builder + for i := 0; i < 30; i++ { + b.WriteString("Paragraph about the deployment pipeline and its careful steps.\n\n") + } + b.WriteString(tail) + return b.String() +} + +// bug3839Shapes are the two failure shapes, as (before, after) bodies: the +// invalid byte adjacent to an edit, which panicked, and an invalid byte +// inserted into valid text, which built a wrong patch. +var bug3839Shapes = []struct { + name string + before, after string +}{ + {"panic", bug3839Long("a\xff"), bug3839Long("a\xffb")}, + {"wrong_patch", bug3839Long("hello world"), bug3839Long("hello \xff world")}, +} + +// newestVersion is the newest item version row's stored form. +func newestItemVersion(t *testing.T, s *store.Store, itemID string) (isDiff bool) { + t.Helper() + if err := s.DB().QueryRow(`SELECT is_diff FROM item_versions WHERE item_id = $1 ORDER BY created_at DESC, version_seq DESC LIMIT 1`, itemID).Scan(&isDiff); err != nil { + t.Fatal(err) + } + return isDiff +} + +// resolvesTo requires some item version to resolve to exactly want. +func itemVersionResolvesTo(t *testing.T, s *store.Store, itemID, current, want string) { + t.Helper() + vs, err := s.ListItemVersionsResolved(itemID, current) + if err != nil { + t.Fatal(err) + } + for _, v := range vs { + if v.Content == want && !v.Unreconstructable { + return + } + } + got := make([]string, 0, len(vs)) + for _, v := range vs { + got = append(got, v.Content[max(0, len(v.Content)-20):]) + } + t.Fatalf("no version resolves to the body it records; version tails: %q", got) +} + +// seedItemBody creates an item with a valid body, then sets the stored body +// to body directly, as a row an older release left behind. ok is false on +// Postgres, which refuses the write; that refusal is pinned. +func seedItemBody(t *testing.T, s *store.Store, pg bool, body string) (*models.Item, bool) { + t.Helper() + ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "BUG-3839"}) + if err != nil { + t.Fatal(err) + } + col, err := s.CreateCollection(ws.ID, models.CollectionCreate{Name: "Docs", Schema: `{"fields":[]}`}) + if err != nil { + t.Fatal(err) + } + item, err := s.CreateItem(ws.ID, col.ID, models.ItemCreate{Title: "Doc", Content: strings.ToValidUTF8(body, "?"), Fields: `{}`}) + if err != nil { + t.Fatal(err) + } + _, err = s.DB().Exec(`UPDATE items SET content = $1 WHERE id = $2`, body, item.ID) + if pg { + if utf8.ValidString(body) { + if err != nil { + t.Fatal(err) + } + return item, true + } + if err == nil { + t.Fatal("Postgres stored a body that is not valid UTF-8") + } + return item, false + } + if err != nil { + t.Fatal(err) + } + return item, true +} + +// UpdateItem (items.go, the version row for an edit) on both shapes. +func TestBUG3839_ItemEditOfInvalidUTF8StoresTheFullBody(t *testing.T) { + for _, backend := range contentBackends() { + for _, sh := range bug3839Shapes { + t.Run(backend.name+"/"+sh.name, func(t *testing.T) { + s := backend.open(t) + pg := backend.name == "Postgres" + item, seeded := seedItemBody(t, s, pg, sh.before) + if !seeded { + return // Postgres refused the stored body: the shape cannot exist there + } + after := sh.after + _, err := s.UpdateItem(item.ID, models.ItemUpdate{Content: &after, LastModifiedBy: "alice", Source: "cli"}) + if pg { + if err == nil { + t.Fatal("Postgres accepted a body that is not valid UTF-8") + } + got, gerr := s.GetItem(item.ID) + if gerr != nil || got.Content != sh.before { + t.Fatalf("the refused write changed the item: %v", gerr) + } + return + } + if err != nil { + t.Fatal(err) + } + if newestItemVersion(t, s, item.ID) { + t.Fatal("the version of a body that is not valid UTF-8 was stored as a patch") + } + itemVersionResolvesTo(t, s, item.ID, after, sh.before) + }) + } + } +} + +// rebaseNewestVersionPatch (items.go): a throttled edit rebases the newest +// version's patch onto the new body, which is where the invalid text lands. +func TestBUG3839_ThrottledEditOntoInvalidUTF8RebasesToTheFullBody(t *testing.T) { + for _, backend := range contentBackends() { + for _, sh := range bug3839Shapes { + t.Run(backend.name+"/"+sh.name, func(t *testing.T) { + s := backend.open(t) + pg := backend.name == "Postgres" + a := bug3839Long("first draft") + item, _ := seedItemBody(t, s, pg, a) + write := func(body string) (*models.Item, error) { + return s.UpdateItem(item.ID, models.ItemUpdate{Content: &body, LastModifiedBy: "alice", Source: "web"}) + } + // First edit: a version of a, stored as a patch. + if _, err := write(strings.ToValidUTF8(sh.before, "?")); err != nil { + t.Fatal(err) + } + if !newestItemVersion(t, s, item.ID) { + t.Fatal("instrument: the first edit's version must be a reverse patch") + } + // Second edit, throttled: rebases that version onto sh.after. + out, err := write(sh.after) + if pg { + if err == nil { + t.Fatal("Postgres accepted a body that is not valid UTF-8") + } + return + } + if err != nil { + t.Fatal(err) + } + if !out.BodyEditedWithoutVersion { + t.Fatal("instrument: the second edit must be throttled, so the rebase runs") + } + if newestItemVersion(t, s, item.ID) { + t.Fatal("the rebased version of a body that is not valid UTF-8 was stored as a patch") + } + itemVersionResolvesTo(t, s, item.ID, sh.after, a) + }) + } + } +} + +// UpdateDocument (documents.go), the legacy documents' version row. +func TestBUG3839_DocumentEditOfInvalidUTF8StoresTheFullBody(t *testing.T) { + for _, backend := range contentBackends() { + for _, sh := range bug3839Shapes { + t.Run(backend.name+"/"+sh.name, func(t *testing.T) { + s := backend.open(t) + pg := backend.name == "Postgres" + ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "BUG-3839"}) + if err != nil { + t.Fatal(err) + } + doc, err := s.CreateDocument(ws.ID, models.DocumentCreate{Title: "Doc", Content: strings.ToValidUTF8(sh.before, "?"), CreatedBy: "user", Source: "web"}) + if err != nil { + t.Fatal(err) + } + _, err = s.DB().Exec(`UPDATE documents SET content = $1 WHERE id = $2`, sh.before, doc.ID) + if pg { + if utf8.ValidString(sh.before) { + if err != nil { + t.Fatal(err) + } + } else { + if err == nil { + t.Fatal("Postgres stored a body that is not valid UTF-8") + } + return + } + } else if err != nil { + t.Fatal(err) + } + after := sh.after + _, err = s.UpdateDocument(doc.ID, models.DocumentUpdate{Content: &after, LastModifiedBy: "user", Source: "cli"}) + if pg { + if err == nil { + t.Fatal("Postgres accepted a body that is not valid UTF-8") + } + return + } + if err != nil { + t.Fatal(err) + } + vs, err := s.ListVersionsResolved(doc.ID, after) + if err != nil || len(vs) == 0 { + t.Fatalf("versions: %d %v", len(vs), err) + } + if vs[0].Content != sh.before { + t.Fatalf("the newest version resolves to a body it does not record (tail %q)", vs[0].Content[max(0, len(vs[0].Content)-20):]) + } + }) + } + } +} + +// planVersionChainRemapTx (attachments.go): an import's attachment remap +// re-diffs each version it can resolve against its remapped successor. Here +// the newest version's patch restores an attachment ref, and the item's +// current body carries an invalid byte next to it, as a direct edit or an +// older release would leave it. ApplyPatch refuses that base, so the remap cannot +// resolve the row and takes its unresolved branch: the patch text itself is +// remapped, and the row stays a diff that reads as patch_failed (lead ruling +// on BUG-3839 checkpoint 11). Before the +// refusal the patch applied fuzzily over the invalid byte, and the row was +// re-diffed from a body it never recorded. +func TestBUG3839_AttachmentRemapOverInvalidUTF8LeavesARowThatReadsPatchFailed(t *testing.T) { + oldID, newID := "00000000-0000-0000-0000-0000000000a1", "00000000-0000-0000-0000-0000000000b2" + ref := func(id string) string { return "![chart](pad-attachment:" + id + ")" } + for _, backend := range contentBackends() { + for _, sh := range []struct{ name, bad string }{{"lone_ff", "\xff"}, {"truncated", "\xe2\x82"}} { + t.Run(backend.name+"/"+sh.name, func(t *testing.T) { + s := backend.open(t) + pg := backend.name == "Postgres" + validCurrent := bug3839Long("See the notes.") + older := bug3839Long("See " + ref(oldID) + " and the notes.") + item, _ := seedItemBody(t, s, pg, validCurrent) + patch := bug3839Patch(t, older, validCurrent) + if !strings.Contains(patch, oldID) { + t.Fatal("instrument: the patch must carry the attachment id, or a remap of its text shows nothing") + } + seedDiffVersion(t, s, item.ID, patch) + current := strings.Replace(validCurrent, " notes.", sh.bad+" notes.", 1) + if _, seeded := seedExisting(t, s, pg, item.ID, current); !seeded { + return + } + if err := s.RemapAttachmentReferencesInWorkspace(item.WorkspaceID, map[string]string{oldID: newID}); err != nil { + t.Fatal(err) + } + var stored string + var isDiff bool + if err := s.DB().QueryRow(`SELECT content, is_diff FROM item_versions WHERE id = 'v1'`).Scan(&stored, &isDiff); err != nil { + t.Fatal(err) + } + if !isDiff || stored != strings.ReplaceAll(patch, oldID, newID) { + t.Fatalf("v1 was not left a diff with its patch text remapped: is_diff=%v", isDiff) + } + vs, err := s.ListItemVersionsResolved(item.ID, strings.ReplaceAll(current, oldID, newID)) + if err != nil || len(vs) != 1 { + t.Fatalf("versions: %d %v", len(vs), err) + } + if !vs[0].Unreconstructable || vs[0].UnreconstructableReason != "patch_failed" { + t.Fatalf("v1 reads unreconstructable=%v reason=%q", vs[0].Unreconstructable, vs[0].UnreconstructableReason) + } + }) + } + } +} + +// bug3839Patch is a reverse patch between valid texts, for fixtures. +func bug3839Patch(t *testing.T, older, newer string) string { + t.Helper() + p, ok := diff.ReversePatch(older, newer) + if !ok { + t.Fatal("fixture texts must be valid UTF-8") + } + return p +} + +// seedExisting sets an existing item's stored body directly; false on +// Postgres for a body that is not valid UTF-8 (refused, as seedItemBody). +func seedExisting(t *testing.T, s *store.Store, pg bool, itemID, body string) (string, bool) { + t.Helper() + _, err := s.DB().Exec(`UPDATE items SET content = $1 WHERE id = $2`, body, itemID) + if pg && !utf8.ValidString(body) { + if err == nil { + t.Fatal("Postgres stored a body that is not valid UTF-8") + } + return "", false + } + if err != nil { + t.Fatal(err) + } + return body, true +} + +// bug3839Unmade is a patch applied over a base it was not made from, valid +// UTF-8 throughout, on which go-diff's PatchApply panics (found by the +// BUG-3839 measurement; TestBUG3839_ApplyPatchOverAnotherValidBaseNeverPanics +// is the population). The version walk applies a patch over another base +// whenever it skips a row. +var bug3839Unmade = struct{ from, to, base string }{"a\né+abb😀", "%a+a😀ab", "é😀ab😀+%"} + +// bug3839Mangled is the same kind of pair on which go-diff instead succeeds +// and returns text that is not valid UTF-8 ("\x80"), from valid text (#2200 +// review r2). +var bug3839Mangled = struct{ from, to, base string }{"b%é😀+a\na%b 😀", "%é", "a \né\n😀"} + +// seedDiffVersion replaces the item's history with one unhashed diff row. +func seedDiffVersion(t *testing.T, s *store.Store, itemID, patch string) { + t.Helper() + if _, err := s.DB().Exec(`DELETE FROM item_versions WHERE item_id = $1`, itemID); err != nil { + t.Fatal(err) + } + if _, err := s.DB().Exec(`INSERT INTO item_versions (id, item_id, content, is_diff, created_at, version_seq) VALUES ('v1', $1, $2, $3, '2026-10-01T00:00:00Z', 1)`, + itemID, patch, true); err != nil { + t.Fatal(err) + } +} + +// History (ListItemVersionsResolved) over a diff row whose patch go-diff +// cannot apply: the current body is not valid UTF-8, or (both backends) the +// patch was made from another base. The row reads as patch_failed instead of +// panicking the request. +func TestBUG3839_ItemHistoryRefusesARowGoDiffCannotApply(t *testing.T) { + valid := bug3839Long("See the notes.") + for _, backend := range contentBackends() { + for _, sh := range []struct{ name, current, patch string }{ + {"invalid_current", bug3839Long("See the\xe2\x82 notes."), bug3839Patch(t, bug3839Long("See notes."), valid)}, + {"unmade_base", bug3839Unmade.base, bug3839Patch(t, bug3839Unmade.to, bug3839Unmade.from)}, + {"mangled_result", bug3839Mangled.base, bug3839Patch(t, bug3839Mangled.to, bug3839Mangled.from)}, + } { + t.Run(backend.name+"/"+sh.name, func(t *testing.T) { + s := backend.open(t) + pg := backend.name == "Postgres" + item, _ := seedItemBody(t, s, pg, valid) + seedDiffVersion(t, s, item.ID, sh.patch) + if _, seeded := seedExisting(t, s, pg, item.ID, sh.current); !seeded { + return + } + vs, err := s.ListItemVersionsResolved(item.ID, sh.current) + if err != nil || len(vs) != 1 { + t.Fatalf("versions: %d %v", len(vs), err) + } + if !vs[0].Unreconstructable || vs[0].UnreconstructableReason != "patch_failed" { + t.Fatalf("row reads unreconstructable=%v reason=%q content tail %q", vs[0].Unreconstructable, vs[0].UnreconstructableReason, vs[0].Content[max(0, len(vs[0].Content)-40):]) + } + }) + } + } +} + +// The legacy documents' walk (ListVersionsResolved) over the same rows. +func TestBUG3839_DocumentHistoryRefusesARowGoDiffCannotApply(t *testing.T) { + valid := bug3839Long("See the notes.") + for _, backend := range contentBackends() { + for _, sh := range []struct{ name, current, patch string }{ + {"invalid_current", bug3839Long("See the\xe2\x82 notes."), bug3839Patch(t, bug3839Long("See notes."), valid)}, + {"unmade_base", bug3839Unmade.base, bug3839Patch(t, bug3839Unmade.to, bug3839Unmade.from)}, + {"mangled_result", bug3839Mangled.base, bug3839Patch(t, bug3839Mangled.to, bug3839Mangled.from)}, + } { + t.Run(backend.name+"/"+sh.name, func(t *testing.T) { + s := backend.open(t) + if backend.name == "Postgres" && !utf8.ValidString(sh.current) { + return // Postgres cannot hold the body (seedItemBody pins the refusal) + } + ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "BUG-3839"}) + if err != nil { + t.Fatal(err) + } + doc, err := s.CreateDocument(ws.ID, models.DocumentCreate{Title: "Doc", Content: valid, CreatedBy: "user", Source: "web"}) + if err != nil { + t.Fatal(err) + } + if _, err := s.DB().Exec(`DELETE FROM versions WHERE document_id = $1`, doc.ID); err != nil { + t.Fatal(err) + } + if _, err := s.DB().Exec(`INSERT INTO versions (id, document_id, content, is_diff, created_at) VALUES ('dv1', $1, $2, $3, '2026-10-01T00:00:00Z')`, + doc.ID, sh.patch, true); err != nil { + t.Fatal(err) + } + vs, err := s.ListVersionsResolved(doc.ID, sh.current) + if err != nil || len(vs) != 1 { + t.Fatalf("versions: %d %v", len(vs), err) + } + if !strings.HasPrefix(vs[0].Content, "[patch error: ") { + t.Fatalf("row resolved to %q", vs[0].Content[max(0, len(vs[0].Content)-40):]) + } + }) + } + } +} + +// rebaseNewestVersionPatch (items.go) applies the newest version's patch to +// the stored body before a throttled edit replaces it. When go-diff cannot +// apply it, the edit lands and the row is left as it was. +func TestBUG3839_ThrottledEditOverARowGoDiffCannotApplyLeavesTheRow(t *testing.T) { + valid := bug3839Long("See the notes.") + for _, backend := range contentBackends() { + for _, sh := range []struct{ name, current, patch string }{ + {"invalid_current", bug3839Long("See the\xe2\x82 notes."), bug3839Patch(t, bug3839Long("See notes."), valid)}, + {"unmade_base", bug3839Unmade.base, bug3839Patch(t, bug3839Unmade.to, bug3839Unmade.from)}, + {"mangled_result", bug3839Mangled.base, bug3839Patch(t, bug3839Mangled.to, bug3839Mangled.from)}, + } { + t.Run(backend.name+"/"+sh.name, func(t *testing.T) { + s := backend.open(t) + pg := backend.name == "Postgres" + item, _ := seedItemBody(t, s, pg, valid) + write := func(body string) (*models.Item, error) { + return s.UpdateItem(item.ID, models.ItemUpdate{Content: &body, LastModifiedBy: "alice", Source: "web"}) + } + // A first edit makes alice's version row, so the next is throttled. + if _, err := write(bug3839Long("First draft.")); err != nil { + t.Fatal(err) + } + var vid string + if err := s.DB().QueryRow(`SELECT id FROM item_versions WHERE item_id = $1 ORDER BY created_at DESC, version_seq DESC LIMIT 1`, item.ID).Scan(&vid); err != nil { + t.Fatal(err) + } + if _, err := s.DB().Exec(`UPDATE item_versions SET content = $1, is_diff = $2, body_sha256 = NULL WHERE id = $3`, sh.patch, true, vid); err != nil { + t.Fatal(err) + } + if _, seeded := seedExisting(t, s, pg, item.ID, sh.current); !seeded { + return + } + out, err := write(bug3839Long("Second draft.")) + if err != nil { + t.Fatal(err) + } + if !out.BodyEditedWithoutVersion { + t.Fatal("instrument: the second edit must be throttled, so the rebase runs") + } + var content string + var isDiff bool + if err := s.DB().QueryRow(`SELECT content, is_diff FROM item_versions WHERE id = $1`, vid).Scan(&content, &isDiff); err != nil { + t.Fatal(err) + } + if content != sh.patch || !isDiff { + t.Fatalf("the row was rewritten: is_diff=%v", isDiff) + } + }) + } + } +} diff --git a/internal/store/documents.go b/internal/store/documents.go index ac4902182..ad35cefcc 100644 --- a/internal/store/documents.go +++ b/internal/store/documents.go @@ -404,8 +404,8 @@ func (s *Store) UpdateDocument(id string, input models.DocumentUpdate) (*models. // Falls back to full content if the diff isn't meaningfully smaller. versionContent := existing.Content isDiff := false - patch := diff.CreateReversePatch(existing.Content, *input.Content) - if diff.IsDiffSmaller(patch, existing.Content) { + // ok is false for text go-diff cannot patch (BUG-3839). + if patch, ok := diff.ReversePatch(existing.Content, *input.Content); ok && diff.IsDiffSmaller(patch, existing.Content) { versionContent = patch isDiff = true } diff --git a/internal/store/items.go b/internal/store/items.go index d6abb131d..ac2cc42fb 100644 --- a/internal/store/items.go +++ b/internal/store/items.go @@ -3075,13 +3075,15 @@ func (s *Store) updateItemWithParentLinkOnce( createdVersionID = vid versionContent := existing.Content isDiff := false - patch := diff.CreateReversePatch(existing.Content, *newBody) + // ok is false for a body that is not valid UTF-8, which go-diff + // cannot patch (BUG-3839): the full body is stored instead. + patch, patchOK := diff.ReversePatch(existing.Content, *newBody) // A reverse patch is resolved backwards from the ROW's body, which // on the applier path (ExternalContent, BUG-3327) is still the old // body until the tab's flush lands: a patch would resolve against // the wrong base in that window. The full body resolves the same // whatever the row holds. - if input.Content != nil && diff.IsDiffSmaller(patch, existing.Content) { + if input.Content != nil && patchOK && diff.IsDiffSmaller(patch, existing.Content) { versionContent = patch isDiff = true } @@ -6209,7 +6211,7 @@ func (s *Store) rebaseNewestVersionPatch(tx *sql.Tx, itemID, oldBody, newBody st return nil } stored, storedDiff := body, false - if patch := diff.CreateReversePatch(body, newBody); diff.IsDiffSmaller(patch, body) { + if patch, ok := diff.ReversePatch(body, newBody); ok && diff.IsDiffSmaller(patch, body) { stored, storedDiff = patch, true } added, removed := diff.LineCounts(body, newBody)