fix(mcp): remote bulk-update patches only status/priority, so it no longer reverts a concurrent field write (BUG-3156) (#1462)

* fix(mcp): remote bulk-update patches only status/priority, so it no longer reverts a concurrent field write (BUG-3156)

dispatchItemBulkUpdate (remote /mcp) still did BUG-3049's read-modify-write:
GET the item, set status/priority on the decoded blob, PATCH the whole blob
back, so any field another client wrote between the two was reverted. The
CLI (stdio) has sent fields_patch since BUG-3049; remote was missed.

It now sends one PATCH per row with fields_patch holding only the keys the
call sets, and no prefetch. Tool surface unchanged, no version bump.

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

* test(mcp): the per-item failure test describes the PATCH-only row, not a GET (BUG-3156, codex round 1)

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

internal/mcp/dispatch_http_bulk_update_revert_test.go

@@ -0,0 +1,102 @@
+package mcp
+
+import (
+	"bytes"
+	"context"
+	"encoding/json"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"sync"
+	"testing"
+
+	"github.com/PerpetualSoftware/pad/internal/models"
+)
+
+// BUG-3156 (a): remote bulk-update is BUG-3049's revert on the transport that
+// fix missed. It read the item, set status on the DECODED blob, and PATCHed
+// the whole blob back, so a field another client wrote between the read and
+// the write was reverted. The CLI has sent `fields_patch` since BUG-3049.
+//
+// The interleave is made deterministic by wrapping the real server: when the
+// bulk-update's PATCH arrives, another client's write to a DIFFERENT field is
+// applied first, exactly where production can land one.
+func TestDispatch_ItemBulkUpdate_DoesNotRevertAConcurrentFieldWrite(t *testing.T) {
+	srv, st := newPadServer(t)
+	wsRec := doJSONReq(t, srv, http.MethodPost, "/api/v1/workspaces", map[string]any{"name": "Revert"})
+	if wsRec.Code != http.StatusCreated {
+		t.Fatalf("create workspace: %d %s", wsRec.Code, wsRec.Body.String())
+	}
+	var ws models.Workspace
+	if err := json.Unmarshal(wsRec.Body.Bytes(), &ws); err != nil {
+		t.Fatal(err)
+	}
+	owner, err := st.CreateUser(models.UserCreate{Email: "owner@example.com", Name: "Owner", Password: "x"})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := st.AddWorkspaceMember(ws.ID, owner.ID, "owner"); err != nil {
+		t.Fatal(err)
+	}
+
+	var once sync.Once
+	var interleaved bool
+	wrapper := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Method == http.MethodPatch && strings.Contains(r.URL.Path, "/items/") {
+			once.Do(func() {
+				// Another client lowers the priority between bulk-update's
+				// read and its write. Same user and auth context, different
+				// request, different field.
+				body, _ := json.Marshal(map[string]any{"fields_patch": map[string]any{"priority": "low"}})
+				other := r.Clone(r.Context())
+				other.Body = io.NopCloser(bytes.NewReader(body))
+				other.ContentLength = int64(len(body))
+				rec := httptest.NewRecorder()
+				srv.ServeHTTP(rec, other)
+				if rec.Code != http.StatusOK {
+					t.Errorf("the interleaved write failed: %d %s", rec.Code, rec.Body.String())
+				}
+				interleaved = true
+			})
+		}
+		srv.ServeHTTP(w, r)
+	})
+	d := &HTTPHandlerDispatcher{Handler: wrapper, UserResolver: fixedUserResolver(owner)}
+
+	res, err := d.Dispatch(WithDispatchInput(context.Background(), map[string]any{
+		"workspace": ws.Slug, "collection": "tasks", "title": "Race me", "priority": "high",
+	}), []string{"item", "create"}, nil)
+	if err != nil || res.IsError {
+		t.Fatalf("create: %v %#v", err, res)
+	}
+	ref, _ := res.StructuredContent.(map[string]any)["ref"].(string)
+	// The create's own PATCH-free path must not have consumed the interleave.
+	if interleaved {
+		t.Fatal("precondition: the interleave fired before the bulk-update")
+	}
+
+	bulk, err := d.Dispatch(WithDispatchInput(context.Background(), map[string]any{
+		"workspace": ws.Slug, "ref": []any{ref}, "status": "in-progress",
+	}), []string{"item", "bulk-update"}, nil)
+	if err != nil || bulk.IsError {
+		t.Fatalf("bulk-update: %v %#v", err, bulk)
+	}
+	if !interleaved {
+		t.Fatal("precondition: the concurrent write never landed between the read and the write")
+	}
+
+	show, err := d.Dispatch(WithDispatchInput(context.Background(), map[string]any{
+		"workspace": ws.Slug, "ref": ref,
+	}), []string{"item", "show"}, nil)
+	if err != nil || show.IsError {
+		t.Fatalf("show: %v %#v", err, show)
+	}
+	fields := itemFieldsAsMap(t, show.StructuredContent.(map[string]any))
+	if fields["status"] != "in-progress" {
+		t.Errorf("status = %v, want in-progress (the bulk-update itself must apply)", fields["status"])
+	}
+	if fields["priority"] != "low" {
+		t.Errorf("priority = %v, want low: the bulk-update reverted another client's write", fields["priority"])
+	}
+}

