fix: bulk move refused a required field the same request supplied (TASK-2878)

Codex round 19 reviewed whether rounds 16-18's fixes broke anything. They did
not — it states explicitly that the collapse, the carried-set recomputation,
the notDefaultKeys change and the non-string-default drop do not narrow owner
behaviour or alter non-nil / never-supplied override behaviour. That negative
result is the round's main product, given that three earlier fixes in this unit
each opened the next round's defect.

It raised three findings. One is fixed here; two are recorded for a ruling.

FIXED — BULK MOVE CHECKED A REQUIRED-FIELD ERROR COMPUTED BEFORE THE OVERRIDE
EXISTED. `MigrateFields` records `required field "status" has no value` when a
source `status` holding a select value cannot migrate into a destination
`status` declared as a required RELATION. The caller supplies a perfectly good
referent in the same request, `result.Fields["status"]` is set from it — and
the check that follows reads `result.Errors`, which was computed before any of
that. So the move was refused for a field the request had just filled, and the
item stayed put.

This is the defect PLAN-2357 DR-12 fixed at the SINGLE move door. The comment
there says it in as many words: "an override that SATISFIED a required
destination field still 400'd". Nobody swept the fix to the bulk door, and it
became reachable when round 11 established that `req.Status` is caller input.
Third instance in this unit of a rule fixed at one door and not at its
siblings, which is the CONVE-18 shape at the level of DOORS rather than call
sites.

Filtering the error list rather than adopting the single door's
validate-the-merged-map shape, deliberately: `ValidateFields` below already
covers the merged map, and switching this check to it would change the error
code for a genuinely-missing required field from `missing_required_fields` to
`validation_error` — a compatibility break to fix a defect that does not need
one. The filter matches MigrateFields' exact rendering per supplied key, not a
substring, so a key whose name contains another key's name cannot collide.

Mutant: restore the unfiltered `result.Errors` -> FAIL, the move is refused for
the supplied field. Control leg included: with NOTHING supplied, the move must
STILL be refused AND still carry `missing_required_fields`, so the test fails
against a build that dropped the check rather than narrowing it.

RECORDED, NOT FIXED — the copy and its preflight disagree about error
PRECEDENCE in two cases. Both doors REFUSE in both cases and both refusals are
non-disclosing; what differs is which error wins.

  a. Overrides `{"owner_ref":"<invisible live item>", "ghost":"x"}`: the
     preflight returns 400 `malformed_override` for the undeclared key, the
     copy returns 400 `validation_error` for the invisible relation.
  b. Override `{"owner_ref":42}`: the preflight returns 400 `invalid_override`,
     the copy returns 400 `validation_error` from store field validation.

Both are real — the pair is specified to give one answer to one body — and
neither is a security or data defect. Closing them means choosing a precedence
and applying it at two doors, which is a response-contract decision and a
reordering of error handling in the MUTATING path. In a unit where three fixes
have each opened the next round's defect, that is not a change to make
unilaterally at the end of a session. Left for a ruling, with the two concrete
bodies above so whoever takes it does not have to re-derive them.

Gates: build, go vet, gofmt clean on this tip. SQLite and Postgres were BOTH
green on f601a2a1 (SQLite server 372.7s / store 376.0s / items / mcp 24.7s,
EXIT=0; PG store 525.8s / server 267.8s, EXIT=0) and are owed again on this
tip; they are NOT claimed here.

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

internal/server/handlers_items_bulk.go

@@ -770,9 +770,26 @@ func (s *Server) bulkMoveCollection(r *http.Request, workspaceID string, item *m
 		result.Fields["status"] = req.Status
 		suppliedByCaller["status"] = req.Status
 	}
-	if len(result.Errors) > 0 {
+	// Filtered against what the CALLER supplied, because result.Errors is
+	// computed by MigrateFields BEFORE any override exists (migrate.go:62).
+	//
+	// This is the defect PLAN-2357 DR-12 fixed at the SINGLE move door and
+	// nobody swept to this one: an override that SATISFIED a required
+	// destination field still 400'd. Reachable here since round 11 made
+	// `status` caller input — move an item whose source `status` is a select
+	// value into a destination whose `status` is a required relation, supply
+	// a perfectly good referent, and the move was refused for a field the
+	// request had just filled (codex round 19).
+	//
+	// Filtering rather than adopting the single door's "validate the merged
+	// map" shape, deliberately: ValidateFields below already covers the
+	// merged map, and switching this check to it would change the error CODE
+	// for a genuinely-missing required field from missing_required_fields to
+	// validation_error — a compatibility break for callers reading the code,
+	// to fix a defect that does not need it.
+	if remaining := requiredErrorsUnsatisfiedBy(result.Errors, suppliedByCaller); len(remaining) > 0 {
 		return nil, &bulkOpError{
-			message: "required fields missing: " + strings.Join(result.Errors, ", "),
+			message: "required fields missing: " + strings.Join(remaining, ", "),
 			code:    "missing_required_fields",
 		}
 	}
@@ -1041,3 +1058,34 @@ func bulkEventDelta(req *bulkItemsRequest) map[string]any {
 	}
 	return nil
 }
