fix(links,server): codex R2 findings (BUG-2804)

R2-1 — ProjectRewrittenLen advanced its cursor past a no-op bracket while
RewriteBracketsAt did not, so the two disagreed about which overlapping
rewrites the guard skips. On `[[A[[B]]]]` with a no-op at 0 and a change at 3,
projection reported 10 bytes / 0 applied while the pass produced 13 / 1 — the
cascade charged the read and never charged the rewrite it then performed, so
the bound leaked on exactly the corrupt and duplicated offsets the defensive
paths exist for.

Projection now follows the pass, not the reverse: the pass's behaviour is
pinned to the descending fold the cascade used to perform, so moving it would
have changed cascade semantics under cover of a bug fix.

The lockstep property test that pins this needed MIXED per-position target
titles — the first version shared one title across a call and passed against
the broken code, because the reproducing shape needs a no-op bracket
overlapping a changing one, which a shared title cannot express. Cascade rows
carry per-row target_title, so mixed is the realistic case.

R2-2 — the 413 mapping covered one of THREE places handleUpdateItem reaches
UpdateItemWithParentLink. The post-R1 population sweep asked whether other
HANDLERS reach the store call and never asked whether this handler reaches it
more than once. All three now share writeItemRenameCascadeTooLarge.

The collab-edit half had a deeper cause than a missing arm. In
applyContentViaCollabOnce's prune-and-direct-write fallback, a failing
directWrite() had its error DISCARDED and the original collab error returned in
its place, so the caller read a deterministic refusal as a recoverable routing
problem and fell through to its own direct write — re-deriving the identical
refusal from scratch. Measured 64 rewritten bodies built for one request
against an expected 32.

Deterministic failures now survive that branch, scoped to the three errors
every call site already treats as final (open-children rejection, update
conflict, cascade refusal). Everything else still returns the collab error,
preserving the graceful-degradation contract the branch exists for.

Status alone cannot discriminate the double-work fix — both behaviours end in
413, since the fall-through reaches the plain path's arm — so the store's build
observer is now reachable from the server package via an exported test-support
setter, and the test asserts the work count.

Gates: gofmt, vet, go build ./... clean; go test ./... PASS on SQLite and
Postgres; go test -race PASS on internal/links and internal/store with zero
data races. Note -race on store runs 705s, past the 600s default timeout.

internal/links/links.go

@@ -244,7 +244,19 @@ func ProjectRewrittenLen(content string, rewrites []BracketRewrite, newTitle, co
 			continue
 		}
 		if bracketUnchanged(content, rw.Position, bracketEnd, newSegment, displaySuffix) {
-			cursor = bracketEnd
+			// Deliberately does NOT advance the cursor, mirroring the real pass
+			// exactly. An earlier version advanced it here, which made the two
+			// loops disagree about which overlapping rewrites the guard skips:
+			// on `[[A[[B]]]]` with a no-op at 0 and a change at 3, projection
+			// reported (10 bytes, 0 applied) while the pass produced 13 bytes
+			// and applied 1 — so the cascade charged a read it never charged
+			// the rewrite for, and the bound leaked on exactly the corrupt or
+			// duplicated offsets the defensive paths exist to handle
+			// (codex R2).
+			//
+			// The pass is the side that must not move: its behaviour is pinned
+			// to the descending fold the cascade used to perform. So the
+			// projection follows it, never the reverse.
 			continue
 		}
 		// Length arithmetic only — NOTHING is built here. Concatenating a

internal/links/rewrite_brackets_diff_test.go

@@ -383,3 +383,105 @@ func TestRewriteBracketsAt_MixedChangedAndUnchangedCountsOnlyTheChanged(t *testi
 		t.Errorf("applied = %d, want 2 — the middle bracket was already [[New]]", applied)
 	}
 }