internal/mcp/dispatch_http_project.go

@@ -275,10 +275,10 @@ func filterAgentAttention(attention []map[string]any) []map[string]any {
 // --- item bulk-update ---
 
 // dispatchItemBulkUpdate iterates the input's `ref` array and applies
-// --status / --priority via the same read-modify-write semantics the
-// item.update path uses (so existing fields survive). Mirrors the
-// CLI's bulkUpdateCmd: at-least-one-of-status-or-priority gating, per-
-// item GET → field merge → PATCH, and a per-item success/error report.
+// --status / --priority as a per-item `fields_patch` (so other fields are
+// never written, BUG-3156). Mirrors the CLI's bulkUpdateCmd:
+// at-least-one-of-status-or-priority gating, one PATCH per item, and a
+// per-item success/error report.
 //
 // The cmdhelp surface marks `ref` as required AND repeatable — agents
 // pass it as either []any (typical JSON array) or []string. Anything
@@ -348,54 +348,27 @@ func (d *HTTPHandlerDispatcher) dispatchItemBulkUpdate(
 	}
 
 	for _, ref := range refs {
-		// Per-item RMW: GET, merge fields, PATCH. Same shape
-		// dispatchItemUpdate uses, but inlined here so a per-item
-		// failure produces a {ref, error} entry instead of aborting.
+		// BUG-3156: ONE PATCH per row carrying `fields_patch` with only the
+		// keys this call sets. This used to be a read-modify-write: GET the
+		// item, set status/priority on the DECODED blob, PATCH the whole
+		// blob back, which reverted any field another client wrote between
+		// the two (BUG-3049's defect, on the transport that fix missed; the
+		// CLI has sent `fields_patch` since). The server merges the patch
+		// under its write lock. There is no prefetch: the PATCH does its own
+		// visibility and not-found checks, and a GET whose only job was the
+		// merge is the defect's shape (the same call BUG-3056 made for
+		// note/decide).
 		itemPath := "/api/v1/workspaces/" + url.PathEscape(workspace) +
 			"/items/" + url.PathEscape(ref)
 
-		getReq, err := d.buildAuthedRequest(ctx, http.MethodGet, itemPath, nil, user)
-		if err != nil {
-			results = append(results, bulkResult{Ref: ref, Error: rowDispatcherError("build request", err)})
-			continue
-		}
-		getRec := httptest.NewRecorder()
-		d.Handler.ServeHTTP(getRec, getReq)
-		if getRec.Code >= 400 {
-			results = append(results, bulkResult{
-				Ref:   ref,
-				Error: rowUpstreamError("read item", itemPath, getRec.Code, getRec.Body.Bytes(), ref),
-			})
-			continue
-		}
-		var existing struct {
-			Fields string `json:"fields"`
-		}
-		if err := json.Unmarshal(getRec.Body.Bytes(), &existing); err != nil {
-			results = append(results, bulkResult{Ref: ref, Error: rowDispatcherError("parse item", err)})
-			continue
-		}
-
-		merged := map[string]any{}
-		if existing.Fields != "" && existing.Fields != "{}" {
-			if err := json.Unmarshal([]byte(existing.Fields), &merged); err != nil {
-				results = append(results, bulkResult{Ref: ref, Error: rowDispatcherError("parse existing fields", err)})
-				continue
-			}
-		}
+		fieldsPatch := map[string]any{}
 		if status != "" {
-			merged["status"] = status
+			fieldsPatch["status"] = status
 		}
 		if priority != "" {
-			merged["priority"] = priority
+			fieldsPatch["priority"] = priority
 		}
-		fieldsJSON, err := json.Marshal(merged)
-		if err != nil {
-			results = append(results, bulkResult{Ref: ref, Error: rowDispatcherError("encode fields", err)})
-			continue
-		}
-		fieldsStr := string(fieldsJSON)
-		patchPayload := map[string]any{"fields": fieldsStr}
+		patchPayload := map[string]any{"fields_patch": fieldsPatch}
 		// IDEA-1494: forward the open-children guard override per-row.
 		// Same flag shape as `pad item bulk-update --force` so the
 		// override travels through both transports identically.

internal/mcp/dispatch_http_project_test.go

@@ -3,6 +3,7 @@ package mcp
 import (
 	"context"
 	"encoding/json"
+	"io"
 	"net/http"
 	"net/url"
 	"strings"
@@ -1131,22 +1132,21 @@ func TestDispatch_ItemBulkUpdate_RequiresStatusOrPriority(t *testing.T) {
 }
 
 func TestDispatch_ItemBulkUpdate_PerItemFailureDoesNotAbort(t *testing.T) {
-	// First ref fails GET (404); second succeeds. The dispatcher
+	// First ref's PATCH answers 404; second succeeds. The dispatcher
 	// must report both — successes get Updated:true, failures get
 	// Error populated. Mirrors the CLI's per-item green/red output.
+	// (Since BUG-3156 each row is a single PATCH with no prefetch, so
+	// the 404 is the update's own not-found answer.)
 	mux := http.NewServeMux()
 	mux.HandleFunc("/api/v1/workspaces/docapp/items/TASK-9", func(w http.ResponseWriter, _ *http.Request) {
 		w.WriteHeader(http.StatusNotFound)
 	})
 	mux.HandleFunc("/api/v1/workspaces/docapp/items/TASK-1", func(w http.ResponseWriter, r *http.Request) {
-		switch r.Method {
-		case http.MethodGet:
-			w.Header().Set("Content-Type", "application/json")
-			_, _ = w.Write([]byte(`{"ref":"TASK-1","fields":"{\"status\":\"open\"}"}`))
-		case http.MethodPatch:
-			w.Header().Set("Content-Type", "application/json")
-			_, _ = w.Write([]byte(`{"ref":"TASK-1"}`))
+		if r.Method != http.MethodPatch {
+			t.Errorf("unexpected %s — bulk-update sends one PATCH per row", r.Method)
 		}
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`{"ref":"TASK-1"}`))
 	})
 	d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})}
 	res, err := d.Dispatch(
@@ -1181,26 +1181,23 @@ func TestDispatch_ItemBulkUpdate_PerItemFailureDoesNotAbort(t *testing.T) {
 	}
 }
 
