diff --git a/internal/mcp/dispatch_http_bulk_update_revert_test.go b/internal/mcp/dispatch_http_bulk_update_revert_test.go new file mode 100644 index 000000000..aefc71760 --- /dev/null +++ b/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"]) + } +} diff --git a/internal/mcp/dispatch_http_project.go b/internal/mcp/dispatch_http_project.go index 81a62a744..145bff275 100644 --- a/internal/mcp/dispatch_http_project.go +++ b/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. diff --git a/internal/mcp/dispatch_http_project_test.go b/internal/mcp/dispatch_http_project_test.go index ff9f32ee8..50fca84d4 100644 --- a/internal/mcp/dispatch_http_project_test.go +++ b/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"]) } }