A merge lost a phone number on its first live run

Found by walking the scenario against production rather than by a test.
Two records, each with a phone; the survivor kept its own, and the
source's simply stopped existing. Searching for it returned nothing.

The first version's rule was "fill the survivor's blanks, never overwrite
what it has", which is right about which value WINS and said nothing
about the one that loses. One person can have two numbers, two spellings
of a name, a work address and a personal one - and a merge that quietly
deletes one is exactly the data loss this file already refuses elsewhere:
"silently turning Alice back into Visitor 3 is data loss the operator
cannot see happen."

The profile is now reconciled field by field in Go rather than in one
clever upsert, because the interesting case was never the winner. Blanks
are still filled and the survivor still keeps its own values, but every
losing value is returned in `discarded` AND appended to the survivor's
notes - the response is read once and the record is read forever.

Notes themselves are additive rather than a winner: two people writing
about one customer wrote two different true things.

mergeProfiles is pure, so the rule is asserted directly - four cases
including the ordinary one, a typed record joining a camera record with
no profile at all, which must add no noise.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGcjxF1cNLcuwc3DAPcnfj

server/internal/api/types.go

@@ -194,6 +194,11 @@ type MergeResult struct {
 	// these on cards and read them aloud, so a merge has to say which one
 	// died rather than leaving somebody to discover it at a counter.
 	RetiredRef string `json:"retired_ref"`
+	// Discarded lists profile values the survivor already had a different
+	// answer for - a second phone number, a different spelling of a name.
+	// They are appended to the survivor's notes as well: this response is
+	// read once and the record is read forever.
+	Discarded []string `json:"discarded,omitempty"`
 }
 
 // MergeRequest names the record to keep.

server/internal/store/api_customers.go

@@ -12,7 +12,9 @@ package store
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/jackc/pgx/v5"
@@ -148,25 +150,39 @@ func (s *Store) MergeVisitors(ctx context.Context, clientID, sourceID, targetID
 		return out, pgx.ErrNoRows
 	}
 
