fix(core): race-safe entry upsert and lossless per-user moves

- upsertEntry inserts with ON CONFLICT DO NOTHING and patches through the update path, so a concurrent creator never aborts the caller's transaction on Postgres. - Undoing a move deletes the forward alias and writes no reverse alias. - Merging into an existing entry keeps 'user' field_sources on combined fields, carries the moved entry's field_sources as-is when reviving a deleted target, concatenates differing review/private notes, records a dropped rating in history, and writes history for re-parented engagements (restorable). - Moves re-point this user's copies (with history) and list items to the target work. - rejectSuggestion requires a pending suggestion; engagements of a deleted entry are not editable; restoring an entry does not re-check custom fields against current definitions. Co-Authored-By: Claude Opus 5.5 <[email protected]>
2026-10-30 19:59:54 +01:00
parent 512e6e9464
commit 7154ca0a61
6 changed files with 327 additions and 83 deletions
@@ -67,7 +67,7 @@ function toEngagement(r: Row): Engagement {
 }

 function snapshot(e: Engagement): Record<string, unknown> {
-  const s: Record<string, unknown> = { deletedAt: e.deletedAt };
+  const s: Record<string, unknown> = { entryId: e.entryId, deletedAt: e.deletedAt };
  for (const k of ENGAGEMENT_PATCH_KEYS) s[k] = e[k];
  return s;
 }
