fix(server): a bulk status or priority change refuses an item whose collection has no such field (BUG-3154) (#1450)

* fix(server): a bulk status or priority change refuses an item whose collection has no such field (BUG-3154)

Both callers of bulkFieldUpdate pass a key the SERVER chose: `priority` for
set-priority, `status` for a status-only move. Validation walks only declared
fields, so on a collection declaring neither, the value was stored as an
orphan field no schema-driven surface renders, and the item was listed under
`updated` although the operation has no meaning there.

bulkFieldUpdate now refuses any change key the item's collection does not
declare (items.UndeclaredOverrideKeys, the helper the move override check
uses) before reading the blob. The item lands in failed[] with
validation_error naming the collection and the field, nothing is written, and
the rest of the batch applies. Putting it in bulkFieldUpdate rather than per op
closes the class for any future fixed-key op; there are exactly two callers
today.

Single-item update still ACCEPTS undeclared keys with
warnings.undeclared_fields (BUG-2850): there the caller types the key and
round-trips whole blobs. Here the caller named an operation.

The web lane menu already gates both actions on the schema declaring the
field, so no UI path reaches the refusal. WebMCP's pad_item.bulk-update does,
so ToolSurfaceVersion goes to 0.44. The entry states that stdio and remote,
which issue a per-item PATCH, still accept the key with a warning.

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

* fix(server): a status override on a bulk collection move refuses a target with no status field (BUG-3154, codex round 1)

bulkMoveCollection merges req.Status after MigrateFields, which keeps only
target-declared fields, and validation walks only declared fields, so a move
into a collection with no `status` field still stored an orphan. It now makes
the same per-item refusal against the stripped target schema, the schema the
override is validated against.

The 0.44 prose now names this path, and says the server-chosen versus
caller-typed line belongs to the HTTP doors, not the catalog call.

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

internal/mcp/instructions.md

@@ -6,7 +6,7 @@ Pad is a project tracker for developers and AI agents — issues (TASK, BUG), pl
 
 If the user is asking general code questions with no project-management thread, you don't need this server.
 
-## Tool surface (v0.43)
+## Tool surface (v0.44)
 
 Ten resource × action tools, plus `pad_set_workspace` (which takes a `workspace` slug only — no action enum). Eleven tools total.
 

internal/mcp/version.go

@@ -1017,6 +1017,34 @@ const CmdhelpVersion = "0.1"
 //     browser tab and BUG-3000 carries the open half — so no surface
 //     here states a duration.
 //
+//     0.44 — BUG-3154. `pad_item.action=bulk-update` over the WebMCP
+//     browser transport now REFUSES, per item, a status or priority change
+//     on an item whose collection does not declare that field. The item is
+//     listed in the bulk response's `failed` array with code
+//     `validation_error` and a message naming the collection and the field,
+//     and nothing is written; the other items in the call still apply. The
+//     door is `POST /workspaces/{ws}/items/bulk` (`set-priority`, and `move`
+//     with a `status`, with or without a `collection`; for a collection move
+//     the TARGET's schema decides). Its request carries an operation and a
+//     value, and the SERVER picks the field key. It used to store the value
+//     as an orphan field no schema renders while reporting the item as
+//     updated. BEHAVIOR bump on
+//     the 0.43 / 0.40 / 0.29 grounds: no tool name, action enum or param
+//     shape changed, but a write door refuses a call it used to accept.
+//
+//     THE TRANSPORTS DISAGREE after this, and it is stated rather than
+//     implied. Local stdio (`pad item bulk-update`) and remote /mcp
+//     (dispatchItemBulkUpdate) never reach the bulk endpoint. They issue a
+//     per-item PATCH, which is the single-item update door and ACCEPTS an
+//     undeclared key, naming it in `warnings.undeclared_fields` (0.27 /
+//     BUG-2850). So the same catalog call is refused per item on WebMCP and
+//     stored with a warning on the other two. The line between them is the
+//     one this entry draws, server-chosen key versus caller-typed key, and
+//     it is a property of the two HTTP doors, not of the catalog call. On
+//     every transport the catalog caller supplies the same `status` or
+//     `priority` param; only the door the transport happens to reach differs. Whether stdio and remote should call
+//     the bulk endpoint is tracked separately; it is not converged here.
+//
 //     0.43 — BUG-2379. `pad_item.action=move` REFUSES a field override
 //     naming a field the DESTINATION collection's schema does not declare,
 //     with 400 `malformed_override` — the same check (UndeclaredOverrideKeys,
@@ -1386,7 +1414,7 @@ const CmdhelpVersion = "0.1"
 //     this surface can receive it; the entry exists so a future action does
 //     not collapse it to permission_denied. When an action that can reach
 //     it is added, that addition is the contract change and owns the bump.
-const ToolSurfaceVersion = "0.43"
+const ToolSurfaceVersion = "0.44"
 
 // MetaVersionURI is the canonical URI of the queryable version document.
 // Lives outside the pad://workspace/{ws}/... namespace because it's a

internal/server/bug3154_bulk_undeclared_key_test.go

@@ -0,0 +1,164 @@
+package server
+
+import (
+	"net/http"
+	"strings"
+	"testing"
+)
+
+// BUG-3154 — a bulk op whose field KEY the server chooses must not write that
+// key onto an item whose collection does not declare it.
+//
+// Both callers of bulkFieldUpdate pass a fixed key: `set-priority` writes
+// `priority`, a status-only `move` writes `status`. Validation walks only the
+// DECLARED fields, so on a collection with neither field the value used to be
+// stored as an orphan key no schema-driven surface renders, and the item was
+// listed under `updated` although its one requested change had no meaning
+// there. The item is now refused into `failed[]`, naming the collection and
+// the field, and nothing is written; the other items in the request still
+// apply, which is bulk's partial-success contract.
+//
+// Each case sends ONE request carrying an item from each kind of collection,
+// so the refusal is shown to be per item rather than per batch — and so the
+// declared leg is the control: a check that refused everything, or that
+// consulted the wrong collection, fails it.
+func TestBulkItems_RefusesAFieldTheItemsCollectionDoesNotDeclare(t *testing.T) {
+	cases := []struct {
+		name  string
+		body  map[string]any
+		field string
+		value string
+	}{
+		{"set-priority", map[string]any{"op": "set-priority", "priority": "high"}, "priority", "high"},
+		{"status-only move", map[string]any{"op": "move", "status": "done"}, "status", "done"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			srv := testServer(t)
+			slug := createWSWithCollections(t, srv)
+			ws, err := srv.store.GetWorkspaceBySlug(slug)
+			if err != nil || ws == nil {
+				t.Fatalf("GetWorkspaceBySlug(%s): %v", slug, err)
+			}
+			// Declares neither `priority` nor `status`.
+			bare := mustSchemaCollection(t, srv, ws.ID, "Bare Notes", `{"fields":[
+				{"key":"note","label":"Note","type":"text"}
+			]}`)
+
+			orphanCandidate := createItem(t, srv, slug, bare.Slug, map[string]interface{}{
+				"title": "No such field here", "fields": `{"note":"hi"}`,
+			})
+			// Tasks declare both fields: the control leg.
+			declared := createBulkTestItem(t, srv, slug, "Declared", `{"status":"open","priority":"low"}`)
+
+			body := map[string]any{"ids": []string{orphanCandidate.Ref, declared.Ref}}
+			for k, v := range tc.body {
+				body[k] = v
+			}
+			rr := doRequest(srv, "POST", "/api/v1/workspaces/"+slug+"/items/bulk", body)
+			if rr.Code != http.StatusOK {
+				t.Fatalf("bulk %s: expected 200 with a per-item failure, got %d: %s", tc.name, rr.Code, rr.Body.String())
+			}
+			var resp bulkItemsResponse
+			parseJSON(t, rr, &resp)
+
+			// Control: the item whose collection declares the field applied.
+			if len(resp.Updated) != 1 || resp.Updated[0].Ref != declared.Ref {
+				t.Fatalf("expected exactly %s under updated, got %+v", declared.Ref, resp.Updated)
+			}
+			if got := itemFields(t, srv, slug, declared.Slug)[tc.field]; got != tc.value {
+				t.Errorf("control: %s %s = %v, want %s", declared.Ref, tc.field, got, tc.value)
+			}
+
+			// The refusal: listed under failed, with a reason naming the
+			// collection and the field — not listed under updated.
+			if len(resp.Failed) != 1 || resp.Failed[0].Ref != orphanCandidate.Ref {
+				t.Fatalf("expected exactly %s under failed, got %+v", orphanCandidate.Ref, resp.Failed)
+			}
+			f := resp.Failed[0]
+			if f.Code != "validation_error" {
+				t.Errorf("failed code = %q, want validation_error", f.Code)
+			}
+			if !strings.Contains(f.Error, bare.Slug) || !strings.Contains(f.Error, `"`+tc.field+`"`) {
+				t.Errorf("failed reason should name collection %q and field %q, got %q", bare.Slug, tc.field, f.Error)
+			}
+
+			// The defect: nothing was written. The stored blob is compared
+			// whole, so a write that added the key — or touched anything
+			// else — fails here.
+			stored := itemFields(t, srv, slug, orphanCandidate.Slug)
+			if _, present := stored[tc.field]; present || len(stored) != 1 || stored["note"] != "hi" {
+				t.Errorf("BUG-3154: refused item's stored fields changed: %v, want only note=hi", stored)
+			}
+			after, err := srv.store.GetItem(orphanCandidate.ID)
+			if err != nil || after == nil {
+				t.Fatalf("reload %s: %v", orphanCandidate.Ref, err)
+			}
+			if after.Seq != orphanCandidate.Seq {
+				t.Errorf("refused item was written: seq %d -> %d", orphanCandidate.Seq, after.Seq)
+			}
+		})
+	}
+}
+
+// The same refusal on a status move COMBINED with a collection move
+// (bulkMoveCollection), found by review round 1. MigrateFields keeps only the
+// target's declared fields, but the status override is merged after it, so a
+// target with no `status` field used to receive an orphan. `collection` is one
+// per request, so the control is a second request: the identical move into a
+// target that DOES declare `status` must apply.
+func TestBulkItems_CollectionMoveRefusesAStatusTheTargetDoesNotDeclare(t *testing.T) {
+	srv := testServer(t)
+	slug := createWSWithCollections(t, srv)
+	ws, err := srv.store.GetWorkspaceBySlug(slug)
+	if err != nil || ws == nil {
+		t.Fatalf("GetWorkspaceBySlug(%s): %v", slug, err)
+	}
+	bare := mustSchemaCollection(t, srv, ws.ID, "Bare Notes", `{"fields":[
+		{"key":"note","label":"Note","type":"text"}
+	]}`)
+	withStatus := mustSchemaCollection(t, srv, ws.ID, "Staged Notes", `{"fields":[
+		{"key":"note","label":"Note","type":"text"},
+		{"key":"status","label":"Status","type":"select","options":["open","done"]}
+	]}`)
+
+	control := createBulkTestItem(t, srv, slug, "Control", `{"status":"open"}`)
+	refused := createBulkTestItem(t, srv, slug, "Refused", `{"status":"open"}`)
+
+	move := func(ref, target string) bulkItemsResponse {
+		t.Helper()
+		rr := doRequest(srv, "POST", "/api/v1/workspaces/"+slug+"/items/bulk", map[string]any{
+			"ids": []string{ref}, "op": "move", "collection": target, "status": "done",
+		})
+		if rr.Code != http.StatusOK {
+			t.Fatalf("bulk move to %s: expected 200, got %d: %s", target, rr.Code, rr.Body.String())
+		}
+		var resp bulkItemsResponse
+		parseJSON(t, rr, &resp)
+		return resp
+	}
+
+	// Control: the target declares status, so the override applies.
+	if resp := move(control.Ref, withStatus.Slug); len(resp.Updated) != 1 || len(resp.Failed) != 0 {
+		t.Fatalf("control move: expected 1 updated / 0 failed, got %+v", resp)
+	}
+	if got := itemFields(t, srv, slug, control.Slug)["status"]; got != "done" {
+		t.Errorf("control: status = %v, want done", got)
+	}
+
+	resp := move(refused.Ref, bare.Slug)
+	if len(resp.Updated) != 0 || len(resp.Failed) != 1 || resp.Failed[0].Ref != refused.Ref {
+		t.Fatalf("expected %s under failed only, got %+v", refused.Ref, resp)
+	}
+	if f := resp.Failed[0]; f.Code != "validation_error" || !strings.Contains(f.Error, bare.Slug) || !strings.Contains(f.Error, `"status"`) {
+		t.Errorf("failed row should be validation_error naming %q and \"status\", got %+v", bare.Slug, f)
+	}
+	after, err := srv.store.GetItem(refused.ID)
+	if err != nil || after == nil {
+		t.Fatalf("reload %s: %v", refused.Ref, err)
+	}
+	if after.CollectionID != refused.CollectionID || after.Seq != refused.Seq {
+		t.Errorf("BUG-3154: refused item was moved or written: collection %s -> %s, seq %d -> %d",
+			refused.CollectionID, after.CollectionID, refused.Seq, after.Seq)
+	}
+}

