From 423074c998138bcddec0a84b8c9bd855b09de0ce Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 16:24:46 +0000 Subject: [PATCH] fix(metadata): register a packages[] artifact per package at the metadata door MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The metadata artifact door iterated the flattened top level and stamped every item with the artifact's own `manifest.id`. For an artifact composed with `composeStacks(…, { manifest: 'preserve' })` that id is one arbitrary member's (`selectManifest`'s 'last' pick), so a two-package artifact registered the module's object under the App package's identity while the ObjectQL load path, reading the same artifact's `packages[]`, owned it under the module's — two answers to "who owns this object", one per door. `_parseAndRegisterArtifact` now reads both shapes (ADR-0130 D4): `packages` present → register each assembled package body's collections stamped with THAT body's id; `packages` absent → the single-`manifest` branch runs exactly as before (D7). Ordering and the entry gate are reused from `resolveArtifactPackageOrder`, the same call the ObjectQL load path makes (D5), so the two readers of one `packages[]` cannot disagree. That function and `artifactPackageId` moved from `@objectstack/objectql` to `@objectstack/core`: objectql depends on metadata, so the door could not import them from where they lived, and core already owns `resolvePluginOrder` and is already a dependency of both readers — no package edge added. objectql re-exports both under their existing names, so its published surface is unchanged. A residual sweep keeps any top-level definition no package body repeats (`packages` composes by `concat`), registered once under the artifact's own identity and logged, so nothing a booted instance sees today disappears. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01UHvF5hyiZjnCyExFnfQB8m --- .../adr0130-metadata-door-reads-packages.md | 75 ++++ .../src/artifact-packages.ts | 22 +- packages/core/src/index.ts | 7 + ...ugin-artifact-packages-attribution.test.ts | 320 ++++++++++++++++++ packages/metadata/src/plugin.ts | 192 ++++++++++- .../objectql/src/artifact-load-path.test.ts | 2 +- packages/objectql/src/index.ts | 9 +- packages/objectql/src/plugin.ts | 2 +- packages/objectql/src/registry.ts | 2 +- .../multi-package-artifact.dogfood.test.ts | 169 ++++++++- .../src/dispatcher-error-vocabulary.ts | 6 +- 11 files changed, 786 insertions(+), 20 deletions(-) create mode 100644 .changeset/adr0130-metadata-door-reads-packages.md rename packages/{objectql => core}/src/artifact-packages.ts (91%) create mode 100644 packages/metadata/src/plugin-artifact-packages-attribution.test.ts diff --git a/.changeset/adr0130-metadata-door-reads-packages.md b/.changeset/adr0130-metadata-door-reads-packages.md new file mode 100644 index 0000000000..df605026f2 --- /dev/null +++ b/.changeset/adr0130-metadata-door-reads-packages.md @@ -0,0 +1,75 @@ +--- +"@objectstack/metadata": patch +"@objectstack/core": minor +"@objectstack/objectql": patch +"@objectstack/runtime": patch +--- + +fix(metadata): register a `packages[]` artifact per package at the metadata door so every object has one owner across every door (#14599) + +A release artifact carrying `packages[]` (ADR-0130 D4) was read at the metadata +door as if it carried one package: `MetadataPlugin._parseAndRegisterArtifact` +iterated the **flattened top level** and stamped every item with the artifact's +own `manifest.id`. For an artifact composed with `composeStacks(…, { manifest: +'preserve' })` that id is one arbitrary member's — `selectManifest`'s `'last'` +pick — so a two-package artifact registered the **module's** object under the +**App** package's identity, while the ObjectQL load path, reading the same +artifact's `packages[]`, owned it under the module's. + +The platform then held two answers to "who owns this object", and which one a +consumer saw depended on the door it went through. Measured on a real boot of +`examples/app-multi-package`: + +- `GET /api/v1/meta/object` served `crm_order` **twice** — the list merge keys + slots by `${packageId}${name}`, so the two differently-attributed copies + landed in two slots; +- `GET /api/v1/meta/object?package=` returned the **module's** + object, because the App-stamped copy was re-ingested into the registry as that + package's contribution; +- the layers door named the App package while the item door and + `GET /api/v1/packages` named the module; +- Studio's Data pillar for the App package listed the module's object — ADR-0130 + Consequences §1.3a ("Studio's scope is the package") did not hold. + +**The door now reads both shapes, and attributes every item to the body it was +found in.** `packages` present → each assembled package body's collections are +registered stamped with **that body's** id; `packages` absent → the single +`manifest` branch runs exactly as before (D7). The owner is read off the body an +item was found in — never reverse-derived by matching a top-level item's name +against a name-to-package index, which would be the second metadata-identity +resolution path #14512's triage rejected by name. + +**Ordering and the entry gate are reused, not re-derived (D5).** The door calls +the same `resolveArtifactPackageOrder` the ObjectQL load path calls, so the two +readers of one `packages[]` cannot disagree about the registration order **or** +about which artifacts are loadable at all. + +⚠️ **`resolveArtifactPackageOrder` / `artifactPackageId` moved to +`@objectstack/core`** — hence the `minor` there. They were in +`@objectstack/objectql`, which **depends on** `@objectstack/metadata`, so the +metadata door could not import them from where they lived; `@objectstack/core` +already owns `resolvePluginOrder` and is already a dependency of both readers, +so hosting them there adds **no edge** to the package graph. `@objectstack/objectql` +re-exports both under their existing names — its published surface is unchanged, +which is why it is graded `patch`. `@objectstack/runtime` is `patch` for the +dispatcher error vocabulary's `file:` anchors, repointed at the new path. + +**Single-package artifacts are byte-for-byte unaffected (D7)**, measured rather +than asserted: the whole `manager.register` sequence for a single-`manifest` +artifact — every call, in order, with the id and version each item was stamped +with — is pinned as a literal in +`packages/metadata/src/plugin-artifact-packages-attribution.test.ts` and was +recorded identically on both legs of the ablation. A real boot of +`examples/app-todo` answers every door identically before and after. + +**Nothing a booted instance can see today disappears.** Every live +`ARTIFACT_FIELD_TO_TYPE` key is a member of `AssembledPackageBodySchema` +(measured, not assumed), so iterating bodies loses no collection; and because +`packages` composes by `concat`, an artifact whose top level carries a +definition no package body repeats keeps it — registered once, attributed to the +artifact's own identity, and logged, because it means the artifact's two halves +disagree about what it ships. + +⛔ The **producer** half is untouched: `composeStacks` and `os build` keep +emitting the flattened top level alongside `packages[]`. Whether they should is +#14512's decision, not this door's. diff --git a/packages/objectql/src/artifact-packages.ts b/packages/core/src/artifact-packages.ts similarity index 91% rename from packages/objectql/src/artifact-packages.ts rename to packages/core/src/artifact-packages.ts index 44300c90eb..06ab9989d9 100644 --- a/packages/objectql/src/artifact-packages.ts +++ b/packages/core/src/artifact-packages.ts @@ -9,6 +9,26 @@ * turns an artifact — either shape — into the ordered list of manifests the * load path registers. * + * ## Why this lives in `@objectstack/core` and not next to one of its readers + * + * There are TWO readers of `packages[]`, not one, and they sit in packages that + * cannot import each other: `ObjectQLPlugin` (`@objectstack/objectql`, which + * calls `registerApp` per package) and `MetadataPlugin` + * (`@objectstack/metadata`, whose artifact door registers each package body's + * collections stamped with that body's id). `@objectstack/objectql` depends on + * `@objectstack/metadata`, so the metadata door physically cannot import this + * module from where it started life. + * + * The alternative — a second read of `packages[]` inside the metadata door — + * would have split more than the sort: this function is also the GATE (it + * parses each entry against `ArtifactPackageSchema` and refuses a duplicate + * package id), so two readers would have disagreed about which artifacts are + * loadable, not just about what order to load them in. `@objectstack/core` + * already owns `resolvePluginOrder` and is already a dependency of both + * readers, so hosting it here adds NO package edge to the graph. ⛔ Do not + * re-home this next to either reader; the next reader will have the same + * problem. + * * ## Both shapes are read (D4), and the fallback is the compatibility mechanism * * - `packages` present → iterate it. @@ -102,7 +122,7 @@ * missing-dependency semantics are re-adjudicated here. */ -import { resolvePluginOrder, type OrderablePlugin } from '@objectstack/core'; +import { resolvePluginOrder, type OrderablePlugin } from './plugin-order.js'; import { ArtifactPackageSchema } from '@objectstack/spec'; /** diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index e824f0db75..7e9d3eb3f0 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -10,6 +10,13 @@ export * from './kernel-base.js'; export * from './kernel.js'; export * from './plugin-order.js'; +// ADR-0130 D4/D5 — the ONE reader of a release artifact's `packages[]`, and the +// ONE place it is ordered. It lives here rather than beside a reader because it +// has two of them in packages that cannot import each other (`@objectstack/ +// objectql`'s load path and `@objectstack/metadata`'s artifact door), and +// because the ordering it performs is `resolvePluginOrder` directly above. +// `@objectstack/objectql` re-exports it, so its published surface is unchanged. +export * from './artifact-packages.js'; export * from './lite-kernel.js'; export * from './types.js'; export * from './logger.js'; diff --git a/packages/metadata/src/plugin-artifact-packages-attribution.test.ts b/packages/metadata/src/plugin-artifact-packages-attribution.test.ts new file mode 100644 index 0000000000..6309f93226 --- /dev/null +++ b/packages/metadata/src/plugin-artifact-packages-attribution.test.ts @@ -0,0 +1,320 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #14599 — the metadata artifact door attributed EVERY item of a multi-package + * artifact to the artifact's own `manifest.id`. + * + * --------------------------------------------------------------------------- + * The defect + * --------------------------------------------------------------------------- + * `_parseAndRegisterArtifact` iterated the FLATTENED top level and stamped each + * item with `metadata.manifest.id`. For an artifact composed with + * `composeStacks(…, { manifest: 'preserve' })` that id is one arbitrary + * member's — `selectManifest`'s `'last'` pick — so a two-package artifact + * registered the module's object under the APP package's identity, while the + * ObjectQL load path, reading the same artifact's `packages[]`, owned it under + * the module's. The platform then held two answers to "who owns `crm_order`": + * `GET /api/v1/meta/object` served the row TWICE (the list merge keys slots by + * `${packageId}${name}`), `?package=` returned the module's object, + * and the layers door and the item door named different owners. + * + * --------------------------------------------------------------------------- + * What is driven, and why the fixture is composed rather than written out + * --------------------------------------------------------------------------- + * Every case goes through `_parseAndRegisterArtifact` — the real door, shared + * by the boot load and the HMR reload — and the two-package fixture is built by + * calling the real producer, `composeStacks(…, { manifest: 'preserve' })`, on + * two ordinary `defineStack`-shaped inputs. A hand-written `packages[]` would + * pin this door against a shape nothing emits; composing means the fixture + * tracks the producer, so the day the producer half of #14512 lands (it stops + * emitting the flattened top level for multi-package artifacts) these cases + * keep asserting the same thing about the same bytes. + * + * The inputs mirror `examples/app-multi-package`, INCLUDING its two deliberate + * properties: the module is listed FIRST (so array order is not what decides + * registration order) and it declares `dependencies` on the app package (so + * `resolveArtifactPackageOrder`'s topological sort is what does). + * + * --------------------------------------------------------------------------- + * The D7 control + * --------------------------------------------------------------------------- + * `singlePackageRegisterSequence` is the whole of the single-`manifest` + * branch's behaviour as a literal: every `manager.register` call, in order, + * with the id and version each item was stamped with. ADR-0130 D7 is that this + * branch did not move, and this literal is what would go red if it ever did. + * It was recorded on both legs of the ablation in the PR body, from the same + * fixture, and compared byte for byte. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { composeStacks } from '@objectstack/spec'; +import { MetadataPlugin } from './plugin.js'; + +const CORE_ID = 'com.example.multi.core'; +const ORDERS_ID = 'com.example.multi.orders'; + +const coreStack = { + manifest: { + id: CORE_ID, + name: 'Multi-Package Core', + namespace: 'crm', + version: '1.0.0', + type: 'app', + engines: { protocol: '^17' }, + }, + objects: [ + { + name: 'crm_account', + label: 'Account', + sharingModel: 'private', + fields: { name: { name: 'name', type: 'text', label: 'Account Name', required: true } }, + }, + ], + apps: [ + { + name: 'multi_crm', + label: 'Multi-Package CRM', + navigation: [ + { id: 'nav_accounts', type: 'object', objectName: 'crm_account', label: 'Accounts' }, + ], + }, + ], +}; + +const ordersStack = { + manifest: { + id: ORDERS_ID, + name: 'Multi-Package Orders', + namespace: 'crm', + // A DIFFERENT version from the app package on purpose: `_packageVersion` + // is stamped from the same body as `_packageId`, so a shared version + // would let a wrong-body stamp pass unnoticed. + version: '2.4.0', + type: 'module', + engines: { protocol: '^17' }, + dependencies: { [CORE_ID]: '^1.0.0' }, + }, + objects: [ + { + name: 'crm_order', + label: 'Order', + sharingModel: 'private', + fields: { + name: { name: 'name', type: 'text', label: 'Order Number', required: true }, + account: { name: 'account', type: 'lookup', label: 'Account', reference: 'crm_account' }, + }, + }, + ], + views: [ + { + object: 'crm_order', + list: { label: 'All Orders', type: 'grid', columns: [{ field: 'name' }] }, + }, + ], +}; + +/** The module is listed FIRST — array order must not be what orders the load. */ +const twoPackageArtifact = () => + JSON.parse(JSON.stringify(composeStacks( + [ordersStack, coreStack] as never, + { manifest: 'preserve' }, + ))); + +/** A single-`manifest` artifact — the D7 branch, no `packages` key anywhere. */ +const singlePackageArtifact = () => JSON.parse(JSON.stringify({ + manifest: coreStack.manifest, + objects: coreStack.objects, + apps: coreStack.apps, + views: ordersStack.views, +})); + +function fakeCtx() { + return { + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + registerService: vi.fn(), + getService: vi.fn(() => undefined), + trigger: vi.fn(), + } as any; +} + +interface RegisterCall { type: string; name: string; packageId?: string; packageVersion?: string } + +/** + * Drive the real door and record every `manager.register` call in order, with + * the provenance the item carried at the moment it was registered. + */ +async function load(artifact: unknown): Promise<{ + plugin: any; + ctx: any; + total: number; + calls: RegisterCall[]; +}> { + const plugin = new MetadataPlugin({ watch: false, config: { bootstrap: 'lazy' } }) as any; + const ctx = fakeCtx(); + const calls: RegisterCall[] = []; + const realRegister = plugin.manager.register.bind(plugin.manager); + plugin.manager.register = async (type: string, name: string, item: any, opts: any) => { + calls.push({ + type, + name, + packageId: item?._packageId, + packageVersion: item?._packageVersion ?? undefined, + }); + return realRegister(type, name, item, opts); + }; + const total = await plugin._parseAndRegisterArtifact(ctx, artifact, 'fixture-14599'); + return { plugin, ctx, total, calls }; +} + +const stampOf = async (plugin: any, type: string, name: string) => { + const item = (await plugin.manager.get(type, name)) as any; + return item === undefined + ? undefined + : { packageId: item._packageId, packageVersion: item._packageVersion ?? undefined }; +}; + +describe('#14599 artifact door — a `packages[]` artifact is registered per package', () => { + it('the fixture carries the card shape (premise guard)', () => { + const artifact = twoPackageArtifact(); + + // Two package bodies… + expect(artifact.packages).toHaveLength(2); + expect(artifact.packages.map((e: any) => e.manifest.id).sort()) + .toEqual([CORE_ID, ORDERS_ID]); + + // …AND the flattened top level carrying the same definitions, which is + // what `preserve` being ADDITIVE means. If the producer half of #14512 + // ever lands, THIS is the assertion that will report it — and the cases + // below keep passing, because they read the bodies. + expect(artifact.objects.map((o: any) => o.name).sort()) + .toEqual(['crm_account', 'crm_order']); + + // The divergence itself: the artifact's own identity is the App + // package, so the pre-fix door stamped the MODULE's object with it. + expect(artifact.manifest.id).toBe(CORE_ID); + }); + + it('stamps every item with the package that owns it, not the artifact manifest', async () => { + const { plugin } = await load(twoPackageArtifact()); + + // The card's headline: pre-fix this was `com.example.multi.core`. + expect(await stampOf(plugin, 'object', 'crm_order')) + .toEqual({ packageId: ORDERS_ID, packageVersion: '2.4.0' }); + + expect(await stampOf(plugin, 'object', 'crm_account')) + .toEqual({ packageId: CORE_ID, packageVersion: '1.0.0' }); + expect(await stampOf(plugin, 'app', 'multi_crm')) + .toEqual({ packageId: CORE_ID, packageVersion: '1.0.0' }); + + // View containers take the same route through the door's other + // registration site, container and expansions alike. + expect(await stampOf(plugin, 'view', 'crm_order')) + .toEqual({ packageId: ORDERS_ID, packageVersion: '2.4.0' }); + expect(await stampOf(plugin, 'view', 'crm_order.default')) + .toEqual({ packageId: ORDERS_ID, packageVersion: '2.4.0' }); + }); + + it('does not register the flattened top-level copy a second time', async () => { + const { total, calls } = await load(twoPackageArtifact()); + + // One register call per definition. Pre-fix the door made exactly these + // calls too — but ALL of them from the flattened top level, all stamped + // with the artifact manifest; the SECOND copy the list door served came + // from the registry's own per-package registration, which the top-level + // copy then collided with under a different `${packageId}${name}` slot. + // What this pins is that the door does not itself contribute a second, + // differently-attributed copy of any definition. + const slots = calls.map((c) => `${c.type}:${c.name}`); + expect(slots).toEqual([...new Set(slots)]); + expect(total).toBe(calls.length); + + // No item is stamped with an id that owns no such definition. + const byPackage = new Map(); + for (const c of calls) { + const list = byPackage.get(c.packageId!) ?? []; + list.push(`${c.type}:${c.name}`); + byPackage.set(c.packageId!, list); + } + expect([...byPackage.keys()].sort()).toEqual([CORE_ID, ORDERS_ID]); + expect(byPackage.get(CORE_ID)!.sort()).toEqual(['app:multi_crm', 'object:crm_account']); + expect(byPackage.get(ORDERS_ID)!.sort()) + .toEqual(['object:crm_order', 'view:crm_order', 'view:crm_order.default']); + }); + + it('registers the packages in topological order, not array order', async () => { + // The fixture lists `orders` first and `orders` depends on `core`. + // Registration order comes from `resolveArtifactPackageOrder`, the same + // call the ObjectQL load path makes (ADR-0130 D5) — reused, so the two + // readers of one `packages[]` cannot disagree. + const { calls } = await load(twoPackageArtifact()); + + const firstCore = calls.findIndex((c) => c.packageId === CORE_ID); + const firstOrders = calls.findIndex((c) => c.packageId === ORDERS_ID); + expect(firstCore).toBeGreaterThanOrEqual(0); + expect(firstOrders).toBeGreaterThanOrEqual(0); + expect(firstCore).toBeLessThan(firstOrders); + + // Each package's items are contiguous — one pass per body, not one + // interleaved pass over a merged list. + const ids = calls.map((c) => c.packageId); + expect(ids).toEqual([...ids].sort((a, b) => (a === b ? 0 : a === CORE_ID ? -1 : 1))); + }); + + it('logs nothing about unowned items when every top-level item has an owner', async () => { + // The residual sweep exists for artifacts whose top level carries + // collections no package body repeats (`packages` is a `concat` key). + // For a normally composed artifact it must register NOTHING and warn + // about nothing — a warning here would mean the sweep is double-reading. + const { ctx } = await load(twoPackageArtifact()); + + const warnings = ctx.logger.warn.mock.calls.map((c: unknown[]) => String(c[0])); + expect(warnings.filter((w: string) => w.includes('top-level metadata item'))).toEqual([]); + }); + + it('keeps a top-level item that NO package body declares, attributed to the artifact', async () => { + // `packages` composes by `concat`, so an artifact built from one stack + // that already carried `packages` and one that did not has top-level + // collections outside every body. Dropping those would take metadata a + // booted instance can see today off every door. + const artifact = twoPackageArtifact(); + artifact.objects.push({ + name: 'crm_orphan', + label: 'Orphan', + sharingModel: 'private', + fields: { name: { name: 'name', type: 'text', label: 'Name', required: true } }, + }); + + const { plugin, ctx } = await load(artifact); + + expect(await stampOf(plugin, 'object', 'crm_orphan')) + .toEqual({ packageId: CORE_ID, packageVersion: '1.0.0' }); + // …and it is said out loud, because it means the artifact's two halves + // disagree about what it ships. + const warnings = ctx.logger.warn.mock.calls.map((c: unknown[]) => String(c[0])); + expect(warnings.some((w: string) => w.includes('top-level metadata item'))).toBe(true); + + // The owned definitions keep their own packages — the sweep never + // overwrites a body's copy with the flattened one. + expect(await stampOf(plugin, 'object', 'crm_order')) + .toEqual({ packageId: ORDERS_ID, packageVersion: '2.4.0' }); + }); + + it('D7: the single-`manifest` branch registers exactly what it always did', async () => { + const { total, calls, ctx } = await load(singlePackageArtifact()); + + // The literal — see the file header. Every call, in order, with its + // stamp. Recorded identically on both legs of the ablation. + const singlePackageRegisterSequence: RegisterCall[] = [ + { type: 'object', name: 'crm_account', packageId: CORE_ID, packageVersion: '1.0.0' }, + { type: 'app', name: 'multi_crm', packageId: CORE_ID, packageVersion: '1.0.0' }, + { type: 'view', name: 'crm_order', packageId: CORE_ID, packageVersion: '1.0.0' }, + { type: 'view', name: 'crm_order.default', packageId: CORE_ID, packageVersion: '1.0.0' }, + ]; + expect(calls).toEqual(singlePackageRegisterSequence); + expect(total).toBe(singlePackageRegisterSequence.length); + + // No `packages` key ⇒ no residual sweep ⇒ nothing to warn about. + const warnings = ctx.logger.warn.mock.calls.map((c: unknown[]) => String(c[0])); + expect(warnings.filter((w: string) => w.includes('top-level metadata item'))).toEqual([]); + }); +}); diff --git a/packages/metadata/src/plugin.ts b/packages/metadata/src/plugin.ts index 0b22c5f2af..ac95906ed1 100644 --- a/packages/metadata/src/plugin.ts +++ b/packages/metadata/src/plugin.ts @@ -3,6 +3,15 @@ import { readFile } from 'node:fs/promises'; import { createHash } from 'node:crypto'; import { Plugin, PluginContext } from '@objectstack/core'; +// ADR-0130 D4/D5 — the ONE reader of a release artifact's `packages[]`. +// It lives in `@objectstack/core` rather than in the load path that first +// needed it because `@objectstack/objectql` depends on THIS package, so the +// artifact door could not import it from there; hosting it in a package both +// readers already depend on adds no edge to the graph. Reused, never +// re-derived: it is the ordering (D5) AND the entry gate, so a second read of +// `packages[]` here would make the two readers disagree about which artifacts +// are loadable at all. +import { resolveArtifactPackageOrder, artifactPackageId } from '@objectstack/core'; import { NodeMetadataManager } from './node-metadata-manager.js'; import { MemoryLoader } from './loaders/memory-loader.js'; import { DEFAULT_METADATA_TYPE_REGISTRY } from '@objectstack/spec/kernel'; @@ -931,9 +940,165 @@ export class MetadataPlugin implements Plugin { const manifestVersion = (metadata as any)?.manifest?.version ?? (metadata as any)?.version ?? undefined; + // ── ADR-0130 D4, at the metadata door: read BOTH shapes, and attribute + // every item to the body it was FOUND in ─────────────────────────── + // + // `packages` ABSENT → the artifact itself IS the one package's body and + // `manifest.id` is its owner (D7). That is the branch every artifact + // built to date takes and it must not move: the loop below runs exactly + // once, over `metadata`, stamped with the two ids computed above. + // + // `packages` PRESENT → the artifact carries N assembled package bodies, + // each `{ ...manifest, ...collections }` (`AssembledPackageBodySchema`), + // whose collections are declared by the same shape the top level parses + // against. Measured, not assumed: every live `ARTIFACT_FIELD_TO_TYPE` + // key is a member of that schema, so iterating bodies loses no + // collection. Registering from the BODY is what gives each item the id + // of the package that owns it. Before this, the flattened top level was + // the only thing read here and every item took the ARTIFACT's + // `manifest.id` — which for a composed artifact is one arbitrary + // member's id (`selectManifest`'s `'last'` pick). So a two-package + // artifact registered the module's object under the app's id, the + // registry owned the same object under the module's, and the platform + // held two answers to "who owns this": the layers door said one, the + // item door and `GET /api/v1/packages` said the other, and the list + // door — keyed `${packageId}${name}` — served the object TWICE (#14599). + // + // ⛔ The owner is read off the body the item was found in — never + // reverse-derived by matching a top-level item's name against a + // name-to-package index. Such an index is a SECOND resolution path for + // metadata identity, which is the road #14512's triage rejected by name + // (its option C) precisely because a dangling one fails silently. + // + // Ordering AND the entry gate both come from `resolveArtifactPackageOrder` + // (`@objectstack/core`) — the same call `ObjectQLPlugin` makes on the + // same artifact, reused rather than re-derived (D5). Two readers of one + // `packages[]` that sorted or validated differently would disagree about + // which artifacts load at all, not merely about the order. It returns + // `[artifact]` for the absent branch, which is what lets both branches + // share the one loop. + const carriesPackages = Array.isArray((metadata as any)?.packages); + const bodies = resolveArtifactPackageOrder(metadata) as Record[]; + + // Which `(type, name)` slots a package body claimed. Read only by the + // residual sweep below, to tell "the flattened top level repeats what a + // package already owns" (the normal case, and the duplicate this fixes) + // from "the top level carries something no package declares". + const ownedByPackage = new Map>(); + const claim = (type: string, name: string): void => { + let names = ownedByPackage.get(type); + if (!names) ownedByPackage.set(type, names = new Set()); + names.add(name); + }; + const claimed = (type: string, name: string): boolean => + ownedByPackage.get(type)?.has(name) === true; + + let totalRegistered = 0; + for (const body of bodies) { + totalRegistered += await this._registerArtifactBodyCollections( + ctx, + memLoader, + body, + carriesPackages + // `registerApp` keys an installed package on `id || name` + // and `artifactPackageId` is that ONE spelling, imported + // from the module that also ordered these bodies. Reading + // the id any other way here is how a door and + // `GET /api/v1/packages` come to name different owners. + ? { + packageId: artifactPackageId(body), + packageVersion: (body as any)?.version ?? undefined, + } + // Unchanged single-package branch — the same two values + // this method has always stamped (D7). + : { packageId: manifestPackageId, packageVersion: manifestVersion }, + { claim: carriesPackages ? claim : undefined }, + ); + } + + if (carriesPackages) { + // The residual sweep, and why it registers nothing for a normally + // composed artifact: `composeStacks(…, { manifest: 'preserve' })` + // is ADDITIVE — it emits the flattened top level AND `packages[]` + // from the same inputs — so every top-level item is already owned + // above and this pass is a no-op. (Whether the producer should keep + // emitting that half at all is #14512's decision, not this door's.) + // + // It exists because `packages` is a `concat` key: an artifact + // composed from one stack that already carried `packages` and one + // that did not has top-level collections no package body repeats. + // Dropping those would take metadata a booted instance can see + // today off every door — a silent regression in the name of fixing + // a duplicate. Such an item falls back to the artifact's own + // identity because nothing better exists, and it is logged, because + // it means the artifact's two halves disagree. + const residual = await this._registerArtifactBodyCollections( + ctx, + memLoader, + metadata, + { packageId: manifestPackageId, packageVersion: manifestVersion }, + { skip: claimed }, + ); + totalRegistered += residual; + if (residual > 0) { + ctx.logger.warn( + `[MetadataPlugin] artifact '${label}' carries ${residual} top-level metadata item(s) that ` + + `none of its ${bodies.length} package bodies declare. They were registered under the ` + + `artifact's own manifest id ('${manifestPackageId ?? ''}') because no package in the ` + + 'artifact claims them, so every door will report that id as their owner. Rebuild the ' + + 'artifact so each collection it ships is carried by the package that owns it.', + ); + } + } + + this.manager.registerLoader(memLoader); + ctx.logger.info('[MetadataPlugin] Artifact metadata loaded', { source: label, totalRegistered }); + return totalRegistered; + } + + + /** + * Register ONE artifact body's collections into the MetadataManager. + * + * A "body" is either the whole artifact (the single-package branch, where + * the artifact and its one package are the same object) or one entry of + * `packages[]` (ADR-0130 D4), which is an assembled + * `{ ...manifest, ...collections }` payload carrying the same collection + * keys the top level does. The loop is identical for both — that is the + * point: there is one ingestion of a collection here, not one per shape. + * + * @param provenance - The `(packageId, packageVersion)` every item found in + * this body is stamped with (ADR-0010 §3.7, via `applyProtection`). It is + * the body's OWN identity, never the enclosing artifact's, which is what + * makes a multi-package artifact's items agree with the registry and with + * `GET /api/v1/packages` about who owns them. + * @param slots.claim - Called with every `(type, name)` this pass + * registered. Passed when reading package bodies; the residual sweep uses + * what it recorded. + * @param slots.skip - Consulted before registering each `(type, name)`. + * Passed ONLY by the residual sweep, so a package body's copy is never + * overwritten by the flattened top-level copy of the same definition — + * the overwrite that re-attributed the item to the artifact's manifest. + * ⛔ It is never passed while reading the bodies themselves: two items of + * one name inside one body still register as they always have (last + * wins), because suppressing that would be a behaviour change on the + * single-package branch D7 pins. + * @returns How many items this body registered. + */ + private async _registerArtifactBodyCollections( + ctx: PluginContext, + memLoader: MemoryLoader, + body: Record, + provenance: { packageId?: string; packageVersion?: string }, + slots: { + claim?: (type: string, name: string) => void; + skip?: (type: string, name: string) => boolean; + } = {}, + ): Promise { + const { packageId, packageVersion } = provenance; let totalRegistered = 0; for (const [field, metaType] of Object.entries(ARTIFACT_FIELD_TO_TYPE)) { - const items = (metadata as any)[field]; + const items = (body as any)[field]; if (!Array.isArray(items) || items.length === 0) continue; for (const item of items) { // Expand aggregated view containers into independent ViewItems @@ -950,24 +1115,30 @@ export class MetadataPlugin implements Plugin { // `getViewsByObject()` had nothing to expand. const viewObject = deriveViewContainerObject(item); if (!viewObject) continue; + // Residual sweep only: a package body already registered this + // container, so the flattened copy is the duplicate, not a + // second definition. + if (slots.skip?.('view', viewObject)) continue; applyProtection(item as any, { - packageId: manifestPackageId, - packageVersion: manifestVersion, + packageId: packageId, + packageVersion: packageVersion, }); await memLoader.save('view', viewObject, item); await this.manager.register('view', viewObject, item, { notify: false }); totalRegistered++; + slots.claim?.('view', viewObject); for (const vi of expandViewContainer(viewObject, item)) { for (const w of vi._diagnostics?.warnings ?? []) { ctx.logger.warn(`[MetadataPlugin] View expansion warning for '${vi.name}': ${w.message}`); } applyProtection(vi as any, { - packageId: manifestPackageId, - packageVersion: manifestVersion, + packageId: packageId, + packageVersion: packageVersion, }); await memLoader.save('view', vi.name, vi); await this.manager.register('view', vi.name, vi, { notify: false }); totalRegistered++; + slots.claim?.('view', vi.name); } continue; } @@ -998,22 +1169,23 @@ export class MetadataPlugin implements Plugin { } } if (!name) continue; + // Residual sweep only — see the call site: this slot is already + // owned by one of the artifact's package bodies. + if (slots.skip?.(metaType, name)) continue; // ADR-0010 §3.7 — translate the author-facing // `protection` block into the private `_lock` envelope // and stamp package provenance in one call. Strips the // public block so it never lands in sys_metadata. applyProtection(item as any, { - packageId: manifestPackageId, - packageVersion: manifestVersion, + packageId: packageId, + packageVersion: packageVersion, }); await memLoader.save(metaType, name, item); await this.manager.register(metaType, name, item, { notify: false }); totalRegistered++; + slots.claim?.(metaType, name); } } - - this.manager.registerLoader(memLoader); - ctx.logger.info('[MetadataPlugin] Artifact metadata loaded', { source: label, totalRegistered }); return totalRegistered; } diff --git a/packages/objectql/src/artifact-load-path.test.ts b/packages/objectql/src/artifact-load-path.test.ts index d09f7ce8f8..7d54f732dd 100644 --- a/packages/objectql/src/artifact-load-path.test.ts +++ b/packages/objectql/src/artifact-load-path.test.ts @@ -40,7 +40,7 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { ObjectKernel } from '@objectstack/core'; import { ObjectQLPlugin } from './plugin.js'; -import { resolveArtifactPackageOrder } from './artifact-packages.js'; +import { resolveArtifactPackageOrder } from '@objectstack/core'; import type { ObjectQL } from './engine.js'; import type { IMetadataService } from '@objectstack/spec/contracts'; diff --git a/packages/objectql/src/index.ts b/packages/objectql/src/index.ts index 9d4a354b8a..050dbd11a6 100644 --- a/packages/objectql/src/index.ts +++ b/packages/objectql/src/index.ts @@ -423,5 +423,10 @@ export type { AuthzInvalidationBridgeOptions } from './authz-invalidation-bridge // door that grows an artifact-loading seam (the CLI, the marketplace install // path, the metadata dev-artifact loader) adopts the SAME read of both shapes // and the SAME single sorter, instead of each re-deriving one. -export { resolveArtifactPackageOrder } from './artifact-packages.js'; -export type { ArtifactPackageError } from './artifact-packages.js'; +// ADR-0130 D4/D5. The implementation moved to `@objectstack/core` when the +// metadata artifact door became its second reader (#14599) — `@objectstack/ +// objectql` cannot host a module `@objectstack/metadata` has to import, since +// this package depends on that one. Re-exported, not re-declared: this +// package's published surface is exactly what it was. +export { resolveArtifactPackageOrder } from '@objectstack/core'; +export type { ArtifactPackageError } from '@objectstack/core'; diff --git a/packages/objectql/src/plugin.ts b/packages/objectql/src/plugin.ts index 32694183ff..a0cf8a5a41 100644 --- a/packages/objectql/src/plugin.ts +++ b/packages/objectql/src/plugin.ts @@ -4,7 +4,7 @@ import { ObjectQL } from './engine.js'; import { assembleMetadataProtocol } from '@objectstack/metadata-protocol'; import type { MetadataAuthoringChannel } from '@objectstack/metadata-protocol'; import { Plugin, PluginContext } from '@objectstack/core'; -import { resolveArtifactPackageOrder, artifactPackageId } from './artifact-packages.js'; +import { resolveArtifactPackageOrder, artifactPackageId } from '@objectstack/core'; import { applyConversionsToStoredItem } from '@objectstack/spec'; import { StorageNameMapping } from '@objectstack/spec/system'; import { LifecycleService } from './lifecycle/lifecycle-service.js'; diff --git a/packages/objectql/src/registry.ts b/packages/objectql/src/registry.ts index 5a2abe9e74..744201953f 100644 --- a/packages/objectql/src/registry.ts +++ b/packages/objectql/src/registry.ts @@ -41,7 +41,7 @@ import { applyProtection } from '@objectstack/spec/shared'; // (`id || name`). The install gate's co-ownership set must name packages by the // same string the artifact loader ordered them by, or a co-owner would be // admitted — or refused — under a key nothing else in the path uses. -import { artifactPackageId } from './artifact-packages.js'; +import { artifactPackageId } from '@objectstack/core'; /** * Reserved namespaces that do not get FQN prefix applied. diff --git a/packages/qa/dogfood/test/multi-package-artifact.dogfood.test.ts b/packages/qa/dogfood/test/multi-package-artifact.dogfood.test.ts index 2031a6ad59..d83ea1a5f2 100644 --- a/packages/qa/dogfood/test/multi-package-artifact.dogfood.test.ts +++ b/packages/qa/dogfood/test/multi-package-artifact.dogfood.test.ts @@ -25,16 +25,29 @@ // // Boots a fixture stack of its own, so it stays out of `SHARED_SHOWCASE`. +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import multiPackageStack from '@objectstack/example-multi-package'; import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { MetadataPlugin } from '@objectstack/metadata'; +import { writeBuildShapedArtifact } from './build-shaped-artifact.js'; const CORE = 'com.example.multi.core'; const ORDERS = 'com.example.multi.orders'; /** One row of `GET /api/v1/packages`, as far as these pins read it. */ interface PackageRow { - manifest?: { id?: string; type?: string; namespace?: string; scope?: string }; + manifest?: { + id?: string; + type?: string; + namespace?: string; + scope?: string; + /** The definitions this package owns — an ASSEMBLED body's `objects` (#14599). */ + objects?: Array<{ name?: string }>; + }; writable?: boolean; } @@ -122,3 +135,157 @@ describe('dogfood: one artifact, two co-owning packages (ADR-0130 D4)', () => { expect([...ql.registry.getNamespaceOwners('crm')].sort()).toEqual([CORE, ORDERS]); }); }); + +// ─────────────────────────────────────────────────────────────────────────── +// #14599 — the METADATA door, which the block above does not reach +// ─────────────────────────────────────────────────────────────────────────── +// +// `bootStack` registers `AppPlugin` and no `MetadataPlugin`, so everything +// above is measured on the ObjectQL registry alone. The defect this block pins +// lives in the OTHER reader — `MetadataPlugin._parseAndRegisterArtifact`, the +// door a real `objectstack dev` / `objectstack serve` boot loads the artifact +// through — and the two only disagree once both are present. That is why this +// block boots the same stack a second time WITH the artifact door mounted, over +// a build-shaped artifact written to a temp file (`artifactSource`), exactly as +// `showcase-object-extension-meta-read.dogfood.test.ts` does. +// +// What it pins, in the words of the card: +// +// 1. `GET /api/v1/meta/object` serves ONE `crm_order` row and ONE +// `crm_account` row, each carrying its OWN package id. Before the fix the +// door registered the flattened top level stamped with the ARTIFACT's +// `manifest.id` (the App package — `selectManifest`'s `'last'` pick), the +// registry owned the same object under the module's id, and the list merge +// — keyed `${packageId}${name}` — served `crm_order` TWICE. +// 2. `?package=` returns exactly what `GET /api/v1/packages` says that +// package owns. Before the fix `?package=` returned the MODULE's +// object, because the core-stamped copy was re-ingested as the App +// package's contribution to `crm_order`. +// 3. The layers door and the item door name the SAME owner. Before the fix +// layers said the App package and the item door said the module — one +// platform, two answers to "who owns `crm_order`". +// +// ⚠️ This suite resolves `@objectstack/metadata` through its `dist/`, so an +// ablation of the door must REBUILD before each leg or it measures the previous +// build (see `scripts/ablation-dist-preflight.mjs`). + +/** One row of a `/meta/` list read, as far as these pins read it. */ +interface MetaRow { name?: string; _packageId?: string; _packageVersion?: string | null } + +describe('dogfood: the metadata door attributes a two-package artifact per package (#14599)', () => { + let stack: VerifyStack; + let token: string; + let tempDir: string; + /** `GET /api/v1/packages` from THIS boot — the door `?package=` is checked against. */ + let packageRows: PackageRow[]; + + beforeAll(async () => { + tempDir = mkdtempSync(join(tmpdir(), 'os-14599-mp-')); + const artifactPath = join(tempDir, 'objectstack.json'); + // The real build lowering, not `JSON.stringify(stack)` (#6293). + writeBuildShapedArtifact(multiPackageStack as unknown as Record, artifactPath); + + stack = await bootStack(multiPackageStack, { + extraPlugins: [ + new MetadataPlugin({ + rootDir: tempDir, + watch: false, + artifactWatch: false, + registerSystemObjects: false, + artifactSource: { mode: 'local-file', path: artifactPath }, + }), + ], + }); + token = await stack.signIn(); + + const pkgRes = await stack.apiAs(token, 'GET', '/packages'); + expect(pkgRes.status, 'GET /api/v1/packages').toBe(200); + const pkgBody = (await pkgRes.json()) as { data?: { packages?: PackageRow[] } }; + expect( + Array.isArray(pkgBody.data?.packages), + `GET /packages answered ${JSON.stringify(pkgBody).slice(0, 300)}`, + ).toBe(true); + packageRows = pkgBody.data?.packages as PackageRow[]; + }, 300_000); + + afterAll(async () => { + await stack?.stop?.(); + if (tempDir) rmSync(tempDir, { recursive: true, force: true }); + }); + + /** + * Read a `/meta/` list door. The shape is ASSERTED rather than defaulted: + * a `?? []` over a key the door does not send would turn "the door answered + * something else" into "there are no rows", and a duplicate-row pin that reads + * an empty list passes for the wrong reason. + */ + const metaList = async (path: string): Promise => { + const res = await stack.apiAs(token, 'GET', path); + expect(res.status, `GET /api/v1${path}`).toBe(200); + const body = (await res.json()) as { items?: MetaRow[] }; + expect(Array.isArray(body.items), `GET /api/v1${path} answered ${JSON.stringify(body).slice(0, 300)}`) + .toBe(true); + return body.items as MetaRow[]; + }; + + const crmRows = (rows: MetaRow[]): MetaRow[] => + rows.filter((r) => typeof r.name === 'string' && r.name.startsWith('crm_')); + + it('serves each object exactly ONCE, owned by the package that declares it', async () => { + const rows = crmRows(await metaList('/meta/object')); + + // The duplicate, as a count: this was 3 rows for 2 objects. + expect(rows.map((r) => r.name).sort()).toEqual(['crm_account', 'crm_order']); + + const byName = new Map(rows.map((r) => [r.name, r])); + // The card's headline. Pre-fix ONE of the two `crm_order` rows carried this + // id and the other carried the App package's. + expect(byName.get('crm_order')?._packageId).toBe(ORDERS); + expect(byName.get('crm_account')?._packageId).toBe(CORE); + }); + + it('`?package=` returns exactly what `GET /api/v1/packages` says that package owns', async () => { + const core = crmRows(await metaList(`/meta/object?package=${CORE}`)); + const orders = crmRows(await metaList(`/meta/object?package=${ORDERS}`)); + + // Pre-fix the App package's read returned BOTH objects — Studio's Data + // pillar for the App package listed the module's Order alongside Account. + expect(core.map((r) => r.name)).toEqual(['crm_account']); + expect(orders.map((r) => r.name)).toEqual(['crm_order']); + + // Agreement with the package door, not two independently asserted lists: + // the defect was precisely that these two doors disagreed. + const owned = (id: string) => { + const row = packageRows.find((r) => r.manifest?.id === id); + const objects = row?.manifest?.objects; + expect(Array.isArray(objects), `GET /packages row '${id}' carries no \`objects\``).toBe(true); + return (objects as Array<{ name?: string }>).map((o) => o?.name).sort(); + }; + expect(core.map((r) => r.name).sort()).toEqual(owned(CORE)); + expect(orders.map((r) => r.name).sort()).toEqual(owned(ORDERS)); + }); + + it('the layers door and the item door name the SAME owner', async () => { + const layersRes = await stack.apiAs(token, 'GET', '/meta/object/crm_order/layers'); + expect(layersRes.status, 'GET /api/v1/meta/object/crm_order/layers').toBe(200); + const layers = (await layersRes.json()) as { + code?: { _packageId?: string }; packageId?: string; provenance?: string; + }; + + // Pre-fix: `com.example.multi.core` in both slots — the metadata service's + // own copy, stamped with the artifact manifest. + expect(layers.code?._packageId).toBe(ORDERS); + expect(layers.packageId).toBe(ORDERS); + + // The item door, asked under the WRONG package on purpose: it answered + // `orders` even pre-fix, and that disagreement with the layers door above + // is what "the platform holds two answers" meant. + const itemRes = await stack.apiAs(token, 'GET', `/meta/object/crm_order?package=${CORE}`); + expect(itemRes.status).toBe(200); + const item = (await itemRes.json()) as { item?: { _packageId?: string }; packageId?: string }; + + expect(item.item?._packageId).toBe(ORDERS); + expect(item.packageId).toBe(ORDERS); + expect(item.packageId).toBe(layers.packageId); + }); +}); diff --git a/packages/runtime/src/dispatcher-error-vocabulary.ts b/packages/runtime/src/dispatcher-error-vocabulary.ts index 3bc491c12d..9a4073efaa 100644 --- a/packages/runtime/src/dispatcher-error-vocabulary.ts +++ b/packages/runtime/src/dispatcher-error-vocabulary.ts @@ -656,7 +656,7 @@ export const UNREGISTERED_CODE_SITES: readonly UnregisteredCodeSite[] = [ // registered code, so they are not demoted at a door, they never reach one. { code: 'INVALID_ARTIFACT_PACKAGES', - file: 'packages/objectql/src/artifact-packages.ts', + file: 'packages/core/src/artifact-packages.ts', shape: 'codehelper', door: 'none', verdict: 'boot-refusal', @@ -676,7 +676,7 @@ export const UNREGISTERED_CODE_SITES: readonly UnregisteredCodeSite[] = [ }, { code: 'INVALID_ARTIFACT_PACKAGE_ENTRY', - file: 'packages/objectql/src/artifact-packages.ts', + file: 'packages/core/src/artifact-packages.ts', shape: 'codehelper', door: 'none', verdict: 'boot-refusal', @@ -697,7 +697,7 @@ export const UNREGISTERED_CODE_SITES: readonly UnregisteredCodeSite[] = [ }, { code: 'DUPLICATE_ARTIFACT_PACKAGE', - file: 'packages/objectql/src/artifact-packages.ts', + file: 'packages/core/src/artifact-packages.ts', shape: 'codehelper', door: 'none', verdict: 'boot-refusal',