fix(objectql): MetadataFacade object writes now reach the map its reads use (#6725)

.changeset/metadata-facade-object-write-read-split.md

@@ -0,0 +1,67 @@
+---
+'@objectstack/objectql': patch
+---
+
+**`MetadataFacade.register('object', …)` now writes where its own object reads
+look — it was a silent no-op before (#6725).**
+
+`MetadataFacade` is exported from `@objectstack/objectql`'s root and `core`
+entrypoints for hosts that want to occupy the kernel's `metadata` slot with a
+`SchemaRegistry`-backed service. Its object write went through
+`SchemaRegistry.registerItem`, which stores into the generic `metadata` map —
+and **every one of its object reads resolves from `objectContributors`**, which
+only `registerObject` populates:
+
+- `getObject(name)` → `registry.getObject`;
+- `get('object', name)` and `exists('object', name)` → `registry.getItem`, which
+  special-cases the object type straight back to `registry.getObject`;
+- `list('object')` and `listNames('object')` → `registry.listItems`, which
+  special-cases to `registry.getAllObjects`.
+
+So an object registered through the facade was readable back through **none** of
+them: `register` resolved successfully and every subsequent read answered
+`undefined` / `[]`. `IMetadataService` (`@objectstack/spec/contracts`) declares
+`getObject(name)` ≡ `get('object', name)` and its own conformance test
+round-trips a `register('object', …)` through both members, so this was a
+shipped contract that could not work. Dormant in-tree only because nothing on
+`main` installs a `MetadataFacade` into the `metadata` slot — a downstream host
+that did (cloud, a third-party kernel) got the split, including the
+write-then-read in ObjectQL's own `bridgeObjectsToMetadataService`, whose
+"already registered?" probe would never answer and so re-registered the full
+object set on every boot.
+
+**What changed.** The facade's object write now performs *both* halves of the
+two-place write the registry documents for a runtime-authored object
+(`SchemaRegistry.unregisterObject`'s header states the invariant; the in-tree
+precedent is `MetadataProtocol.applyObjectRegistryMutation`, which does exactly
+this): `registerObject` for the contributor entry every read resolves, plus the
+existing `registerItem` for the stored document. Both spellings of the type
+(`'object'` and `'objects'`) are covered, because both are special-cased on the
+read side.
+
+Consequences a caller can observe:
+
+- `getObject` / `get` / `exists` / `list` / `listNames` / `listObjects` now
+  answer a facade-registered object. What they answer is the **runtime-effective**
+  object the contract promises: system columns injected, primary title
+  designated, `extend` contributions merged.
+- An object arriving with no `_packageId` is runtime-authored by definition, so
+  it is registered under the platform's `'sys_metadata'` sentinel and stamped
+  `_provenance: 'org'`. It therefore does **not** read as code-shipped —
+  `getArtifactItem` / `isArtifactBacked` exclude both — and does not become
+  un-editable. An object carrying a real `_packageId` is registered under it and
+  keeps `_provenance: 'package'`. The stored document keeps its as-authored,
+  unstamped shape; only the contributor copy carries registry coordinates.
+- `register('object', …)` can now **throw** where it previously succeeded and did
+  nothing: claiming an object another package already owns is refused by
+  ADR-0029. The contributor write runs first so a refusal writes nothing at all.
+- `unregister('object', name)` removes the object from both places. Without this
+  the fix would have re-opened #6808 from the other side — a removal that empties
+  only the generic map leaves `getObject`, which the data plane dispatches on,
+  serving a deleted object for the life of the process. It refuses, per ADR-0029,
+  an object still extended by another package.
+
+No in-tree caller changes behaviour: `new MetadataFacade(...)` appears nowhere on
+`main` outside this package's own tests, and the `metadata` slot is filled by
+`MetadataManager` or `createMemoryMetadata`, both of which already round-tripped
+correctly.

packages/objectql/src/metadata-facade.test.ts

@@ -50,9 +50,181 @@ describe('MetadataFacade provenance passthrough', () => {
 
         // getItem('object', …) routes to the merged-object path, so read the
         // generic collection directly to inspect what register() stored.
+        //
+        // [#6725] The direct read is STILL the right instrument here, and for
+        // the same reason as before: this pin is about the STORED document, and
+        // the two object reads answer the contributor copy — which now exists,
+        // and which deliberately does carry the `'sys_metadata'` sentinel (see
+        // the round-trip suite below). Reading through `get('object', …)` would
+        // silently retarget this assertion at the other document and stop
+        // guarding what it was written to guard.
         const stored = (registry as any).metadata.get('object')?.get('task');
         expect(stored).toBeDefined();
         expect(stored._packageId).toBeUndefined();
         expect(stored._provenance).toBeUndefined();
     });
+
+    it('keeps the stored document unstamped even when the contributor copy is stamped', async () => {
+        // The hazard the copy in `registerObjectBothPlaces` exists for:
+        // `applyProtection` stamps IN PLACE and `applySystemFields` returns its
+        // input unchanged when there is nothing to inject (`systemFields: false`
+        // takes that path), so a shared reference would leak the sentinel into
+        // the entry the pin above guards.
+        await facade.register('object', 'nothing_injected', {
+            name: 'nothing_injected',
+            label: 'No injection',
+            fields: {},
+            systemFields: false,
+        });
+
+        const stored = (registry as any).metadata.get('object')?.get('nothing_injected');
+        expect(stored._packageId).toBeUndefined();
+        expect(stored._provenance).toBeUndefined();
+
+        // …while the contributor copy — a different document — is stamped.
+        expect((registry.getObject('nothing_injected') as any)._packageId).toBe('sys_metadata');
+    });
+});
+
+/**
+ * [#6725] The write/read pin.
+ *
+ * `MetadataFacade.register('object', …)` wrote through `registerItem` into the
+ * generic `metadata` map, while every one of this class's object reads resolves
+ * from `objectContributors` — so an object written through the public facade was
+ * not readable back through the public facade. `IMetadataService`
+ * (`@objectstack/spec/contracts`) declares `getObject(name)` ≡
+ * `get('object', name)` and its own conformance test round-trips a
+ * `register('object', …)` through both members; this file is the gate for the
+ * facade's half of that.
+ *
+ * Refs #6725, #6505 / PR #6723, #6808, ADR-0010, ADR-0029.
+ */
+describe('MetadataFacade object write/read round-trip', () => {
+    let registry: SchemaRegistry;
+    let facade: MetadataFacade;
+
+    beforeEach(() => {
+        registry = new SchemaRegistry({ multiTenant: false });
+        facade = new MetadataFacade(registry);
+    });
+
+    const taskDefinition = () => ({ name: 'task', label: 'Task', fields: {} });
+
+    it('reads a registered object back through BOTH getObject and get', async () => {
+        await facade.register('object', 'task', taskDefinition());
+
+        const viaGetObject = await facade.getObject('task');
+        const viaGet = await facade.get('object', 'task');
+
+        // Anti-vacuity: before the fix both members answered `undefined`, which
+        // an identity assertion alone would have called agreement.
+        expect(viaGetObject).toBeDefined();
+        expect((viaGetObject as any).name).toBe('task');
+        expect((viaGetObject as any).label).toBe('Task');
+        expect(viaGetObject).toBe(viaGet);
+    });
+
+    it('reads it back through the enumeration members too', async () => {
+        await facade.register('object', 'task', taskDefinition());
+
+        expect(await facade.exists('object', 'task')).toBe(true);
+        expect(await facade.listNames('object')).toEqual(['task']);
+        expect(await facade.listObjects()).toHaveLength(1);
+        const listed = await facade.list('object');
+        expect(listed.map((o: any) => o.name)).toEqual(['task']);
+    });
+
+    it('closes the same split for the plural `objects` spelling', async () => {
+        // `registry.getItem` / `listItems` special-case both spellings to the
+        // contributor path, so a write that handled only the singular left this
+        // one broken in exactly the same way.
+        await facade.register('objects', 'lead', { name: 'lead', label: 'Lead', fields: {} });
+
+        expect(await facade.getObject('lead')).toBeDefined();
+        expect(await facade.get('objects', 'lead')).toBeDefined();
+        expect(await facade.get('object', 'lead')).toBeDefined();
+    });
+
+    it('serves the runtime-effective object, as the contract says it does', async () => {
+        // #6505 / PR #6723: `getObject` answers the object as the engine runs
+        // it, not the document its author wrote. The materialization seam is
+        // `registerObject`'s, so it only runs now that the write reaches it.
+        const multiTenantRegistry = new SchemaRegistry({ multiTenant: true });
+        const multiTenantFacade = new MetadataFacade(multiTenantRegistry);
+
+        await multiTenantFacade.register('object', 'task', taskDefinition());
+
+        const effective = (await multiTenantFacade.getObject('task')) as any;
+        expect(effective.fields.organization_id).toBeDefined();
+        expect(effective.fields.created_at).toBeDefined();
+    });
+
+    it('registers a package-less object under the sentinel, not as an artifact', async () => {
+        await facade.register('object', 'task', taskDefinition());
+
+        const owner = registry.getObjectOwner('task');
+        expect(owner?.packageId).toBe('sys_metadata');
+        // ADR-0010: runtime-authored, so it must not read as code-shipped —
+        // `getArtifactItem` is what write authorization consults.
+        expect((registry.getObject('task') as any)._provenance).toBe('org');
+        expect(registry.getArtifactItem('object', 'task')).toBeUndefined();
+    });
+
+    it('registers a package-stamped object under its own package id', async () => {
+        await facade.register('object', 'crm_account', {
+            name: 'crm_account',
+            label: 'Account',
+            fields: {},
+            _packageId: 'com.example.crm',
+        });
+
+        expect(registry.getObjectOwner('crm_account')?.packageId).toBe('com.example.crm');
+        const served = registry.getObject('crm_account') as any;
+        expect(served._packageId).toBe('com.example.crm');
+        expect(served._provenance).toBe('package');
+        expect(registry.getArtifactItem('object', 'crm_account')).toBeDefined();
+    });
+
+    it('re-registering the same object replaces it rather than accumulating owners', async () => {
+        await facade.register('object', 'task', taskDefinition());
+        await facade.register('object', 'task', { ...taskDefinition(), label: 'Task v2' });
+
+        expect(((await facade.getObject('task')) as any).label).toBe('Task v2');
+        expect(registry.getObjectContributors('task')).toHaveLength(1);
+        expect(await facade.listObjects()).toHaveLength(1);
+    });
+
+    it('refuses to claim an object another package owns, and writes nothing', async () => {
+        registry.registerObject({ name: 'task', label: 'Owned', fields: {} } as never, 'com.example.owner');
+
+        // ADR-0029 — one owner per object. The contributor write runs first
+        // precisely so the refusal leaves the generic map untouched too.
+        await expect(
+            facade.register('object', 'task', { ...taskDefinition(), _packageId: 'com.example.other' }),
+        ).rejects.toThrow(/already owned by package "com.example.owner"/);
+
+        expect((registry as any).metadata.get('object')?.size ?? 0).toBe(0);
+        expect(((await facade.getObject('task')) as any).label).toBe('Owned');
+    });
+
+    it('unregisters an object out of BOTH places it was written into', async () => {
+        await facade.register('object', 'task', taskDefinition());
+        expect(await facade.getObject('task')).toBeDefined();
+
+        await facade.unregister('object', 'task');
+
+        // #6808: removing only the generic-map half left `getObject` — what the
+        // data plane dispatches on — serving a deleted object for the life of
+        // the process.
+        expect(await facade.getObject('task')).toBeUndefined();
+        expect(await facade.get('object', 'task')).toBeUndefined();
+        expect(await facade.exists('object', 'task')).toBe(false);
+        expect(await facade.listObjects()).toHaveLength(0);
+        expect((registry as any).metadata.get('object')?.get('task')).toBeUndefined();
+    });
+
+    it('unregistering an object nothing registered stays a no-op', async () => {
+        await expect(facade.unregister('object', 'absent')).resolves.toBeUndefined();
+    });
 });

