From 4b99652cfd366bfdd41b1fd12200d73a76ee8750 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 18 Jul 2026 15:47:31 +0000 Subject: [PATCH] fix(metadata-protocol): unscoped metadata list dedupes package-aware, not by bare name (ADR-0048 #1828) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `getMetaItems` merged registry items, `sys_metadata` overlay rows, draft-preview rows, and MetadataService items into `Map`s keyed by bare `name`. When two installed packages ship the same `type/name` (e.g. `page/home`) and either has an overlay, the DB-overlay merge collapsed them to one row (last-write-wins), so an unscoped `GET /meta/:type` returned only one — and the frontend prefer-local resolution, which reads the unscoped list, could no longer tell the two packages' rows apart. Key the three merge sites (plus the env/org `mergedMap`) by `(package, name)` via a shared `mergePackageAwareOverlay` helper whose per-package resolution mirrors `getMetaItem(name, packageId=P)` (scoped row, else package-less/global fallback), so: - two packages' same-name rows stay distinct, each with its own `_packageId`; - a package-less (env-wide) overlay still wins over the single artifact it customizes — ADR-0005 precedence and single-package behaviour unchanged; - a legacy row whose active/draft layers disagree on package attribution still collapses (draft wins), matching the single-item path. Two provenance fixes so a collision no longer mislabels `_packageId`: - scope the registry-hydration artifact graft to the row's own `package_id`; - `SchemaRegistry.getArtifactItem(type, name, packageId)` no longer returns a bare-key overlay owned by a DIFFERENT package. The bare slot holds one row (last hydrate wins); once the list stopped collapsing, the list-decorate step (`lookupArtifactItem(type, name, )`) would otherwise graft that one package's id onto every same-name row. Found by dogfooding a real two-package collision against the running server. Tests: two-package overlay collision keeps both rows with correct `_packageId`; single-package env-wide overlay unchanged; MetadataService collision keeps both; `getArtifactItem` rejects a foreign-package bare entry. Verified end-to-end on a booted showcase app (unscoped `/meta/page` returns both rows, each correct `_packageId`; scoped reads isolate). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01DKFjwN2EVXPB2CJwNzPs8c --- .../metadata-list-package-aware-dedup.md | 19 ++ packages/metadata-protocol/src/protocol.ts | 238 +++++++++++++----- packages/objectql/src/protocol-meta.test.ts | 61 +++++ .../registry-prefer-local-resolution.test.ts | 15 ++ packages/objectql/src/registry.ts | 13 +- 5 files changed, 280 insertions(+), 66 deletions(-) create mode 100644 .changeset/metadata-list-package-aware-dedup.md diff --git a/.changeset/metadata-list-package-aware-dedup.md b/.changeset/metadata-list-package-aware-dedup.md new file mode 100644 index 0000000000..b972e05216 --- /dev/null +++ b/.changeset/metadata-list-package-aware-dedup.md @@ -0,0 +1,19 @@ +--- +'@objectstack/metadata-protocol': patch +--- + +fix(metadata-protocol): unscoped metadata list dedupes package-aware, not by bare name (ADR-0048 #1828) + +`getMetaItems` merged registry items, `sys_metadata` overlay rows, draft-preview +rows, and MetadataService items into `Map`s keyed by bare `name`, so two installed +packages shipping the same `type/name` (e.g. `page/home`) collapsed to one row +(last-write-wins) on an unscoped `GET /meta/:type` whenever either package had an +overlay — and the frontend prefer-local resolution, which reads that list, could +no longer tell the two packages' rows apart. + +The three merge sites (plus the env/org pre-merge) now key by `(package, name)`, +mirroring `getMetaItem`'s scoped-then-global-fallback resolution: colliding rows +stay distinct each with its own `_packageId`, a package-less (env-wide) overlay +still wins over the single artifact it customizes (ADR-0005 precedence and +single-package behaviour unchanged), and the registry-hydration artifact graft is +scoped to each row's own `package_id` so a collision no longer mislabels provenance. diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index e28bd4d925..36b582e14a 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -369,6 +369,104 @@ function mergeArtifactProtection(item: unknown, artifactItem: unknown): unknown return out; } +/** + * ADR-0048 (#1828) — composite dedup identity for the unscoped metadata list. + * + * Two installed packages may legitimately ship the same `type`/`name` + * (e.g. `page/home`); the SchemaRegistry already stores them under distinct + * `${packageId}:${name}` keys. Any list-merge that deduplicates by bare `name` + * collapses the two packages' rows into one (last-write-wins), which is the + * bug this key closes. A `NUL` separator keeps names containing `:` unambiguous. + */ +function metaItemKey(packageId: string | null | undefined, name: unknown): string { + return `${packageId ?? ''}\u0000${String(name)}`; +} + +/** + * ADR-0048 (#1828) — package-aware overlay merge for the unscoped metadata list. + * + * `baseItems` (the lower layer: registry artifacts, or the running result) and + * `records` (the higher layer: active `sys_metadata` overlays, or draft rows) + * are merged so that: + * + * • Two installed packages shipping the same `type/name` stay TWO rows — + * resolution is per `(package, name)`, not bare `name`, so a higher-layer + * row no longer collapses a same-name row from a different package. + * • For each package `P` that owns a row of a given name, the winner is the + * LATEST contribution that is either `P`'s own row or a package-less + * ("global", `package_id IS NULL`) row — mirroring + * `getMetaItem(name, packageId=P)`'s "scoped-then-global-fallback" + * resolution, so the list and single-item paths agree. This is also why a + * legacy row whose active/draft layers disagree on package attribution + * still collapses (a package-less active row + its `package_id`-bearing + * draft resolve to the one package slot, draft winning). + * • A name with NO package-owned row resolves to its latest package-less + * contribution — the pre-existing env-wide behaviour, unchanged. + * + * `transform(data, prev)` runs on each `records` body before it enters the + * merge (view-identity healing, draft tagging); `prev` is the base row it + * shadows at the same slot (or any same-name base row), else undefined. + */ +function mergePackageAwareOverlay( + baseItems: unknown[], + records: Array<{ data: unknown; packageId: string | undefined }>, + transform?: (data: any, prev: any) => any, +): unknown[] { + // Per-name, layer-ordered contributions; `pkg: undefined` = package-less. + const buckets = new Map>(); + const order: string[] = []; // first-seen name order → stable output + const push = (name: string, pkg: string | undefined, item: any) => { + let list = buckets.get(name); + if (!list) { buckets.set(name, (list = [])); order.push(name); } + list.push({ pkg, item }); + }; + + for (const raw of baseItems) { + const item = raw as any; + if (item && typeof item === 'object' && 'name' in item) { + push(item.name, (item._packageId ?? undefined) as string | undefined, item); + } + } + for (const { data, packageId } of records) { + const body = data as any; + if (!(body && typeof body === 'object' && 'name' in body)) continue; + // The base row this record shadows at its own slot (for view-identity + // healing): a same-package row, else a package-less one, else any + // same-name row it stands in for. + const list = buckets.get(body.name); + const prev = list + ? (list.find((c) => c.pkg === packageId)?.item + ?? list.find((c) => c.pkg === undefined)?.item + ?? list[0]?.item) + : undefined; + push(body.name, packageId, transform ? transform(body, prev) : body); + } + + const out: unknown[] = []; + for (const name of order) { + const list = buckets.get(name)!; + const reals = Array.from(new Set(list.filter((c) => c.pkg !== undefined).map((c) => c.pkg))); + if (reals.length === 0) { + out.push(list[list.length - 1].item); // latest package-less row wins + continue; + } + for (const real of reals) { + // getMetaItem(name, real) resolution: latest row that is `real`'s + // own or package-less (global fallback). + let chosen: any; + for (const c of list) { + if (c.pkg === real || c.pkg === undefined) chosen = c.item; + } + if (chosen === undefined) continue; + // A package-less body standing in for package `real` must carry + // `real`'s provenance (the base row it replaced was `real`'s). + if (chosen._packageId === undefined) chosen = { ...chosen, _packageId: real }; + out.push(chosen); + } + } + return out; +} + /** * Simple hash function for ETag generation (browser-compatible) * Uses a basic hash algorithm instead of crypto.createHash @@ -1608,63 +1706,69 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol { }; const envWideRecords = await queryByOrg(null); const orgRecords = orgId ? await queryByOrg(orgId) : []; - // org-specific rows override env-wide rows on name collision + // org-specific rows override env-wide rows on name collision. + // ADR-0048 (#1828) — key by (package, name), not bare name, so a + // package A row and a package B row of the same name do not + // collapse; org-over-env precedence still holds within each slot. const mergedMap = new Map(); - for (const r of envWideRecords) mergedMap.set(r.name, r); - for (const r of orgRecords) mergedMap.set(r.name, r); + for (const r of envWideRecords) mergedMap.set(metaItemKey(r.package_id, r.name), r); + for (const r of orgRecords) mergedMap.set(metaItemKey(r.package_id, r.name), r); const records = Array.from(mergedMap.values()); if (records && records.length > 0) { - const byName = new Map(); - for (const existing of items) { - const entry = existing as any; - if (entry && typeof entry === 'object' && 'name' in entry) { - byName.set(entry.name, entry); - } - } - for (const record of records) { + const isView = (PLURAL_TO_SINGULAR[request.type] ?? request.type) === 'view'; + // Parse each overlay body once and surface its persisted + // software-package binding so the sidebar package filter and + // provenance classification see overlay rows the way they see + // registry items. + const overlays = records.map((record) => { const data = typeof record.metadata === 'string' ? JSON.parse(record.metadata) : record.metadata; - if (data && typeof data === 'object' && 'name' in data) { - // Surface the persisted software-package binding so the - // sidebar package filter and provenance classification - // see overlay rows the same way they see registry items. - const recPkg = (record as { package_id?: string | null }).package_id ?? undefined; - if (recPkg && (data as any)._packageId === undefined) { - (data as any)._packageId = recPkg; - } - // #2555 — heal identity-less view overlays already in the - // DB (persisted by pre-fix saves): a raw-config row would - // replace the flattened package entry wholesale, dropping - // viewKind/object and vanishing the view from every - // consumer that filters on them (switcher endpoint). - // Inherit the identity fields from the shadowed entry; - // the overlay's own fields still win. - if ((PLURAL_TO_SINGULAR[request.type] ?? request.type) === 'view') { - const patch = viewIdentityPatch(data as Record, byName.get(data.name)); - if (patch) Object.assign(data, patch); - } - byName.set(data.name, data); + const recPkg = (record as { package_id?: string | null }).package_id ?? undefined; + if (recPkg && data && typeof data === 'object' && (data as any)._packageId === undefined) { + (data as any)._packageId = recPkg; } - // Only hydrate the global registry for unscoped calls — - // scoped project entries must not leak process-wide. - // Graft the artifact's protection envelope onto the - // overlay body BEFORE registering: the plain-key entry - // written here shadows the packaged artifact on - // `registry.getItem`, and a bare overlay body would - // strip `_lock`/`_packageId`/`_provenance` from every - // registry-direct reader (ADR-0010 §3.3 — an overlay - // must never loosen a packaged lock). - if (this.environmentId === undefined && data && typeof data === 'object') { - const artifact = this.lookupArtifactItem(request.type, (data as any).name); - this.engine.registry.registerItem( - request.type, - mergeArtifactProtection(data, artifact), - 'name' as any, - ); + return { data, packageId: recPkg }; + }); + + // ADR-0048 (#1828) — package-aware merge: a package-scoped row + // overlays ONLY its own package's entry, so two installed + // packages shipping the same `type/name` (e.g. `page/home`) are + // not collapsed to one. #2555 — heal identity-less view overlays + // from the entry they shadow (a raw-config row would otherwise + // drop viewKind/object and vanish the view from switcher/list + // consumers); the overlay's own fields still win. + items = mergePackageAwareOverlay(items, overlays, (data, prev) => { + if (isView && data && typeof data === 'object') { + const patch = viewIdentityPatch(data as Record, prev); + if (patch) Object.assign(data as Record, patch); + } + return data; + }); + + // Only hydrate the global registry for unscoped (control-plane) + // calls — scoped project entries must not leak process-wide. + // Graft the artifact's protection envelope onto the overlay body + // BEFORE registering: the plain-key entry written here shadows + // the packaged artifact on `registry.getItem`, and a bare + // overlay body would strip `_lock`/`_packageId`/`_provenance` + // from every registry-direct reader (ADR-0010 §3.3 — an overlay + // must never loosen a packaged lock). ADR-0048 (#1828) — scope + // the artifact lookup to the row's OWN package so a colliding + // overlay no longer grafts the first-registered package's + // provenance/lock onto another package's row. + if (this.environmentId === undefined) { + for (const { data, packageId: recPkg } of overlays) { + if (data && typeof data === 'object' && 'name' in data) { + const artifact = this.lookupArtifactItem(request.type, (data as any).name, recPkg); + this.engine.registry.registerItem( + request.type, + mergeArtifactProtection(data, artifact), + 'name' as any, + ); + } } } - items = Array.from(byName.values()); } } catch { // DB not available — fall through with whatever we already have. @@ -1698,21 +1802,22 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol { }; const draftRecords = [...(await queryDrafts(null)), ...(orgId ? await queryDrafts(orgId) : [])]; if (draftRecords.length > 0) { - const byName = new Map(); - for (const existing of items) { - const entry = existing as any; - if (entry && typeof entry === 'object' && 'name' in entry) byName.set(entry.name, entry); - } - for (const record of draftRecords) { + // ADR-0048 (#1828) — package-aware draft overlay (parity with + // the active-overlay merge above): a package-scoped draft + // previews only its own package's entry, so two packages' + // same-name drafts stay distinct. Draft rows win over active. + const drafts = draftRecords.map((record) => { const data = typeof record.metadata === 'string' ? JSON.parse(record.metadata) : record.metadata; - if (data && typeof data === 'object' && 'name' in data) { - const recPkg = (record as { package_id?: string | null }).package_id ?? undefined; - if (recPkg && (data as any)._packageId === undefined) (data as any)._packageId = recPkg; - (data as any)._draft = true; - byName.set(data.name, data); + const recPkg = (record as { package_id?: string | null }).package_id ?? undefined; + if (recPkg && data && typeof data === 'object' && (data as any)._packageId === undefined) { + (data as any)._packageId = recPkg; } - } - items = Array.from(byName.values()); + return { data, packageId: recPkg }; + }); + items = mergePackageAwareOverlay(items, drafts, (data) => { + if (data && typeof data === 'object') (data as any)._draft = true; + return data; + }); } } catch { // DB unavailable — serve the active result unchanged. @@ -1733,12 +1838,14 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol { runtimeItems = runtimeItems.filter((item: any) => item?._packageId === packageId); } if (runtimeItems && runtimeItems.length > 0) { - // Merge, avoiding duplicates by name + // Merge, avoiding duplicates. ADR-0048 (#1828) — key by + // (package, name), not bare name, so a runtime item from one + // package does not collapse a same-name item from another. const itemMap = new Map(); for (const item of items) { const entry = item as any; if (entry && typeof entry === 'object' && 'name' in entry) { - itemMap.set(entry.name, entry); + itemMap.set(metaItemKey(entry._packageId ?? undefined, entry.name), entry); } } for (const item of runtimeItems) { @@ -1752,8 +1859,9 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol { // view overlays disappear from list endpoints on // refresh (detail endpoint kept showing the // overlay because it uses a different code path). - if (!itemMap.has(entry.name)) { - itemMap.set(entry.name, entry); + const key = metaItemKey(entry._packageId ?? undefined, entry.name); + if (!itemMap.has(key)) { + itemMap.set(key, entry); } } } diff --git a/packages/objectql/src/protocol-meta.test.ts b/packages/objectql/src/protocol-meta.test.ts index 26c11e0a42..d7c2148c5c 100644 --- a/packages/objectql/src/protocol-meta.test.ts +++ b/packages/objectql/src/protocol-meta.test.ts @@ -869,6 +869,67 @@ describe('ObjectStackProtocolImplementation - Metadata Persistence', () => { expect(byPkg['com.globex.crm']).toBe('Globex Home'); }); + it('keeps both colliding rows when EACH package has its own sys_metadata overlay (ADR-0048 #1828)', async () => { + // The #1828 gap: two installed packages ship `page/home` AND each has + // a package-scoped sys_metadata overlay. The DB-overlay merge deduped + // by bare `name`, collapsing the two rows to one (last-write-wins). + // Each overlay must merge onto its OWN package's entry → two rows. + registry.registerItem('page', { name: 'home', label: 'Acme (artifact)' }, 'name', 'com.acme.crm'); + registry.registerItem('page', { name: 'home', label: 'Globex (artifact)' }, 'name', 'com.globex.crm'); + mockEngine.find.mockResolvedValue([ + { type: 'page', name: 'home', state: 'active', package_id: 'com.acme.crm', metadata: JSON.stringify({ name: 'home', label: 'Acme (overlay)' }) }, + { type: 'page', name: 'home', state: 'active', package_id: 'com.globex.crm', metadata: JSON.stringify({ name: 'home', label: 'Globex (overlay)' }) }, + ]); + + const result = await protocol.getMetaItems({ type: 'page' }); + const homes = (result.items as any[]).filter((i) => i.name === 'home'); + + expect(homes).toHaveLength(2); + const byPkg = Object.fromEntries(homes.map((h: any) => [h._packageId, h.label])); + // Overlay wins over the artifact, and neither package collapses. + expect(byPkg['com.acme.crm']).toBe('Acme (overlay)'); + expect(byPkg['com.globex.crm']).toBe('Globex (overlay)'); + }); + + it('single-package env-wide (package-less) overlay still overlays the artifact (ADR-0005 unchanged, #1828)', async () => { + // A package-less overlay (package_id IS NULL) must still WIN over the + // one artifact it customizes and stay a single row — the package-aware + // merge must not split a global overlay off from its lone artifact. + registry.registerItem('page', { name: 'home', label: 'Artifact' }, 'name', 'com.acme.crm'); + mockEngine.find.mockResolvedValue([ + { type: 'page', name: 'home', state: 'active', package_id: null, metadata: JSON.stringify({ name: 'home', label: 'Customized' }) }, + ]); + + const result = await protocol.getMetaItems({ type: 'page' }); + const homes = (result.items as any[]).filter((i) => i.name === 'home'); + + expect(homes).toHaveLength(1); + expect(homes[0].label).toBe('Customized'); // overlay wins (ADR-0005) + expect(homes[0]._packageId).toBe('com.acme.crm'); // provenance retained + }); + + it('keeps both colliding rows from the MetadataService baseline (ADR-0048 #1828)', async () => { + // Third merge site: two packages contribute a runtime `agent/assistant` + // via MetadataService. The merge deduped by bare name, dropping one. + const metadataService = { + list: vi.fn().mockResolvedValue([ + { name: 'assistant', label: 'Acme Assistant', _packageId: 'com.acme.crm' }, + { name: 'assistant', label: 'Globex Assistant', _packageId: 'com.globex.crm' }, + ]), + }; + const services = new Map([['metadata', metadataService]]); + const scoped = new ObjectStackProtocolImplementation(mockEngine, () => services); + mockEngine.find.mockResolvedValue([]); + + const result = await scoped.getMetaItems({ type: 'agent' }); + const agents = (result.items as any[]).filter((i) => i.name === 'assistant'); + + expect(agents).toHaveLength(2); + const byPkg = Object.fromEntries(agents.map((a: any) => [a._packageId, a.label])); + expect(byPkg['com.acme.crm']).toBe('Acme Assistant'); + expect(byPkg['com.globex.crm']).toBe('Globex Assistant'); + }); + it('should fall back to DB when registry is empty for type', async () => { mockEngine.find.mockResolvedValue([ { diff --git a/packages/objectql/src/registry-prefer-local-resolution.test.ts b/packages/objectql/src/registry-prefer-local-resolution.test.ts index 383a449826..865dd048ec 100644 --- a/packages/objectql/src/registry-prefer-local-resolution.test.ts +++ b/packages/objectql/src/registry-prefer-local-resolution.test.ts @@ -60,4 +60,19 @@ describe('SchemaRegistry — package-scoped resolution (ADR-0048 §3.3)', () => expect(registry.getItem('flow', 'cleanup', 'com.a.sys')?.title).toBe('A'); expect(registry.getItem('flow', 'cleanup', 'com.b.sys')?.title).toBe('B'); }); + + it('getArtifactItem does not return a bare overlay owned by a DIFFERENT package (ADR-0048 #1828)', () => { + // A runtime/DB overlay hydrated under the bare key carries package A's + // provenance. Once the unscoped metadata list stopped collapsing colliding + // rows, the protocol decorates each row via getArtifactItem(type, name, + // ). Package B's row must NOT pick up A's bare overlay as + // its "artifact", or B's `_packageId`/lock gets mislabeled as A. + registry.registerItem('page', { name: 'home', title: 'A overlay', _packageId: 'com.acme.a' }, 'name'); + + // Asking within package B misses — the bare entry belongs to A, not B. + expect(registry.getArtifactItem('page', 'home', 'com.acme.b')).toBeUndefined(); + // Asking within A returns it; a package-less caller keeps the legacy first-match. + expect(registry.getArtifactItem('page', 'home', 'com.acme.a')?.title).toBe('A overlay'); + expect(registry.getArtifactItem('page', 'home')?.title).toBe('A overlay'); + }); }); diff --git a/packages/objectql/src/registry.ts b/packages/objectql/src/registry.ts index 95b715bb05..e2c83b1344 100644 --- a/packages/objectql/src/registry.ts +++ b/packages/objectql/src/registry.ts @@ -1181,8 +1181,19 @@ export class SchemaRegistry { if (it && it._packageId && it._packageId !== 'sys_metadata') return item as T; } } + // Bare-key fallback: a runtime/DB overlay rehydrated under the plain name. + // ADR-0048 (#1828) — when the caller resolves *within a package*, only accept + // this entry if it belongs to that package. The bare slot holds a single + // row (last hydrate wins), so once the unscoped list stopped collapsing + // colliding rows, an overlay from package A hydrated here would otherwise be + // grafted as the "artifact" of package B's same-name row — mislabeling its + // `_packageId`/lock. A package-less caller (`currentPackageId` omitted) keeps + // the legacy best-effort first-match. const direct = collection.get(name) as any; - if (direct && direct._packageId && direct._packageId !== 'sys_metadata') { + if ( + direct && direct._packageId && direct._packageId !== 'sys_metadata' && + (!currentPackageId || direct._packageId === currentPackageId) + ) { return direct as T; } return undefined;