fix(mcp): an empty parent param is not a hierarchy directive (BUG-2850)

Codex round 10: three P2, no P1. One fixed, one already filed, one
refuted and reverted.

[FIXED] An empty top-level `parent` was counted as an alias directive,
so `parent: ""` with `fields:{"plan":"X"}` refused a perfectly good
call. Every declared string param on this tool treats "" as NOT
PROVIDED — it is why promotedParamValue does, and why `assign: ""` is
deliberately inert — so a client that fills declared optional params
with their zero value rather than omitting them got a refusal for
asking one hierarchy question. My own round-9 snapshot carried this
forward from the out[]-based check it replaced.

Deliberately NOT applied to the other empty forms: `field:["parent="]`
and `fields:{"parent":""}` are the documented CLEAR signal
(BUG-2013 / BUG-2078), so they are semantically effective and still
conflict. One is a param left blank, the other is an instruction that
happens to look like one. Both directions are pinned, and the mutation
matrix drives both:

  remove the empty-param exclusion -> only EmptyParentParamIsNotAnAliasConflict fails
  extend it to the field-array clear -> only EmptyClearFormsStillConflict fails

[ALREADY FILED] The transport-dependent whitespace finding is BUG-2870,
ruled out of this PR's scope. Round 10 did add something the filing
missed and BUG-2870 now records it: the divergence covers VALUES too
(`--field "cost= 3"` stores the number 3 remotely and the string " 3"
over stdio), which is worse than the key half because both doors report
success and only the stored type differs.

[REFUTED, REVERTED] "Artifact imports discard createItemChecked's
undeclared-field warnings." True as a code reading — this handler builds
its own response shape and ignores item.Warnings — but the condition is
unreachable. artifact.Decode populates Fields exclusively from
FieldKeysForKind via a closed switch over a typed frontmatter struct, so
a key outside that per-kind list never enters the map. Verified both
ways before reverting, because the import door takes raw bytes and the
hand-written case is the one that mattered: Encode drops extra keys, and
Decode of a hand-written artifact carrying extra frontmatter keys drops
them too.

I had written the merge and a test for it before checking; the test
could not pass through the public door, which is what exposed the
finding rather than my fix. Reverted to a comment recording the
mechanism, so the next reader — or the next round — does not re-find it.
A branch nothing can enter is not defence in depth, it is a claim that
something is handled when it never happens.

gofmt clean · go vet clean · go test ./... green (29 packages)

internal/mcp/catalog_item_fields.go

