fix(metadata-protocol)!: the protocol install primitive parses manifest.id, and the duplicate door parses its target id (#19417)

.changeset/19417-protocol-install-primitive-parses-manifest-id.md

@@ -0,0 +1,90 @@
+---
+'@objectstack/metadata-protocol': minor
+---
+
+fix(metadata-protocol): the protocol install primitive parses the manifest's `id` leg, and the duplicate door parses its target id (#19417)
+
+Clause-②: no (narrowing)
+
+**BREAKING for callers of the protocol install and duplicate doors** —
+`ObjectStackProtocolImplementation.installPackage` and `duplicatePackage` now
+refuse a package id that is not reverse-domain notation, throwing a `400`-tagged
+error carrying the declaration's own sentence. Both used to install and report
+success.
+
+The accept set only shrinks back to what the published declaration has always
+said. `MANIFEST_ID_PATTERN` is declared once in
+`packages/spec/src/kernel/manifest.zod.ts` and referenced by both faces of one
+identity — `ManifestSchema.id`, what an author writes, and
+`PackageSchema.manifestId`, what the registry stores and publishes by.
+`installPackage` parsed nothing at all: it spread the request into `any` and
+handed it to `SchemaRegistry.installPackage` with a second `as any`, so
+`id: 'pkg-a'` — or `com.example.my_erp` — installed and PERSISTED while
+`defineStack()`, `os build`, `os validate` and the publish face all refused the
+same id. That is «declared ≠ enforced» on a published contract, and nothing in
+`packages/spec` moves for it: the declaration was already right.
+
+**Why the primitive and not only a door.** #19473 landed the same parse at the
+HTTP door (`POST /api/v1/packages`). That door is ONE caller of this primitive —
+it routes through `protocol.installPackage` whenever the protocol service
+resolves. `duplicatePackage` is a second, and an embedder holding the protocol
+object is a third. A gate on one door buys that door; this one is on the method
+every caller passes through.
+
+The gate asks the declaration **by reference** — `ManifestSchema.shape.id` —
+rather than keeping a copy of the grammar, so a future move of the
+reverse-domain rule reaches this seam with no further edit. The sentence the
+caller reads is the declaration's own (`manifestIdRefusal`), **surfaced rather
+than reworded**: it names the key, echoes the value, lists the two examples and
+carries a suggestion arm that verifies its candidate against the pattern before
+offering it. Installing `id: 'com.example.my_erp'` now throws, with:
+
+```text
+Invalid package id 'com.example.my_erp' on `manifest.id`. Expected
+reverse-domain notation ('com.steedos.crm', 'org.apache.superset') — lowercase
+dot-separated segments; hyphens allowed inside a segment, underscores are not.
+Did you mean 'com.example.my-erp'?
+```
+
+**The duplicate door refuses BEFORE it mints anything.** `duplicatePackage`
+builds its target manifest and writes it through `installPackage` inside a
+deliberately best-effort `catch {}` — a refusal raised only there would be
+swallowed and the caller would read `success: true` on a package with no
+manifest row. So the target id is parsed at the top of the method, ahead of the
+row scan and ahead of the copy loop, and the refusal names the key the caller
+actually wrote (`targetPackageId`).
+
+**One assumption, one implementation.** The duplicate door derived both
+namespaces with a raw `id.split('.').pop()` while `installPackage` derived the
+same default with the spec helper `deriveNamespaceFromPackageId`, which
+sanitises to the namespace charset, truncates to 20 and answers `null` when
+nothing valid comes out. That mattered: the target namespace is spliced into
+every copied object name as `${namespace}_${short}`, and an object name is
+`/^[a-z_][a-z0-9_]*$/`. The Studio's own default duplicate id —
+`<sourceId>-copy` — therefore minted `leave-copy_ticket`, a name the object
+declaration refuses. Both sides now use the helper, so a duplicate of
+`com.example.leave` into `com.example.leave-copy` is namespaced `leave_copy`.
+An explicitly declared `targetNamespace` still wins untouched; when neither an
+explicit nor a derivable namespace exists the door refuses loudly, naming
+`targetNamespace` as the remedy, instead of renaming rows with an empty prefix.
+
+**What is not affected.** Boot-time and in-process installs that reach
+`SchemaRegistry.installPackage` / `ObjectQL.registerApp` directly never pass
+through this primitive, so nothing about how a package is loaded from disk or
+registered by a plugin changes. A conforming manifest installs exactly as
+before, versionless and namespace-less manifests included — the version default
+and the namespace default still run, now behind the id gate rather than ahead of
+it.
+
+**Scope — the `id` leg alone.** `InstallPackageRequestSchema` / `ManifestSchema`
+are still not parsed whole here. The residual classes the HTTP door's own
+docblock records are untouched by this change and are each their own narrowing
+of a published contract.
+
+**If you are refused.** Give the package an id in reverse-domain notation —
+lowercase dot-separated segments, hyphens allowed inside a segment, underscores
+not. The refusal names the key, echoes what you wrote and, where a mechanical
+repair exists, offers one it has already checked against the rule, so the
+prescription arrives with the failure rather than in a changelog.
+
+<!-- adr-0087: not-required (no-migration-prescription) Nothing authorable is removed, renamed or reshaped: no spec key, no export, no stored row. `objectstack migrate meta` has nothing to reach, because there is no old spelling that maps to a new one — a caller supplies an id the declaration already required, and an id's repair changes the package's identity, so no mechanical mapping could be prescribed even in principle. The refusal itself carries the remedy. -->

packages/metadata-protocol/src/protocol.install-manifest-id.test.ts

@@ -0,0 +1,244 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * [#19417] The protocol install primitive parses `manifest.id` through the
+ * declaration that owns it.
+ *
+ * ---------------------------------------------------------------------------
+ * The gap these pins close
+ * ---------------------------------------------------------------------------
+ * `MANIFEST_ID_PATTERN` (`packages/spec/src/kernel/manifest.zod.ts`) is the
+ * reverse-domain rule declared ONCE and referenced by both faces of the package
+ * identity — `ManifestSchema.id` (what an author writes) and
+ * `PackageSchema.manifestId` (what the registry stores and publishes by).
+ * `ObjectStackProtocolImplementation.installPackage` never parsed it: the
+ * request was spread into `any` and handed to the registry with a second
+ * `as any`, so `pkg-a` and `com.example.my_erp` installed and persisted while
+ * `defineStack()`, `os build`, `os validate` and the publish face all refused
+ * the same id.
+ *
+ * #19473 landed the same parse at the HTTP door (`POST /packages`,
+ * `packages/runtime/src/domains/packages.ts`). That door is ONE caller of this
+ * primitive; `duplicatePackage` is a second and an embedder holding the
+ * protocol object is a third — which is why the gate belongs here.
+ *
+ * ---------------------------------------------------------------------------
+ * Both directions are pinned, deliberately
+ * ---------------------------------------------------------------------------
+ * A refusal pin alone cannot tell "the rule is enforced" from "this door stopped
+ * installing anything": every refusal case is answered by a lit control on a
+ * conforming id that still installs, and by the assertion that the registry was
+ * never reached on the refused ones.
+ *
+ * The refusal text is compared against `manifestIdRefusal` itself rather than
+ * retyped here: the sentence is the declaration's, SURFACED, and a pin that
+ * restated it would go green on a reworded fourth sentence for one rule.
+ */
+import { describe, it, expect, vi } from 'vitest';
+import { manifestIdRefusal } from '@objectstack/spec/kernel';
+import { ObjectStackProtocolImplementation } from './protocol.js';
+
+/** Ids `MANIFEST_ID_PATTERN` refuses, one per reason it refuses them. */
+const REFUSED = [
+    ['a bare word — no reverse-domain prefix at all', 'pkg-a'],
+    ['an underscore inside a segment', 'com.example.my_erp'],
+    ['the empty string', ''],
+    ['a segment opening with a digit', 'com.4example.crm'],
+] as const;
+
+/** Ids the declaration admits — the lit controls. */
+const ADMITTED = ['com.example.crm', 'com.example.my-erp', 'org.apache.superset'] as const;
+
+function makeImpl() {
+    const registryCalls: Array<{ manifest: any }> = [];
+    const engine = {
+        registry: {
+            installPackage: (manifest: any) => {
+                registryCalls.push({ manifest });
+                return { manifest, status: 'installed', enabled: true };
+            },
+        },
+        find: async () => [],
+    };
+    const publish = vi.fn(async () => ({ success: true }));
+    const services = new Map<string, any>([['package', { publish }]]);
+    const impl = new ObjectStackProtocolImplementation(engine as never, () => services);
+    return { impl, registryCalls, publish };
+}
+
+/** The thrown refusal, or a failure naming what happened instead. */
+async function refusalOf(run: () => Promise<unknown>): Promise<any> {
+    try {
+        await run();
+    } catch (e) {
+        return e;
+    }
+    throw new Error('expected the call to be refused, but it resolved');
+}
+
+describe('[#19417] installPackage parses `manifest.id` through its declaration', () => {
+    for (const [why, id] of REFUSED) {
+        it(`refuses ${why} (${JSON.stringify(id)}) before any write`, async () => {
+            const { impl, registryCalls, publish } = makeImpl();
+            const err = await refusalOf(() => (impl as any).installPackage({
+                manifest: { id, name: 'X', version: '1.0.0' },
+            }));
+            // The envelope: an HTTP boundary answers 400, not the 500 an
+            // unannotated throw earns (`resolveThrownHttpError`).
+            expect(err.statusCode).toBe(400);
+            // The sentence is the DECLARATION's, surfaced — not this door's.
+            expect(err.message).toBe(manifestIdRefusal('manifest.id', id));
+            // Neither writer ran: not the in-memory registry, not the durable row.
+            expect(registryCalls).toHaveLength(0);
+            expect(publish).not.toHaveBeenCalled();
+        });
+    }
+
+    it('refuses a manifest carrying no `id` at all, and still never writes', async () => {
+        const { impl, registryCalls, publish } = makeImpl();
+        const err = await refusalOf(() => (impl as any).installPackage({
+            manifest: { name: 'X', version: '1.0.0' },
+        }));
+        expect(err.statusCode).toBe(400);
+        expect(registryCalls).toHaveLength(0);
+        expect(publish).not.toHaveBeenCalled();
+    });
+
+    it('carries the declaration\'s mechanical repair, not just its rule', async () => {
+        const { impl } = makeImpl();
+        const err = await refusalOf(() => (impl as any).installPackage({
+            manifest: { id: 'com.example.my_erp', version: '1.0.0' },
+        }));
+        // The repair arm is the whole difference between a rule restated and a
+        // fix; `manifestIdRefusal` verifies its candidate before offering it.
+        expect(err.message).toContain('com.example.my-erp');
+    });
+
+    for (const id of ADMITTED) {
+        it(`lit control — '${id}' still installs`, async () => {
+            const { impl, registryCalls } = makeImpl();
+            const res: any = await (impl as any).installPackage({
+                manifest: { id, name: 'X', version: '1.0.0' },
+            });
+            expect(registryCalls).toHaveLength(1);
+            expect(registryCalls[0].manifest.id).toBe(id);
+            expect(res.package.status).toBe('installed');
+        });
+    }
+
+    it('the raw value is parsed — a padded id is refused, not laundered by a trim', async () => {
+        const { impl, registryCalls } = makeImpl();
+        const err = await refusalOf(() => (impl as any).installPackage({
+            manifest: { id: '  com.example.crm  ', version: '1.0.0' },
+        }));
+        expect(err.statusCode).toBe(400);
+        expect(registryCalls).toHaveLength(0);
+    });
+});
+
+/** A duplicate-door harness: one source package holding one object row. */
+function makeDuplicateImpl(sourceId = 'com.example.leave') {
+    const rows = [{
+        id: 'r_1',
+        type: 'object',
+        name: 'leave_ticket',
+        organization_id: null,
+        package_id: sourceId,
+        state: 'active',
+        metadata: JSON.stringify({ name: 'leave_ticket', label: 'Ticket' }),
+    }];
+    const installed: any[] = [];
+    const engine: any = {
+        find: vi.fn(async () => rows),
+        registry: {
+            getPackage: vi.fn(() => ({
+                manifest: { id: sourceId, name: 'Leave', namespace: 'leave', version: '1.0.0' },
+            })),
+            installPackage: vi.fn((manifest: any) => {
+                installed.push(manifest);
+                return { manifest, status: 'installed', enabled: true };
+            }),
+        },
+    };
+    const impl = new ObjectStackProtocolImplementation(engine as never, () => new Map());
+    const saveMetaItem = vi.spyOn(impl, 'saveMetaItem' as never);
+    (saveMetaItem as any).mockResolvedValue({ success: true } as never);
+    return { impl, engine, installed, saveMetaItem };
+}
+
+describe('[#19417] duplicatePackage refuses a target id the declaration refuses', () => {
+    for (const [why, id] of REFUSED) {
+        it(`refuses ${why} (${JSON.stringify(id)}) before anything is minted`, async () => {
+            const { impl, engine, installed, saveMetaItem } = makeDuplicateImpl();
+            const err = await refusalOf(() => (impl as any).duplicatePackage({
+                sourcePackageId: 'com.example.leave',
+                targetPackageId: id,
+            }));
+            expect(err.statusCode).toBe(400);
+            // The key named is the one the caller actually wrote.
+            expect(err.message).toBe(manifestIdRefusal('targetPackageId', id));
+            // ⭐ Nothing was minted and nothing was scanned: the refusal
+            // precedes the manifest write AND the copy loop, so no empty shell
+            // is left behind. The manifest write below sits inside a
+            // best-effort `catch {}` — a refusal raised only there would be
+            // swallowed and reported as `success: true`.
+            expect(installed).toHaveLength(0);
+            expect(engine.registry.installPackage).not.toHaveBeenCalled();
+            expect(engine.find).not.toHaveBeenCalled();
+            expect(saveMetaItem).not.toHaveBeenCalled();
+        });
+    }
+
+    it('lit control — a conforming target still duplicates, rows and all', async () => {
+        const { impl, installed, saveMetaItem } = makeDuplicateImpl();
+        const res: any = await (impl as any).duplicatePackage({
+            sourcePackageId: 'com.example.leave',
+            targetPackageId: 'com.example.leave-copy',
+        });
+        expect(res.success).toBe(true);
+        expect(res.copiedCount).toBe(1);
+        expect(installed).toHaveLength(1);
+        expect(installed[0].id).toBe('com.example.leave-copy');
+        expect(saveMetaItem).toHaveBeenCalledTimes(1);
+    });
+});
+
+describe('[#19417] duplicatePackage derives its namespace with the spec helper', () => {
+    it('the Studio default `<sourceId>-copy` yields a LEGAL object-name prefix', async () => {
+        const { impl, installed, saveMetaItem } = makeDuplicateImpl();
+        await (impl as any).duplicatePackage({
+            sourcePackageId: 'com.example.leave',
+            targetPackageId: 'com.example.leave-copy',
+        });
+        // `deriveNamespaceFromPackageId` sanitises the hyphen; the raw
+        // `split('.').pop()` this replaced answered 'leave-copy', and an object
+        // name is /^[a-z_][a-z0-9_]*$/ — so the copy used to be minted under
+        // names the object declaration refuses.
+        expect(installed[0].namespace).toBe('leave_copy');
+        const written = (saveMetaItem as any).mock.calls.map((c: any[]) => c[0].name);
+        expect(written).toEqual(['leave_copy_ticket']);
+    });
+
+    it('an explicit `targetNamespace` still wins', async () => {
+        const { impl, installed } = makeDuplicateImpl();
+        await (impl as any).duplicatePackage({
+            sourcePackageId: 'com.example.leave',
+            targetPackageId: 'com.example.leave-copy',
+            targetNamespace: 'leave2',
+        });
+        expect(installed[0].namespace).toBe('leave2');
+    });
+
+    it('refuses loudly when no namespace can be derived, naming the remedy', async () => {
+        const { impl, installed } = makeDuplicateImpl();
+        const err = await refusalOf(() => (impl as any).duplicatePackage({
+            sourcePackageId: 'com.example.leave',
+            // Admitted by the id pattern, but a single-letter final segment
+            // cannot carry the namespace charset's 2–20 char rule.
+            targetPackageId: 'com.example.a',
+        }));
+        expect(err.statusCode).toBe(400);
+        expect(err.message).toContain('targetNamespace');
+        expect(installed).toHaveLength(0);
+    });
+});