-	// Profile: visitor_profiles is UNIQUE on visitor_id, so the two cannot
-	// simply both move. Blanks on the survivor are filled from the source and
-	// nothing the survivor already holds is overwritten - which is exactly
-	// right for the case this exists for, a hand-typed name and phone being
-	// joined to the face that was recognised later.
-	if _, err := tx.Exec(ctx, `
-		INSERT INTO visitor_profiles (visitor_id, client_id, full_name, phone,
-		                              email, gender, notes, collected_by, collected_at)
-		SELECT $2::uuid, client_id, full_name, phone, email, gender, notes,
-		       collected_by, collected_at
-		  FROM visitor_profiles WHERE visitor_id = $1::uuid
-		ON CONFLICT (visitor_id) DO UPDATE SET
-		  full_name = CASE WHEN visitor_profiles.full_name = '' THEN EXCLUDED.full_name ELSE visitor_profiles.full_name END,
-		  phone     = CASE WHEN visitor_profiles.phone     = '' THEN EXCLUDED.phone     ELSE visitor_profiles.phone END,
-		  email     = CASE WHEN visitor_profiles.email     = '' THEN EXCLUDED.email     ELSE visitor_profiles.email END,
-		  gender    = CASE WHEN visitor_profiles.gender    = '' THEN EXCLUDED.gender    ELSE visitor_profiles.gender END,
-		  notes     = CASE WHEN visitor_profiles.notes     = '' THEN EXCLUDED.notes     ELSE visitor_profiles.notes END,
-		  updated_at = now()`, sourceID, targetID); err != nil {
-		return out, fmt.Errorf("merge profile: %w", err)
+	// Profile: visitor_profiles is UNIQUE on visitor_id, so the two cannot both
+	// move and something has to win. Merged field by field in Go rather than in
+	// one clever upsert, because the interesting case is not which value wins -
+	// it is what happens to the one that loses.
+	//
+	// Blanks on the survivor are filled from the source. Where BOTH hold a
+	// value the survivor keeps its own and the loser is recorded in `discarded`
+	// and appended to notes. Dropping it silently was the first version's
+	// behaviour and it lost a phone number on the very first live run: one
+	// person can have two numbers, and a merge that quietly deletes one is
+	// precisely the data loss an operator cannot see happen.
+	var src, dst profileFields
+	if err := readProfile(ctx, tx, sourceID, &src); err != nil {
+		return out, fmt.Errorf("read source profile: %w", err)
+	}
+	if err := readProfile(ctx, tx, targetID, &dst); err != nil {
+		return out, fmt.Errorf("read target profile: %w", err)
+	}
+	if src.exists {
+		merged, discarded := mergeProfiles(src, dst)
+		out.Discarded = discarded
+		if _, err := tx.Exec(ctx, `
+			INSERT INTO visitor_profiles (visitor_id, client_id, full_name, phone,
+			                              email, gender, notes)
+			VALUES ($1::uuid, $2::uuid, $3, $4, $5, $6, $7)
+			ON CONFLICT (visitor_id) DO UPDATE SET
+			  full_name = EXCLUDED.full_name, phone = EXCLUDED.phone,
+			  email = EXCLUDED.email, gender = EXCLUDED.gender,
+			  notes = EXCLUDED.notes, updated_at = now()`,
+			targetID, clientID, merged.fullName, merged.phone, merged.email,
+			merged.gender, merged.notes); err != nil {
+			return out, fmt.Errorf("merge profile: %w", err)
+		}
 	}
 
 	for _, q := range []struct {
@@ -236,3 +252,62 @@ func (s *Store) MergeVisitors(ctx context.Context, clientID, sourceID, targetID
 func isAutoLabel(label string, number int64) bool {
 	return label == fmt.Sprintf("Visitor %d", number)
 }
+
+// profileFields is the part of a profile a merge has to reconcile.
+type profileFields struct {
+	exists                                bool
+	fullName, phone, email, gender, notes string
+}
+
+func readProfile(ctx context.Context, tx pgx.Tx, visitorID string, out *profileFields) error {
+	err := tx.QueryRow(ctx, `
+		SELECT full_name, phone, email, gender, notes
+		  FROM visitor_profiles WHERE visitor_id = $1::uuid`, visitorID).
+		Scan(&out.fullName, &out.phone, &out.email, &out.gender, &out.notes)
+	if errors.Is(err, pgx.ErrNoRows) {
+		return nil
+	}
+	out.exists = err == nil
+	return err
+}
+
+// mergeProfiles keeps the survivor's own values, fills its blanks from the
+// source, and returns everything that lost so nothing disappears silently.
+func mergeProfiles(src, dst profileFields) (profileFields, []string) {
+	out := dst
+	var discarded []string
+	keep := func(field string, mine *string, theirs string) {
+		switch {
+		case theirs == "":
+		case *mine == "":
+			*mine = theirs
+		case *mine != theirs:
+			discarded = append(discarded, field+": "+theirs)
+		}
+	}
+	keep("name", &out.fullName, src.fullName)
+	keep("phone", &out.phone, src.phone)
+	keep("email", &out.email, src.email)
+	keep("gender", &out.gender, src.gender)
+
+	// Notes are additive rather than a winner: two people writing about one
+	// customer wrote two different true things.
+	if src.notes != "" && src.notes != out.notes {
+		if out.notes == "" {
+			out.notes = src.notes
+		} else {
+			out.notes += "\n" + src.notes
+		}
+	}
+	// And the losers land in notes too, because the response is read once and
+	// the record is read forever.
+	if len(discarded) > 0 {
+		line := "merged, also known as - " + strings.Join(discarded, ", ")
+		if out.notes == "" {
+			out.notes = line
+		} else {
+			out.notes += "\n" + line
+		}
+	}
+	return out, discarded
+}

server/internal/store/api_customers_test.go

@@ -0,0 +1,71 @@
+package store
+
+import (
+	"strings"
+	"testing"
+)
+
+// mergeProfiles is pure, so the rule it encodes can be asserted without a
+// database - and it is the rule that matters: what happens to the value that
+// LOSES. The first version dropped it, and lost a phone number on the first
+// live run.
+func TestMergingProfilesKeepsWhatLoses(t *testing.T) {
+	src := profileFields{exists: true, fullName: "Asha M", phone: "111", email: "a@b.c"}
+	dst := profileFields{exists: true, fullName: "Asha Menon", phone: "222"}
+
+	out, discarded := mergeProfiles(src, dst)
+
+	if out.fullName != "Asha Menon" || out.phone != "222" {
+		t.Errorf("survivor's own values must win: got %q / %q", out.fullName, out.phone)
+	}
+	if out.email != "a@b.c" {
+		t.Errorf("a blank must be filled from the source, got %q", out.email)
+	}
+	if len(discarded) != 2 {
+		t.Fatalf("discarded %v, want the losing name and phone", discarded)
+	}
+	// In the record, not only in the response: the response is read once.
+	for _, want := range []string{"Asha M", "111"} {
+		if !strings.Contains(out.notes, want) {
+			t.Errorf("notes must retain %q: %q", want, out.notes)
+		}
+	}
+}
+
+// Nothing to reconcile is the ordinary case - a hand-typed record joining a
+// camera record that has no profile at all - and it must add no noise.
+func TestMergingIntoAnEmptyProfileDiscardsNothing(t *testing.T) {
+	src := profileFields{exists: true, fullName: "Asha Menon", phone: "111"}
+	out, discarded := mergeProfiles(src, profileFields{})
+
+	if len(discarded) != 0 {
+		t.Errorf("discarded %v, want none", discarded)
+	}
+	if out.fullName != "Asha Menon" || out.phone != "111" {
+		t.Errorf("the typed details must reach the surviving record: %+v", out)
+	}
+	if out.notes != "" {
+		t.Errorf("no conflict should leave no note, got %q", out.notes)
+	}
+}
+
+// Identical values are not a conflict.
+func TestIdenticalProfileValuesAreNotDiscarded(t *testing.T) {
+	p := profileFields{exists: true, fullName: "Asha Menon", phone: "111"}
+	out, discarded := mergeProfiles(p, p)
+	if len(discarded) != 0 || out.notes != "" {
+		t.Errorf("identical profiles produced %v / notes %q", discarded, out.notes)
+	}
+}
+
+// Two people writing about one customer wrote two different true things.
+func TestNotesAreAdditiveRatherThanAWinner(t *testing.T) {
+	out, _ := mergeProfiles(
+		profileFields{exists: true, notes: "prefers window seat"},
+		profileFields{exists: true, notes: "allergic to nuts"})
+	for _, want := range []string{"window seat", "allergic to nuts"} {
+		if !strings.Contains(out.notes, want) {
+			t.Errorf("notes lost %q: %q", want, out.notes)
+		}
+	}
+}