Skip to content

Commit cc4144a

Browse files
committed
wip: import the artifact package-id owner in nav-contribution-groups
Claude-Session: https://claude.ai/code/session_01DvvamiacK328idtBYJBxV3 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 551139b commit cc4144a

2 files changed

Lines changed: 189 additions & 25 deletions

File tree

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

Lines changed: 143 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,11 @@
5151
// The production import stays lazy and stays where it is: this line decides
5252
// only WHERE the first load is paid in THIS suite, and `os build`'s cold path
5353
// must not pull the data engine in to judge a stack with no contributions.
54-
import '@objectstack/objectql/core';
54+
import { ObjectQL } from '@objectstack/objectql/core';
5555
import { describe, it, expect } from 'vitest';
5656
import { composeStacks, normalizeStackInput, ObjectStackDefinitionSchema } from '@objectstack/spec';
57-
import { artifactPackagesOf, collectNavGroupInputs, findNavGroupDiagnostics } from './nav-contribution-groups.js';
57+
import { artifactPackages } from './artifact-packages.js';
58+
import { collectNavGroupInputs, findNavGroupDiagnostics } from './nav-contribution-groups.js';
5859

5960
type AnyRec = Record<string, unknown>;
6061

@@ -148,8 +149,13 @@ const parsedArtifact = (group: string): AnyRec => {
148149
* file keep passing while the rule the commands actually run drifted — which is
149150
* the same defect one layer down that the shared `checkNavContributionGroups`
150151
* exists to prevent.
152+
*
153+
* ⭐ [#18490] That is now the OWNER's rule and not a mirror of it:
154+
* `nav-contribution-groups.ts` used to export its own `artifactPackagesOf`,
155+
* which had already drifted from `artifactPackages` on an empty
156+
* `manifest.id` + `manifest.name` (pinned at the foot of this file).
151157
*/
152-
const packagesOf = (parsed: AnyRec) => artifactPackagesOf(parsed);
158+
const packagesOf = (parsed: AnyRec) => artifactPackages(parsed);
153159

154160
describe('#14553 — `os build` checks `navigationContributions[].group` across one composed artifact', () => {
155161
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
175181
it('needs only the parsed stack — the commands call it with one argument', async () => {
176182
// Both `os compile` and `os validate` reach this through
177183
// `findNavGroupDiagnostics(result.data)`, letting the package walk default
178-
// to `artifactPackagesOf`. Pinned because the explicit-packages form is
184+
// to `artifactPackages`. Pinned because the explicit-packages form is
179185
// what every other case here exercises, so a default that silently stopped
180186
// deriving would leave this file green while both commands went blind.
181187
const typod = artifact('sales_grp');
@@ -287,3 +293,136 @@ describe('#14553 — `os build` checks `navigationContributions[].group` across
287293
expect(found[0].group).toBe('nav_accounts');
288294
});
289295
});
296+
297+
/**
298+
* #18490 — the package-id rule has ONE owner again, and importing it decided
299+
* the one input the two copies disagreed on.
300+
*
301+
* `nav-contribution-groups.ts` used to carry its own `artifactPackagesOf`. It
302+
* differed from `artifactPackages` in a single guard: a non-empty check on
303+
* `manifest.name`. For a package whose `manifest.id` AND `manifest.name` are
304+
* both `''` the owner answered `''` and the copy answered `packages[<index>]`.
305+
* Deleting the copy chose `''`, so what this block pins is not the string but
306+
* the REASON it is right: `''` is what the runtime fold names that package,
307+
* and `packages[<index>]` is a spelling the runtime cannot produce at all.
308+
*
309+
* ⛔ Neither side's rule is re-spelled here. The build's answer comes out of
310+
* the shipped `findNavGroupDiagnostics`, the runtime's out of a real
311+
* `ObjectQL`, and the assertion is that the two STRINGS match — so the day
312+
* either rule moves, this reds instead of agreeing with itself.
313+
*
314+
* `ObjectQL` is taken from `@objectstack/objectql/core`, the entry this file
315+
* already loads at module top (see the note there): the pin adds assertions,
316+
* not a module graph.
317+
*/
318+
describe('#18490 — the artifact package-id rule is the owner\'s, and both doors name one package alike', () => {
319+
const EMPTY_PKG_NAMESPACE = 'ord';
320+
321+
/** The contributing package, with the divergent identity: both keys `''`. */
322+
const emptyIdOrdersStack = (group: string) => ({
323+
manifest: {
324+
id: '',
325+
name: '',
326+
namespace: EMPTY_PKG_NAMESPACE,
327+
version: '1.0.0',
328+
type: 'module' as const,
329+
navigationContributions: [{
330+
app: APP,
331+
group,
332+
items: [{ id: 'nav_orders', type: 'object' as const, objectName: 'ord_order', label: 'Orders' }],
333+
}],
334+
},
335+
objects: [{
336+
name: 'ord_order',
337+
label: 'Order',
338+
sharingModel: 'private' as const,
339+
fields: { name: { name: 'name', type: 'text' as const, label: 'Order Number', required: true } },
340+
}],
341+
});
342+
343+
/** Through the SAME parse chain `compile.ts` and `validate.ts` run. */
344+
const parsedEmptyIdArtifact = (group: string): AnyRec => {
345+
const composed = composeStacks(
346+
[emptyIdOrdersStack(group), coreStack()],
347+
{ manifest: 'preserve' },
348+
) as unknown as Record<string, unknown>;
349+
const normalized = normalizeStackInput(composed, { onConversionNotice: () => {} });
350+
const result = ObjectStackDefinitionSchema.safeParse(normalized);
351+
if (!result.success) {
352+
throw new Error(`fixture does not parse: ${JSON.stringify(result.error.issues.slice(0, 3))}`);
353+
}
354+
return result.data as unknown as AnyRec;
355+
};
356+
357+
it('an empty `manifest.id` AND `manifest.name` REACHES this check — the divergence is not hypothetical', () => {
358+
// The floor under the two assertions below. `ManifestSchema` requires both
359+
// keys as strings and constrains neither to be non-empty, so `''` parses —
360+
// which is the only reason the two rules' disagreement was ever reachable
361+
// from `os build` / `os validate` at all. If a future spec change refuses
362+
// it, this reds FIRST and says the pins under it now measure nothing.
363+
const parsed = parsedEmptyIdArtifact(GROUP);
364+
const ids = packagesOf(parsed).map((pkg) => pkg.id).sort();
365+
expect(ids).toEqual(['', CORE_ID]);
366+
});
367+
368+
it('the build names that package EXACTLY as the runtime fold does', async () => {
369+
const parsed = parsedEmptyIdArtifact('sales_grp');
370+
const built = await findNavGroupDiagnostics(parsed);
371+
expect(built).toHaveLength(1);
372+
373+
// The same artifact installed into a real registry: `registerApp` derives
374+
// the id it registers a contribution under, `getApp` runs the fold, and
375+
// the fold records its own diagnostic for the relocation.
376+
const engine = new ObjectQL();
377+
const core = coreStack();
378+
engine.registerApp({ ...core.manifest, apps: core.apps });
379+
engine.registerApp({ ...emptyIdOrdersStack('sales_grp').manifest });
380+
engine.registry.getApp(APP);
381+
const folded = engine.registry.getAppNavDiagnostics(APP);
382+
expect(folded).toHaveLength(1);
383+
384+
// The whole point of the card, in two lines: one package, two doors, one
385+
// name — the `packageId` field a consumer reads and the sentence an author
386+
// reads.
387+
expect(built[0].packageId).toBe(folded[0].packageId);
388+
expect(built[0].message).toBe(folded[0].message);
389+
});
390+
391+
it('⛔ and that shared name is NOT the deleted copy\'s positional spelling', async () => {
392+
// Stated as its own assertion because the pin above would also pass if
393+
// BOTH doors moved to `packages[0]`. The runtime has no positional
394+
// fallback (`manifest.id || manifest.name`), so a build printing one is a
395+
// build naming a package the runtime never will.
396+
const parsed = parsedEmptyIdArtifact('sales_grp');
397+
const built = await findNavGroupDiagnostics(parsed);
398+
expect(built[0].packageId).toBe('');
399+
expect(built[0].packageId).not.toBe('packages[0]');
400+
expect(built[0].message).not.toContain('packages[0]');
401+
});
402+
403+
it('two packages that both resolve to the empty id still produce TWO findings', async () => {
404+
// The id is carried and printed, never used as a map key, a dedupe key or
405+
// a sort key on this path. Pinned because "both collapse to one finding"
406+
// is the failure mode an empty id would have if it ever became one, and it
407+
// would present as a REPORT GOING QUIET rather than as an error.
408+
const second = {
409+
manifest: {
410+
id: '',
411+
name: '',
412+
navigationContributions: [{
413+
app: APP,
414+
group: 'sales_grp',
415+
items: [{ id: 'nav_second', type: 'object' as const, objectName: 'ord_order', label: 'Second' }],
416+
}],
417+
},
418+
};
419+
const parsed = parsedEmptyIdArtifact('sales_grp');
420+
const widened: AnyRec = {
421+
...parsed,
422+
packages: [...((parsed.packages ?? []) as unknown[]), second],
423+
};
424+
const found = await findNavGroupDiagnostics(widened);
425+
expect(found).toHaveLength(2);
426+
expect(found.map((d) => d.packageId)).toEqual(['', '']);
427+
});
428+
});

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

Lines changed: 46 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,42 @@
3939
* contribution: `os build`'s cold path should not pull the data engine in to
4040
* judge two empty arrays.
4141
*
42+
* ## Why the package-id rule is imported too, and which answer that chose
43+
*
44+
* `artifactPackages` (`./artifact-packages.ts`) is the declared sole owner of
45+
* "which package is this", and this module used to carry a second copy of it.
46+
* The two had already drifted on one input — a package whose `manifest.id` AND
47+
* `manifest.name` are both the empty string, which
48+
* `ObjectStackDefinitionSchema` accepts (both keys are required strings on
49+
* `ManifestSchema`; neither has a non-empty constraint, so `''` parses). The
50+
* owner computed that package's id as `''`; the copy computed it as
51+
* `packages[<index>]`.
52+
*
53+
* ⭐ Importing the owner therefore CHOSE `''`, and that is the answer this path
54+
* wants — measured against the runtime, which is the authority this module
55+
* declares it mirrors two sections up. `ObjectQL.registerApp` derives the id it
56+
* registers a contribution under as `manifest.id || manifest.name` and has NO
57+
* positional fallback, so the fold registers that package under `''` and its
58+
* relocation diagnostic reads `Package "" contributes …`. Under the deleted
59+
* copy the build printed `Package "packages[0]" …` for the same artifact —
60+
* two doors naming one package differently, which is the whole defect the
61+
* shared `checkNavContributionGroups` exists to prevent, one field over.
62+
*
63+
* ⚠️ Two bounds on that reading, because they are where it could be wrong
64+
* rather than merely narrow:
65+
*
66+
* - **The id is not a key here.** It is carried onto the diagnostic as
67+
* `packageId` and printed; nothing on this path uses it as a map key, a
68+
* dedupe key or a sort key, so two packages that both resolve to `''` still
69+
* produce two findings rather than collapsing into one. Measured, not read.
70+
* - **`artifactPackages` declares a precondition this path meets.** It reads
71+
* the PARSED stack and does not re-check entry shape, so a `null` element
72+
* would throw where the copy returned a positional id. Both commands call
73+
* `findNavGroupDiagnostics(result.data)`, and every malformed element —
74+
* `null`, a string, a non-object `manifest`, a missing one — is refused by
75+
* `ArtifactPackageSchema` before that. ⛔ Do not hand this function a
76+
* hand-built `packages[]` that has not been through the parse.
77+
*
4278
* ## Why the findings ride the declared `warnings` key, and why BOTH commands
4379
* ## compute them
4480
*
@@ -63,35 +99,24 @@
6399

64100
import type { NavContributionGroupDiagnostic } from '@objectstack/objectql';
65101

102+
import { artifactPackages } from './artifact-packages.js';
103+
66104
type AnyRec = Record<string, unknown>;
67105

68106
const asArray = (v: unknown): unknown[] => (Array.isArray(v) ? v : []);
69107
const asRec = (v: unknown): AnyRec | undefined =>
70108
v && typeof v === 'object' && !Array.isArray(v) ? (v as AnyRec) : undefined;
71109

72-
/** One artifact package, in the `{ id, body }` shape the commands walk. */
73-
export interface CompiledPackage {
74-
readonly id: string;
75-
readonly body: AnyRec;
76-
}
77-
78110
/**
79-
* The artifact's package entries, derived from the PARSED stack.
111+
* One artifact package, in the `{ id, body }` shape the commands walk — the
112+
* shape `artifactPackages` returns, narrowed to the two facts read here.
80113
*
81-
* Mirrors `compile.ts`' `artifactPackages` id rule — `manifest.id`, falling
82-
* back to `name`, then to the positional spelling — because that is the string
83-
* the runtime registers a contribution under, so a command names a package the
84-
* same way the fold does. Derived here rather than passed in, so both commands
85-
* reach the check through ONE call that needs only the parsed stack.
114+
* ⛔ The id RULE is not re-spelled: this is a structural parameter type, and
115+
* `./artifact-packages.ts` remains the only place that computes an id.
86116
*/
87-
export function artifactPackagesOf(parsed: AnyRec): CompiledPackage[] {
88-
return asArray(parsed.packages).map((entry, index) => {
89-
const body = asRec((entry as { manifest?: unknown })?.manifest) ?? {};
90-
const id = typeof body.id === 'string' && body.id !== ''
91-
? body.id
92-
: (typeof body.name === 'string' && body.name !== '' ? body.name : `packages[${index}]`);
93-
return { id, body };
94-
});
117+
export interface CompiledPackage {
118+
readonly id: string;
119+
readonly body: AnyRec;
95120
}
96121

97122
/**
@@ -170,7 +195,7 @@ export function collectNavGroupInputs(
170195
*/
171196
export async function findNavGroupDiagnostics(
172197
parsed: AnyRec,
173-
packages: readonly CompiledPackage[] = artifactPackagesOf(parsed),
198+
packages: readonly CompiledPackage[] = artifactPackages(parsed),
174199
): Promise<NavContributionGroupDiagnostic[]> {
175200
const { apps, contributions } = collectNavGroupInputs(parsed, packages);
176201
if (contributions.length === 0 || apps.length === 0) return [];

0 commit comments

Comments
 (0)