diff --git a/.changeset/meta-list-runtime-baseline-dedup.md b/.changeset/meta-list-runtime-baseline-dedup.md new file mode 100644 index 0000000000..e87c3ad6e8 --- /dev/null +++ b/.changeset/meta-list-runtime-baseline-dedup.md @@ -0,0 +1,51 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): `GET /api/v1/meta/` stops listing a skill twice after a runtime PUT (#7654) + +`PUT /api/v1/meta/skill/` returned 200 and then `GET /api/v1/meta/skill` +served the skill **twice** — the store-override row and the package row, side by +side, disagreeing about `active`. Nothing about the pair told a caller which one +was the effective document. + +`getMetaItems` merges three layers, and two of them answered the identity +question differently: + +- `mergePackageAwareOverlay` — the `sys_metadata` overlay merge — resolves per + `(slot, package)` and treats a **package-less** row as *standing in for* each + package's row of that name, which is exactly how + `getMetaItem(name, packageId=P)` resolves. +- the MetadataService merge one layer below keyed a hand-rolled `Map` on + `(package, name)` with **strict** equality, so a package-less row occupied a + slot of its own instead of standing in for anything. + +A runtime PUT carries no `?package=`, so the row it writes is +`package_id IS NULL`. For a type whose baseline arrives through the +MetadataService rather than the SchemaRegistry — `skill`, `agent`, `tool` reach +it through that service's own loaders — the registry listing is empty, so the +overlay merge had no base row to take provenance from and left the override body +with no `_packageId`. Its key then missed the package-bearing baseline row in +the merge below, the "already present, do not overwrite" guard never fired, and +both rows were served. + +The MetadataService merge now runs **that same package-aware resolution** rather +than a second implementation of it: the runtime listing is the base layer and +the registry-plus-overlay result is the higher one, so the documented precedence +(a `sys_metadata` customization wins over the artifact baseline) is preserved +while the two steps can no longer disagree about what a package-less row means. + +**Not a `skill` special case.** The same shape was measured duplicating for +`agent`, `tool` and `page`; the mechanism is the merge's attribution rule, not +the type, and the fix closes the mirrored attribution too (a package-less +baseline under a package-bearing higher row). Where a name is shipped by two +packages, both rows are still served — ADR-0048 resolution is unchanged — and a +package-less override now reaches both of their slots. + +Also visible: the surviving row carries the `_packageId` of the package it +overrides, so provenance, the package filter and the disabled-package filter see +an override the way they already see a registry item. + +Unaffected: i18n bundles (`email_template`) keep every locale — the slot, and so +the discriminator, is computed by the same function either way — and a type with +no `metadata` service installed takes the same path it always did. diff --git a/packages/metadata-protocol/src/protocol.meta-list-runtime-baseline-dedup.test.ts b/packages/metadata-protocol/src/protocol.meta-list-runtime-baseline-dedup.test.ts new file mode 100644 index 0000000000..4c6a33927e --- /dev/null +++ b/packages/metadata-protocol/src/protocol.meta-list-runtime-baseline-dedup.test.ts @@ -0,0 +1,288 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7654] `GET /api/v1/meta/` serves ONE row per name after a runtime PUT. + * + * --------------------------------------------------------------------------- + * The gap this file pins + * --------------------------------------------------------------------------- + * `getMetaItems` merges three layers, and until this card two of them answered + * the identity question differently: + * + * • `mergePackageAwareOverlay` (the `sys_metadata` overlay merge) resolves per + * `(slot, package)` and treats a package-LESS row as STANDING IN for each + * package's row of that name — the same resolution + * `getMetaItem(name, packageId=P)` performs. + * • the MetadataService merge one layer below keyed a hand-rolled `Map` on + * `(package, name)` with STRICT equality, so a package-less row occupied a + * slot of its OWN rather than standing in for anything. + * + * A runtime `PUT /api/v1/meta//` sends no `?package=`, so the row it + * writes is `package_id IS NULL`. For a type whose baseline lives in the + * MetadataService rather than the SchemaRegistry — `skill`, `agent`, `tool` + * reach it through its own loaders — the registry listing is empty, so the + * overlay merge has no base row to take provenance from and the override body + * leaves it with NO `_packageId`. Its key then missed the package-bearing + * baseline row in the MetadataService merge, the "already present" guard never + * fired, and the list served the override row AND the package row: the card's + * `GET /api/v1/meta/skill` double listing, after a 200 PUT. + * + * --------------------------------------------------------------------------- + * Why this is NOT skill-specific — and why the fix is not a skill special case + * --------------------------------------------------------------------------- + * The card located the defect on `skill`, and the measurement that opened this + * work found `agent`, `tool` and `page` duplicate identically under the same + * shape: the mechanism is the merge's attribution rule, not the type. `skill` + * is simply a type whose rows arrive through the MetadataService, which is the + * precondition — hence the `agent` case below, which fails on `origin/main` for + * exactly the same reason and would keep failing under any fix that special- + * cased `skill`, or that "registered `skill` like every other type". + * + * --------------------------------------------------------------------------- + * Why there was no coverage before + * --------------------------------------------------------------------------- + * The MetadataService merge runs ONLY when a `metadata` service is installed + * AND answers non-empty for the type; the overlay merge runs only when + * `sys_metadata` yields an active row. A harness that omits either passes + * against the bug — the same blind spot #7774 recorded one merge over — so the + * first case below asserts the precondition itself. + * + * --------------------------------------------------------------------------- + * Reverse verification, direction predicted BEFORE running + * --------------------------------------------------------------------------- + * Restoring the hand-rolled `itemMap` body of the MetadataService merge must + * turn the duplication cases RED naming two rows where one is expected, and + * must leave `protocol.i18n-bundle-list-merge.test.ts` GREEN — that suite pins + * the merge's bundle behaviour, which this change does not touch (the slot, and + * therefore the discriminator, is computed by the same + * `mergePackageAwareOverlay` either way). A red there would mean this change + * altered an identity it promised not to. Measured in the PR body. + */ +import { describe, expect, it } from 'vitest'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +const PKG = 'com.acme.showcase'; +const OTHER_PKG = 'com.acme.partner'; + +interface Row { + id: string; + type: string; + name: string; + organization_id: string | null; + package_id: string | null; + state: string; + metadata: string; +} + +/** A `sys_metadata` row; `package_id` defaults to NULL — what a runtime PUT writes. */ +function row( + partial: Omit, 'metadata'> & { name: string; type: string; metadata: unknown }, +): Row { + return { + id: `row_${partial.type}_${partial.name}_${partial.package_id ?? 'global'}`, + organization_id: null, + package_id: null, + state: 'active', + ...partial, + metadata: JSON.stringify(partial.metadata), + }; +} + +/** + * The registry stub answers `listItems` from `opts.items`, tagged `__type` for + * the harness and `_packageId` the way the real `SchemaRegistry` tags them. The + * card's shape leaves it EMPTY: a skill's baseline arrives through the + * MetadataService. + */ +function makeEngine(opts: { items?: any[]; rows?: Row[] } = {}) { + return { + registry: { + listItems: (type: string, packageId?: string) => { + const all = (opts.items ?? []) as any[]; + const forType = all.filter((i) => i.__type === type); + return (packageId ? forType.filter((i) => i._packageId === packageId) : forType) + .map(({ __type, ...rest }) => rest); + }, + isPackageDisabled: () => false, + registerItem: () => {}, + getItem: () => undefined, + applyNavContributions: (app: unknown) => app, + }, + async find(table: string, q: { where: Record }) { + if (table !== 'sys_metadata') return []; + return (opts.rows ?? []).filter((r) => { + for (const [k, v] of Object.entries(q.where)) { + if (v === undefined) continue; + if ((r as any)[k] !== v) return false; + } + return true; + }); + }, + async findOne() { return null; }, + } as any; +} + +/** A `metadata` service that answers `list(type)` from a fixed table. */ +function servicesWithMetadata(byType: Record) { + return () => new Map([ + ['metadata', { list: async (type: string) => byType[type] ?? [] }], + ]); +} + +function protocolWith( + engine: any, + services?: Record, +): ObjectStackProtocolImplementation { + const p = new ObjectStackProtocolImplementation(engine); + if (services) (p as any).getServicesRegistry = servicesWithMetadata(services); + return p; +} + +function skill(name: string, extra: Record = {}) { + return { name, label: `Skill ${name}`, ...extra }; +} + +/** `name|package|active` per served row, sorted — the three facts the card is about. */ +function served(items: any[]): string[] { + return items + .map((i) => `${i.name}|${i._packageId ?? '(none)'}|${String(i.active)}`) + .sort(); +} + +describe('[#7654] the /meta list serves one row per name after a runtime PUT', () => { + describe('the precondition this defect hides behind', () => { + it('no `metadata` service installed: the merge never runs and the override is already single', async () => { + // This case passes on `origin/main` too, and that is the point — it + // is the shape of harness that made the defect invisible. + const engine = makeEngine({ + rows: [row({ type: 'skill', name: 'formula-helper', metadata: skill('formula-helper', { active: true }) })], + }); + const res: any = await protocolWith(engine).getMetaItems({ type: 'skill' }); + expect(served(res.items)).toEqual(['formula-helper|(none)|true']); + }); + + it('a `metadata` service answering non-empty is what arms the merge', async () => { + // No override row: the baseline passes through untouched, one row. + const engine = makeEngine(); + const res: any = await protocolWith(engine, { + skill: [{ ...skill('formula-helper', { active: false }), _packageId: PKG }], + }).getMetaItems({ type: 'skill' }); + expect(served(res.items)).toEqual(['formula-helper|com.acme.showcase|false']); + }); + }); + + describe("the card's shape: a package-less override over a MetadataService baseline", () => { + it('serves ONE skill row, the override body, carrying the package it overrides', async () => { + const engine = makeEngine({ + rows: [row({ type: 'skill', name: 'formula-helper', metadata: skill('formula-helper', { active: true }) })], + }); + const res: any = await protocolWith(engine, { + skill: [{ ...skill('formula-helper', { active: false }), _packageId: PKG }], + }).getMetaItems({ type: 'skill' }); + + // On `origin/main` this served TWO rows: + // formula-helper|(none)|true ← the override + // formula-helper|com.acme.showcase|false ← the package row + expect(res.items).toHaveLength(1); + expect(served(res.items)).toEqual(['formula-helper|com.acme.showcase|true']); + }); + + it('is not skill-specific: `agent` duplicates and is fixed by the same resolution', async () => { + const engine = makeEngine({ + rows: [row({ type: 'agent', name: 'triage', metadata: { name: 'triage', active: true } })], + }); + const res: any = await protocolWith(engine, { + agent: [{ name: 'triage', active: false, _packageId: PKG }], + }).getMetaItems({ type: 'agent' }); + expect(served(res.items)).toEqual(['triage|com.acme.showcase|true']); + }); + + it('also collapses the MIRRORED attribution — a package-less baseline under a package-bearing row', async () => { + // The same disagreement seen from the other side: the higher layer + // names a package, the MetadataService baseline does not. + const engine = makeEngine({ + items: [{ __type: 'skill', ...skill('formula-helper', { active: false }), _packageId: PKG }], + rows: [row({ + type: 'skill', + name: 'formula-helper', + package_id: PKG, + metadata: skill('formula-helper', { active: true }), + })], + }); + const res: any = await protocolWith(engine, { + skill: [skill('formula-helper', { active: false })], + }).getMetaItems({ type: 'skill' }); + expect(served(res.items)).toEqual(['formula-helper|com.acme.showcase|true']); + }); + }); + + describe('what the merge must keep doing', () => { + it('the overlay still WINS over the MetadataService baseline', async () => { + // The guard the hand-rolled loop existed for: saved per-org + // dashboard / view overlays disappeared from list endpoints on + // refresh when the baseline was allowed to win. + const engine = makeEngine({ + rows: [row({ + type: 'dashboard', + name: 'sales', + package_id: PKG, + metadata: { name: 'sales', label: 'Customized' }, + })], + }); + const res: any = await protocolWith(engine, { + dashboard: [{ name: 'sales', label: 'Shipped', _packageId: PKG }], + }).getMetaItems({ type: 'dashboard' }); + expect(res.items).toHaveLength(1); + expect((res.items as any[])[0].label).toBe('Customized'); + }); + + it('ADR-0048: two packages shipping the same name stay TWO rows', async () => { + const engine = makeEngine(); + const res: any = await protocolWith(engine, { + skill: [ + { ...skill('summarize', { active: true }), _packageId: PKG }, + { ...skill('summarize', { active: false }), _packageId: OTHER_PKG }, + ], + }).getMetaItems({ type: 'skill' }); + expect(served(res.items)).toEqual([ + 'summarize|com.acme.partner|false', + 'summarize|com.acme.showcase|true', + ]); + }); + + it('a package-less override reaches BOTH packages that ship the name', async () => { + // The resolution this change adopts, stated as behaviour: a + // package-less row stands in for each package's slot — exactly what + // `getMetaItem(name, packageId=P)` answers for either P. + const engine = makeEngine({ + rows: [row({ type: 'skill', name: 'summarize', metadata: skill('summarize', { active: true }) })], + }); + const res: any = await protocolWith(engine, { + skill: [ + { ...skill('summarize', { active: false }), _packageId: PKG }, + { ...skill('summarize', { active: false }), _packageId: OTHER_PKG }, + ], + }).getMetaItems({ type: 'skill' }); + expect(served(res.items)).toEqual([ + 'summarize|com.acme.partner|true', + 'summarize|com.acme.showcase|true', + ]); + }); + + it('a runtime item with NO override is served unchanged alongside an overridden sibling', async () => { + const engine = makeEngine({ + rows: [row({ type: 'skill', name: 'formula-helper', metadata: skill('formula-helper', { active: true }) })], + }); + const res: any = await protocolWith(engine, { + skill: [ + { ...skill('formula-helper', { active: false }), _packageId: PKG }, + { ...skill('untouched', { active: false }), _packageId: PKG }, + ], + }).getMetaItems({ type: 'skill' }); + expect(served(res.items)).toEqual([ + 'formula-helper|com.acme.showcase|true', + 'untouched|com.acme.showcase|false', + ]); + }); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 5fb0a1cb0d..b427e4039b 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -4585,50 +4585,66 @@ export class ObjectStackProtocolImplementation implements runtimeItems = runtimeItems.filter((item: any) => item?._packageId === packageId); } if (runtimeItems && runtimeItems.length > 0) { - // 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. + // Merge, avoiding duplicates. ADR-0048 (#1828) — resolution + // is per `(slot, package)` and never by bare `name`, so a + // runtime item from one package does not collapse a + // same-name item from another. // - // [#7774] …and by `(package, name, locale)` for a type - // whose identity the spec declares as a pair. This loop is - // where the i18n bundle actually died: `items` already - // held both members (the registry keeps them since #7730), - // the second `set` overwrote the first, and - // `GET /meta/email_template` served one locale. It only - // ever ran with a `metadata` service installed AND - // answering non-empty for the type — which is why the + // [#7774] …and the slot carries the bundle discriminator + // for a type whose identity the spec declares as a pair. + // This merge is where the i18n bundle actually died: + // `items` already held both members (the registry keeps + // them since #7730), the second write overwrote the first, + // and `GET /meta/email_template` served one locale. It only + // ever runs with a `metadata` service installed AND + // answering non-empty for the type — which is why that // regression was invisible to every suite that omits one. - const itemMap = new Map(); - const entryKey = (entry: any): string => - metaItemKey( - entry._packageId ?? undefined, - entry.name, - itemDiscriminator(request.type, entry), - ); - for (const item of items) { - const entry = item as any; - if (entry && typeof entry === 'object' && 'name' in entry) { - itemMap.set(entryKey(entry), entry); - } - } - for (const item of runtimeItems) { - const entry = item as any; - if (entry && typeof entry === 'object' && 'name' in entry) { - // Do not overwrite entries already present in the - // map: those came from sys_metadata (customization - // overlays) or the SchemaRegistry and must win - // over the MetadataService's artifact baseline. - // Without this guard, saved per-org dashboard / - // view overlays disappear from list endpoints on - // refresh (detail endpoint kept showing the - // overlay because it uses a different code path). - const key = entryKey(entry); - if (!itemMap.has(key)) { - itemMap.set(key, entry); - } - } - } - items = Array.from(itemMap.values()); + // + // [#7654] THE RESOLUTION IS THE OVERLAY MERGE'S, and it is + // now literally that function instead of a second + // implementation of the same idea. As a hand-rolled `Map` + // keyed on `(package, name)` with STRICT equality, this step + // disagreed with {@link mergePackageAwareOverlay} — which + // runs one layer above it on the very same list — about the + // package-LESS row, and the disagreement reached the wire as + // a duplicate. A package-less row does not merely occupy a + // slot of its own: it STANDS IN for each package's row of + // that name, which is how `getMetaItem(name, packageId=P)` + // resolves and what the overlay merge already implements. + // + // A runtime `PUT /api/v1/meta//` carries no + // `?package=`, so `sys_metadata` takes a + // `package_id IS NULL` row. For a type the SchemaRegistry + // has nothing for — `skill`, `agent` and `tool` reach the + // MetadataService through its own loaders, so the registry + // listing is empty and this baseline is the only package row + // there is — that overlay leaves the merge above UNSTAMPED + // (it stamps `_packageId` from the base row it displaced, + // and there was none). Its key then missed the + // package-bearing baseline row here, the "already present" + // guard below never fired, and `GET /api/v1/meta/skill` + // listed the override row AND the package row after a 200 + // PUT. The mirrored attribution — a package-less runtime + // baseline under a package-bearing higher row — duplicated + // for the same reason and is closed by the same call. + // + // Layer order is the one the guard this replaces stated: + // entries from `sys_metadata` (customization overlays) or + // the SchemaRegistry WIN over the MetadataService's artifact + // baseline — without that, saved per-org dashboard / view + // overlays disappeared from list endpoints on refresh while + // the detail endpoint kept showing them. So `items` is the + // higher layer (`records`) and the runtime listing is the + // base, and "latest contribution wins" reproduces the guard + // rather than reversing it. + items = mergePackageAwareOverlay( + request.type, + runtimeItems as unknown[], + (items as any[]).map((it) => ({ + data: it, + packageId: ((it as any)?._packageId ?? undefined) as string | undefined, + })), + ); } } } catch {