internal/server/handlers_items_bulk.go

@@ -525,6 +525,24 @@ func (s *Server) bulkFieldUpdate(r *http.Request, workspaceID string, item *mode
 		return nil, &bulkOpError{message: "failed to parse collection schema"}
 	}
 
+	// REFUSE a key the item's collection does not declare (BUG-3154). Every
+	// caller passes a key the SERVER chose (`status` for a status-only move,
+	// `priority` for set-priority) and validation below walks only declared
+	// fields, so on a collection without that field the value was written as
+	// an orphan no schema-driven surface renders, and the item was reported
+	// under `updated` although the operation has no meaning there. Refused per
+	// ITEM, so the rest of the batch still applies.
+	//
+	// Deliberately NOT the accept-and-warn of a single-item update
+	// (BUG-2850): there the caller TYPES the key and round-trips whole blobs;
+	// here the caller named an operation and the key is ours.
+	if undeclared := items.UndeclaredOverrideKeys(changes, schema.Fields); len(undeclared) > 0 {
+		return nil, &bulkOpError{
+			code:    "validation_error",
+			message: fmt.Sprintf("collection %q has no %q field, so this operation does not apply to this item", coll.Slug, undeclared[0]),
+		}
+	}
+
 	fieldMap := make(map[string]any)
 	if item.Fields != "" && item.Fields != "{}" {
 		// REFUSE an unreadable stored blob rather than discard it (BUG-3049,
@@ -906,6 +924,18 @@ func (s *Server) bulkMoveCollection(r *http.Request, workspaceID string, item *m
 	// checked for visibility (codex round 11).
 	suppliedByCaller := map[string]any{}
 	if req.Status != "" {
+		// BUG-3154: the same refusal bulkFieldUpdate makes for a status-only
+		// move. MigrateFields keeps only target-declared fields, but this
+		// override is merged after it and validation walks only declared
+		// fields, so a target with no `status` field stored an orphan here.
+		// Checked against the stripped schema, the one the override is
+		// validated against below and the one BUG-2379's move check uses.
+		if undeclared := items.UndeclaredOverrideKeys(map[string]any{"status": req.Status}, items.SchemaForMigratedFields(targetSchema).Fields); len(undeclared) > 0 {
+			return nil, &bulkOpError{
+				code:    "validation_error",
+				message: fmt.Sprintf("collection %q has no %q field, so this operation does not apply to this item", targetColl.Slug, undeclared[0]),
+			}
+		}
 		result.Fields["status"] = req.Status
 		suppliedByCaller["status"] = req.Status
 	}