@@ -127,7 +127,7 @@ async function load(
    .select(["e.media_type_override", "w.media_type"])
    .where("g.id", "=", engagementId)
    .where("e.user_id", "=", userId);
-  if (!opts.includeDeleted) q = q.where("g.deleted_at", "is", null);
+  if (!opts.includeDeleted) q = q.where("g.deleted_at", "is", null).where("e.deleted_at", "is", null);
  const r = await q.executeTakeFirst();
  if (!r) throw new NotFoundError("engagement");
  return { eg: toEngagement(r), mediaType: r.media_type_override ?? r.media_type };
@@ -140,7 +140,7 @@ async function write(
  mediaType: string,
  patch: Partial<EngagementPatch>,
  actor: Actor,
-  extra: Partial<Pick<Row, "deleted_at">> = {},
+  extra: Partial<Pick<Row, "deleted_at" | "entry_id">> = {},
 ): Promise<Engagement> {
  const cols = await toColumns(k, userId, mediaType, patch);
  const fieldSources = { ...current.fieldSources };
@@ -258,6 +258,18 @@ export async function restoreEngagementFromHistory(
  const { eg, mediaType } = await load(k, userId, engagementId, { includeDeleted: true });
  const patch: Record<string, unknown> = {};
  for (const key of ENGAGEMENT_PATCH_KEYS) if (key in before) patch[key] = before[key];
-  const extra = "deletedAt" in before ? { deleted_at: (before.deletedAt as string | null) ?? null } : {};
+  const extra: Partial<Pick<Row, "deleted_at" | "entry_id">> =
+    "deletedAt" in before ? { deleted_at: (before.deletedAt as string | null) ?? null } : {};
+  if (typeof before.entryId === "string" && before.entryId !== eg.entryId) {
+    // Undo a merge's re-parenting; the old entry must be this user's.
+    const owner = await k
+      .selectFrom("entries")
+      .select("id")
+      .where("id", "=", before.entryId)
+      .where("user_id", "=", userId)
+      .executeTakeFirst();
+    if (!owner) throw new NotFoundError("entry");
+    extra.entry_id = before.entryId;
+  }
  await write(k, userId, eg, mediaType, patch as Partial<EngagementPatch>, "user", extra);
 }
@@ -232,6 +232,7 @@ async function normalisePatch(
  userId: string,
  mediaType: string,
  patch: Partial<EntryPatch>,
+  opts: { trustCustomFields?: boolean } = {},
 ): Promise<Partial<EntryPatch>> {
  checkKeys("patch", patch, ENTRY_PATCH_KEYS);
  const out: Partial<EntryPatch> = {};
@@ -262,10 +263,14 @@ async function normalisePatch(
  }
  if ("overrides" in patch) out.overrides = checkOverrides(patch.overrides);
  if ("customFields" in patch) {
-    const effective = out.mediaTypeOverride ?? mediaType;
-    const defs = await customFieldsFor(k, userId, { mediaType: effective, level: "entry" });
-    validateCustomValues(defs, patch.customFields as Record<string, unknown>);
-    out.customFields = patch.customFields;
+    const cf = patch.customFields;
+    if (cf === null || typeof cf !== "object" || Array.isArray(cf)) throw new ValidationError("customFields", "not_object");
+    if (!opts.trustCustomFields) {
+      const effective = out.mediaTypeOverride ?? mediaType;
+      const defs = await customFieldsFor(k, userId, { mediaType: effective, level: "entry" });
+      validateCustomValues(defs, cf);
+    }
+    out.customFields = cf;
  }
  return out;
 }
@@ -291,29 +296,43 @@ function checkOverrides(o: unknown): EntryOverrides {
  return out;
 }

+interface WriteOpts {
+  /** Non-patch columns. */
+  extra?: Partial<Pick<EntryRow, "work_id" | "deleted_at">>;
+  /** Per-key `field_sources` for patched keys instead of `actor`; `undefined` removes the key. */
+  sources?: Record<string, Actor | undefined>;
+  /** History `before` override: `null` records a creation. */
+  historyBefore?: Record<string, unknown> | null;
+  /** Extra keys merged into the history row's before/after snapshots. */
+  historyExtra?: { before: Record<string, unknown>; after: Record<string, unknown> };
+}
+
 /**
- * Write `cols` to an entry, set `field_sources[key] = actor` for each patched key, and record history.
- * `extra` carries non-patch columns (`work_id`, `deleted_at`).
+ * Write a patch to an entry, set `field_sources[key] = actor` for each patched key (unless
+ * `opts.sources` says otherwise), and record history.
 */
 async function writeEntry(
  k: Executor,
  current: Entry,
  patch: Partial<EntryPatch>,
  actor: Actor,
-  extra: Partial<Pick<EntryRow, "work_id" | "deleted_at">> = {},
-  sources?: FieldSources,
+  opts: WriteOpts = {},
 ): Promise<Entry> {
-  const fieldSources = { ...current.fieldSources };
-  for (const key of Object.keys(patch)) fieldSources[key] = sources?.[key] ?? actor;
-  const cols = { ...patchToColumns(patch), ...extra, field_sources: toJson(fieldSources), updated_at: nowIso() };
+  const fieldSources: Record<string, Actor | undefined> = { ...current.fieldSources };
+  for (const key of Object.keys(patch)) {
+    fieldSources[key] = opts.sources && key in opts.sources ? opts.sources[key] : actor;
+    if (fieldSources[key] === undefined) delete fieldSources[key];
+  }
+  const cols = { ...patchToColumns(patch), ...opts.extra, field_sources: toJson(fieldSources), updated_at: nowIso() };
  await k.updateTable("entries").set(cols).where("id", "=", current.id).execute();
  const after = await loadEntry(k, current.userId, current.id, { includeDeleted: true });
+  const before = opts.historyBefore === undefined ? snapshot(current) : opts.historyBefore;
  await recordHistory(k, {
    userId: current.userId,
    entity: "entry",
    entityId: current.id,
-    before: snapshot(current),
-    after: snapshot(after),
+    before: before === null ? null : { ...before, ...opts.historyExtra?.before },
+    after: { ...snapshot(after), ...opts.historyExtra?.after },
    actor,
  });
  return after;
@@ -372,39 +391,45 @@ export async function upsertEntry(
 ): Promise<Entry> {
  return inTx(db, tx, async (k) => {
    const work = await resolveVisibleWork(k, userId, workId);
-    const existing = await findEntry(k, userId, work.id);
+    let existing = await findEntry(k, userId, work.id);
    const p = await normalisePatch(k, userId, existing?.mediaTypeOverride ?? work.media_type, patch);
-    if (existing) {
-      if (Object.keys(p).length === 0 && existing.deletedAt === null) return existing;
-      return writeEntry(k, existing, p, actor, existing.deletedAt === null ? {} : { deleted_at: null });
+    let created = false;
+    if (!existing) {
+      // Insert-or-ignore, then patch through the update path: a concurrent creator never aborts our
+      // transaction (Postgres poisons a transaction on a unique violation).
+      const now = nowIso();
+      const id = newId();
+      await k
+        .insertInto("entries")
+        .values({
+          id,
+          user_id: userId,
+          work_id: work.id,
+          status: "none",
+          rating: null,
+          review: null,
+          private_notes: null,
+          favorite: 0,
+          tags: toJson([]),
+          custom_fields: toJson({}),
+          visibility: await defaultVisibility(k, userId),
+          overrides: toJson({}),
+          media_type_override: null,
+          field_sources: toJson({}),
+          created_at: now,
+          updated_at: now,
+          deleted_at: null,
+        })
+        .onConflict((oc) => oc.columns(["user_id", "work_id"]).doNothing())
+        .execute();
+      existing = (await findEntry(k, userId, work.id))!;
+      created = existing.id === id;
    }
-    const now = nowIso();
-    const fieldSources: FieldSources = {};
-    for (const key of Object.keys(p)) fieldSources[key] = actor;
-    const row: EntryRow = {
-      id: newId(),
-      user_id: userId,
-      work_id: work.id,
-      status: "none",
-      rating: null,
-      review: null,
-      private_notes: null,
-      favorite: 0,
-      tags: toJson([]),
-      custom_fields: toJson({}),
-      visibility: await defaultVisibility(k, userId),
-      overrides: toJson({}),
-      media_type_override: null,
-      ...patchToColumns(p),
-      field_sources: toJson(fieldSources),
-      created_at: now,
-      updated_at: now,
-      deleted_at: null,
-    };
-    await k.insertInto("entries").values(row).execute();
-    const entry = toEntry(row);
-    await recordHistory(k, { userId, entity: "entry", entityId: entry.id, before: null, after: snapshot(entry), actor });
-    return entry;
+    if (!created && Object.keys(p).length === 0 && existing.deletedAt === null) return existing;
+    return writeEntry(k, existing, p, actor, {
+      extra: existing.deletedAt === null ? {} : { deleted_at: null },
+      historyBefore: created ? null : undefined,
+    });
  });
 }

@@ -418,7 +443,7 @@ export async function softDeleteEntry(
 ): Promise<void> {
  await inTx(db, tx, async (k) => {
    const e = await loadEntry(k, userId, entryId);
-    await writeEntry(k, e, {}, actor, { deleted_at: nowIso() });
+    await writeEntry(k, e, {}, actor, { extra: { deleted_at: nowIso() } });
  });
 }

@@ -433,11 +458,13 @@ export async function restoreEntryFromHistory(
  const patch: Record<string, unknown> = {};
  for (const key of ENTRY_PATCH_KEYS) if (key in before) patch[key] = before[key];
  if (typeof before.workId === "string" && before.workId !== current.workId) {
-    // Undo a move: drop the user's alias and un-tombstone their own private work, then move back.
+    // Undo a move: drop the forward alias and un-tombstone the user's own private work, then move back
+    // without writing a reverse alias (later syncs onto the target must not land on the old work).
    await k
      .deleteFrom("work_aliases")
      .where("user_id", "=", userId)
      .where("from_work_id", "=", before.workId)
+      .where("to_work_id", "=", current.workId)
      .execute();
    await k
      .updateTable("works")
@@ -445,25 +472,27 @@ export async function restoreEntryFromHistory(
      .where("id", "=", before.workId)
      .where("owner_user_id", "=", userId)
      .execute();
-    await moveEntry(k, userId, entryId, before.workId, "user");
+    await moveEntry(k, userId, entryId, before.workId, "user", { undo: true });
  }
  const fresh = await loadEntry(k, userId, entryId, { includeDeleted: true });
  const work = await k.selectFrom("works").select("media_type").where("id", "=", fresh.workId).executeTakeFirstOrThrow();
-  const p = await normalisePatch(k, userId, fresh.mediaTypeOverride ?? work.media_type, patch as Partial<EntryPatch>);
+  // The user's own past data: custom values are not re-checked against today's definitions.
+  const p = await normalisePatch(k, userId, fresh.mediaTypeOverride ?? work.media_type, patch as Partial<EntryPatch>, {
+    trustCustomFields: true,
+  });
  const extra = "deletedAt" in before ? { deleted_at: (before.deletedAt as string | null) ?? null } : {};
-  await writeEntry(k, fresh, p, "user", extra);
+  await writeEntry(k, fresh, p, "user", { extra });
 }

 const isEmpty = (v: unknown) =>
-  v === null || v === false || v === "none" || (Array.isArray(v) && v.length === 0) ||
+  v === null || v === false || v === "none" || v === "" || (Array.isArray(v) && v.length === 0) ||
  (typeof v === "object" && v !== null && Object.keys(v).length === 0);

+/** Separator used when two entries' review or private notes are combined by a merge. */
+export const MERGE_TEXT_SEPARATOR = "\n\n---\n\n";
+
 /**
- * Move the user's entry to another work (per-user merge, spec §6.1). Leaves a `work_aliases` row so
- * old references resolve, and tombstones the user's own private draft work into the target.
- * If the user already has an entry on the target, the two merge: the target's values win, gaps are
- * filled from the moved entry, tags/custom fields/overrides are combined, engagements are re-parented,
- * and the moved entry is soft-deleted. (A soft-deleted target entry is revived with the moved values.)
+ * Move the user's entry to another work (per-user merge, spec §6.1). See `moveEntry`.
 * Returns the surviving entry.
 */
 export async function moveEntryToWork(
@@ -477,7 +506,30 @@ export async function moveEntryToWork(
  return inTx(db, tx, (k) => moveEntry(k, userId, entryId, targetWorkId, actor));
 }

-async function moveEntry(k: Executor, userId: string, entryId: string, targetWorkId: string, actor: Actor): Promise<Entry> {
+/** The field_sources value for a combined field: "user" if either side was the user's, else the destination's. */
+function combinedSource(src: Entry, dst: Entry, key: string): Actor | undefined {
+  return src.fieldSources[key] === "user" || dst.fieldSources[key] === "user" ? "user" : dst.fieldSources[key];
+}
+
+/**
+ * Move an entry to `targetWorkId`.
+ * - No entry on the target: the entry's `work_id` changes.
+ * - A live entry on the target: merge into it. Its values win; empty fields are filled from the moved
+ *   entry; tags/custom fields/overrides are combined; differing review/private notes are concatenated;
+ *   a conflicting rating is kept but the moved rating is recorded in the history row's `before`.
+ *   Engagements are re-parented (with history) and the moved entry is soft-deleted.
+ * - A soft-deleted entry on the target: revived with the moved entry's values and field_sources.
+ * Then (unless `undo`): this user's copies and list items follow to the target, a `work_aliases` row
+ * maps the old work to the target, and the user's own private draft is tombstoned into the target.
+ */
+async function moveEntry(
+  k: Executor,
+  userId: string,
+  entryId: string,
+  targetWorkId: string,
+  actor: Actor,
+  opts: { undo?: boolean } = {},
+): Promise<Entry> {
  const src = await loadEntry(k, userId, entryId);
  const target = await resolveVisibleWork(k, userId, targetWorkId);
  if (target.id === src.workId) throw new ValidationError("targetWorkId", "same_work");
@@ -485,31 +537,51 @@ async function moveEntry(k: Executor, userId: string, entryId: string, targetWor

  let result: Entry;
  if (!dst) {
-    result = await writeEntry(k, src, {}, actor, { work_id: target.id });
+    result = await writeEntry(k, src, {}, actor, { extra: { work_id: target.id } });
+  } else if (dst.deletedAt !== null) {
+    const all: Partial<EntryPatch> = {};
+    for (const key of ENTRY_PATCH_KEYS) (all as Record<string, unknown>)[key] = src[key];
+    const sources: Record<string, Actor | undefined> = {};
+    for (const key of ENTRY_PATCH_KEYS) sources[key] = src.fieldSources[key];
+    result = await writeEntry(k, dst, all, actor, { extra: { deleted_at: null }, sources });
+    await moveEngagements(k, userId, src.id, dst.id, actor);
+    await writeEntry(k, src, {}, actor, { extra: { deleted_at: nowIso() } });
  } else {
-    const merged: Partial<EntryPatch> = {};
-    const sources: FieldSources = {};
-    const take = <K extends keyof EntryPatch>(key: K, v: EntryPatch[K]) => {
-      merged[key] = v;
-      if (src.fieldSources[key]) sources[key] = src.fieldSources[key];
-    };
+    const merged: Record<string, unknown> = {};
+    const sources: Record<string, Actor | undefined> = {};
+    let historyExtra: WriteOpts["historyExtra"];
    for (const key of ENTRY_PATCH_KEYS) {
-      if (dst.deletedAt !== null) {
-        take(key, src[key]);
-      } else if (key === "tags") {
+      const s = src[key];
+      const d = dst[key];
+      if (key === "tags") {
        const tags = [...new Set([...dst.tags, ...src.tags])];
-        if (tags.length !== dst.tags.length) take(key, tags);
+        if (tags.length !== dst.tags.length) {
+          merged[key] = tags;
+          sources[key] = combinedSource(src, dst, key);
+        }
      } else if (key === "customFields" || key === "overrides") {
-        const combined = { ...src[key], ...dst[key] };
-        if (Object.keys(combined).length !== Object.keys(dst[key]).length) take(key, combined as never);
-      } else if (isEmpty(dst[key]) && !isEmpty(src[key])) {
-        take(key, src[key]);
+        const combined = { ...(s as object), ...(d as object) };
+        if (Object.keys(combined).length !== Object.keys(d as object).length) {
+          merged[key] = combined;
+          sources[key] = combinedSource(src, dst, key);
+        }
+      } else if (isEmpty(d) && !isEmpty(s)) {
+        merged[key] = s;
+        sources[key] = src.fieldSources[key];
+      } else if ((key === "review" || key === "privateNotes") && !isEmpty(s) && s !== d) {
+        merged[key] = `${d as string}${MERGE_TEXT_SEPARATOR}${s as string}`;
+        sources[key] = combinedSource(src, dst, key);
+      } else if (key === "rating" && s !== null && s !== d) {
+        historyExtra = { before: { rating: s }, after: { rating: d } };
      }
    }
-    result = await writeEntry(k, dst, merged, actor, dst.deletedAt === null ? {} : { deleted_at: null }, sources);
-    await k.updateTable("engagements").set({ entry_id: dst.id }).where("entry_id", "=", src.id).execute();
-    await writeEntry(k, src, {}, actor, { deleted_at: nowIso() });
+    result = await writeEntry(k, dst, merged as Partial<EntryPatch>, actor, { sources, historyExtra });
+    await moveEngagements(k, userId, src.id, dst.id, actor);
+    await writeEntry(k, src, {}, actor, { extra: { deleted_at: nowIso() } });
  }
+  if (opts.undo) return result;
+
+  await repointCopiesAndListItems(k, userId, src.workId, target.id, actor);

  // Aliases: re-point any that led to the old work, drop one leaving the target (no cycles), add ours.
  await k
@@ -541,7 +613,49 @@ async function moveEntry(k: Executor, userId: string, entryId: string, targetWor
    .where("owner_user_id", "=", userId)
    .execute();
  return result;
+}

+/** Re-parent an entry's live and deleted engagements, writing an `entryId` history row for each. */
+async function moveEngagements(k: Executor, userId: string, fromEntryId: string, toEntryId: string, actor: Actor) {
+  const rows = await k.selectFrom("engagements").select("id").where("entry_id", "=", fromEntryId).execute();
+  for (const { id } of rows) {
+    await k.updateTable("engagements").set({ entry_id: toEntryId, updated_at: nowIso() }).where("id", "=", id).execute();
+    await recordHistory(k, {
+      userId,
+      entity: "engagement",
+      entityId: id,
+      before: { entryId: fromEntryId },
+      after: { entryId: toEntryId },
+      actor,
+    });
+  }
+}
+
+/** Point this user's copies (with history) and the items of this user's lists at the new work. */
+async function repointCopiesAndListItems(k: Executor, userId: string, fromWorkId: string, toWorkId: string, actor: Actor) {
+  const copies = await k
+    .selectFrom("copies")
+    .select("id")
+    .where("user_id", "=", userId)
+    .where("work_id", "=", fromWorkId)
+    .execute();
+  for (const { id } of copies) {
+    await k.updateTable("copies").set({ work_id: toWorkId, updated_at: nowIso() }).where("id", "=", id).execute();
+    await recordHistory(k, {
+      userId,
+      entity: "copy",
+      entityId: id,
+      before: { workId: fromWorkId },
+      after: { workId: toWorkId },
+      actor,
+    });
+  }
+  await k
+    .updateTable("list_items")
+    .set({ work_id: toWorkId })
+    .where("work_id", "=", fromWorkId)
+    .where("list_id", "in", k.selectFrom("lists").select("id").where("user_id", "=", userId))
+    .execute();
 }

 /** Insert a private draft work owned by the user (no external IDs). Returns its id. */
@@ -832,5 +946,6 @@ export async function acceptSuggestion(db: Db, userId: string, suggestionId: str
 /** Reject a match suggestion. */
 export async function rejectSuggestion(db: Db, userId: string, suggestionId: string): Promise<void> {
  const s = await loadSuggestion(db.k, userId, suggestionId);
+  if (s.status !== "pending") throw new ValidationError("status", "not_pending");
  await db.k.updateTable("match_suggestions").set({ status: "rejected" }).where("id", "=", s.id).execute();
 }
@@ -7,7 +7,7 @@ import {
  softDeleteEngagement,
  updateEngagement,
 } from "../src/engagements.ts";
-import { upsertEntry } from "../src/entries.ts";
+import { softDeleteEntry, upsertEntry } from "../src/entries.ts";
 import { NotFoundError, ValidationError } from "../src/errors.ts";
 import { getHistory, restoreVersion } from "../src/history.ts";
 import { makeUser, makeWork } from "./user-layer-fixtures.ts";
@@ -114,6 +114,14 @@ forEachDialect("engagements", (getDb) => {
    expect((await listEngagements(db, userId, entry.id)).map((e) => e.id)).toEqual([early.id, later.id, undated.id]);
  });

+  test("engagements of a soft-deleted entry cannot be changed", async () => {
+    const { db, userId, entry } = await setup();
+    const eg = await addEngagement(db, userId, entry.id, {}, "user");
+    await softDeleteEntry(db, userId, entry.id, "user");
+    await expect(updateEngagement(db, userId, eg.id, { rating: 1 }, "user")).rejects.toThrow(NotFoundError);
+    await expect(softDeleteEngagement(db, userId, eg.id, "user")).rejects.toThrow(NotFoundError);
+  });
+
  test("another user's engagements are not found", async () => {
    const { db, userId, entry } = await setup();
    const bob = await makeUser(db, "bob");
@@ -2,6 +2,7 @@ import { expect, test } from "bun:test";
 import { newId } from "@sttracker/db/ids";
 import { forEachDialect } from "@sttracker/db/testing";
 import { defineCustomField } from "../src/custom-fields.ts";
+import { addEngagement, listEngagements } from "../src/engagements.ts";
 import { NotFoundError, ValidationError } from "../src/errors.ts";
 import {
  getEntry,
@@ -179,6 +180,23 @@ forEachDialect("entries", (getDb) => {
    expect([...seen].sort()).toEqual([...titles].sort());
  });

+  test("concurrent upserts of a new entry yield one entry with both patches", async () => {
+    const db = getDb();
+    const userId = await makeUser(db);
+    const workId = await makeWork(db);
+    const [a, b] = await Promise.all([
+      upsertEntry(db, userId, workId, { rating: 70 }, "plugin:trakt:c1"),
+      upsertEntry(db, userId, workId, { tags: ["cosy"] }, "user"),
+    ]);
+    expect(a.id).toBe(b.id);
+    const rows = await db.k.selectFrom("entries").selectAll().where("user_id", "=", userId).execute();
+    expect(rows).toHaveLength(1);
+    const view = await getEntry(db, userId, a.id);
+    expect(view.rating).toBe(70);
+    expect(view.tags).toEqual(["cosy"]);
+    expect(view.fieldSources).toEqual({ rating: "plugin:trakt:c1", tags: "user" });
+  });
+
  test("listEntries filters, sorts and paginates", async () => {
    const db = getDb();
    const userId = await makeUser(db);
@@ -302,4 +320,58 @@ forEachDialect("entries", (getDb) => {
    expect(merged.tags.sort()).toEqual(["x", "y"]);
    await expect(getEntry(db, userId, src.id)).rejects.toThrow(NotFoundError);
  });
+
+  test("merge keeps 'user' field_sources on combined fields", async () => {
+    const db = getDb();
+    const userId = await makeUser(db);
+    const a = await makeWork(db, { title: "A" });
+    const b = await makeWork(db, { title: "B" });
+    const src = await upsertEntry(db, userId, a, { tags: ["x"], overrides: { year: 1 } }, "plugin:trakt:c1");
+    await upsertEntry(db, userId, b, { tags: ["y"], status: "completed" }, "user");
+    await upsertEntry(db, userId, b, { overrides: { title: "B!" } }, "plugin:hardcover:c2");
+    const merged = await moveEntryToWork(db, userId, src.id, b, "plugin:steam:c3");
+    expect(merged.tags.sort()).toEqual(["x", "y"]);
+    expect(merged.overrides).toEqual({ year: 1, title: "B!" });
+    expect(merged.fieldSources).toEqual({ tags: "user", status: "user", overrides: "plugin:hardcover:c2" });
+  });
+
+  test("merge into a soft-deleted entry carries the moved field_sources as-is", async () => {
+    const db = getDb();
+    const userId = await makeUser(db);
+    const a = await makeWork(db, { title: "A" });
+    const b = await makeWork(db, { title: "B" });
+    const src = await upsertEntry(db, userId, a, { rating: 60, review: "mine" }, "user");
+    const dst = await upsertEntry(db, userId, b, { status: "dropped", tags: ["old"] }, "plugin:trakt:c1");
+    await softDeleteEntry(db, userId, dst.id, "user");
+    const merged = await moveEntryToWork(db, userId, src.id, b, "plugin:trakt:c2");
+    expect(merged.id).toBe(dst.id);
+    expect(merged.deletedAt).toBeNull();
+    expect(merged.rating).toBe(60);
+    expect(merged.status).toBe("none");
+    expect(merged.tags).toEqual([]);
+    expect(merged.fieldSources).toEqual({ rating: "user", review: "user" });
+  });
+
+  test("merge concatenates differing text, records the dropped rating and moves engagements with history", async () => {
+    const db = getDb();
+    const userId = await makeUser(db);
+    const a = await makeWork(db, { title: "A" });
+    const b = await makeWork(db, { title: "B" });
+    const src = await upsertEntry(db, userId, a, { review: "from A", privateNotes: "same", rating: 40 }, "user");
+    const dst = await upsertEntry(db, userId, b, { review: "from B", privateNotes: "same", rating: 90 }, "user");
+    const eg = await addEngagement(db, userId, src.id, { platform: "Kindle" }, "user");
+    const merged = await moveEntryToWork(db, userId, src.id, b, "user");
+    expect(merged.review).toBe("from B\n\n---\n\nfrom A");
+    expect(merged.privateNotes).toBe("same");
+    expect(merged.rating).toBe(90);
+    const [h] = await getHistory(db, userId, "entry", dst.id);
+    expect(h!.before.rating).toBe(40);
+    expect(h!.after.rating).toBe(90);
+    expect(h!.before.review).toBe("from B");
+
+    expect((await listEngagements(db, userId, dst.id)).map((e) => e.id)).toEqual([eg.id]);
+    const [egh] = await getHistory(db, userId, "engagement", eg.id);
+    expect(egh!.before).toEqual({ entryId: src.id });
+    expect(egh!.after).toEqual({ entryId: dst.id });
+  });
 });
@@ -1,5 +1,6 @@
 import { expect, test } from "bun:test";
 import { forEachDialect } from "@sttracker/db/testing";
+import { defineCustomField } from "../src/custom-fields.ts";
 import { NotFoundError } from "../src/errors.ts";
 import { getEntry, moveEntryToWork, quickAdd, softDeleteEntry, upsertEntry } from "../src/entries.ts";
 import { getHistory, recordHistory, restoreVersion } from "../src/history.ts";
@@ -80,9 +81,25 @@ forEachDialect("history", (getDb) => {
    expect(back.draft).toBe(true);
    const work = await db.k.selectFrom("works").selectAll().where("id", "=", draft.workId).executeTakeFirstOrThrow();
    expect(work.merged_into_id).toBeNull();
-    // the shared work now aliases back to the draft for this user; no alias cycle remains
-    const aliases = await db.k.selectFrom("work_aliases").selectAll().where("user_id", "=", userId).execute();
-    expect(aliases.map((a) => [a.from_work_id, a.to_work_id])).toEqual([[shared, draft.workId]]);
+    // undoing removes the forward alias and writes no reverse alias
+    expect(await db.k.selectFrom("work_aliases").selectAll().where("user_id", "=", userId).execute()).toEqual([]);
+    // a later sync onto the shared work lands on the shared work, not on the draft
+    const synced = await upsertEntry(db, userId, shared, { rating: 80 }, "plugin:hardcover:c1");
+    expect(synced.workId).toBe(shared);
+    expect(synced.id).not.toBe(draft.id);
+    expect((await getEntry(db, userId, draft.id)).rating).toBeNull();
+  });
+
+  test("restoreVersion does not re-check custom fields against today's definitions", async () => {
+    const db = getDb();
+    const userId = await makeUser(db);
+    const def = await defineCustomField(db, userId, { name: "Mood", type: "text" });
+    const e = await upsertEntry(db, userId, await makeWork(db), { customFields: { [def.id]: "cosy" } }, "user");
+    await upsertEntry(db, userId, e.workId, { customFields: {} }, "user");
+    await db.k.deleteFrom("custom_field_definitions").where("id", "=", def.id).execute();
+    const [h] = await getHistory(db, userId, "entry", e.id);
+    await restoreVersion(db, userId, h!.id);
+    expect((await getEntry(db, userId, e.id)).customFields).toEqual({ [def.id]: "cosy" });
  });

  test("another user's history is invisible", async () => {
@@ -3,7 +3,10 @@ import type { Db } from "@sttracker/db/db";
 import { newId, nowIso } from "@sttracker/db/ids";
 import { forEachDialect } from "@sttracker/db/testing";
 import { acceptSuggestion, getEntry, quickAdd, rejectSuggestion } from "../src/entries.ts";
-import { NotFoundError } from "../src/errors.ts";
+import { createCopy, listCopies } from "../src/copies.ts";
+import { NotFoundError, ValidationError } from "../src/errors.ts";
+import { getHistory } from "../src/history.ts";
+import { addListItem, createList, getList } from "../src/lists.ts";
 import { makeUser, makeWork } from "./user-layer-fixtures.ts";

 async function suggest(db: Db, userId: string, workId: string, candidateWorkId: string): Promise<string> {
@@ -46,5 +49,22 @@ forEachDialect("suggestions", (getDb) => {
    expect(await statusOf(db, s)).toBe("pending");
    await rejectSuggestion(db, ada, s);
    expect(await statusOf(db, s)).toBe("rejected");
+    await expect(rejectSuggestion(db, ada, s)).rejects.toThrow(ValidationError);
+    await expect(acceptSuggestion(db, ada, s)).rejects.toThrow(ValidationError);
+  });
+
+  test("accepting moves this user's copies and list items to the target work", async () => {
+    const db = getDb();
+    const userId = await makeUser(db);
+    const draft = await quickAdd(db, userId, { title: "Dune??", mediaType: "book" });
+    const target = await makeWork(db, { title: "Dune", externalIds: { isbn13: "1" } });
+    const copy = await createCopy(db, userId, { workId: draft.workId, format: "paperback" });
+    const list = await createList(db, userId, { name: "Desert" });
+    await addListItem(db, userId, list.id, { workId: draft.workId });
+    await acceptSuggestion(db, userId, await suggest(db, userId, draft.workId, target));
+    expect((await listCopies(db, userId, { workId: target })).map((c) => c.id)).toEqual([copy.id]);
+    const [ch] = await getHistory(db, userId, "copy", copy.id);
+    expect(ch!.after).toEqual({ workId: target });
+    expect((await getList(db, userId, list.id)).items.map((i) => i.work.title)).toEqual(["Dune"]);
  });
 });