packages/objectql/src/metadata-facade.ts

@@ -2,6 +2,25 @@
 
 import { SchemaRegistry } from './registry.js';
 
+/**
+ * The two spellings of the object metadata type. `SchemaRegistry.getItem` /
+ * `listItems` special-case BOTH to the contributor path, so a write that
+ * handled only the singular left the plural with the same read/write split
+ * (#6725).
+ */
+function isObjectType(type: string): boolean {
+  return type === 'object' || type === 'objects';
+}
+
+/**
+ * The owning-package id used for an object registered through this facade with
+ * no `_packageId` of its own. The platform's sentinel for "an overlay row bound
+ * to no package" — `isArtifactBacked` (metadata-protocol) and
+ * `SchemaRegistry.getArtifactItem` both exclude it explicitly, so it cannot
+ * turn a runtime-authored object into an artifact-backed one.
+ */
+const RUNTIME_AUTHORED_PACKAGE_ID = 'sys_metadata';
+
 /**
  * MetadataFacade
  *
@@ -32,13 +51,90 @@ export class MetadataFacade {
     // definition and must not become artifact-backed (protocol.ts
     // isArtifactBacked gates write authorization on _packageId).
     const packageId = definition?._packageId;
-    if (type === 'object') {
-      this.registry.registerItem(type, definition, 'name' as any, packageId);
+    if (isObjectType(type)) {
+      this.registerObjectBothPlaces(type, definition, packageId);
     } else {
       this.registry.registerItem(type, definition, definition.id ? 'id' as any : 'name' as any, packageId);
     }
   }
 
+  /**
+   * [#6725] An `object` lives in TWO places in a `SchemaRegistry`, and this
+   * write has to reach both of them.
+   *
+   * `SchemaRegistry.unregisterObject`'s header states the invariant directly:
+   * "a runtime-authored `object` is written into TWO places (`metadata['object']`
+   * via `registerItem` and `objectContributors` via `registerObject`)". This
+   * method used to perform only the first half, so a facade write landed
+   * nowhere any facade read looks — `registerItem` stores into the generic
+   * `metadata` map, while EVERY object read resolves from `objectContributors`:
+   *
+   *   - `getObject`  → `registry.getObject`  → `objectContributors`
+   *   - `get('object', …)` / `exists` → `registry.getItem`, which special-cases
+   *     the object type straight back to `registry.getObject`
+   *   - `list('object')` / `listNames('object')` → `registry.listItems`, which
+   *     special-cases to `registry.getAllObjects`
+   *
+   * So `register('object', …)` was a silent no-op as far as this class's own
+   * contract is concerned: `IMetadataService` (`@objectstack/spec/contracts`)
+   * declares `getObject(name)` ≡ `get('object', name)` and its own conformance
+   * test round-trips a `register('object', …)` through both members. Dormant
+   * in-tree only because nothing on `main` installs a `MetadataFacade` into the
+   * `metadata` slot — but the class is exported from this package's root and
+   * `core` entrypoints, so a downstream host got the split.
+   *
+   * The shape mirrors the one in-tree precedent for the same write,
+   * `MetadataProtocol.applyObjectRegistryMutation` (metadata-protocol), which
+   * calls `registerItem` AND `registerObject` with `packageId || 'sys_metadata'`.
+   * Neither half is redundant: the contributor entry is the runtime-effective
+   * object every read resolves (post-materialization, extensions merged), the
+   * generic-map entry is the stored document, and the contract's `getObject`
+   * TSDoc (#6505) already tells consumers those are different things.
+   *
+   * ── The contributor copy is a COPY, deliberately ──
+   *
+   * `registerObject` runs `applyProtection`, which stamps `_packageId` /
+   * `_provenance` **in place**, and `applySystemFields` returns its input
+   * unchanged on the no-injection path (`sys_*`, `systemFields: false`, …).
+   * Handing it the same reference `registerItem` stores would therefore write a
+   * synthetic package id onto the generic-map entry — precisely what the
+   * "never invents a synthetic package id for object registrations" pin in
+   * `metadata-facade.test.ts` forbids, and what `isArtifactBacked` keys write
+   * authorization off. The generic-map entry keeps its unstamped, as-authored
+   * shape; only the contributor copy carries the registry's coordinates.
+   *
+   * ── Why `_provenance: 'org'` on the package-less copy ──
+   *
+   * `registerObject` demands a package id, and an item that arrived here with no
+   * `_packageId` is runtime-authored by definition (see `register` above). The
+   * `'sys_metadata'` sentinel is the id the platform already uses for exactly
+   * that case. Stamping `'org'` alongside it is not belt-and-braces: without it
+   * `applyProtection` would default the copy to `_provenance: 'package'` and
+   * label a runtime-authored object a code artifact — the axis `isTenantAuthored`
+   * (registry.ts) exists to keep straight, and the misclassification behind
+   * cloud#970. An item that DID carry a real `_packageId` is registered under it
+   * and keeps `'package'`, which is true of it.
+   *
+   * ── Ordering ──
+   *
+   * The contributor write goes first because it is the half that can refuse:
+   * `registerObject` throws when another package already owns the name
+   * (ADR-0029). Failing before `registerItem` runs means a refused registration
+   * writes nothing at all, rather than re-opening the half-written split from
+   * the other side.
+   */
+  private registerObjectBothPlaces(type: string, definition: any, packageId: string | undefined): void {
+    this.registry.registerObject(
+      packageId
+        ? { ...definition }
+        : { ...definition, _provenance: 'org' },
+      // `||`, not `??`: an empty-string binding is "no package", the same
+      // normalisation the protocol write path applies.
+      packageId || RUNTIME_AUTHORED_PACKAGE_ID,
+    );
+    this.registry.registerItem(type, definition, 'name' as any, packageId);
+  }
+
   /**
    * Get a metadata item by type and name.
    *
@@ -70,8 +166,25 @@ export class MetadataFacade {
 
   /**
    * Unregister a metadata item
+   *
+   * [#6725] An object leaves both places it was written into, for the same
+   * reason {@link register} writes both: `unregisterItem` only empties the
+   * generic `metadata` map, which no object read consults. Removing one half is
+   * the exact shape of #6808 — the row was gone and `metadata['object']` was
+   * empty while `getObject(name)` kept serving the deleted object for the life
+   * of the process, and `getObject` is what the data plane dispatches on. Now
+   * that the write reaches `objectContributors`, a removal that did not would
+   * make every facade-registered object undeletable through this contract.
+   *
+   * `unregisterObject` is idempotent (`false` when nothing is registered under
+   * the name) and refuses, by design, an object still extended by another
+   * package — ADR-0029, the same judgement `unregisterObjectsByPackage`
+   * encodes. It runs first so a refusal removes nothing at all.
    */
   async unregister(type: string, name: string): Promise<void> {
+    if (isObjectType(type)) {
+      this.registry.unregisterObject(name);
+    }
     this.registry.unregisterItem(type, name);
   }