fix(mcp): apply the null and hierarchy guards to promoted keys too (BUG-2850)

Codex round 4, and both findings are the same defect in my own round-3 fix:
ORDERING. The null guard and the hierarchy-key guard sat BELOW the branch
that promotes status/priority/category/parent/role/assign/tags onto dedicated
params, so every promoted key walked around both.

- `fields: {"tags": null}` reached the promoted branch and became a silent
  no-op, instead of the refusal the null rule documents.
- `fields: {"parent": 42}` was accepted here and dropped later by the
  handler — the same silent-drop shape this whole bug is about, reintroduced
  by a guard I added to prevent a different instance of it.

Both guards now run before that branch, so they apply to every key. A guard a
whole class of keys bypasses is not a guard.

Pinned separately from the generic-path cases, because the generic-path tests
passed throughout: they never exercised a promoted key, which is exactly why
the hole survived round 3. Reverting the hoist fails the new test. Well-formed
promoted keys still promote — tested, so the hoist did not break promotion
while closing the bypass.

Gates: gofmt clean, go vet clean, go test ./... 29 packages ok.

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

internal/mcp/catalog_item_fields.go

@@ -173,6 +173,40 @@ func reshapeItemFields(prefix string, input map[string]any) (map[string]any, *mc
 		if strings.Contains(k, "=") {
 			return nil, errStructured(prefix, fmt.Errorf("fields.%q: field keys cannot contain '='", k))
 		}
+		// THESE TWO GUARDS RUN BEFORE THE PROMOTED BRANCH, and that ordering
+		// is the fix, not a detail (codex round 4). They were below it, so
+		// `tags: null` and `parent: 42` reached the promoted path and skipped
+		// both: the null was converted into a silent no-op, and a numeric
+		// parent was dropped later by the handler. A guard that a whole class
+		// of keys walks around is not a guard.
+		// NULL stays refused (BUG-2850 lifts objects and arrays, not this).
+		// A null in a fields map has no agreed meaning — "store JSON null" and
+		// "clear this field" are both readable from it, and Pad already has an
+		// explicit clear vocabulary (clear_parent, clear_assigned_user). Giving
+		// null a silent meaning here would be inventing semantics inside a bug
+		// fix; if a clear-by-null is ever wanted it should be ruled and named.
+		if v == nil {
+			return nil, errStructured(prefix, fmt.Errorf(
+				"fields.%s: null has no defined write semantics — omit the key to leave it unchanged", k))
+		}
+
+		// HIERARCHY PSEUDO-KEYS TAKE A STRING REF, ALWAYS (BUG-2850, codex
+		// round 3). `plan` is not in padItemPromotedFieldKeys, so before this
+		// it fell to the generic path and — once structures stopped being
+		// refused — a `fields:{"plan":{…}}` reached the server natively. There
+		// `extractParentLink` reads any PRESENT non-string plan/parent as a
+		// hierarchy directive, drops the key, and on update CLEARS the item's
+		// existing parent link. So lifting the nested refusal quietly opened a
+		// path where a malformed value silently detaches an item from its
+		// parent. A structure has no meaning here at all: the only value these
+		// keys take is a ref.
+		if hierarchyPseudoFieldKeys[k] {
+			if _, isString := v.(string); !isString {
+				return nil, errStructured(prefix, fmt.Errorf(
+					"fields.%s must be a string ref (e.g. %q) — a %T here would be read as a hierarchy directive and could detach the item", k, "PLAN-12", v))
+			}
+		}
+
 		if padItemPromotedFieldKeys[k] {
 			existing, has := out[k]
 			if has {
@@ -191,34 +225,6 @@ func reshapeItemFields(prefix string, input map[string]any) (map[string]any, *mc
 			out[k] = v
 			continue
 		}
-		// HIERARCHY PSEUDO-KEYS TAKE A STRING REF, ALWAYS (BUG-2850, codex
-		// round 3). `plan` is not in padItemPromotedFieldKeys, so before this
-		// it fell to the generic path and — once structures stopped being
-		// refused — a `fields:{"plan":{…}}` reached the server natively. There
-		// `extractParentLink` reads any PRESENT non-string plan/parent as a
-		// hierarchy directive, drops the key, and on update CLEARS the item's
-		// existing parent link. So lifting the nested refusal quietly opened a
-		// path where a malformed value silently detaches an item from its
-		// parent. A structure has no meaning here at all: the only value these
-		// keys take is a ref.
-		if hierarchyPseudoFieldKeys[k] {
-			if _, isString := v.(string); !isString {
-				return nil, errStructured(prefix, fmt.Errorf(
-					"fields.%s must be a string ref (e.g. %q) — a %T here would be read as a hierarchy directive and could detach the item", k, "PLAN-12", v))
-			}
-		}
-
-		// NULL stays refused (BUG-2850 lifts objects and arrays, not this).
-		// A null in a fields map has no agreed meaning — "store JSON null" and
-		// "clear this field" are both readable from it, and Pad already has an
-		// explicit clear vocabulary (clear_parent, clear_assigned_user). Giving
-		// null a silent meaning here would be inventing semantics inside a bug
-		// fix; if a clear-by-null is ever wanted it should be ruled and named.
-		if v == nil {
-			return nil, errStructured(prefix, fmt.Errorf(
-				"fields.%s: null has no defined write semantics — omit the key to leave it unchanged", k))
-		}
-
 		// NATIVE FORM, ALWAYS (BUG-2850). The value goes into fieldsNative
 		// with its JSON type intact — a number stays a number, an object stays
 		// an object. Whether a door can USE that depends on the door, which is

internal/mcp/catalog_item_fields_test.go

@@ -473,3 +473,52 @@ func TestPadItemUpdate_StringHierarchyKeyStillAccepted(t *testing.T) {
 		t.Fatal("expected the update to dispatch")
 	}
 }
+
+// The guards must apply to PROMOTED keys too (BUG-2850, codex round 4).
+//
+// `tags`, `parent`, `status` and friends are handled by an earlier branch that
+// promotes them onto dedicated top-level params. The null and hierarchy guards
+// were written below that branch, so every promoted key walked around both:
+// `tags: null` became a silent no-op instead of the documented refusal, and
+// `parent: 42` was accepted here and dropped later by the handler. A guard a
+// whole class of keys bypasses is not a guard, which is why these cases are
+// pinned separately from the generic-path ones above.
+func TestPadItemUpdate_GuardsApplyToPromotedKeys(t *testing.T) {
+	cases := map[string]map[string]any{
+		"null tags":      {"tags": nil},
+		"null status":    {"status": nil},
+		"numeric parent": {"parent": float64(42)},
+		"bool parent":    {"parent": true},
+	}
+	for name, fields := range cases {
+		t.Run(name, func(t *testing.T) {
+			disp, msg, isErr := dispatchPadItem(t, map[string]any{
+				"action": "update",
+				"ref":    "TASK-5",
+				"fields": fields,
+			})
+			if !isErr {
+				t.Fatalf("expected refusal, got success: %s", msg)
+			}
+			if len(disp.gotPath) != 0 {
+				t.Errorf("must not dispatch; dispatched %v", disp.gotPath)
+			}
+		})
+	}
+}
+
+// ...and the promoted keys still work with well-formed values, so hoisting the
+// guards did not break promotion itself.
+func TestPadItemUpdate_PromotedKeysStillPromote(t *testing.T) {
+	disp, msg, isErr := dispatchPadItem(t, map[string]any{
+		"action": "update",
+		"ref":    "TASK-5",
+		"fields": map[string]any{"status": "done", "tags": []any{"a", "b"}},
+	})
+	if isErr {
+		t.Fatalf("well-formed promoted keys must still be accepted: %s", msg)
+	}
+	if len(disp.gotPath) == 0 {
+		t.Fatal("expected the update to dispatch")
+	}
+}