fix(items): undo of an emptied line, and a patch that changes nothing is refused (PLAN-3578)

Retro CONVE-735 findings on #2058 and #2055.

Undo: an edit that deleted a line's only content recorded empty,
line-clipped context on both sides, so its inverse had no anchor and the
undo was refused as an empty anchor. ApplyWithContext now reaches across
the line breaks when the clipped context around an edit that wrote nothing
is empty. A patch that empties the WHOLE body still leaves nothing to
anchor on; its undo now says so and points at History instead of "old is
empty".

No-op: an edit whose old equals its new, or edits that cancel out, used to
answer success and advance seq with no version behind it, against "every
patch is a version". Both are refused 400, writing nothing: per edit up
front in the handler, and the combined case through bodypatch.ErrNoChange
on the no-room path.

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

internal/bodypatch/bodypatch.go

@@ -7,6 +7,7 @@
 package bodypatch
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 	"unicode/utf8"
@@ -56,6 +57,7 @@ func (e *AnchorAmbiguousError) Error() string {
 // ones before it, and returns the new body. All or nothing: the first edit
 // that cannot apply returns its error and no body.
 func Apply(body string, edits []Edit) (string, error) {
+	original := body
 	if len(edits) == 0 {
 		return "", fmt.Errorf("no edits")
 	}
@@ -96,9 +98,17 @@ func Apply(body string, edits []Edit) (string, error) {
 		}
 		body = body[:at[0]] + e.New + body[at[0]+len(e.Old):]
 	}
+	if body == original {
+		return "", ErrNoChange
+	}
 	return body, nil
 }
 
+// ErrNoChange: the edits, applied in order, leave the body exactly as it was
+// (retro CONVE-735 finding on #2055). A patch is a version; one that changes
+// nothing is refused rather than advancing seq with no version behind it.
+var ErrNoChange = errors.New("the edits change nothing")
+
 // matches returns every start offset of sub in s, overlapping ones included:
 // "aa" occurs twice in "aaa", and a uniqueness check that counted it once
 // would let an edit land on whichever one a scan happened to find first.
@@ -156,19 +166,26 @@ func ApplyWithContext(body string, edits []Edit) (string, []AppliedEdit, error)
 		p := at[0]
 		body = body[:p] + e.New + body[p+len(e.Old):]
 		end := p + len(e.New)
-		out = append(out, AppliedEdit{Old: e.Old, New: e.New, CtxBefore: ctxBefore(body, p), CtxAfter: ctxAfter(body, end)})
+		cb, ca := ctxBefore(body, p, true), ctxAfter(body, end, true)
+		if cb+e.New+ca == "" {
+			// The edit emptied its line (retro CONVE-735 finding on #2058):
+			// line-clipped context is empty on both sides, which would leave
+			// the undo no anchor at all. Reach across the line breaks instead.
+			cb, ca = ctxBefore(body, p, false), ctxAfter(body, end, false)
+		}
+		out = append(out, AppliedEdit{Old: e.Old, New: e.New, CtxBefore: cb, CtxAfter: ca})
 	}
 	return body, out, nil
 }
 