+
+// requiredErrorsUnsatisfiedBy drops the required-field errors MigrateFields
+// raised for keys the caller then supplied a value for.
+//
+// MigrateFields formats these as `required field %q has no value`, so the
+// match is against that exact rendering for each supplied key rather than a
+// substring of the message — a substring test would also match a key whose
+// name contains another key's name.
+func requiredErrorsUnsatisfiedBy(errs []string, supplied map[string]any) []string {
+	if len(errs) == 0 || len(supplied) == 0 {
+		return errs
+	}
+	satisfied := make(map[string]bool, len(supplied))
+	for k, v := range supplied {
+		if v == nil {
+			continue
+		}
+		if str, isStr := v.(string); isStr && strings.TrimSpace(str) == "" {
+			continue
+		}
+		satisfied[fmt.Sprintf("required field %q has no value", k)] = true
+	}
+	out := errs[:0:0]
+	for _, e := range errs {
+		if satisfied[e] {
+			continue
+		}
+		out = append(out, e)
+	}
+	return out
+}

internal/server/relation_referents_doors_test.go

@@ -1394,3 +1394,65 @@ func TestRelationDoors_NullOverrideDoesNotExemptDefaultFromVisibility(t *testing
 			"the drop above is not visibility-dependent", v, ok)
 	}
 }
+
+// A supplied value that SATISFIES a required destination field must not be
+// refused by a required-field error computed before the override existed
+// (codex round 19).
+//
+// PLAN-2357 DR-12 fixed exactly this at the SINGLE move door — the comment
+// there records that `result.Errors` is computed by MigrateFields BEFORE any
+// override, so an override that satisfied a required field still 400'd — and
+// nobody swept the fix to the bulk door. It became reachable when round 11
+// established that `req.Status` on a bulk move is caller input.
+//
+// The shape: a source `status` holding a select value cannot migrate into a
+// destination `status` declared as a required relation, so MigrateFields drops
+// it and records the required-field error. The caller supplies a perfectly
+// good referent in the same request, and the move was refused for a field the
+// request had just filled.
+func TestRelationDoors_BulkMoveAcceptsASuppliedValueForARequiredRelation(t *testing.T) {
+	f := newDoorFixture(t)
+	dst := mustSchemaCollection(t, f.srv, f.ws.ID, "Bulk Required Relation", fmt.Sprintf(`{"fields":[
+		{"key":"status","label":"Status","type":"relation","collection":%q,"required":true}
+	]}`, f.people.Slug))
+
+	item := f.seed(`{"status":"open"}`)
+	rr := f.call(f.srv.handleBulkItems, "POST",
+		"/api/v1/workspaces/"+f.ws.Slug+"/items/bulk", nil,
+		map[string]any{"op": "move", "ids": []string{item.ID}, "collection": dst.Slug,
+			"status": f.target.Ref})
+	if rr.Code != http.StatusOK {
+		t.Fatalf("bulk move: got %d, want 200: %s", rr.Code, rr.Body.String())
+	}
+	var out bulkItemsResponse
+	if err := json.Unmarshal(rr.Body.Bytes(), &out); err != nil {
+		t.Fatalf("parse bulk response: %v: %s", err, rr.Body.String())
+	}
+	if len(out.Failed) > 0 {
+		t.Fatalf("the move was refused for a required field the SAME request supplied: %+v",
+			out.Failed)
+	}
+	if v, ok := f.storedRelationKey(item.ID, "status"); !ok || v != f.target.ID {
+		t.Fatalf("the supplied referent was not stored (%#v, present=%v), want %q",
+			v, ok, f.target.ID)
+	}
+
+	// Control: with NOTHING supplied for the required relation, the move must
+	// STILL be refused, and with the same code. Without this leg the test
+	// passes against a build that stopped checking required fields at all.
+	item2 := f.seed(`{"status":"open"}`)
+	rr2 := f.call(f.srv.handleBulkItems, "POST",
+		"/api/v1/workspaces/"+f.ws.Slug+"/items/bulk", nil,
+		map[string]any{"op": "move", "ids": []string{item2.ID}, "collection": dst.Slug})
+	var out2 bulkItemsResponse
+	if err := json.Unmarshal(rr2.Body.Bytes(), &out2); err != nil {
+		t.Fatalf("parse bulk response: %v: %s", err, rr2.Body.String())
+	}
+	if len(out2.Failed) == 0 {
+		t.Fatalf("a move leaving a REQUIRED relation empty was accepted: %+v", out2)
+	}
+	if got := out2.Failed[0].Code; got != "missing_required_fields" {
+		t.Fatalf("failure code = %q, want missing_required_fields — the filter must narrow "+
+			"the error list, not replace the check (message: %s)", got, out2.Failed[0].Error)
+	}
+}