fix(cli): os package publish honours or refuses a declared manifest.id, never substitutes one

.changeset/publish-honours-or-refuses-declared-manifest-id.md

@@ -0,0 +1,21 @@
+---
+'@objectstack/cli': minor
+---
+
+`os package publish` no longer publishes under a manifest id the author did not write. A `manifest.id` the artifact declares is now used or refused — never silently swapped for a derived one.
+
+Before this, `deriveManifestId` adopted `manifest.id` only when it parsed as `PackageSchema.manifestId`, and any other declared value fell through to `local.<manifest.name slug>`. Nothing said so: the substituted id appeared in the ordinary progress line, byte-identical to the run where the artifact declared no id at all.
+
+```
+manifest.id = 'crm'      before: → Registering package 'local.acme-crm'...   (exit 0)
+manifest.name = 'Acme CRM'
+                          after: ✗ Invalid manifest-id 'crm'. …             (exit 1)
+```
+
+`sys_package.manifest_id` is **immutable once set** — "renaming a package requires creating a new package" — so the value chosen there is a permanent, globally unique identifier. Choosing it silently, against the author's own declaration, is the one field that must not be rewritten without a word.
+
+- **A declared `manifest.id` reaches the existing preflight gate.** If it is not a manifest id the control plane accepts, the publish refuses before any network call, quoting the schema's own issue and description and naming where the id came from. No second rule is introduced in the CLI: the judgement is still `PackageSchema.manifestId`, which is the same schema node `CreatePackageRequestSchema.manifestId` declares for the `manifest_id` this command POSTs.
+- **Honouring the declared value instead was not available.** The values that used to fall through are, by construction, exactly the ones that schema rejects, so forwarding one would only move the same refusal to the server, later and with a worse message.
+- **Absent, blank and non-string `manifest.id` are unchanged** — none of those is a declaration, and each still derives from `manifest.name`, then the artifact filename.
+
+What to do if a publish that worked now refuses: the message names the three ways out. Fix `manifest.id` in `objectstack.config.ts` to a reverse-domain id and rebuild; remove the key to keep publishing under the derived `local.…` id (the value the previous release was already using); or pass `--manifest-id`. Every id the control plane accepts publishes with unchanged bytes.

packages/cli/src/commands/package/publish.ts