+
+// TestProjectRewrittenLen_IsLockstepWithTheRealPass pins the invariant the cap
+// depends on: what the projection PREDICTS must be exactly what the rewrite
+// DOES, for every input including the defensive ones.
+//
+// This is the property codex R2 found broken. The projection advanced its
+// cursor past a no-op bracket while the real pass did not, so on corrupt,
+// duplicated or overlapping offsets the two disagreed about which rewrites the
+// overlap guard skips — projection reporting (unchanged, 0 applied) where the
+// pass actually applied one and grew the body. The cascade charges from the
+// projection, so the bound's guarantee leaked on precisely the inputs the
+// defensive paths exist for.
+//
+// The corpus deliberately includes NESTED-LOOKING brackets like `[[A[[B]]]]`,
+// where two valid `[[` offsets share a closing `]]` and therefore genuinely
+// overlap. Random offsets alone rarely produce that shape, which is why it is
+// constructed rather than left to chance.
+func TestProjectRewrittenLen_IsLockstepWithTheRealPass(t *testing.T) {
+	rng := rand.New(rand.NewSource(20260902))
+	titles := []string{"Old", "old", "A|B", "Other", "A[[B", "B"}
+	news := []string{"New", "N", "A[[B", strings.Repeat("N", 20), ""}
+
+	overlapping := []string{
+		"[[A[[B]]]]",
+		"[[Old[[Old]]]]",
+		"x [[A[[B]]]] y",
+		"[[A[[B]]]] [[Old]]",
+	}
+
+	check := func(t *testing.T, content string, rewrites []BracketRewrite, newTitle, collSlug string) {
+		t.Helper()
+		wantLen, wantApplied := ProjectRewrittenLen(content, rewrites, newTitle, collSlug)
+		got, gotApplied := RewriteBracketsAt(content, rewrites, newTitle, collSlug)
+		if len(got) != wantLen || gotApplied != wantApplied {
+			t.Fatalf("projection and the real pass disagree\n content=%q rewrites=%+v new=%q slug=%q\n"+
+				" projected len=%d applied=%d\n actual    len=%d applied=%d (result %q)",
+				content, rewrites, newTitle, collSlug, wantLen, wantApplied, len(got), gotApplied, got)
+		}
+	}
+
+	// MIXED target titles per position, not one title for the whole call.
+	// Cascade rows carry their own target_title (one row may have stored
+	// "Old" and another "tasks/Old"), and a single shared title is exactly
+	// what hid this divergence from the first version of this test: the
+	// reproducing case needs one bracket to be a NO-OP while an overlapping
+	// one CHANGES, which a single title cannot express.
+	for _, content := range overlapping {
+		offs := bracketOffsets(content)
+		for _, newTitle := range news {
+			for ti := range titles {
+				rewrites := make([]BracketRewrite, 0, len(offs))
+				for j, p := range offs {
+					rewrites = append(rewrites, BracketRewrite{
+						Position:    p,
+						TargetTitle: titles[(ti+j)%len(titles)],
+					})
+				}
+				check(t, content, rewrites, newTitle, "")
+			}
+		}
+	}
+
+	// The exact shape codex R2 named, asserted directly so the regression has
+	// a named home rather than depending on the random search rediscovering it:
+	// bracket [0,8) is a no-op, bracket [3,8) overlaps it and changes.
+	check(t, "[[A[[B]]]]", []BracketRewrite{
+		{Position: 0, TargetTitle: "A[[B"},
+		{Position: 3, TargetTitle: "B"},
+	}, "A[[B", "")
+
+	const iterations = 20000
+	for i := 0; i < iterations; i++ {
+		content := randomBracketContent(rng)
+		if rng.Intn(4) == 0 {
+			content += overlapping[rng.Intn(len(overlapping))]
+		}
+		target := titles[rng.Intn(len(titles))]
+		newTitle := news[rng.Intn(len(news))]
+
+		positions := bracketOffsets(content)
+		if rng.Intn(3) == 0 && len(content) > 0 {
+			positions = append(positions, rng.Intn(len(content)))
+		}
+		if rng.Intn(4) == 0 && len(positions) > 0 {
+			positions = append(positions, positions[rng.Intn(len(positions))])
+		}
+		if len(positions) == 0 {
+			continue
+		}
+		rng.Shuffle(len(positions), func(a, b int) { positions[a], positions[b] = positions[b], positions[a] })
+
+		rewrites := make([]BracketRewrite, 0, len(positions))
+		for j, p := range positions {
+			tt := target
+			if rng.Intn(2) == 0 {
+				tt = titles[(i+j)%len(titles)]
+			}
+			rewrites = append(rewrites, BracketRewrite{Position: p, TargetTitle: tt})
+		}
+		check(t, content, rewrites, newTitle, "")
+	}
+}

internal/server/handlers_collab.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/PerpetualSoftware/pad/internal/collab"
 	"github.com/PerpetualSoftware/pad/internal/models"
+	"github.com/PerpetualSoftware/pad/internal/store"
 	"github.com/go-chi/chi/v5"
 	"github.com/gorilla/websocket"
 )
@@ -456,6 +457,28 @@ func (s *Server) applyContentViaCollab(r *http.Request, itemID, markdown string,
 	return collab.ErrRoomActiveDuringPrune
 }
 
