From bab04cbabee2e95fb1004fcea9184b440a9aa6c1 Mon Sep 17 00:00:00 2001 From: Jules Exel Date: Wed, 22 Jul 2026 16:23:43 -0400 Subject: [PATCH 1/2] PROD-1492: halt sync on model/template mapping-validation errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related fixes so a mapping-inconsistency guard actually stops the sync instead of being swallowed and silently continuing. 1. Template validation was swallowed. The guid-level orchestrator only re-threw errors containing "Model validation failed", but the template guard throws "Page template validation failed" — so it fell through to a yellow warning and the sync kept going. Broaden the re-throw match to any "validation failed" so template (and future) guards hard-stop. 2. Duplicate model reference-name was undetected. When a source model is deleted and recreated (new ID, same referenceName), the mapping keeps two records sharing that name — one pointing at the dead source model. The content-side lookup is first-match-wins, so it can latch onto the dead record and SILENTLY skip that model's content, after which pages referencing it fail with "No content mapping". The ID-based rename guard misses this (different source IDs = duplicate name, not duplicate ID). Add ModelMapper.getDuplicateSourceReferenceNames() and a pre-push integrity gate in pushModels that throws "Model validation failed: duplicate model mapping ..." before any writes. Tests: new orchestrate-pushers test (template failure aborts before content/pages) and new model-pusher test (duplicate-name mapping throws and writes nothing). Added per-test mapping-file cleanup so seeded mappings no longer leak between model-pusher tests. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/lib/mappers/model-mapper.ts | 40 ++++++++++++++++ src/lib/pushers/model-pusher.ts | 23 +++++++++ src/lib/pushers/orchestrate-pushers.ts | 6 ++- src/lib/pushers/tests/model-pusher.test.ts | 48 +++++++++++++++++++ .../pushers/tests/orchestrate-pushers.test.ts | 27 +++++++++++ 5 files changed, 142 insertions(+), 2 deletions(-) diff --git a/src/lib/mappers/model-mapper.ts b/src/lib/mappers/model-mapper.ts index 9fff61f1..636d5fa3 100644 --- a/src/lib/mappers/model-mapper.ts +++ b/src/lib/mappers/model-mapper.ts @@ -57,6 +57,46 @@ export class ModelMapper { return mapping; } + /** + * PROD-1492: detect stale duplicate model mappings — two or more records that share a source + * `referenceName` but point at different source IDs. This happens when a source model is deleted + * and recreated (new ID, same reference name): the dead record lingers in the mapping alongside + * the live one. Because the content-side lookup (`getModelMappingByReferenceName`) is first-match- + * wins, it can latch onto the dead record, fail to read its (deleted) source model, and silently + * skip that model's content — after which every page referencing it fails. An Agility instance + * cannot have two models with the same reference name, so any such duplicate in the mapping is + * corruption, never a legitimate state. + * + * Returns one entry per offending reference name with its conflicting source IDs; empty when clean. + */ + getDuplicateSourceReferenceNames(): { referenceName: string; sourceIDs: number[] }[] { + // Group by lowercased reference name using a plain object + arrays (no Map/Set iteration, + // which would need tsconfig `downlevelIteration` this project doesn't enable). + const groups: Record = {}; + for (const m of this.mappings) { + if (!m.sourceReferenceName) continue; + const key = m.sourceReferenceName.toLowerCase(); + if (!groups[key]) { + groups[key] = { referenceName: m.sourceReferenceName, sourceIDs: [] }; + } + if (groups[key].sourceIDs.indexOf(m.sourceID) === -1) { + groups[key].sourceIDs.push(m.sourceID); + } + } + + const duplicates: { referenceName: string; sourceIDs: number[] }[] = []; + for (const key of Object.keys(groups)) { + const entry = groups[key]; + if (entry.sourceIDs.length > 1) { + duplicates.push({ + referenceName: entry.referenceName, + sourceIDs: entry.sourceIDs.slice().sort((a, b) => a - b), + }); + } + } + return duplicates; + } + getMappedEntity(mapping: ModelMapping, type: "source" | "target"): mgmtApi.Model | null { if (!mapping) return null; //fetch the model from the file system based on source or target GUID diff --git a/src/lib/pushers/model-pusher.ts b/src/lib/pushers/model-pusher.ts index b71f29c8..149cddf3 100644 --- a/src/lib/pushers/model-pusher.ts +++ b/src/lib/pushers/model-pusher.ts @@ -91,6 +91,29 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp const referenceMapper = new ModelMapper(sourceGuid[0], targetGuid[0]); + // PROD-1492: fail fast on stale duplicate model mappings. When a source model is deleted and + // recreated (new ID, same reference name), the mapping ends up with two records sharing that name + // — one pointing at the dead source model, one at the live one. The content-side lookup is + // first-match-wins, so it can latch onto the dead record and SILENTLY skip that model's content + // (no per-item log), after which every page referencing it fails with "No content mapping". + // The ID-based rename guard below misses this because the two records have DIFFERENT source IDs + // (a duplicate reference name, not a duplicate ID). Throw a "Model validation failed" error so the + // orchestrator halts the sync before any partial push, rather than dropping content unnoticed. + const duplicateMappings = referenceMapper.getDuplicateSourceReferenceNames(); + if (duplicateMappings.length > 0) { + const detail = duplicateMappings + .map((d) => `"${d.referenceName}" (source IDs: ${d.sourceIDs.join(", ")})`) + .join("; "); + throw new Error( + `Model validation failed: duplicate model mapping detected for ${detail}. ` + + `Two mapping records share a reference name but point at different source model IDs — ` + + `this indicates a model that was deleted and recreated on the source, leaving a stale mapping. ` + + `Continuing would silently skip that model's content and fail the pages that reference it. ` + + `Stopping sync to avoid a partial push; remove the stale target model and re-sync. ` + + `Please contact AgilityCMS Support to resolve this issue` + ); + } + const apiClient = getApiClient(); let successful = 0; diff --git a/src/lib/pushers/orchestrate-pushers.ts b/src/lib/pushers/orchestrate-pushers.ts index 3bb6400b..6475172b 100644 --- a/src/lib/pushers/orchestrate-pushers.ts +++ b/src/lib/pushers/orchestrate-pushers.ts @@ -247,8 +247,10 @@ export class Pushers { } } } catch (error: any) { - // Re-throw validation errors immediately to stop sync - if (error?.message?.includes("Model validation failed")) { + // Re-throw validation errors immediately to stop sync. + // Matches any " validation failed" halt (e.g. "Model validation failed", + // "Page template validation failed") so every guard stops a partial push. + if (error?.message?.includes("validation failed")) { throw error; } // For other errors, log but don't stop (legacy behavior for guid-level ops) diff --git a/src/lib/pushers/tests/model-pusher.test.ts b/src/lib/pushers/tests/model-pusher.test.ts index ad0b069a..51e8de56 100644 --- a/src/lib/pushers/tests/model-pusher.test.ts +++ b/src/lib/pushers/tests/model-pusher.test.ts @@ -17,6 +17,11 @@ afterAll(() => { beforeEach(() => { resetState(); setState({ rootPath: tmpDir, sourceGuid: "src-model-u", targetGuid: "tgt-model-u", token: "test-token" }); + // Mapping files are stored centrally at /mappings and would otherwise ACCUMULATE across + // tests (shared tmpDir + fixed guids), leaking one test's seeded mappings into the next. Clear them + // so each test starts from an empty mapping — required now that pushModels validates the whole + // mapping for duplicate reference names (PROD-1492). + fs.rmSync(path.join(tmpDir, "mappings"), { recursive: true, force: true }); initializeGuidLogger("src-model-u", "push"); jest.spyOn(console, "log").mockImplementation(() => {}); jest.spyOn(console, "warn").mockImplementation(() => {}); @@ -204,6 +209,49 @@ describe("pushModels — source-side rename orphans a mapping and halts the sync }); }); +// ─── pushModels — duplicate reference-name mapping halts the sync (PROD-1492) ────── + +describe("pushModels — deleted-and-recreated model leaves a duplicate mapping and halts the sync (PROD-1492)", () => { + it('throws "Model validation failed" (and writes nothing) when two mapping records share a reference name with different source IDs', async () => { + const { ModelMapper } = await import("lib/mappers/model-mapper"); + + // Seed the mapping exactly as it looks after a deleted-and-recreated source model: + // dead source 46 -> target 138 and live source 48 -> target 139, both named "PromoBanner". + const seeder = new ModelMapper(state.sourceGuid[0], state.targetGuid[0]); + seeder.addMapping( + { id: 46, referenceName: "PromoBanner", lastModifiedDate: new Date(2025, 0, 1).toISOString() } as any, + { id: 138, referenceName: "PromoBanner", lastModifiedDate: new Date(2025, 0, 1).toISOString() } as any + ); + seeder.addMapping( + { id: 48, referenceName: "PromoBanner", lastModifiedDate: new Date(2025, 5, 1).toISOString() } as any, + { id: 139, referenceName: "PromoBanner", lastModifiedDate: new Date(2025, 5, 1).toISOString() } as any + ); + + const saveModel = jest.fn().mockResolvedValue(makeModel({ id: 999 })); + jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel)); + + const { pushModels } = await import("../model-pusher"); + + // Only the live model (48) still exists on the source; the dead record (46) is the stale duplicate. + const liveModel = makeModel({ + id: 48, + referenceName: "PromoBanner", + lastModifiedDate: new Date(2025, 5, 1).toISOString(), + }); + const targetModel = makeModel({ + id: 139, + referenceName: "PromoBanner", + lastModifiedDate: new Date(2025, 5, 1).toISOString(), + }); + + // The integrity gate must detect the duplicate reference name and stop the whole sync with a + // "Model validation failed" error — before any model is written. + await expect(pushModels([liveModel], [targetModel])).rejects.toThrow(/Model validation failed/); + + expect(saveModel).not.toHaveBeenCalled(); + }); +}); + // ─── PROD-2211: honest failure reporting ────────────────────────────────────── describe("pushModels — failed update is reported as failed, not success (PROD-2211)", () => { diff --git a/src/lib/pushers/tests/orchestrate-pushers.test.ts b/src/lib/pushers/tests/orchestrate-pushers.test.ts index f8bfe00a..8ef03235 100644 --- a/src/lib/pushers/tests/orchestrate-pushers.test.ts +++ b/src/lib/pushers/tests/orchestrate-pushers.test.ts @@ -360,4 +360,31 @@ describe("Pushers.instanceOrchestrator — models-first ordering (PROD-2202)", ( }), ]); }); + + it("a template-validation failure aborts the sync before content or pages are pushed (PROD-1492)", async () => { + setState({ sourceGuid: "src-u", targetGuid: "tgt-u", locales: "en-us" }); + await stubDataLoader(); + const spies = await stubAllHandlers(); + + // Templates run as a guid-level op; a mapping inconsistency must halt the sync just like models. + spies["pushTemplates"].mockRejectedValue( + new Error('Page template validation failed: mapping inconsistency for template "LeftSideBarTemplate" (ID: 2).') + ); + + const pushers = new Pushers(); + const results = await pushers.instanceOrchestrator(); + + // The templates handler ran and threw; content/pages in the locale loop were never reached. + expect(spies["pushTemplates"]).toHaveBeenCalledTimes(1); + expect(spies["pushContent"]).not.toHaveBeenCalled(); + expect(spies["pushPages"]).not.toHaveBeenCalled(); + + // The failure is recorded on the guid orchestration result and carries the validation message. + expect(results[0].failed).toEqual([ + expect.objectContaining({ + operation: "guid-orchestration", + error: expect.stringContaining("Page template validation failed"), + }), + ]); + }); }); From 248ebd38c840cd5e897cce740a0dfa848ef7532f Mon Sep 17 00:00:00 2001 From: Jules Exel Date: Thu, 23 Jul 2026 15:34:48 -0400 Subject: [PATCH 2/2] PROD-1492: only halt on duplicate model mappings when a source ID is still live The duplicate-reference-name gate added in #191 halted the sync on ANY mapping that had two records sharing a source referenceName. That over-fires on a model deleted OUTRIGHT: both records point at source models that no longer exist (e.g. the case-only "ChangeLog"/"Changelog" pair, source IDs 110 & 117, both absent from the pull). There is no content of that type in the push and nothing references it, so halting is a false positive that blocks the whole sync over stale mapping residue. Scope the gate to duplicates where at least one conflicting source ID is still live in the current pull. That preserves the PROD-1492 fix (dead + live record, e.g. PromoBanner 46-dead / 48-live, still hard-stops the sync) while ignoring fully-dead residue silently. Tests: model-pusher.test.ts now covers both the live-vs-dead halt and the fully-dead no-halt path. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/lib/pushers/model-pusher.ts | 13 ++++++- src/lib/pushers/tests/model-pusher.test.ts | 43 ++++++++++++++++++++-- 2 files changed, 51 insertions(+), 5 deletions(-) diff --git a/src/lib/pushers/model-pusher.ts b/src/lib/pushers/model-pusher.ts index 149cddf3..6bffe869 100644 --- a/src/lib/pushers/model-pusher.ts +++ b/src/lib/pushers/model-pusher.ts @@ -99,7 +99,18 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp // The ID-based rename guard below misses this because the two records have DIFFERENT source IDs // (a duplicate reference name, not a duplicate ID). Throw a "Model validation failed" error so the // orchestrator halts the sync before any partial push, rather than dropping content unnoticed. - const duplicateMappings = referenceMapper.getDuplicateSourceReferenceNames(); + // + // Scope the gate to duplicates that still matter: only halt when at least one of the conflicting + // source IDs is still LIVE in this pull (i.e. present in `models`) — that is the PROD-1492 case + // (dead record + live record, e.g. PromoBanner 46-dead / 48-live) where content would be dropped. + // If EVERY record for a reference name points at a source model that no longer exists, the model + // was deleted outright: there is no content of that type in the push and nothing references it, so + // halting would be a false positive that blocks the whole sync over stale mapping residue. Ignore + // those silently and continue. + const liveSourceIDs = new Set(models.map((m) => m.id)); + const duplicateMappings = referenceMapper + .getDuplicateSourceReferenceNames() + .filter((d) => d.sourceIDs.some((id) => liveSourceIDs.has(id))); if (duplicateMappings.length > 0) { const detail = duplicateMappings .map((d) => `"${d.referenceName}" (source IDs: ${d.sourceIDs.join(", ")})`) diff --git a/src/lib/pushers/tests/model-pusher.test.ts b/src/lib/pushers/tests/model-pusher.test.ts index 51e8de56..c4f62261 100644 --- a/src/lib/pushers/tests/model-pusher.test.ts +++ b/src/lib/pushers/tests/model-pusher.test.ts @@ -212,11 +212,11 @@ describe("pushModels — source-side rename orphans a mapping and halts the sync // ─── pushModels — duplicate reference-name mapping halts the sync (PROD-1492) ────── describe("pushModels — deleted-and-recreated model leaves a duplicate mapping and halts the sync (PROD-1492)", () => { - it('throws "Model validation failed" (and writes nothing) when two mapping records share a reference name with different source IDs', async () => { + it('throws "Model validation failed" (and writes nothing) when a live model shares a reference name with a dead duplicate record', async () => { const { ModelMapper } = await import("lib/mappers/model-mapper"); - // Seed the mapping exactly as it looks after a deleted-and-recreated source model: - // dead source 46 -> target 138 and live source 48 -> target 139, both named "PromoBanner". + // Seed the mapping exactly as it looks after a deleted-and-recreated source model (the PROD-1492 + // PromoBanner case): dead source 46 -> target 138 and live source 48 -> target 139, same name. const seeder = new ModelMapper(state.sourceGuid[0], state.targetGuid[0]); seeder.addMapping( { id: 46, referenceName: "PromoBanner", lastModifiedDate: new Date(2025, 0, 1).toISOString() } as any, @@ -232,7 +232,7 @@ describe("pushModels — deleted-and-recreated model leaves a duplicate mapping const { pushModels } = await import("../model-pusher"); - // Only the live model (48) still exists on the source; the dead record (46) is the stale duplicate. + // The live model (48) still exists on the source; the dead record (46) is the stale duplicate. const liveModel = makeModel({ id: 48, referenceName: "PromoBanner", @@ -250,6 +250,41 @@ describe("pushModels — deleted-and-recreated model leaves a duplicate mapping expect(saveModel).not.toHaveBeenCalled(); }); + + it("does NOT halt when both duplicate records are for a model deleted outright (neither source ID is live)", async () => { + const { ModelMapper } = await import("lib/mappers/model-mapper"); + + // Real 63b1dc5d-us2 -> 95bfb840-us2 shape: two case-only "changelog" records, BOTH stale — + // ChangeLog source 110 -> target 18 and Changelog source 117 -> target 24. + // Neither source model exists in the current pull anymore (the model was fully deleted), so this + // is stale mapping residue, not a delete-and-recreate. The gate must NOT halt the sync over it. + const seeder = new ModelMapper(state.sourceGuid[0], state.targetGuid[0]); + seeder.addMapping( + { id: 110, referenceName: "ChangeLog", lastModifiedDate: new Date(2025, 0, 1).toISOString() } as any, + { id: 18, referenceName: "ChangeLog", lastModifiedDate: new Date(2025, 0, 1).toISOString() } as any + ); + seeder.addMapping( + { id: 117, referenceName: "Changelog", lastModifiedDate: new Date(2025, 1, 1).toISOString() } as any, + { id: 24, referenceName: "Changelog", lastModifiedDate: new Date(2025, 1, 1).toISOString() } as any + ); + + const saveModel = jest.fn().mockResolvedValue(makeModel({ id: 999 })); + jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel)); + + const { pushModels } = await import("../model-pusher"); + + // The push carries only a live, unrelated model — neither 110 nor 117 is present. + const liveModel = makeModel({ + id: 55, + referenceName: "FooterLinks", + lastModifiedDate: new Date(2025, 5, 1).toISOString(), + }); + + // Must resolve (not throw): the dead-duplicate is logged and ignored, sync continues. + await expect(pushModels([liveModel], [])).resolves.toEqual( + expect.objectContaining({ status: "success" }) + ); + }); }); // ─── PROD-2211: honest failure reporting ──────────────────────────────────────