-// ctxBefore is up to ContextChars bytes before p, not crossing a newline, and
-// never splitting a UTF-8 sequence.
-func ctxBefore(s string, p int) string {
+// ctxBefore is up to ContextChars bytes before p, not crossing a newline when
+// clip is set, and never splitting a UTF-8 sequence.
+func ctxBefore(s string, p int, clip bool) string {
 	from := p - ContextChars
 	if from < 0 {
 		from = 0
 	}
-	if nl := strings.LastIndexByte(s[from:p], '\n'); nl >= 0 {
+	if nl := strings.LastIndexByte(s[from:p], '\n'); clip && nl >= 0 {
 		from += nl + 1
 	}
 	for from < p && !utf8.RuneStart(s[from]) {
@@ -177,13 +194,14 @@ func ctxBefore(s string, p int) string {
 	return s[from:p]
 }
 
-// ctxAfter is up to ContextChars bytes from p, stopping before a newline.
-func ctxAfter(s string, p int) string {
+// ctxAfter is up to ContextChars bytes from p, stopping before a newline when
+// clip is set.
+func ctxAfter(s string, p int, clip bool) string {
 	to := p + ContextChars
 	if to > len(s) {
 		to = len(s)
 	}
-	if nl := strings.IndexByte(s[p:to], '\n'); nl >= 0 {
+	if nl := strings.IndexByte(s[p:to], '\n'); clip && nl >= 0 {
 		to = p + nl
 	}
 	for to > p && to < len(s) && !utf8.RuneStart(s[to]) {

internal/bodypatch/undo_test.go

@@ -77,3 +77,13 @@ func TestContextIsBoundedAndLineClipped(t *testing.T) {
 		t.Fatalf("context = %q / %q", a.CtxBefore, a.CtxAfter)
 	}
 }
+
+// Retro CONVE-735 finding on #2058: an edit that deletes a line's only
+// content left empty line-clipped context on both sides, so the inverse's
+// anchor was "" and the undo was refused as an empty anchor.
+func TestUndoOfALineEmptiedOut(t *testing.T) {
+	t.Run("middle line", func(t *testing.T) { roundTrip(t, "alpha\nbeta\ngamma\n", []Edit{{Old: "beta", New: ""}}) })
+	t.Run("first line", func(t *testing.T) { roundTrip(t, "alpha\nbeta\n", []Edit{{Old: "alpha", New: ""}}) })
+	t.Run("last line, no trailing newline", func(t *testing.T) { roundTrip(t, "alpha\nbeta", []Edit{{Old: "beta", New: ""}}) })
+	t.Run("between blank lines", func(t *testing.T) { roundTrip(t, "a\n\nb\n\nc", []Edit{{Old: "b", New: ""}}) })
+}

internal/server/handlers_items_patch.go

@@ -166,6 +166,17 @@ func (s *Server) handlePatchItem(w http.ResponseWriter, r *http.Request) {
 			return
 		}
 		req.Edits = bodypatch.Invert(applied)
+		for _, e := range req.Edits {
+			if e.Old == "" {
+				// The patch emptied the whole body: no text is left for an
+				// anchored undo to name (retro CONVE-735 finding on #2058).
+				// Every patch writes a version, so History can restore it.
+				writeError2(w, http.StatusBadRequest, "bad_request",
+					"that patch emptied the whole body, so there is no text left to anchor an undo on; nothing was written. Restore the version before it from the item's history instead",
+					map[string]interface{}{"undo": req.Undo})
+				return
+			}
+		}
 		p.undoOf, p.undoItemID = rec.ID, rec.ItemID
 	}
 	if len(req.Edits) == 0 {
@@ -183,6 +194,15 @@ func (s *Server) handlePatchItem(w http.ResponseWriter, r *http.Request) {
 				map[string]interface{}{"edit": i})
 			return
 		}
+		// A no-op edit would advance seq and write no version, against the
+		// contract that every patch is a version (retro CONVE-735 finding on
+		// #2055). It is refused, writing nothing.
+		if e.Old == e.New {
+			writeError2(w, http.StatusBadRequest, "bad_request",
+				fmt.Sprintf("edit %d changes nothing (old and new are the same); nothing was written", i),
+				map[string]interface{}{"edit": i})
+			return
+		}
 	}
 	p.edits = req.Edits
 	key, kerr := idempotencyKey(r, req.OpKey)
@@ -266,6 +286,10 @@ func (s *Server) writePatchRefusal(w http.ResponseWriter, ref string, err error)
 		writeError2(w, http.StatusBadRequest, "bad_request", empty.Error(), map[string]interface{}{"edit": empty.Index})
 		return true
 	}
+	if errors.Is(err, bodypatch.ErrNoChange) {
+		writeError2(w, http.StatusBadRequest, "bad_request", ref+": the edits change nothing (together they leave the body as it was); nothing was written", map[string]interface{}{"ref": ref})
+		return true
+	}
 	return false
 }
 

internal/server/plan3578_retro_fixes_test.go

@@ -0,0 +1,56 @@
+package server
+
+import (
+	"net/http"
+	"strings"
+	"testing"
+)
+
+// Retro CONVE-735 findings on #2055 and #2058.
+
+// A no-op edit, or edits that cancel out, used to report success and advance
+// seq with no version behind it. They are refused and write nothing.
+func TestPLAN3578_NoOpPatchIsRefused(t *testing.T) {
+	t.Parallel()
+	f := newPatchFixture(t, testServer(t), "alpha\nbeta\n")
+	before := f.current(t)
+	for name, e := range map[string][]map[string]any{
+		"one edit, old == new": edits("beta", "beta"),
+		"two edits cancel out": edits("beta", "BETA", "BETA", "beta"),
+	} {
+		code, out := f.patch(t, map[string]any{"edits": e})
+		if code != http.StatusBadRequest {
+			t.Fatalf("%s: %d %v", name, code, out)
+		}
+	}
+	if after := f.current(t); after.Seq != before.Seq || after.Content != before.Content {
+		t.Fatalf("a refused no-op wrote: seq %d → %d", before.Seq, after.Seq)
+	}
+}
+
+// Deleting a line's only content left the undo with no anchor; it is
+// undoable now. A patch that empties the WHOLE body still cannot be undone
+// by anchor, and says so (History restores it).
+func TestPLAN3578_UndoOfAnEmptiedLine(t *testing.T) {
+	t.Parallel()
+	f := newPatchFixture(t, testServer(t), "alpha\nbeta\ngamma\n")
+	code, out := f.patch(t, map[string]any{"edits": edits("beta", "")})
+	if code != http.StatusOK {
+		t.Fatalf("patch: %d %v", code, out)
+	}
+	code, out = f.patch(t, map[string]any{"undo": out["patch_id"]})
+	if code != http.StatusOK || f.current(t).Content != "alpha\nbeta\ngamma\n" {
+		t.Fatalf("undo: %d %v, body %q", code, out, f.current(t).Content)
+	}
+
+	g := newPatchFixture(t, testServer(t), "only")
+	code, out = g.patch(t, map[string]any{"edits": edits("only", "")})
+	if code != http.StatusOK {
+		t.Fatalf("emptying patch: %d %v", code, out)
+	}
+	code, out = g.patch(t, map[string]any{"undo": out["patch_id"]})
+	msg, _ := out["error"].(map[string]any)["message"].(string)
+	if code != http.StatusBadRequest || !strings.Contains(msg, "history") {
+		t.Fatalf("undo of a whole-body clear: %d %v", code, out)
+	}
+}