diff --git a/.changeset/18490-nav-contribution-groups-imports-package-id-owner.md b/.changeset/18490-nav-contribution-groups-imports-package-id-owner.md new file mode 100644 index 00000000000..8c0d17c521b --- /dev/null +++ b/.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[]` ``. +- **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[]`, 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`. diff --git a/packages/cli/src/utils/nav-contribution-groups.package-id.test.ts b/packages/cli/src/utils/nav-contribution-groups.package-id.test.ts new file mode 100644 index 00000000000..bc8e7b2ef58 --- /dev/null +++ b/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[]`. + * + * 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[]` 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; + +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; + 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(['', '']); + }); +}); diff --git a/packages/cli/src/utils/nav-contribution-groups.test.ts b/packages/cli/src/utils/nav-contribution-groups.test.ts index 043c0f27b56..d82a769f548 100644 --- a/packages/cli/src/utils/nav-contribution-groups.test.ts +++ b/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; @@ -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'); diff --git a/packages/cli/src/utils/nav-contribution-groups.ts b/packages/cli/src/utils/nav-contribution-groups.ts index d4b626b38c4..fe0c8f3bf05 100644 --- a/packages/cli/src/utils/nav-contribution-groups.ts +++ b/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[]`. + * + * ⭐ 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; 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 { const { apps, contributions } = collectNavGroupInputs(parsed, packages); if (contributions.length === 0 || apps.length === 0) return [];