PROD-2315: handle cross-kind same-name model collision (clear error + attributed cascade)

src/lib/pushers/model-pusher.ts

@@ -20,6 +20,26 @@ function modelTypeMatches(a: mgmtApi.Model, b: mgmtApi.Model): boolean {
   return aType === bType;
 }
 
+/** Human-readable model kind for messages. 1 = content, 2 = component/module. */
+function modelKindName(model: mgmtApi.Model): string {
+  const t = (model as any)?.contentDefinitionTypeID;
+  return t === 1 ? "content" : t === 2 ? "component/module" : `type ${t}`;
+}
+
+/**
+ * PROD-2315: message for a cross-kind reference-name collision — a same-named model exists on both
+ * sides but as different kinds (content vs component/module). Agility forbids content and component
+ * models from sharing a reference name, so this can never be created; the user must reconcile it.
+ */
+function crossKindCollisionMessage(source: mgmtApi.Model, target: mgmtApi.Model): string {
+  return (
+    `Model "${source.referenceName}" is a ${modelKindName(source)} model on the source, but a ` +
+    `${modelKindName(target)} model with that reference name already exists on the target. ` +
+    `Agility does not allow content and component models to share a reference name — ` +
+    `rename one of them (or remove the target model), then re-sync.`
+  );
+}
+
 /**
  * Re-query the target for a model matching (referenceName, contentDefinitionTypeID).
  *
@@ -82,6 +102,8 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp
   let shouldUpdateFields = [];
   let shouldSkip: { model: mgmtApi.Model; reason: string }[] = [];
   let stubCreated = [];
+  // PROD-2315: same reference name on both sides but a different kind (content vs component).
+  const crossKindConflicts: { model: mgmtApi.Model; target: mgmtApi.Model }[] = [];
 
   // PROD-2250: process source models that already have a mapping before brand-new
   // (unmapped) models. Reconciling existing target models first — using their known
@@ -180,6 +202,22 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp
       }
     }
 
+    // PROD-2315: a target model with the same referenceName exists, but as a different KIND
+    // (content vs component/module). modelTypeMatches() already excluded it from
+    // targetModelByReference, so it would otherwise fall through to "create" and hit a guaranteed
+    // server 409 (Agility forbids content + component models sharing a name). Because
+    // targetModelByReference used modelTypeMatches — which returns true when either side's type is
+    // unknown — crossKindTarget is non-null ONLY when both types are known and differ. Type-unknown
+    // pulls are unaffected and keep today's behavior.
+    const crossKindTarget =
+      !sourceMapping && !targetModelByReference
+        ? targetData.find((t) => t.referenceName === sourceModel.referenceName) || null
+        : null;
+    if (crossKindTarget) {
+      crossKindConflicts.push({ model: sourceModel, target: crossKindTarget });
+      continue;
+    }
+
     if (!sourceMapping && !targetModel) {
       shouldCreateStub.push(sourceModel);
       continue;
@@ -239,6 +277,15 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp
         detail: reason,
       });
     }
+    // PROD-2315: preview cross-kind collisions as conflicts (a real run would fail them).
+    for (const { model, target } of crossKindConflicts) {
+      preflightReport.record({
+        phase: "Models",
+        action: "conflict",
+        name: model.referenceName,
+        detail: crossKindCollisionMessage(model, target),
+      });
+    }
     return {
       status: "success",
       successful: shouldCreateStub.length + shouldUpdateFields.length,
@@ -249,14 +296,18 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp
   }
 
   for (const model of shouldCreateStub) {
-    const result = await createNewModel(model, referenceMapper, apiClient, targetGuid[0], logger);
+    const { result, error } = await createNewModel(model, referenceMapper, apiClient, targetGuid[0], logger);
     if (result === "created") {
       stubCreated.push(model);
     } else {
       failed++;
       failureDetails.push({
         name: model.referenceName,
-        error: `Failed to create model "${model.referenceName}" (ID: ${model.id})`,
+        // PROD-2315: surface the server's reason (e.g. the 409 detail) in the ERROR SUMMARY instead
+        // of the generic message; the detail was previously only in the push-log file.
+        error: error
+          ? `Failed to create model "${model.referenceName}" (ID: ${model.id}): ${error}`
+          : `Failed to create model "${model.referenceName}" (ID: ${model.id})`,
         guid: sourceGuid[0],
       });
     }
@@ -294,6 +345,16 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp
     skipped++;
   }
 
+  // PROD-2315: cross-kind reference-name collisions can never be created (Agility forbids content and
+  // component models sharing a name). Fail them with an actionable message that reaches the ERROR
+  // SUMMARY, rather than attempting a create that is guaranteed to 409.
+  for (const { model, target } of crossKindConflicts) {
+    const message = crossKindCollisionMessage(model, target);
+    logger.model.error(model, new Error(message), targetGuid[0]);
+    failed++;
+    failureDetails.push({ name: model.referenceName, error: message, guid: sourceGuid[0] });
+  }
+
   return {
     status: "success",
     successful,
@@ -312,7 +373,7 @@ const createNewModel = async (
   apiClient: mgmtApi.ApiClient,
   targetGuid: string,
   logger: Logs
-): Promise<"created" | "updated" | "skipped" | "failed"> => {
+): Promise<{ result: "created" | "failed"; error?: string }> => {
   try {
     // process the model without fields
     const createPayload = {
@@ -324,7 +385,7 @@ const createNewModel = async (
     const newModel = await apiClient.modelMethods.saveModel(createPayload, targetGuid);
     logger.model.created(model, "created", targetGuid);
     referenceMapper.addMapping(model, newModel);
-    return "created";
+    return { result: "created" };
   } catch (error: any) {
     // PROD-2211: saveModel can reject even though the API created the model server-side. Before
     // declaring failure — which leaves the mapping file without a row and wedges future syncs —
@@ -334,10 +395,11 @@ const createNewModel = async (
     if (recovered) {
       logger.model.created(model, "created", targetGuid);
       referenceMapper.addMapping(model, recovered);
-      return "created";
+      return { result: "created" };
     }
     logger.model.error(model, error, targetGuid);
-    return "failed";
+    // PROD-2315: return the server's message so the caller can surface it in the ERROR SUMMARY.
+    return { result: "failed", error: error?.message ? String(error.message) : undefined };
   }
 };
 

src/lib/pushers/tests/model-pusher.test.ts

@@ -340,9 +340,8 @@ describe("pushModels — exists in target without mapping, non-default (PROD-221
     expect(mapper.getModelMappingByID(501, "source")?.targetID).toBe(10);
   });
 
-  it("does NOT adopt a same-name target model of a different type (type-blind lookup)", async () => {
-    const createdStub = makeModel({ id: 888, referenceName: "PromoBanner", contentDefinitionTypeID: 2 });
-    const saveModel = jest.fn().mockResolvedValue(createdStub);
+  it("does NOT adopt a same-name target model of a different type; flags it as a cross-kind collision (type-blind lookup + PROD-2315)", async () => {
+    const saveModel = jest.fn();
     jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel));
 
     const { pushModels } = await import("../model-pusher");
@@ -352,7 +351,11 @@ describe("pushModels — exists in target without mapping, non-default (PROD-221
 
     const result = await pushModels([sourceModel], [targetContentList]);
 
-    expect(saveModel).toHaveBeenCalled(); // goes through create, not adopted
+    // Not adopted (types differ), and — PROD-2315 — no longer blindly created: recognized as a
+    // forbidden cross-kind reference-name collision and failed with an actionable message rather
+    // than attempting a create that is guaranteed to 409.
+    expect(saveModel).not.toHaveBeenCalled();
+    expect(result.failed).toBe(1);
     expect(result.skipped).toBe(0);
   });
 });
@@ -479,3 +482,101 @@ describe("pushModels — mapped-before-unmapped ordering (PROD-2250)", () => {
     expect(result.successful).toBe(4);
   });
 });
+
+// ─── pushModels — cross-kind reference-name collision (PROD-2315) ──────────────
+
+describe("pushModels — cross-kind reference-name collision (PROD-2315)", () => {
+  it("fails a same-name model of a different kind with an actionable message instead of creating it", async () => {
+    const saveModel = jest.fn();
+    jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel));
+
+    const { pushModels } = await import("../model-pusher");
+
+    // Source LinkCard is a component (type 2); target LinkCard is a content model (type 1).
+    const sourceModel = makeModel({ referenceName: "LinkCard", contentDefinitionTypeID: 2 });
+    const targetModel = makeModel({ id: 500, referenceName: "LinkCard", contentDefinitionTypeID: 1 });
+
+    const result = await pushModels([sourceModel], [targetModel]);
+
+    // Guaranteed-409 create is never attempted…
+    expect(saveModel).not.toHaveBeenCalled();
+    // …and it is reported as a failure with an actionable message.
+    expect(result.failed).toBe(1);
+    expect(result.successful).toBe(0);
+    expect(result.failureDetails).toHaveLength(1);
+    expect(result.failureDetails![0].name).toBe("LinkCard");
+    expect(result.failureDetails![0].error).toMatch(/component\/module model on the source/);
+    expect(result.failureDetails![0].error).toMatch(/content model with that reference name already exists/);
+  });
+
+  it("does NOT flag a same-name model when the kinds are unknown (falls back to adopt-by-reference)", async () => {
+    const saveModel = jest.fn();
+    jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel));
+
+    const { pushModels } = await import("../model-pusher");
+
+    // Neither side has contentDefinitionTypeID → modelTypeMatches() is true → adopt, not a collision.
+    const now = new Date().toISOString();
+    const sourceModel = makeModel({ referenceName: "AmbiguousKind", lastModifiedDate: now });
+    const targetModel = makeModel({ id: 501, referenceName: "AmbiguousKind", lastModifiedDate: now });
+
+    const result = await pushModels([sourceModel], [targetModel]);
+
+    expect(saveModel).not.toHaveBeenCalled(); // adopted, not created
+    expect(result.failed).toBe(0);
+  });
+
+  it("does NOT flag a same-name same-kind model (still adopts by reference, PROD-2211)", async () => {
+    const saveModel = jest.fn();
+    jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel));
+
+    const { pushModels } = await import("../model-pusher");
+
+    const now = new Date().toISOString();
+    const sourceModel = makeModel({ referenceName: "SameKind", contentDefinitionTypeID: 1, lastModifiedDate: now });
+    const targetModel = makeModel({ id: 502, referenceName: "SameKind", contentDefinitionTypeID: 1, lastModifiedDate: now });
+
+    const result = await pushModels([sourceModel], [targetModel]);
+
+    expect(saveModel).not.toHaveBeenCalled();
+    expect(result.failed).toBe(0);
+  });
+
+  it("previews the collision as a conflict under --preflight without calling saveModel", async () => {
+    const saveModel = jest.fn();
+    jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel));
+    state.preflight = true;
+
+    const { pushModels } = await import("../model-pusher");
+
+    const sourceModel = makeModel({ referenceName: "LinkCard", contentDefinitionTypeID: 2 });
+    const targetModel = makeModel({ id: 503, referenceName: "LinkCard", contentDefinitionTypeID: 1 });
+
+    const result = await pushModels([sourceModel], [targetModel]);
+
+    expect(saveModel).not.toHaveBeenCalled();
+    expect(result.status).toBe("success"); // preflight is a dry run
+  });
+});
+
+// ─── pushModels — create failure surfaces the server error (PROD-2315) ─────────
+
+describe("pushModels — create failure surfaces the server error (PROD-2315)", () => {
+  it("includes the server error message in failureDetails when a create fails", async () => {
+    const saveModel = jest
+      .fn()
+      .mockRejectedValue(new Error("A content model with the reference name 'Foo' already exists."));
+    // getContentModules returns [] (default) so findTargetModelAfterSave finds no recovery.
+    jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel));
+
+    const { pushModels } = await import("../model-pusher");
+
+    const sourceModel = makeModel({ referenceName: "Foo" });
+
+    const result = await pushModels([sourceModel], []);
+
+    expect(result.failed).toBe(1);
+    expect(result.failureDetails).toHaveLength(1);
+    expect(result.failureDetails![0].error).toMatch(/A content model with the reference name 'Foo' already exists\./);
+  });
+});