fix(cli): nav-contribution-groups imports the artifact package-id owner instead of carrying a second copy

.changeset/18490-nav-contribution-groups-imports-package-id-owner.md

@@ -0,0 +1,16 @@
+---
+"@objectstack/cli": patch
+---
+
+`os build` / `os validate` name an artifact package the same way the runtime fold does when its `manifest.id` and `manifest.name` are both empty — `nav-contribution-groups.ts` no longer carries its own copy of the artifact package-id rule and imports the declared owner instead (#18490).
+
+Clause-②: no
+
+`packages/cli/src/utils/artifact-packages.ts` declares itself the sole owner of "which package is this", and says why in its own header: *"⛔ A second copy is the one that must not happen. … Two readers computing 'which package is this' slightly differently is how one entry comes to judge a different set of packages than the other while both look right."* `nav-contribution-groups.ts` exported a second implementation, `artifactPackagesOf`, which differed from the owner in one guard — a non-empty check on `manifest.name` — and the two had already drifted on a real input.
+
+- **The divergent input is reachable, measured rather than assumed.** `ManifestSchema` requires `id` and `name` as strings and constrains neither to be non-empty, so `{ manifest: { id: '', name: '', … } }` parses green through the same `normalizeStackInput` + `ObjectStackDefinitionSchema` chain both commands run. For that package the owner answered `''` and the deleted copy answered `` `packages[<index>]` ``.
+- **Importing the owner chose `''`, and `''` is the answer this path needs.** `ObjectQL.registerApp` derives the id it registers a navigation contribution under as `manifest.id || manifest.name`, with no positional fallback, so the read-time fold names that package `''` and prints `Package "" contributes …`. The build used to print `Package "packages[0]" …` for the same artifact — two doors naming one package differently, which is the divergence the shared `checkNavContributionGroups` predicate exists to prevent, one field over.
+- **What an author sees change**: for an artifact package with an empty `id` *and* an empty `name`, the `packageId` on a `nav_contribution_group_missing` warning — and the package name inside its message — is now `''` instead of `packages[<index>]`, in both `os build` and `os validate`, matching what the runtime already reports at boot. Every package with a non-empty `id` or `name` is unaffected: both rules answered identically there, measured on the control legs.
+- **The id is carried and printed, never keyed on.** Two packages that both resolve to `''` still produce two findings rather than collapsing into one — pinned, because that failure mode would present as a report going quiet rather than as an error.
+
+`artifactPackagesOf` is removed. It was never reachable through this package's `exports` map (`.`, `./console`, `./hook-body`), so no consumer import can break; the removal is internal to `dist`.

packages/cli/src/utils/nav-contribution-groups.package-id.test.ts

@@ -0,0 +1,195 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * #18490 — the artifact package-id rule has ONE owner again, and importing it
+ * DECIDED the one input the two copies disagreed on.
+ *
+ * ## What this file measures, and against what
+ *
+ * `packages/cli/src/utils/artifact-packages.ts` declares itself the sole owner
+ * of "which package is this". `nav-contribution-groups.ts` carried a second
+ * implementation, `artifactPackagesOf`, differing from the owner in a single
+ * guard — a non-empty check on `manifest.name`. For a package whose
+ * `manifest.id` AND `manifest.name` are both `''` the owner answered `''` and
+ * the copy answered `packages[<index>]`.
+ *
+ * Deleting the copy chose `''`. What this file pins is not that string but the
+ * REASON it is the right one: `''` is what the RUNTIME fold names that package,
+ * and `packages[<index>]` is a spelling the runtime cannot produce at all
+ * (`ObjectQL.registerApp` derives `manifest.id || manifest.name`, with no
+ * positional fallback). `nav-contribution-groups.ts`'s own header says the id
+ * it carries is "the string the runtime registers a contribution under, so a
+ * command names a package the same way the fold does" — this is that sentence,
+ * measured.
+ *
+ * ⛔ NEITHER side's rule is re-spelled here. The build's answer comes out of
+ * the shipped `findNavGroupDiagnostics`, the runtime's out of a real
+ * `ObjectQL`, and the assertion is that the two STRINGS match — so the day
+ * either rule moves, this reds instead of agreeing with itself.
+ *
+ * ## Why this is a file of its own, and why it is INTEGRATION tier
+ *
+ * The cross-door half constructs `new ObjectQL(`, which is a KERNEL signal in
+ * `vitest-tiers.ts`'s predicate — so a file carrying it is integration tier by
+ * derivation, not by naming. Folding these cases into the unit-tier
+ * `nav-contribution-groups.test.ts` would have moved that whole file, and its
+ * nine existing #14553 pins with it, out of the tier they were written for.
+ * Splitting keeps the tier change confined to the cases that actually boot a
+ * registry. ⛔ Do not merge this file back into that one.
+ */
+
+import { describe, it, expect } from 'vitest';
+import { composeStacks, normalizeStackInput, ObjectStackDefinitionSchema } from '@objectstack/spec';
+import { ObjectQL } from '@objectstack/objectql/core';
+import { findNavGroupDiagnostics } from './nav-contribution-groups.js';
+import { artifactPackages } from './artifact-packages.js';
+
+type AnyRec = Record<string, unknown>;
+
+const APP = 'multi_crm';
+const GROUP = 'sales_group';
+const TYPO = 'sales_grp';
+const CORE_ID = 'com.example.multi.core';
+
+/** The App package — owns the app and the group container the module aims at. */
+const coreStack = () => ({
+  manifest: {
+    id: CORE_ID,
+    name: 'Multi-Package Core',
+    namespace: 'crm',
+    version: '1.0.0',
+    type: 'app' as const,
+  },
+  objects: [{
+    name: 'crm_account',
+    label: 'Account',
+    sharingModel: 'private' as const,
+    fields: { name: { name: 'name', type: 'text' as const, label: 'Account Name', required: true } },
+  }],
+  apps: [{
+    name: APP,
+    label: 'Multi-Package CRM',
+    navigation: [{
+      id: GROUP,
+      type: 'group' as const,
+      label: 'Sales',
+      children: [{ id: 'nav_accounts', type: 'object' as const, objectName: 'crm_account', label: 'Accounts' }],
+    }],
+  }],
+});
+
+/**
+ * The contributing package, carrying the divergent identity: both id keys `''`.
+ *
+ * ⚠️ Its own namespace, not the app package's. The composed-artifact leg does
+ * not care, but the runtime leg INSTALLS both packages into one registry and
+ * ADR-0048 refuses a second package under a namespace another already owns —
+ * a conflict that would make this file red for a reason that has nothing to do
+ * with what it measures.
+ */
+const emptyIdOrdersStack = (group: string) => ({
+  manifest: {
+    id: '',
+    name: '',
+    namespace: 'ord',
+    version: '1.0.0',
+    type: 'module' as const,
+    navigationContributions: [{
+      app: APP,
+      group,
+      items: [{ id: 'nav_orders', type: 'object' as const, objectName: 'ord_order', label: 'Orders' }],
+    }],
+  },
+  objects: [{
+    name: 'ord_order',
+    label: 'Order',
+    sharingModel: 'private' as const,
+    fields: { name: { name: 'name', type: 'text' as const, label: 'Order Number', required: true } },
+  }],
+});
+
+/** The artifact as the commands actually hand it down — through their own parse. */
+const parsedEmptyIdArtifact = (group: string): AnyRec => {
+  const composed = composeStacks(
+    [emptyIdOrdersStack(group), coreStack()],
+    { manifest: 'preserve' },
+  ) as unknown as Record<string, unknown>;
+  const normalized = normalizeStackInput(composed, { onConversionNotice: () => {} });
+  const result = ObjectStackDefinitionSchema.safeParse(normalized);
+  if (!result.success) {
+    throw new Error(`fixture does not parse: ${JSON.stringify(result.error.issues.slice(0, 3))}`);
+  }
+  return result.data as unknown as AnyRec;
+};
+
+describe('#18490 — one package, two doors, one name', () => {
+  it('an empty `manifest.id` AND `manifest.name` REACHES this check — the divergence is not hypothetical', () => {
+    // The floor under everything below. `ManifestSchema` requires both keys as
+    // strings and constrains NEITHER to be non-empty, so `''` parses — which is
+    // the only reason the two rules could ever disagree on a stack `os build`
+    // or `os validate` would actually accept. If a spec change starts refusing
+    // it, this reds FIRST and says the pins under it now measure nothing.
+    const parsed = parsedEmptyIdArtifact(GROUP);
+    expect(artifactPackages(parsed).map((pkg) => pkg.id).sort()).toEqual(['', CORE_ID]);
+  });
+
+  it('the build names that package EXACTLY as the runtime fold does', async () => {
+    // The build door: the shipped derivation, reached the way both commands
+    // reach it — `findNavGroupDiagnostics(result.data)`, one argument.
+    const built = await findNavGroupDiagnostics(parsedEmptyIdArtifact(TYPO));
+    expect(built).toHaveLength(1);
+
+    // The runtime door: the same two packages installed into a real registry.
+    // `registerApp` derives the id it registers the contribution under, the
+    // read-time fold relocates the mis-aimed items, and recording that
+    // relocation is what produces the diagnostic.
+    const engine = new ObjectQL();
+    const core = coreStack();
+    engine.registerApp({ ...core.manifest, apps: core.apps });
+    engine.registerApp({ ...emptyIdOrdersStack(TYPO).manifest });
+    engine.registry.getApp(APP);
+    const folded = engine.registry.getAppNavDiagnostics(APP);
+    expect(folded).toHaveLength(1);
+
+    // The card, in two lines: the `packageId` field a consumer reads, and the
+    // sentence an author reads.
+    expect(built[0].packageId).toBe(folded[0].packageId);
+    expect(built[0].message).toBe(folded[0].message);
+  });
+
+  it('⛔ and that shared name is NOT the deleted copy\'s positional spelling', async () => {
+    // Its own assertion, because the pin above would also pass if BOTH doors
+    // moved to `packages[0]`. The runtime has no positional fallback, so a
+    // build printing one is a build naming a package the runtime never will.
+    const built = await findNavGroupDiagnostics(parsedEmptyIdArtifact(TYPO));
+    expect(built[0].packageId).toBe('');
+    expect(built[0].packageId).not.toBe('packages[0]');
+    expect(built[0].message).not.toContain('packages[0]');
+  });
+
+  it('two packages that both resolve to the empty id still produce TWO findings', async () => {
+    // The id is CARRIED and PRINTED on this path — never a map key, a dedupe
+    // key or a sort key. Pinned because "both collapse into one finding" is the
+    // failure an empty id would cause if it ever became one, and it would
+    // present as the report going QUIET rather than as an error.
+    const second = {
+      manifest: {
+        id: '',
+        name: '',
+        navigationContributions: [{
+          app: APP,
+          group: TYPO,
+          items: [{ id: 'nav_second', type: 'object' as const, objectName: 'ord_order', label: 'Second' }],
+        }],
+      },
+    };
+    const parsed = parsedEmptyIdArtifact(TYPO);
+    const widened: AnyRec = {
+      ...parsed,
+      packages: [...((parsed.packages ?? []) as unknown[]), second],
+    };
+    const found = await findNavGroupDiagnostics(widened);
+    expect(found).toHaveLength(2);
+    expect(found.map((d) => d.packageId)).toEqual(['', '']);
+  });
+});

packages/cli/src/utils/nav-contribution-groups.test.ts

@@ -54,7 +54,8 @@
 import '@objectstack/objectql/core';
 import { describe, it, expect } from 'vitest';
 import { composeStacks, normalizeStackInput, ObjectStackDefinitionSchema } from '@objectstack/spec';
-import { artifactPackagesOf, collectNavGroupInputs, findNavGroupDiagnostics } from './nav-contribution-groups.js';
+import { artifactPackages } from './artifact-packages.js';
+import { collectNavGroupInputs, findNavGroupDiagnostics } from './nav-contribution-groups.js';
 
 type AnyRec = Record<string, unknown>;
 
@@ -148,8 +149,13 @@ const parsedArtifact = (group: string): AnyRec => {
  * file keep passing while the rule the commands actually run drifted — which is
  * the same defect one layer down that the shared `checkNavContributionGroups`
  * exists to prevent.
+ *
+ * ⭐ [#18490] That is now the OWNER's rule and not a mirror of it:
+ * `nav-contribution-groups.ts` used to export its own `artifactPackagesOf`,
+ * which had already drifted from `artifactPackages` on an empty
+ * `manifest.id` + `manifest.name` (pinned at the foot of this file).
  */
-const packagesOf = (parsed: AnyRec) => artifactPackagesOf(parsed);
+const packagesOf = (parsed: AnyRec) => artifactPackages(parsed);
 
 describe('#14553 — `os build` checks `navigationContributions[].group` across one composed artifact', () => {
   it('the fixture really is a two-package artifact — the floor under every reading below', async () => {
@@ -175,7 +181,7 @@ describe('#14553 — `os build` checks `navigationContributions[].group` across
   it('needs only the parsed stack — the commands call it with one argument', async () => {
     // Both `os compile` and `os validate` reach this through
     // `findNavGroupDiagnostics(result.data)`, letting the package walk default
-    // to `artifactPackagesOf`. Pinned because the explicit-packages form is
+    // to `artifactPackages`. Pinned because the explicit-packages form is
     // what every other case here exercises, so a default that silently stopped
     // deriving would leave this file green while both commands went blind.
     const typod = artifact('sales_grp');

packages/cli/src/utils/nav-contribution-groups.ts

@@ -39,6 +39,42 @@
  * contribution: `os build`'s cold path should not pull the data engine in to
  * judge two empty arrays.
  *
+ * ## Why the package-id rule is imported too, and which answer that chose
+ *
+ * `artifactPackages` (`./artifact-packages.ts`) is the declared sole owner of
+ * "which package is this", and this module used to carry a second copy of it.
+ * The two had already drifted on one input — a package whose `manifest.id` AND
+ * `manifest.name` are both the empty string, which
+ * `ObjectStackDefinitionSchema` accepts (both keys are required strings on
+ * `ManifestSchema`; neither has a non-empty constraint, so `''` parses). The
+ * owner computed that package's id as `''`; the copy computed it as
+ * `packages[<index>]`.
+ *
+ * ⭐ Importing the owner therefore CHOSE `''`, and that is the answer this path
+ * wants — measured against the runtime, which is the authority this module
+ * declares it mirrors two sections up. `ObjectQL.registerApp` derives the id it
+ * registers a contribution under as `manifest.id || manifest.name` and has NO
+ * positional fallback, so the fold registers that package under `''` and its
+ * relocation diagnostic reads `Package "" contributes …`. Under the deleted
+ * copy the build printed `Package "packages[0]" …` for the same artifact —
+ * two doors naming one package differently, which is the whole defect the
+ * shared `checkNavContributionGroups` exists to prevent, one field over.
+ *
+ * ⚠️ Two bounds on that reading, because they are where it could be wrong
+ * rather than merely narrow:
+ *
+ *  - **The id is not a key here.** It is carried onto the diagnostic as
+ *    `packageId` and printed; nothing on this path uses it as a map key, a
+ *    dedupe key or a sort key, so two packages that both resolve to `''` still
+ *    produce two findings rather than collapsing into one. Measured, not read.
+ *  - **`artifactPackages` declares a precondition this path meets.** It reads
+ *    the PARSED stack and does not re-check entry shape, so a `null` element
+ *    would throw where the copy returned a positional id. Both commands call
+ *    `findNavGroupDiagnostics(result.data)`, and every malformed element —
+ *    `null`, a string, a non-object `manifest`, a missing one — is refused by
+ *    `ArtifactPackageSchema` before that. ⛔ Do not hand this function a
+ *    hand-built `packages[]` that has not been through the parse.
+ *
  * ## Why the findings ride the declared `warnings` key, and why BOTH commands
  * ## compute them
  *
@@ -63,35 +99,24 @@
 
 import type { NavContributionGroupDiagnostic } from '@objectstack/objectql';
 
+import { artifactPackages } from './artifact-packages.js';
+
 type AnyRec = Record<string, unknown>;
 
 const asArray = (v: unknown): unknown[] => (Array.isArray(v) ? v : []);
 const asRec = (v: unknown): AnyRec | undefined =>
   v && typeof v === 'object' && !Array.isArray(v) ? (v as AnyRec) : undefined;
 
-/** One artifact package, in the `{ id, body }` shape the commands walk. */
-export interface CompiledPackage {
-  readonly id: string;
-  readonly body: AnyRec;
-}
-
 /**
- * The artifact's package entries, derived from the PARSED stack.
+ * One artifact package, in the `{ id, body }` shape the commands walk — the
+ * shape `artifactPackages` returns, narrowed to the two facts read here.
  *
- * Mirrors `compile.ts`' `artifactPackages` id rule — `manifest.id`, falling
- * back to `name`, then to the positional spelling — because that is the string
- * the runtime registers a contribution under, so a command names a package the
- * same way the fold does. Derived here rather than passed in, so both commands
- * reach the check through ONE call that needs only the parsed stack.
+ * ⛔ The id RULE is not re-spelled: this is a structural parameter type, and
+ * `./artifact-packages.ts` remains the only place that computes an id.
  */
-export function artifactPackagesOf(parsed: AnyRec): CompiledPackage[] {
-  return asArray(parsed.packages).map((entry, index) => {
-    const body = asRec((entry as { manifest?: unknown })?.manifest) ?? {};
-    const id = typeof body.id === 'string' && body.id !== ''
-      ? body.id
-      : (typeof body.name === 'string' && body.name !== '' ? body.name : `packages[${index}]`);
-    return { id, body };
-  });
+export interface CompiledPackage {
+  readonly id: string;
+  readonly body: AnyRec;
 }
 
 /**
@@ -170,7 +195,7 @@ export function collectNavGroupInputs(
  */
 export async function findNavGroupDiagnostics(
   parsed: AnyRec,
-  packages: readonly CompiledPackage[] = artifactPackagesOf(parsed),
+  packages: readonly CompiledPackage[] = artifactPackages(parsed),
 ): Promise<NavContributionGroupDiagnostic[]> {
   const { apps, contributions } = collectNavGroupInputs(parsed, packages);
   if (contributions.length === 0 || apps.length === 0) return [];