@@ -190,9 +190,29 @@ func reshapeItemFields(prefix string, input map[string]any) (map[string]any, *mc
 	// top-level message honest.
 	origHierarchyParams := map[string]any{}
 	for _, alias := range hierarchyAliasKeys {
-		if v, ok := input[alias]; ok {
-			origHierarchyParams[alias] = v
+		v, ok := input[alias]
+		if !ok {
+			continue
 		}
+		// AN EMPTY STRING IS NOT PROVIDED (codex round 10). Every declared
+		// string param on this tool follows that convention — it is why
+		// promotedParamValue treats "" as absent and why `assign: ""` is
+		// deliberately inert. Counting it here made `parent: ""` collide with
+		// a perfectly good `fields.plan`, refusing a call that asks for one
+		// hierarchy directive and passes the other as a padded-out zero
+		// value, which is exactly what a client that fills every declared
+		// optional param does.
+		//
+		// NOT the same as an empty value in the `field` ARRAY or in `fields`:
+		// `field:["parent="]` and `fields:{"parent":""}` are the documented
+		// CLEAR signal (BUG-2013 / BUG-2078), so they are semantically
+		// effective and stay conflicts. The asymmetry is real and load-
+		// bearing: one is a param left blank, the other is an explicit
+		// instruction that happens to look like one.
+		if str, isString := v.(string); isString && str == "" {
+			continue
+		}
+		origHierarchyParams[alias] = v
 	}
 
 	// The same values with their JSON types intact (BUG-2850).

internal/mcp/catalog_item_fields_test.go

@@ -1063,3 +1063,64 @@ func TestPadItemUpdate_StringIdentityRefStillAccepted(t *testing.T) {
 		t.Errorf("identity refs lost: %v", disp.gotArgs)
 	}
 }
+
+// --- codex round 10 ---
+
+// TestPadItemUpdate_EmptyParentParamIsNotAnAliasConflict: an empty top-level
+// `parent` is NOT PROVIDED, so it must not collide with `fields.plan`
+// (BUG-2850, codex round 10).
+//
+// Every declared string param on this tool follows that convention — it is
+// why promotedParamValue treats "" as absent and why `assign: ""` is
+// deliberately inert. Counting it as a hierarchy directive refused a
+// perfectly good call from any client that fills declared optional params
+// with their zero value rather than omitting them, which is a common client
+// shape and exactly who this would have hit.
+func TestPadItemUpdate_EmptyParentParamIsNotAnAliasConflict(t *testing.T) {
+	disp, msg, isErr := dispatchPadItem(t, map[string]any{
+		"action": "update",
+		"ref":    "TASK-5",
+		"parent": "",
+		"fields": map[string]any{"plan": "PLAN-12"},
+	})
+	if isErr {
+		t.Fatalf("an empty parent param is not a directive; expected success, got: %s", msg)
+	}
+	if len(disp.gotPath) == 0 {
+		t.Fatal("expected the update to dispatch")
+	}
+}
+
+// ...and the EFFECTIVE empty forms still conflict, because they are not the
+// same thing. `field:["parent="]` and `fields:{"parent":""}` are the
+// documented CLEAR signal (BUG-2013 / BUG-2078) — an explicit instruction
+// that happens to look like a blank param. If this ever goes green, the
+// round-10 fix has been over-applied and a clear-plus-set pair is resolving
+// silently instead of refusing.
+func TestPadItemUpdate_EmptyClearFormsStillConflictWithTheAlias(t *testing.T) {
+	cases := map[string]map[string]any{
+		"field array clear vs fields.plan": {
+			"field":  []any{"parent="},
+			"fields": map[string]any{"plan": "PLAN-12"},
+		},
+		"fields.parent clear vs field array plan": {
+			"field":  []any{"plan=PLAN-12"},
+			"fields": map[string]any{"parent": ""},
+		},
+	}
+	for name, extra := range cases {
+		t.Run(name, func(t *testing.T) {
+			input := map[string]any{"action": "update", "ref": "TASK-5"}
+			for k, v := range extra {
+				input[k] = v
+			}
+			disp, msg, isErr := dispatchPadItem(t, input)
+			if !isErr {
+				t.Fatalf("an explicit clear is a directive and must still conflict; got success: %s", msg)
+			}
+			if len(disp.gotPath) != 0 {
+				t.Errorf("must not dispatch; dispatched %v", disp.gotPath)
+			}
+		})
+	}
+}

internal/server/handlers_artifact_import.go

@@ -207,6 +207,25 @@ func (s *Server) handleImportArtifact(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// item.Warnings is deliberately NOT merged into `warnings` here, and this
+	// note exists so the omission is not re-found as a defect (codex round 10
+	// raised it; the claim was checked and does not hold on this path).
+	//
+	// createItemChecked names undeclared field keys in item.Warnings, and this
+	// handler does build its own response shape — so on a code reading, the
+	// warnings look dropped. They cannot occur. artifact.Decode populates
+	// Fields exclusively from FieldKeysForKind via a closed switch over a
+	// typed frontmatter struct (internal/artifact/decode.go), so a key outside
+	// that per-kind list never enters the map, whatever bytes arrive. Verified
+	// both ways before removing the merge: Encode drops extra keys, and Decode
+	// of a HAND-WRITTEN artifact carrying extra frontmatter keys drops them
+	// too — the import door takes raw bytes, so the hand-written case is the
+	// one that mattered.
+	//
+	// Merging would therefore add a branch nothing can enter, and a test for
+	// it cannot pass through the public door. If artifact ever grows
+	// passthrough fields, this is the line to revisit.
+
 	writeJSON(w, http.StatusCreated, artifactImportResponse{
 		Ref:      item.Ref,
 		Slug:     item.Slug,