-func TestDispatch_ItemBulkUpdate_MergesExistingFields(t *testing.T) {
-	// Bulk-update applies the same RMW merge item.update uses: the
-	// existing priority "high" must survive when only --status is
-	// being changed.
+func TestDispatch_ItemBulkUpdate_PatchesOnlyTheKeysItSets(t *testing.T) {
+	// BUG-3156: bulk-update used to GET the item, set status on the decoded
+	// blob and PATCH the WHOLE blob back (this test then asserted that the
+	// untouched keys were carried, which pinned the revert). It now sends
+	// one PATCH per row with `fields_patch` holding only what the call set,
+	// and no `fields` member, so it cannot write any other key.
 	mux := http.NewServeMux()
-	patchedFields := ""
+	var got map[string]any
+	methods := []string{}
 	mux.HandleFunc("/api/v1/workspaces/docapp/items/TASK-1", func(w http.ResponseWriter, r *http.Request) {
-		switch r.Method {
-		case http.MethodGet:
-			w.Header().Set("Content-Type", "application/json")
-			_, _ = w.Write([]byte(`{"ref":"TASK-1","fields":"{\"status\":\"open\",\"priority\":\"high\",\"category\":\"bug\"}"}`))
-		case http.MethodPatch:
-			body := make([]byte, r.ContentLength)
-			_, _ = r.Body.Read(body)
-			var got map[string]any
+		methods = append(methods, r.Method)
+		if r.Method == http.MethodPatch {
+			body, _ := io.ReadAll(r.Body)
 			_ = json.Unmarshal(body, &got)
-			patchedFields, _ = got["fields"].(string)
-			w.Header().Set("Content-Type", "application/json")
-			_, _ = w.Write([]byte(`{"ref":"TASK-1"}`))
 		}
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`{"ref":"TASK-1"}`))
 	})
 	d := &HTTPHandlerDispatcher{Handler: mux, UserResolver: fixedUserResolver(&models.User{ID: "u"})}
 	_, _ = d.Dispatch(
@@ -1211,18 +1208,15 @@ func TestDispatch_ItemBulkUpdate_MergesExistingFields(t *testing.T) {
 		}),
 		[]string{"item", "bulk-update"}, nil,
 	)
-	var fields map[string]any
-	if err := json.Unmarshal([]byte(patchedFields), &fields); err != nil {
-		t.Fatalf("decode patched fields: %v", err)
+	if len(methods) != 1 || methods[0] != http.MethodPatch {
+		t.Fatalf("requests = %v, want exactly one PATCH (no prefetch)", methods)
 	}
-	if fields["status"] != "in-progress" {
-		t.Errorf("status not updated: %v", fields)
+	if _, has := got["fields"]; has {
+		t.Fatalf("the PATCH carries a whole `fields` blob, which reverts concurrent writes: %v", got)
 	}
-	if fields["priority"] != "high" {
-		t.Errorf("priority should survive RMW: %v", fields)
-	}
-	if fields["category"] != "bug" {
-		t.Errorf("category should survive RMW: %v", fields)
+	patch, _ := got["fields_patch"].(map[string]any)
+	if len(patch) != 1 || patch["status"] != "in-progress" {
+		t.Fatalf("fields_patch = %v, want exactly {status: in-progress}", got["fields_patch"])
 	}
 }