Skip to content

Commit 93917d1

Browse files
fix(cli): nav-contribution-groups imports the artifact package-id owner instead of carrying a second copy (#18710)
Fixes #18490 ## What changed `packages/cli/src/utils/nav-contribution-groups.ts` no longer carries its own copy of the artifact package-id rule. `artifactPackagesOf` is **deleted**; `artifactPackages` — the owner, in `packages/cli/src/utils/artifact-packages.ts` — is imported and used as `findNavGroupDiagnostics`' default package walk. This is the same move `permission-set-name-collisions.ts` already made, for the reason that module states in its own header. `CompiledPackage` stays: it is a structural parameter type naming the two facts this module reads, not a second spelling of the id rule. ## The half the card left open, measured before anything was edited The card is explicit that importing the owner **is a decision, not a merge**: the owner answers `''` for a package whose id keys are empty, the deleted copy answered the positional spelling `packages[0]`. The dispatch asked which is right for the nav path, and required it measured rather than reasoned. **M1 — the divergent input is REACHABLE.** `ManifestSchema` requires `id` and `name` as strings and constrains neither to be non-empty, so `{ manifest: { id: '', name: '', … } }` parses green through the very `normalizeStackInput` + `ObjectStackDefinitionSchema` chain both commands run. It narrows the card's framing: **both** keys have to be empty. With `id` absent the parse refuses (`packages.0.manifest.id: expected string, received undefined`); with `name` absent, likewise. With `id: 'ok'` and `name: ''` the two rules agreed already. **M2 — behaviour does change, and only there.** On the reachable input the two rules produce different diagnostics; on every other input, byte-identical output (control legs: `id` ok + `name` empty, and both ok). | package identity | copy (deleted) | owner (imported) | |:--|:--|:--| | `id: ''`, `name: ''` | `packageId: 'packages[0]'` | `packageId: ''` | | `id: 'com.example…'`, `name: ''` | `com.example…` | `com.example…` (identical) | | `id: 'com.example…'`, `name: 'Orders'` | `com.example…` | `com.example…` (identical) | **M3 — the runtime is the judge, and it answers `''`.** `nav-contribution-groups.ts`' 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". Asked of a real `ObjectQL`: `registerApp` derives `manifest.id || manifest.name`, which has **no positional fallback at all**, so the fold registers that package under `''` and prints: ``` [Registry] [nav_contribution_group_missing] Package "" contributes 1 navigation item [nav_orders] into group "sales_grp" of app "multi_crm", … ``` The deleted copy made `os build` print `Package "packages[0]"` for that same artifact. ⇒ **the owner's `''` is not merely different, it is the one that matches the runtime**, and `packages[0]` is a name the runtime cannot produce. The STOP-AND-REPORT condition in the dispatch is therefore **not** triggered — the measurement came out in favour of the direction triage settled, and it could have come out the other way: had `registerApp` carried a positional fallback, or dropped a contribution whose id is empty, the copy would have been the runtime-matching side. **M2b — the id is carried and printed, never keyed on.** Two packages that both resolve to `''` still produce **two** findings under both rules; nothing on this path uses the id as a map key, a dedupe key, a route segment or a sort key. Downstream it is spread into the `warnings` array of both commands' JSON payloads, unkeyed. **M4/M5 — the one robustness delta, and why it cannot be reached.** The owner does not re-check entry shape (its header declares that precondition). A `null` element of `packages[]` throws under the owner where the copy returned a positional id. Every malformed element — `null`, a string, a non-object `manifest`, a missing one — is refused by `ArtifactPackageSchema` before either command's `findNavGroupDiagnostics(result.data)` sees it; measured, all five refused. The remaining shapes (`manifest` a string, `manifest` absent) produce identical output under both rules anyway. ⚠️ **Two different claims, kept different**: this PR shows no *current* shipped artifact with empty id keys — it does not look for one, and the card says the blast radius is unmeasured. What it shows is that the input is **accepted by the schema both commands run**, so the divergence was reachable rather than latent-by-construction. ## Tests `packages/cli/src/utils/nav-contribution-groups.package-id.test.ts` — new, four pins: 1. the empty-`id`-and-`name` artifact **parses** (the floor: if a spec change starts refusing it, this reds first and says the pins under it now measure nothing); 2. **the build names that package exactly as the runtime fold does** — build side from the shipped `findNavGroupDiagnostics`, runtime side from a real `ObjectQL`, neither rule re-spelled, asserting the two `packageId` strings and the two messages are equal; 3. and that shared name is **not** the deleted copy's positional spelling (stated separately, because pin 2 would also pass if *both* doors moved to `packages[0]`); 4. two empty-id packages still produce two findings. **Why a separate file.** `new ObjectQL(` is a KERNEL signal in `packages/cli/vitest-tiers.ts`, so a file carrying it is integration tier *by derivation*. Measured: adding the pin to `nav-contribution-groups.test.ts` moved that whole file — and its nine existing #14553 pins — out of the unit tier (unit 212 → 211, integration 47 → 48). Splitting confines the tier change to the cases that actually boot a registry. The unit file now differs only by the import repoint and two comments, and `unitTestFiles()` places it back in `unit`. **Ablation** — the fix reverted to its pre-change blob, the new pins re-run, then restored: ``` pre-mutation : fe0c8f3 (== HEAD blob) post-mutation: d4b626b (== the BASE blob) artifactPackagesOf occurrences: 0 -> 2 'artifactPackages(parsed)' : 1 -> 0 ablated run: 3 failed | 1 passed (4) AssertionError: expected 'packages[0]' to be '' post-restore : fe0c8f3 == HEAD blob; git diff HEAD empty ``` The one pin that stays green under ablation is the reachability floor, which reads the owner directly — the in-run control that the harness is not simply broken. ## Verification, at `6a886b8a4` | what | result | |:--|:--| | `pnpm lint` — **repo-wide**, `eslint . --no-inline-config`, full population, no narrowing | exit 0 | | `pnpm --filter @objectstack/cli typecheck` (incl. `check:test-typecheck`) | exit 0 | | `pnpm --filter @objectstack/cli exec vitest run --project unit` | 212 files, 3026 tests, exit 0 | | `pnpm --filter @objectstack/cli exec vitest run --project integration` | 48 files, 413 tests, exit 0 | | `dispatch-gates.mjs --commands --repo objectstack-ai/objectstack`, every family run, reconciled with `--ran` carrying each exit code | **60 derived, 60 run, 0 NOT-MEASURED, 0 UNRUN** | The integration tier was run locally because this diff **adds** an integration-tier file; it touches no existing one, and no spawn entry. An earlier reconciliation at an intermediate commit recorded `exit 3` for `check:dual-build-cjs-loads` and `check:i18n-coverage` (PREREQUISITE NOT MET — unbuilt `dist`). Both were re-run at the final commit and returned real verdicts (`104 published require entry point(s) … load`; `13 config(s), 621 baselined untranslated string(s), none new`). ## Changeset Clause-②: no `patch` for `@objectstack/cli`. The declaration above is **measured, not assumed**. Published bytes move: `files[]` is `["dist", …]` and `artifactPackagesOf` appears in `dist/utils/nav-contribution-groups.js` and its `.d.ts` (positive control `findNavGroupDiagnostics` also hits). ⇒ `skip-changeset` does not apply. The diff adds no key, arm, export or registration; it removes one, and that removal reaches no consumer — the package's `exports` map publishes `.`, `./console`, `./hook-body` and `./package.json`, and none of the three entry `.d.ts` files names the symbol. ## In-flight fence — verified rather than trusted PR #18675 (card #18491) is the only in-flight claim whose roster names `findNavGroupDiagnostics`, which is defined in the file edited here. Read at its head: its diff is one file, `packages/cli/test/validate-build-gate-parity.test.ts`, and its closed-ledger scan extracts **bare-identifier call sites in `compile.ts` and `validate.ts`**. This change touches neither command, and moves that function's name, signature (two parameters, same names, same types — only the default argument's expression changes) and export not at all. ⇒ the seat's benign judgement holds. The other claim, card #18402, faces `packages/rest/src/` and is disjoint. ## Acceptance notes - **Docs-drift sweep — NOT FALSIFIED.** Predicate stated before reading: *hand-written docs describe the artifact package-id fallback, or show a `nav_contribution_group_missing` package id, such that this change makes that text wrong.* Swept by symbol (`artifactPackagesOf`, `artifactPackages`, `nav_contribution_group_missing`) and by input shape (`packages[0]`, `packages[i]`, the bracketed-index spelling, `Package ""`) over `content/docs/`, `skills/`, `docs/adr/`. Controls both ways: positive `navigationContributions` hit 6 files, negative nonsense token hit 0. One hand-written hit, `content/docs/ui/setup-app.mdx`, says the diagnostic names "the contributing package" generically and shows no id spelling — still true, and marginally *more* true, since its "the same finding" claim about the two doors was what the divergence quietly falsified. The other two hits are under `content/docs/references/`, auto-generated, and reference the error code rather than the id rule. - `collectNavGroupInputs`' single-package branch (no `packages[]`) derives its own `packageId` from the top-level manifest — `manifest.id` then `manifest.name`, both string-guarded, with no positional fallback. It is not the artifact package-id rule and cannot be: with no `packages[]` there is no index. Noted, not filed; no PR or seat is routed to that expression by this change. - A cross-door pin now exists on the CLI side only. The runtime half of #14553 is pinned in `packages/objectql/src/registry-nav-contribution-group-semantics.test.ts`, and nothing there covers the empty-id identity. Noted, not filed — out of the declared face (`packages/cli/src/utils/`), and the new pin reads the real runtime, so the fact is guarded from one side. Would-be carrier: the next card touching that objectql suite. --- _Generated by [Claude Code](https://claude.ai/code)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 02e19a7 commit 93917d1

4 files changed

Lines changed: 266 additions & 24 deletions

File tree

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
---
2+
"@objectstack/cli": patch
3+
---
4+
5+
`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).
6+
7+
Clause-②: no
8+
9+
`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.
10+
11+
- **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>]` ``.
12+
- **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.
13+
- **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.
14+
- **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.
15+
16+
`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`.
Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,195 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #18490 — the artifact package-id rule has ONE owner again, and importing it
5+
* DECIDED the one input the two copies disagreed on.
6+
*
7+
* ## What this file measures, and against what
8+
*
9+
* `packages/cli/src/utils/artifact-packages.ts` declares itself the sole owner
10+
* of "which package is this". `nav-contribution-groups.ts` carried a second
11+
* implementation, `artifactPackagesOf`, differing from the owner in a single
12+
* guard — a non-empty check on `manifest.name`. For a package whose
13+
* `manifest.id` AND `manifest.name` are both `''` the owner answered `''` and
14+
* the copy answered `packages[<index>]`.
15+
*
16+
* Deleting the copy chose `''`. What this file pins is not that string but the
17+
* REASON it is the right one: `''` is what the RUNTIME fold names that package,
18+
* and `packages[<index>]` is a spelling the runtime cannot produce at all
19+
* (`ObjectQL.registerApp` derives `manifest.id || manifest.name`, with no
20+
* positional fallback). `nav-contribution-groups.ts`'s own header says the id
21+
* it carries is "the string the runtime registers a contribution under, so a
22+
* command names a package the same way the fold does" — this is that sentence,
23+
* measured.
24+
*
25+
* ⛔ NEITHER side's rule is re-spelled here. The build's answer comes out of
26+
* the shipped `findNavGroupDiagnostics`, the runtime's out of a real
27+
* `ObjectQL`, and the assertion is that the two STRINGS match — so the day
28+
* either rule moves, this reds instead of agreeing with itself.
29+
*
30+
* ## Why this is a file of its own, and why it is INTEGRATION tier
31+
*
32+
* The cross-door half constructs `new ObjectQL(`, which is a KERNEL signal in
33+
* `vitest-tiers.ts`'s predicate — so a file carrying it is integration tier by
34+
* derivation, not by naming. Folding these cases into the unit-tier
35+
* `nav-contribution-groups.test.ts` would have moved that whole file, and its
36+
* nine existing #14553 pins with it, out of the tier they were written for.
37+
* Splitting keeps the tier change confined to the cases that actually boot a
38+
* registry. ⛔ Do not merge this file back into that one.
39+
*/
40+
41+
import { describe, it, expect } from 'vitest';
42+
import { composeStacks, normalizeStackInput, ObjectStackDefinitionSchema } from '@objectstack/spec';
43+
import { ObjectQL } from '@objectstack/objectql/core';
44+
import { findNavGroupDiagnostics } from './nav-contribution-groups.js';
45+
import { artifactPackages } from './artifact-packages.js';
46+
47+
type AnyRec = Record<string, unknown>;
48+
49+
const APP = 'multi_crm';
50+
const GROUP = 'sales_group';
51+
const TYPO = 'sales_grp';
52+
const CORE_ID = 'com.example.multi.core';
53+
54+
/** The App package — owns the app and the group container the module aims at. */
55+
const coreStack = () => ({
56+
manifest: {
57+
id: CORE_ID,
58+
name: 'Multi-Package Core',
59+
namespace: 'crm',
60+
version: '1.0.0',
61+
type: 'app' as const,
62+
},
63+
objects: [{
64+
name: 'crm_account',
65+
label: 'Account',
66+
sharingModel: 'private' as const,
67+
fields: { name: { name: 'name', type: 'text' as const, label: 'Account Name', required: true } },
68+
}],
69+
apps: [{
70+
name: APP,
71+
label: 'Multi-Package CRM',
72+
navigation: [{
73+
id: GROUP,
74+
type: 'group' as const,
75+
label: 'Sales',
76+
children: [{ id: 'nav_accounts', type: 'object' as const, objectName: 'crm_account', label: 'Accounts' }],
77+
}],
78+
}],
79+
});
80+
81+
/**
82+
* The contributing package, carrying the divergent identity: both id keys `''`.
83+
*
84+
* ⚠️ Its own namespace, not the app package's. The composed-artifact leg does
85+
* not care, but the runtime leg INSTALLS both packages into one registry and
86+
* ADR-0048 refuses a second package under a namespace another already owns —
87+
* a conflict that would make this file red for a reason that has nothing to do
88+
* with what it measures.
89+
*/
90+
const emptyIdOrdersStack = (group: string) => ({
91+
manifest: {
92+
id: '',
93+
name: '',
94+
namespace: 'ord',
95+
version: '1.0.0',
96+
type: 'module' as const,
97+
navigationContributions: [{
98+
app: APP,
99+
group,
100+
items: [{ id: 'nav_orders', type: 'object' as const, objectName: 'ord_order', label: 'Orders' }],
101+
}],
102+
},
103+
objects: [{
104+
name: 'ord_order',
105+
label: 'Order',
106+
sharingModel: 'private' as const,
107+
fields: { name: { name: 'name', type: 'text' as const, label: 'Order Number', required: true } },
108+
}],
109+
});
110+
111+
/** The artifact as the commands actually hand it down — through their own parse. */
112+
const parsedEmptyIdArtifact = (group: string): AnyRec => {
113+
const composed = composeStacks(
114+
[emptyIdOrdersStack(group), coreStack()],
115+
{ manifest: 'preserve' },
116+
) as unknown as Record<string, unknown>;
117+
const normalized = normalizeStackInput(composed, { onConversionNotice: () => {} });
118+
const result = ObjectStackDefinitionSchema.safeParse(normalized);
119+
if (!result.success) {
120+
throw new Error(`fixture does not parse: ${JSON.stringify(result.error.issues.slice(0, 3))}`);
121+
}
122+
return result.data as unknown as AnyRec;
123+
};
124+
125+
describe('#18490 — one package, two doors, one name', () => {
126+
it('an empty `manifest.id` AND `manifest.name` REACHES this check — the divergence is not hypothetical', () => {
127+
// The floor under everything below. `ManifestSchema` requires both keys as
128+
// strings and constrains NEITHER to be non-empty, so `''` parses — which is
129+
// the only reason the two rules could ever disagree on a stack `os build`
130+
// or `os validate` would actually accept. If a spec change starts refusing
131+
// it, this reds FIRST and says the pins under it now measure nothing.
132+
const parsed = parsedEmptyIdArtifact(GROUP);
133+
expect(artifactPackages(parsed).map((pkg) => pkg.id).sort()).toEqual(['', CORE_ID]);
134+
});
135+
136+
it('the build names that package EXACTLY as the runtime fold does', async () => {
137+
// The build door: the shipped derivation, reached the way both commands
138+
// reach it — `findNavGroupDiagnostics(result.data)`, one argument.
139+
const built = await findNavGroupDiagnostics(parsedEmptyIdArtifact(TYPO));
140+
expect(built).toHaveLength(1);
141+
142+
// The runtime door: the same two packages installed into a real registry.
143+
// `registerApp` derives the id it registers the contribution under, the
144+
// read-time fold relocates the mis-aimed items, and recording that
145+
// relocation is what produces the diagnostic.
146+
const engine = new ObjectQL();
147+
const core = coreStack();
148+
engine.registerApp({ ...core.manifest, apps: core.apps });
149+
engine.registerApp({ ...emptyIdOrdersStack(TYPO).manifest });
150+
engine.registry.getApp(APP);
151+
const folded = engine.registry.getAppNavDiagnostics(APP);
152+
expect(folded).toHaveLength(1);
153+
154+
// The card, in two lines: the `packageId` field a consumer reads, and the
155+
// sentence an author reads.
156+
expect(built[0].packageId).toBe(folded[0].packageId);
157+
expect(built[0].message).toBe(folded[0].message);
158+
});
159+
160+
it('⛔ and that shared name is NOT the deleted copy\'s positional spelling', async () => {
161+
// Its own assertion, because the pin above would also pass if BOTH doors
162+
// moved to `packages[0]`. The runtime has no positional fallback, so a
163+
// build printing one is a build naming a package the runtime never will.
164+
const built = await findNavGroupDiagnostics(parsedEmptyIdArtifact(TYPO));
165+
expect(built[0].packageId).toBe('');
166+
expect(built[0].packageId).not.toBe('packages[0]');
167+
expect(built[0].message).not.toContain('packages[0]');
168+
});
169+
170+
it('two packages that both resolve to the empty id still produce TWO findings', async () => {
171+
// The id is CARRIED and PRINTED on this path — never a map key, a dedupe
172+
// key or a sort key. Pinned because "both collapse into one finding" is the
173+
// failure an empty id would cause if it ever became one, and it would
174+
// present as the report going QUIET rather than as an error.
175+
const second = {
176+
manifest: {
177+
id: '',
178+
name: '',
179+
navigationContributions: [{
180+
app: APP,
181+
group: TYPO,
182+
items: [{ id: 'nav_second', type: 'object' as const, objectName: 'ord_order', label: 'Second' }],
183+
}],
184+
},
185+
};
186+
const parsed = parsedEmptyIdArtifact(TYPO);
187+
const widened: AnyRec = {
188+
...parsed,
189+
packages: [...((parsed.packages ?? []) as unknown[]), second],
190+
};
191+
const found = await findNavGroupDiagnostics(widened);
192+
expect(found).toHaveLength(2);
193+
expect(found.map((d) => d.packageId)).toEqual(['', '']);
194+
});
195+
});

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

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,8 @@
5454
import '@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');

‎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)