@@ -106,33 +106,51 @@ export interface DerivedManifestId {
 }
 
 /**
- * Derive a reverse-domain manifest_id when the user hasn't passed --manifest-id.
+ * Decide the manifest_id when the user hasn't passed --manifest-id.
  * Order of precedence:
- *   1. artifact.manifest.id (only when it is a manifest id the control plane accepts)
+ *   1. artifact.manifest.id — whenever the artifact DECLARES one
  *   2. local.<artifact.manifest.name slug>
  *   3. local.<artifact filename without extension>
  *
- * Step 1 is gated by the schema, not by a local look-alike test: `manifest.id`
- * is a bare `z.string()` in `ManifestSchema`, so an artifact may carry any
- * shape at all, and the previous test forwarded `com.acme.repair_desk` and
- * friends unchanged. The old extra `explicit.includes('.')` condition is gone
- * because the schema subsumes it — its pattern requires at least two segments,
- * so a dotless id can never parse. That is why a bare `crm` was already blocked
- * here while the explicit `--manifest-id` path let it through: two paths, two
- * strictnesses, neither of them the declared one.
+ * ## Step 1 is a declaration, not a candidate
  *
- * Steps 2 and 3 are the CLI's own invention and are **not** guaranteed valid:
- * `slugify` has no letter-first rule, so a manifest named `2024 App` derives
- * `local.2024-app`, which the schema rejects. That is refused at the single
- * gate in `run()` with the source named, rather than normalised: `manifestId`
- * is immutable once published ("renaming a package requires creating a new
- * package"), so silently minting a different permanent global identifier than
- * the one the inputs imply is worse than saying what is wrong.
+ * A declared `manifest.id` is either used or refused. It is never *skipped* in
+ * favour of a derived id, and that is the whole point of this branch's shape:
+ * `manifestId` is immutable once published ("renaming a package requires
+ * creating a new package"), so the id chosen here is a permanent, globally
+ * unique identifier. Falling through on a declared-but-unusable value published
+ * an id the author never wrote and cannot rename afterwards, and said nothing —
+ * the substituted id appeared in the ordinary progress line, indistinguishable
+ * from the case where the artifact declared no id at all.
+ *
+ * So an unusable declared value reaches the single gate in `run()` and is
+ * refused there, quoting the schema and naming where the id came from.
+ *
+ * ⛔ Honouring such a value instead is not available, and not because anything
+ * downstream depends on the derivation — `deriveManifestId` has exactly one
+ * non-test caller. It is unavailable because the values that used to fall
+ * through here are, by construction, exactly the ones
+ * `PackageSchema.shape.manifestId` rejects — and
+ * `CreatePackageRequestSchema.manifestId` IS that same schema node, not a copy
+ * of it, so it is the declared shape of the `manifest_id` this command POSTs.
+ * Forwarding one would only move the same refusal to the server, later and with
+ * a worse message.
+ *
+ * An absent, non-string or blank `manifest.id` is not a declaration, and those
+ * fall through to step 2 exactly as before.
+ *
+ * ## Steps 2 and 3 are the CLI's own invention
+ *
+ * They are **not** guaranteed valid: `slugify` has no letter-first rule, so a
+ * manifest named `2024 App` derives `local.2024-app`, which the schema rejects.
+ * That is refused at the same gate with the source named, rather than
+ * normalised — minting a different permanent identifier than the inputs imply
+ * is worse than saying what is wrong.
  */
 export function deriveManifestId(artifact: any, artifactPath: string): DerivedManifestId {
   const explicit = artifact?.manifest?.id;
-  if (typeof explicit === 'string' && isManifestId(explicit)) {
-    return { id: explicit, source: 'artifact-manifest-id' };
+  if (typeof explicit === 'string' && explicit.trim()) {
+    return { id: explicit.trim(), source: 'artifact-manifest-id' };
   }
   const name = artifact?.manifest?.name;
   if (typeof name === 'string' && name.trim()) {
@@ -148,9 +166,18 @@ export function deriveManifestId(artifact: any, artifactPath: string): DerivedMa
 function manifestIdRemedy(source: ManifestIdSource | 'explicit'): string {
   switch (source) {
     case 'artifact-manifest-id':
+      // Names the conflict: the value is the author's own declaration, and the
+      // command is telling them it will not quietly publish under a different
+      // one. Removing the key is listed because it is the only way back to the
+      // derived id — which used to happen silently.
+      return 'It is the `manifest.id` declared by the compiled artifact, and `os package publish` '
+        + 'publishes under the id you declared or refuses — it does not substitute a derived one, '
+        + 'because manifest_id is immutable once published. Fix `manifest.id` in objectstack.config.ts '
+        + 'and rebuild, remove it to derive an id from `manifest.name`, or pass --manifest-id.';
     case 'artifact-manifest-name':
-      return 'It was derived from the compiled artifact. Pass --manifest-id, set `manifestId` in '
-        + 'objectstack.manifest.json, or fix `manifest.id` in objectstack.config.ts and rebuild.';
+      return 'It was derived from the compiled artifact, from `manifest.name`. Pass --manifest-id, set '
+        + '`manifestId` in objectstack.manifest.json, or declare `manifest.id` in objectstack.config.ts '
+        + 'and rebuild.';
     case 'artifact-filename':
       return 'It was derived from the artifact filename. Pass --manifest-id, set `manifestId` in '
         + 'objectstack.manifest.json, or give the app a `manifest.name` and rebuild.';

packages/cli/test/package-publish-manifest-id.test.ts

@@ -26,6 +26,25 @@
  * same schema, and the dot condition is gone because the schema subsumes it
  * (its pattern needs at least two segments).
  *
+ * ## The derive path carries a DECLARATION, not a candidate (#16891)
+ *
+ * Asking the same schema on both paths closed the "admitted here, refused by
+ * the server" hole; it did not close the derive path's *silent* one. A declared
+ * `manifest.id` that failed that parse used to fall through to
+ * `local.<manifest.name slug>` and publish under it without a word, and
+ * `manifest_id` is immutable once published — so the author acquired a
+ * permanent, globally unique identifier they never wrote and cannot rename.
+ * The output made it invisible: `Registering package 'local.acme-crm'...` was
+ * byte-identical to the run where the artifact declared no id at all.
+ *
+ * A declared id is now used or refused, never skipped, and the last describe
+ * block below pins the two runs apart — the property the defect violated.
+ * Honouring an unusable declared value is not on the table: the values that
+ * reached the fall-through are, by construction, exactly the ones
+ * `PackageSchema.manifestId` rejects, and `CreatePackageRequestSchema.manifestId`
+ * IS that schema node, so forwarding one only relocates the same refusal to the
+ * server.
+ *
  * ## What separates a fix from a re-transcription
  *
  * Re-typing the schema's regex into this file would turn every refusal
@@ -199,24 +218,33 @@ describe('os package publish — the manifest-id rule is the spec manifest-id ru
 
   describe('derive path', () => {
     for (const { id, why, admittedOnDerivePathBefore } of RELAXATIONS) {
-      it(`refuses to adopt '${id}' (${why}${admittedOnDerivePathBefore ? '' : ' — already blocked before the fix'})`, async () => {
-        // Unit half: the deriver does not forward the illegal shape…
+      it(`refuses a declared '${id}' (${why}${admittedOnDerivePathBefore ? ' — reached the server before #16889' : ''})`, async () => {
+        // Unit half: a DECLARED id is carried to the gate, never swapped for a
+        // derived one. `deriveManifestId` documents its result as "NOT
+        // guaranteed valid" precisely so this branch can hand an unusable
+        // declaration onwards to be refused by name.
         const derived = deriveManifestId(
           { manifest: { id, name: 'Acme CRM' } },
           '/nowhere/objectstack.json',
         );
-        expect(derived.id).not.toBe(id);
-        expect(derived).toEqual({ id: 'local.acme-crm', source: 'artifact-manifest-name' });
-        expect(isManifestId(derived.id)).toBe(true);
+        expect(derived).toEqual({ id, source: 'artifact-manifest-id' });
+        expect(isManifestId(derived.id)).toBe(false);
 
-        // …and end to end, nothing resembling it reaches the wire.
+        // …and end to end the publish refuses before any network call, rather
+        // than proceeding under `local.acme-crm`.
         const path = await artifactAt({ id, name: 'Acme CRM', version: '1.2.0' });
         const calls = stubCloud();
-        await PackagePublish.run([path]);
+        const { exitCode, output } = await runPublish([path]);
 
-        expect(calls).toHaveLength(2);
-        expect(calls[0].body.manifest_id).not.toBe(id);
-        expect(MANIFEST_ID.safeParse(calls[0].body.manifest_id).success).toBe(true);
+        expect(exitCode).toBe(1);
+        expect(calls).toEqual([]);
+        // The refusal names the author's own value…
+        expect(output).toContain(`Invalid manifest-id '${id}'`);
+        // …and never the id that used to be substituted for it. This is the
+        // assertion the defect could not satisfy: `manifestId` is immutable
+        // once published, so a permanent identifier the author did not write
+        // must not be minted, and must not be reported as if it were theirs.
+        expect(output).not.toContain('local.acme-crm');
       });
     }
 
@@ -233,10 +261,61 @@ describe('os package publish — the manifest-id rule is the spec manifest-id ru
       expect(calls[0].body.manifest_id).toBe(LEGAL_ID);
     });
 
-    it('falls back to the artifact filename when the artifact names nothing usable', () => {
-      expect(deriveManifestId({ manifest: { id: 'com..acme' } }, '/tmp/build/objectstack.json'))
+    it('falls back to the artifact filename when the artifact names nothing at all', () => {
+      expect(deriveManifestId({ manifest: {} }, '/tmp/build/objectstack.json'))
         .toEqual({ id: 'local.objectstack', source: 'artifact-filename' });
     });
+
+    // An absent key is not a declaration. Neither is a blank one, nor a
+    // non-string: `ManifestSchema.id` is `z.string()`, so those shapes are
+    // off-spec input rather than an id the author chose, and they keep
+    // deriving exactly as they did before.
+    it.each([
+      ['absent', undefined],
+      ['empty string', ''],
+      ['whitespace', '   '],
+      ['non-string', 42],
+    ])('does not treat a %s `manifest.id` as a declaration', (_label, value) => {
+      expect(deriveManifestId({ manifest: { id: value, name: 'Acme CRM' } }, '/nowhere/objectstack.json'))
+        .toEqual({ id: 'local.acme-crm', source: 'artifact-manifest-name' });
+    });
+  });
+
+  // -------------------------------------------------------------------------
+  // The card's own reproduction: the two cases must be TELLABLE APART.
+  //
+  // Before this fix both printed `Registering package 'local.acme-crm'...` and
+  // both POSTed `manifest_id: local.acme-crm` — byte-identical output for
+  // "the author declared an id we discarded" and "the author declared no id".
+  // That indistinguishability IS the defect, so it is pinned as one assertion
+  // over both runs rather than as two separate expectations.
+  // -------------------------------------------------------------------------
+
+  describe('a discarded declaration is distinguishable from no declaration', () => {
+    async function publishWith(manifest: Record<string, unknown>) {
+      const path = await artifactAt(manifest);
+      const calls = stubCloud();
+      const { exitCode, output } = await runPublish([path]);
+      return { exitCode: exitCode ?? 0, wire: calls[0]?.body?.manifest_id, output };
+    }
+
+    it("declaring manifest.id = 'crm' no longer looks like declaring nothing", async () => {
+      const declared = await publishWith({ id: 'crm', name: 'Acme CRM', version: '1.2.0' });
+      // Fresh artifact dir for the control run.
+      await rm(dir, { recursive: true, force: true });
+      dir = '';
+      const silent = await publishWith({ name: 'Acme CRM', version: '1.2.0' });
+
+      // The control run is unchanged by this card — an artifact that declares
+      // no id still publishes under the derived one.
+      expect(silent).toMatchObject({ exitCode: 0, wire: 'local.acme-crm' });
+
+      // The declaring run is refused, and every observable differs.
+      expect(declared.exitCode).toBe(1);
+      expect(declared.wire).toBeUndefined();
+      expect(declared.output).not.toBe(silent.output);
+      expect(declared.output).toContain("'crm'");
+    });
   });
 
   // -------------------------------------------------------------------------