+// isDeterministicWriteFailure reports whether an error from a direct-write
+// callback is a settled answer rather than a transient condition.
+//
+// These three are the errors handleUpdateItem already treats as FINAL at every
+// call site: a rejection, a conflict, and a refusal. None can come out
+// differently on a retry, so a fallback path that swallows one and retries is
+// doing the work twice to reach the same answer — and, worse, may reach it by
+// a route that reports it differently.
+func isDeterministicWriteFailure(err error) bool {
+	if err == nil {
+		return false
+	}
+	if _, ok := asOpenChildrenGuardError(err); ok {
+		return true
+	}
+	if _, ok := asUpdateConflictError(err); ok {
+		return true
+	}
+	var tooLarge *store.ItemRenameCascadeTooLargeError
+	return errors.As(err, &tooLarge)
+}
+
 func (s *Server) applyContentViaCollabOnce(r *http.Request, itemID, markdown string, directWrite directWriteFn) error {
 	err := s.collab.ApplyExternalContent(itemID, markdown)
 	switch {
@@ -518,6 +541,22 @@ func (s *Server) applyContentViaCollabOnce(r *http.Request, itemID, markdown str
 			// review round 6.
 			return paErr
 		default:
+			// A DETERMINISTIC failure from directWrite is the caller's answer,
+			// not a collab routing problem, so it must survive this branch
+			// (BUG-2804 / codex R2). Returning `err` here discards it and hands
+			// the caller the original collab error instead, which reads as
+			// "couldn't route through an applier" — recoverable — so the caller
+			// falls through to its own direct write and re-derives the identical
+			// refusal from scratch. Measured: a refused rename ran the whole
+			// cascade TWICE, 64 rewritten bodies built for one request.
+			//
+			// Scoped to errors that cannot come out differently on a second
+			// attempt. Everything else keeps returning `err`, preserving the
+			// graceful-degradation contract this branch exists for: a prune
+			// failure or a transient write fault should still fall through.
+			if isDeterministicWriteFailure(paErr) {
+				return paErr
+			}
 			slog.Warn("collab: failed to prune op-log on direct-write fallback",
 				"item_id", itemID,
 				"error", paErr,

internal/server/handlers_items.go

@@ -892,6 +892,37 @@ func (s *Server) handleGetItem(w http.ResponseWriter, r *http.Request) {
 }
 
 // handleUpdateItem updates an existing item (fields, content, or both).
+
+// writeItemRenameCascadeTooLarge answers a BUG-2804 cascade refusal as 413 and
+// reports whether it handled the error.
+//
+// SHARED because handleUpdateItem reaches store.UpdateItemWithParentLink from
+// THREE places — the plain path, the collab-snapshot callback, and the
+// collab-edit callback — each with its own error block. The plain path was
+// mapped first and the other two were missed, which is a population error, not
+// a typo: the sweep that established "bulk and restore never set Title"
+// checked other HANDLERS and never asked whether this handler had more than
+// one error block (codex R2).
+//
+// The refusal is DETERMINISTIC and permanent, which is why the collab-edit
+// path must treat it as final rather than letting it fall through to the
+// direct write: falling through re-runs the whole cascade, re-reads every
+// linking body and re-charges the projection, only to refuse identically. The
+// caller waits twice for one answer.
+func writeItemRenameCascadeTooLarge(w http.ResponseWriter, err error) bool {
+	var tooLarge *store.ItemRenameCascadeTooLargeError
+	if !errors.As(err, &tooLarge) {
+		return false
+	}
+	// Composed from TYPED fields, never by splicing err.Error() — every wrapper
+	// the call path added would otherwise be published to the client.
+	writeError(w, http.StatusRequestEntityTooLarge, "rename_cascade_too_large",
+		fmt.Sprintf("This rename would rewrite more linked content than the server will process in one "+
+			"operation: at least %d bytes, and the limit is %d. Reduce the number of items linking "+
+			"this title, or split the rename, and try again.", tooLarge.Processed, tooLarge.Max))
+	return true
+}
+
 func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) {
 	workspaceID, ok := s.getWorkspaceID(w, r)
 	if !ok {
@@ -1542,6 +1573,12 @@ func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) {
 				writeUpdateConflictError(w, itemRefOrSlug(*item), conflict)
 				return
 			}
+			// BUG-2804 / codex R2: without this the cascade refusal reaches the
+			// client as a 500 from this path only, while the plain path answers
+			// 413 for the identical store error.
+			if writeItemRenameCascadeTooLarge(w, err) {
+				return
+			}
 			// Mirror the main UpdateItem path: map UNIQUE constraint /
 			// duplicate key races (e.g. concurrent edits both racing the
 			// invocation_slug partial unique index) to 409 conflict so
@@ -1619,6 +1656,12 @@ func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) {
 			// the same race again.
 			writeUpdateConflictError(w, itemRefOrSlug(*item), conflict)
 			return
+		} else if writeItemRenameCascadeTooLarge(w, err) {
+			// FINAL, like the guard and conflict arms above. The refusal is
+			// deterministic, so the fall-through to the direct write below would
+			// re-run the entire cascade — every linking body read and projected a
+			// second time — and refuse identically (codex R2).
+			return
 		} else if errors.Is(err, collab.ErrApplierAmbiguous) {
 			// BUG-2276 residual 2 (P1, mixed-deploy window): a legacy (non-bracket-
 			// capable) applier was caught by a concurrent version restore and its
@@ -1668,12 +1711,7 @@ func (s *Server) handleUpdateItem(w http.ResponseWriter, r *http.Request) {
 		// fields and never by splicing err.Error(), because every wrapper the
 		// call path added on the way up would otherwise be published to the
 		// client.
-		var cascadeTooLarge *store.ItemRenameCascadeTooLargeError
+		if writeItemRenameCascadeTooLarge(w, err) {
-		if errors.As(err, &cascadeTooLarge) {
-			writeError(w, http.StatusRequestEntityTooLarge, "rename_cascade_too_large",
-				fmt.Sprintf("This rename would rewrite more linked content than the server will process in one "+
-					"operation: at least %d bytes, and the limit is %d. Reduce the number of items linking "+
-					"this title, or split the rename, and try again.", cascadeTooLarge.Processed, cascadeTooLarge.Max))
 			return
 		}
 		// Map UNIQUE constraint races (e.g. concurrent updates that both

internal/server/handlers_items_rename_cascade_test.go

@@ -144,3 +144,125 @@ func TestItemRename_OrdinaryCascadeStillSucceeds(t *testing.T) {
 		t.Errorf("found %d rewritten linkers, want 3: the cascade did not run", n)
 	}
 }
+
+// TestItemRenameCascadeTooLarge_MappedOnTheCollabSnapshotPath closes codex R2's
+// second finding.
+//
+// handleUpdateItem reaches store.UpdateItemWithParentLink from THREE places,
+// each with its own error block: the plain path, the collab-snapshot callback,
+// and the collab-edit callback. R1 mapped the first. This pins the second, on
+// which the identical store refusal was still answering 500.
+//
+// Why the miss is worth naming: the CONVE-18 sweep I ran after R1 asked "do
+// other HANDLERS reach this?" and correctly answered no. It never asked
+// "does THIS handler reach it more than once?" — so the population was scoped
+// to the wrong axis, and a grep for the store call inside one function would
+// have found all three immediately.
+func TestItemRenameCascadeTooLarge_MappedOnTheCollabSnapshotPath(t *testing.T) {
+	srv := testServerWithCollab(t)
+	slug := createWSWithCollections(t, srv)
+
+	rr := doRequest(srv, "POST", "/api/v1/workspaces/"+slug+"/collections/tasks/items", map[string]interface{}{
+		"title": "Old",
+	})
+	if rr.Code != http.StatusCreated {
+		t.Fatalf("create target: %d: %s", rr.Code, rr.Body.String())
+	}
+	var target models.Item
+	parseJSON(t, rr, &target)
+
+	const body = 1 << 20
+	perLinker := int64(body) * 2
+	linkers := int(store.MaxItemRenameCascadeBytes/perLinker) + 2
+	linkerBody := "[[Old]]" + strings.Repeat("x", body-len("[[Old]]"))
+	for i := 0; i < linkers; i++ {
+		rr = doRequest(srv, "POST", "/api/v1/workspaces/"+slug+"/collections/tasks/items", map[string]interface{}{
+			"title":   fmt.Sprintf("Linker %d", i),
+			"content": linkerBody,
+		})
+		if rr.Code != http.StatusCreated {
+			t.Fatalf("create linker %d: %d: %s", i, rr.Code, rr.Body.String())
+		}
+	}
+
+	// ?source=collab-snapshot with content routes through the collab-snapshot
+	// callback rather than the plain path.
+	rr = doRequest(srv, "PATCH",
+		"/api/v1/workspaces/"+slug+"/items/"+target.Slug+"?source=collab-snapshot",
+		map[string]interface{}{
+			"title":         "New",
+			"content":       "rewritten by the editor",
+			"op_log_cursor": 0,
+		})
+
+	if rr.Code != http.StatusRequestEntityTooLarge {
+		t.Fatalf("collab-snapshot path: got %d, want 413 — the same store refusal the plain path "+
+			"answers 413 for: %s", rr.Code, rr.Body.String())
+	}
+	if !strings.Contains(rr.Body.String(), "rename_cascade_too_large") {
+		t.Errorf("response lacks the error code: %s", rr.Body.String())
+	}
+}
+
+// TestItemRenameCascadeTooLarge_CollabEditPathDoesNotRunTheCascadeTwice closes
+// the other half of codex R2's second finding.
+//
+// The collab-edit path treats most callback errors as recoverable and falls
+// through to a direct write — graceful degradation for applier timeouts. A
+// cascade refusal is NOT recoverable: it is deterministic, so the fall-through
+// re-reads every linking body, re-charges the projection, and refuses
+// identically. The caller waits twice for one answer.
+//
+// STATUS CANNOT PIN THIS. Both behaviours end in 413 — the fall-through reaches
+// the plain path's arm — so a status assertion passes either way. The
+// observable that discriminates is how much WORK the cascade did, counted via
+// the store's build observer.
+func TestItemRenameCascadeTooLarge_CollabEditPathDoesNotRunTheCascadeTwice(t *testing.T) {
+	srv := testServerWithCollab(t)
+	slug := createWSWithCollections(t, srv)
+
+	rr := doRequest(srv, "POST", "/api/v1/workspaces/"+slug+"/collections/tasks/items", map[string]interface{}{
+		"title": "Old",
+	})
+	if rr.Code != http.StatusCreated {
+		t.Fatalf("create target: %d: %s", rr.Code, rr.Body.String())
+	}
+	var target models.Item
+	parseJSON(t, rr, &target)
+
+	const body = 1 << 20
+	perLinker := int64(body) * 2
+	admits := int(store.MaxItemRenameCascadeBytes / perLinker)
+	linkers := admits + 2
+	linkerBody := "[[Old]]" + strings.Repeat("x", body-len("[[Old]]"))
+	for i := 0; i < linkers; i++ {
+		rr = doRequest(srv, "POST", "/api/v1/workspaces/"+slug+"/collections/tasks/items", map[string]interface{}{
+			"title":   fmt.Sprintf("Linker %d", i),
+			"content": linkerBody,
+		})
+		if rr.Code != http.StatusCreated {
+			t.Fatalf("create linker %d: %d: %s", i, rr.Code, rr.Body.String())
+		}
+	}
+
+	var built int
+	srv.store.SetCascadeBuildObserver(func(int) { built++ })
+	defer srv.store.SetCascadeBuildObserver(nil)
+
+	// Title + content with collab enabled and no collab-snapshot marker routes
+	// through the collab-edit callback.
+	rr = doRequest(srv, "PATCH", "/api/v1/workspaces/"+slug+"/items/"+target.Slug, map[string]interface{}{
+		"title":   "New",
+		"content": "rewritten by the editor",
+	})
+
+	if rr.Code != http.StatusRequestEntityTooLarge {
+		t.Fatalf("collab-edit path: got %d, want 413: %s", rr.Code, rr.Body.String())
+	}
+	if built != admits {
+		t.Errorf("cascade built %d bodies for ONE request, want %d — a second full cascade ran, "+
+			"which means the deterministic refusal fell through to the direct write and was "+
+			"re-derived from scratch", built, admits)
+	}
+	t.Logf("built %d bodies for one refused request (cap admits %d sources)", built, admits)
+}

internal/store/wiki_links.go

@@ -1575,6 +1575,23 @@ func snippetAround(content string, position int) string {
 	return snippet
 }
 
+// SetCascadeBuildObserver installs a TEST-SUPPORT observer called once per
+// rewritten body the item rename cascade BUILDS, with that body's length. Pass
+// nil to clear.
+//
+// Exported for the same reason SetCollabRoomManager is: the property it makes
+// observable — how much work a single request causes the cascade to do — is
+// only reachable from the SERVER package, whose tests cannot touch this
+// package's unexported fields. It is the instrument behind the collab-edit
+// double-work test (codex R2), which asserts that a refused rename runs the
+// cascade ONCE rather than falling through and running it again.
+//
+// Not safe for concurrent use with in-flight renames; tests set it before
+// issuing requests and clear it after.
+func (s *Store) SetCascadeBuildObserver(fn func(bytes int)) {
+	s.onItemCascadeBodyBuilt = fn
+}
+
 // buildCascadeBody builds one rewritten body and reports the build through the
 // test seam, as ONE operation.
 //