diff --git a/.changeset/nav-contribution-group-relocation-diagnostic.md b/.changeset/nav-contribution-group-relocation-diagnostic.md new file mode 100644 index 0000000000..4a7f199ee6 --- /dev/null +++ b/.changeset/nav-contribution-group-relocation-diagnostic.md @@ -0,0 +1,51 @@ +--- +"@objectstack/objectql": patch +"@objectstack/cli": patch +--- + +fix(objectql,cli): a navigation contribution relocated past a missing group now says so, at `warn` and at build time + +A package that injects navigation into another package's app names the target +container by id (`navigationContributions[].group`). When that id matches no +`type: "group"` node in the target app, `SchemaRegistry.applyNavContributions` +appends the items at the app's **top level** and continues. + +That relocation is unchanged, deliberately. The merge is a read-time fold +precisely so registration order does not matter — `registerAppNavContribution` +does not require the target app to exist yet — and a package contributing into +an *optional* group has to keep working. Refusing would trade both away. + +What changes is that it is no longer invisible. The only trace used to be one +log line gated at `info`/`debug`, so a deployment running at +`OS_REGISTRY_LOG=warn` watched its information architecture change in complete +silence: a typo'd group id — exactly what an AI author emits — turned a nested +menu entry into a top-level one, and because the entry was still *present*, no +smoke test noticed. That is worse than a dropped entry, which someone notices. + +The trace is now a real diagnostic naming the contributing package, the target +app, the missing group id and the relocated items: + +- **At runtime.** An ADR-0038 `BuildIssue`-family record (ADR-0112 D6c — a + diagnostics code, lowercase and out of the error ledger) is carried on the + app and reachable as `registry.getAppNavDiagnostics(appName)`, and announced + through `console.warn`, so it survives `OS_REGISTRY_LOG=warn` and reaches + `os doctor` / boot output. Emitted once per registry per distinct mis-aim: + the fold runs on every read of the app, and a line printed per request is as + unreadable as one never printed. A deployment that asks for `silent` still + gets silence, and still keeps the record. +- **At authoring time.** `os build` and `os validate` answer the same question + over a composed artifact, through the same predicate, and report the same + finding where an author sees it first — in the text output and in `--json` + under the existing `warnings` key, beside the authoring-rule advisories and + capability hints. A contribution aimed at an app no package in the artifact + ships is not reported: contributing into an app another artifact installs is + the supported case, and is why the merge is a fold. + +**Nothing is refused.** No new failure, no ordering constraint, no change to +what installs or to what `os build` accepts — a diagnostic was added and a +refusal was not. + +`examples/app-multi-package` now demonstrates the mechanism it was missing: the +App package publishes a `sales_group` container and the Orders module +contributes its nav entry into it, which is what a module split converts an +app's own navigation into. diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 6990eb7383..4361fd2f8a 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -235,7 +235,7 @@ should recognise it instead of re-deriving it. rule-materialised grant that the next reconcile silently restores. 5. **`applySystemFields` does not read this flag.** It is named as if it did. - `packages/objectql/src/registry.ts:464` is **schema-side column + `packages/objectql/src/registry.ts:475` is **schema-side column provisioning** — which columns an object carries — and consumes `ExecutionContext.isSystem` zero times. The write-time ownership behaviour people attribute to it is row 2, in `plugin-security`. diff --git a/content/docs/ui/setup-app.mdx b/content/docs/ui/setup-app.mdx index ca6baf5dbd..40d0a28637 100644 --- a/content/docs/ui/setup-app.mdx +++ b/content/docs/ui/setup-app.mdx @@ -79,6 +79,37 @@ A few notable entries: live in `plugin-audit`, but they are not contributed as Setup nav entries.) +### When a contribution names an anchor that is not there + +Aiming at a `group` id the target app does not declare is **not** refused and +the entry is **not** dropped: the items are appended at the app's **top level** +and the merge continues. That is deliberate — the merge is a read-time fold +precisely so registration order does not matter (a contributor may register +before the app it aims at), and a contribution into an *optional* anchor has to +keep working when the plugin owning that anchor is not loaded. + +It is, however, **loud**. The relocation emits a +`nav_contribution_group_missing` diagnostic at `warn` — naming the contributing +package, the target app, the missing group id and the relocated items — so it +survives `OS_REGISTRY_LOG=warn` and appears in boot output. The same finding is +carried on the app itself, readable as +`registry.getAppNavDiagnostics(appName)`, and it is raised once per distinct +mis-aim rather than once per read of the app. + +`os build` **and `os validate`** answer the same question at compile time +whenever the contributing package and the target app are composed into one +artifact. Both print the finding and both carry it in `--json` under the +existing `warnings` key, beside the authoring-rule advisories and the +capability hints — the payload is deliberately closed, so a new class of +finding fills a declared key rather than adding one. They **report** there; +neither fails the build. + +⚠️ The anchor id is the whole contract between a shell and its contributors, +and a contributor cannot see the shell's ids at authoring time. A typo +therefore produces a menu that renders, passes a smoke test, and has silently +moved the entry one level up — which is why the diagnostic exists rather than a +refusal. + ## Why a shell + contributions The Setup App is a shell of empty group anchors rather than a fixed diff --git a/examples/app-multi-package/src/packages/core/index.ts b/examples/app-multi-package/src/packages/core/index.ts index 5508dac9b0..0f2815bc18 100644 --- a/examples/app-multi-package/src/packages/core/index.ts +++ b/examples/app-multi-package/src/packages/core/index.ts @@ -43,8 +43,20 @@ export default defineStack({ name: 'multi_crm', label: 'Multi-Package CRM', description: 'Accounts, plus whatever modules this artifact delivers alongside', + // The group is a CONTAINER this package owns and modules aim at + // (ADR-0029 D7). It is the App package's half of the split: a module + // cannot declare a group inside an app it does not own, so the app has + // to publish the container its modules contribute into — which is what + // makes `navigationContributions[].group` resolvable at all. navigation: [ - { id: 'nav_accounts', type: 'object', objectName: 'crm_account', label: 'Accounts', icon: 'building' }, + { + id: 'sales_group', + type: 'group', + label: 'Sales', + children: [ + { id: 'nav_accounts', type: 'object', objectName: 'crm_account', label: 'Accounts', icon: 'building' }, + ], + }, ], }, ], diff --git a/examples/app-multi-package/src/packages/orders/index.ts b/examples/app-multi-package/src/packages/orders/index.ts index 096c8a18cd..46ea1919c8 100644 --- a/examples/app-multi-package/src/packages/orders/index.ts +++ b/examples/app-multi-package/src/packages/orders/index.ts @@ -32,6 +32,22 @@ import { defineStack } from '@objectstack/spec'; * legal and is the whole point of the split: cross-package lookups are accepted * (ADR-0130 §1.5), while a package's own app navigation pointing at a foreign * object is not — which is why the navigation lives with the App package. + * + * ## Why it also carries a `navigationContributions` entry (#14553) + * + * The other half of that same rule. R3 refuses an app's OWN `navigation` entry + * naming another package's object, so a module split converts every such entry + * into a contribution owned by the module — which is exactly what this one is: + * `crm_order` is reachable from the App's menu without the App package + * knowing the object exists. + * + * ⚠️ `group` names `sales_group`, a container the CORE package declares. A + * module cannot see that id at authoring time, and a typo in it does not fail: + * the runtime RELOCATES the items to the app's top level and says so + * (`nav_contribution_group_missing`, at `warn`), and `os build` reports the + * same finding at compile time. This fixture is where that is measured — keep + * the id spelled correctly here, so a build of this example stays clean and the + * pin that typos it has something to differ from. */ export default defineStack({ manifest: { @@ -46,6 +62,19 @@ export default defineStack({ // it as the topological edge that registers core BEFORE orders (ADR-0130 // D5, ADR-0116's one sorter) — the array order below is not what decides. dependencies: { 'com.example.multi.core': '^1.0.0' }, + + // ADR-0029 D7 — `navigationContributions` is a MANIFEST key, not a stack + // collection: it describes what this PACKAGE injects into someone else's + // app, so it travels with the package identity. + navigationContributions: [ + { + app: 'multi_crm', + group: 'sales_group', + items: [ + { id: 'nav_orders', type: 'object', objectName: 'crm_order', label: 'Orders', icon: 'shopping-cart' }, + ], + }, + ], }, objects: [ diff --git a/packages/cli/src/commands/compile.ts b/packages/cli/src/commands/compile.ts index c1f3d0453f..fcbacb9f65 100644 --- a/packages/cli/src/commands/compile.ts +++ b/packages/cli/src/commands/compile.ts @@ -42,6 +42,10 @@ import { errorCodeFields, } from '../utils/format.js'; import { checkProtocolVersionGap } from '../utils/protocol-version-gap.js'; +// [#14553] The compile-time half of the navigation-contribution group ruling. +// Reports; never refuses — the runtime still relocates, deliberately. +import { findNavGroupDiagnostics } from '../utils/nav-contribution-groups.js'; +import type { NavContributionGroupDiagnostic } from '@objectstack/objectql'; /** * The artifact's package entries, as `{ index, id, body }` (ADR-0130 D4). @@ -177,11 +181,23 @@ export default class Compile extends Command { let capProviderWarnings: Array<{ token: string; message: string }> = []; let unknownKeyWarnings: string[] = []; let docWarnings: DocIssue[] = []; + // [#14553] A member of `warningsSoFar()`, NOT a payload key of its own. + // + // ⛔ The first cut made it a separate top-level key and two standing pins + // refused it by name — `build-json-advisory-parity` and + // `build-json-undeclared-key-parity`, both titled "adds NO new top-level + // key to the payload — this fills a declared key, it is not a new + // surface". #11643 and #11727 each faced this choice and filled + // `warnings`. `os validate` computes the same list, so the parity the + // third pin in that file asserts ("nothing rides in build that validate + // does not also report") holds rather than being weakened to fit. + let navGroupWarnings: NavContributionGroupDiagnostic[] = []; const warningsSoFar = () => [ ...ruleAdvisories, ...docWarnings, ...unknownKeyWarnings, ...capProviderWarnings, + ...navGroupWarnings, ]; // [#12125] The ADR-0087 D2 conversion notices, hoisted for the SAME reason // and under the SAME ruling as the four lists above — one field over. The @@ -446,6 +462,37 @@ export default class Compile extends Command { } } + // 3b-bis. [#14553] Navigation contributions whose `group` names no group + // in the target app. RUNS ON EVERY BUILD, artifact or not — the block + // above is skipped for a single-package stack, but a stack that + // declares an app AND contributes into it has the identical defect + // and `collectNavGroupInputs` reads it from the top-level manifest. + // + // ⛔ REPORTS, NEVER REFUSES. The maintainer ruled option B: the + // runtime keeps relocating the items to the app's top level (the fold + // stays order-independent, contributions into optional groups keep + // working) and the failure becomes VISIBLE instead. Making this exit + // non-zero would be option A wearing a warning's clothes, and would + // narrow what `os build` accepts — which the ruling explicitly does + // not do. + // + // A contribution whose target app is NOT in this compilation unit + // yields nothing: contributing into an app another artifact ships is + // the supported cross-artifact case, and is precisely why the merge + // is a read-time fold. Only the composed case can be judged here. + navGroupWarnings = await findNavGroupDiagnostics(result.data as Record); + if (navGroupWarnings.length > 0 && !flags.json) { + console.log(''); + printWarning( + `Navigation contributions aimed at a group the target app does not declare ` + + `(${navGroupWarnings.length}) — the items still install, RELOCATED to the app's top level`, + ); + printBulletList( + navGroupWarnings.map((d) => `[${d.code}] ${d.message} Fix: ${d.fix}`), + { noun: 'navigation-contribution diagnostic' }, + ); + } + // 3c. [#3366] Installable-provider preflight. Every capability the app // DECLARES in `requires: [...]` must have a provider resolvable in the // active edition. A `requires` entry whose provider has NO installable diff --git a/packages/cli/src/commands/validate.ts b/packages/cli/src/commands/validate.ts index 9bc527a104..6d9ae54023 100644 --- a/packages/cli/src/commands/validate.ts +++ b/packages/cli/src/commands/validate.ts @@ -30,11 +30,17 @@ import { formatZodErrors, collectMetadataStats, printMetadataStats, + printWarning, + printBulletList, emitJson, isExitSignal, errorCodeFields, } from '../utils/format.js'; import { checkProtocolVersionGap } from '../utils/protocol-version-gap.js'; +// [#14553] The navigation-contribution group check, shared with `os compile`. +// Reports; never refuses — the runtime still relocates, deliberately. +import { findNavGroupDiagnostics } from '../utils/nav-contribution-groups.js'; +import type { NavContributionGroupDiagnostic } from '@objectstack/objectql'; export default class Validate extends Command { static override description = @@ -120,12 +126,25 @@ export default class Validate extends Command { let unknownKeyWarnings: string[] = []; let docWarnings: DocIssue[] = []; let structuralWarnings: string[] = []; + // [#14553] Computed HERE as well as in `os compile`, not only there. The + // #11727 residue pin asserts that nothing rides in build's `warnings` that + // validate does not also report, and the two commands being one wall with + // two doors is the #4409 / #4463 discipline this list already follows. + let navGroupWarnings: NavContributionGroupDiagnostic[] = []; const warningsSoFar = () => [ ...ruleAdvisories, ...docWarnings, ...unknownKeyWarnings, ...capProviderWarnings, ...structuralWarnings, + // [#14553] APPENDED, and the position is load-bearing. #12047's + // `the order lives at ONE site` pin matches the five members above as + // CONTIGUOUS source text — that is how it proves the order is defined + // once rather than re-spelled per exit. Slotting a sixth member (or even + // a comment) between them breaks that match, so a new member goes on the + // end and the pin keeps guarding exactly what it was written to guard. + // ⛔ Do not "fix" that pin by loosening its regex. + ...navGroupWarnings, ]; // [#12125] The ADR-0087 D2 conversion notices, hoisted for the SAME reason // and under the SAME ruling as the five lists above — one field over. The @@ -269,6 +288,26 @@ export default class Validate extends Command { // an advisory `pnpm add` hint. Mirrors the `os build` gate exactly. // // Not a registry rule: it reads `node_modules`, not the stack. + // [#14553] Navigation contributions whose `group` names no group in an + // app this same compilation unit ships. Reports, never refuses: the + // runtime relocates the items to the app's top level deliberately + // (the read-time fold stays order-independent, contributions into + // optional groups keep working), so what was missing was visibility, + // not a gate. A contribution aimed at an app no package here ships is + // NOT reported — that is the supported cross-artifact case. + navGroupWarnings = await findNavGroupDiagnostics(result.data as Record); + if (navGroupWarnings.length > 0 && !flags.json) { + console.log(''); + printWarning( + `Navigation contributions aimed at a group the target app does not declare ` + + `(${navGroupWarnings.length}) — the items still install, RELOCATED to the app's top level`, + ); + printBulletList( + navGroupWarnings.map((d) => `[${d.code}] ${d.message} Fix: ${d.fix}`), + { noun: 'navigation-contribution diagnostic' }, + ); + } + if (!flags.json) printStep('Checking capability providers (#3366)...'); const capProviderPreflight = preflightRequiredCapabilities({ requires: Array.isArray((config as { requires?: unknown[] }).requires) diff --git a/packages/cli/src/utils/nav-contribution-groups.test.ts b/packages/cli/src/utils/nav-contribution-groups.test.ts new file mode 100644 index 0000000000..043c0f27b5 --- /dev/null +++ b/packages/cli/src/utils/nav-contribution-groups.test.ts @@ -0,0 +1,289 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #14553 — `os build`'s compile-time half: a `navigationContributions[].group` + * that names no group in an app the SAME artifact ships. + * + * ## What this file measures, and against what + * + * The maintainer ruled (2026-09-02, verbatim 「同意」) that the runtime keeps + * relocating such a contribution to the app's top level and says so at `warn`, + * and that `os build` reports the same finding at compile time "where an AI + * author sees it first". The runtime half is pinned in + * `packages/objectql/src/registry-nav-contribution-group-semantics.test.ts`; + * this file is the build half. + * + * The inputs run through the REAL `composeStacks(…, { manifest: 'preserve' })` + * rather than a hand-written artifact literal, because the whole question is + * what a COMPOSED artifact looks like — a literal would pin this check against + * a shape nothing produces, and would go on passing the day composition + * changes. They mirror `examples/app-multi-package`, which is the fixture the + * ruling names, including its two deliberate properties (the module is listed + * first; it declares `dependencies` on the app package). + * + * ⚠️ MIRRORED, not imported. `packages/metadata`'s artifact-attribution suite + * takes the same approach for the same example and states the reason: a test + * reaching outside its own package for a fixture is invisible to + * `turbo ls --affected` and to the `test` task's input hashing unless it is + * spelled the way `check:cross-package-test-inputs` recognises, and the shape + * under test here is three keys deep. The example is edited in the same PR to + * carry the correct spelling, so the fixture below and the example agree by + * construction; what this file pins is the DERIVATION, and `sales_group` / + * `multi_crm` are named identically in both so a reader can diff them by eye. + * + * ## Why the typo is applied to the composed artifact and not authored twice + * + * Both legs compose from ONE pair of stacks and differ in a single character + * position — the `group` id — so nothing else can drift between the clean and + * the typo'd reading. A second hand-authored fixture would be a second thing to + * keep in sync, and a divergence in it would read as a finding. + */ + +// [#7668 family, `check:test-source-alias`] A MODULE-TOP side-effect load of +// the dependency `findNavGroupDiagnostics` imports dynamically. This package +// resolves `@objectstack/objectql/core` through its `dist/`, so the first call +// transforms that whole module graph — measured here at over vitest's 5s +// default, inside a test body, which is a CLOCKED window. Paying it during +// COLLECTION (which vitest clocks against nothing) is the convention: clocked +// windows measure behaviour, never loading. ⛔ Do not "fix" a timeout here by +// widening it — that relocates the cliff to the next heavier shard. +// +// The production import stays lazy and stays where it is: this line decides +// only WHERE the first load is paid in THIS suite, and `os build`'s cold path +// must not pull the data engine in to judge a stack with no contributions. +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'; + +type AnyRec = Record; + +const APP = 'multi_crm'; +const GROUP = 'sales_group'; +const CORE_ID = 'com.example.multi.core'; +const ORDERS_ID = 'com.example.multi.orders'; + +/** The App package — owns the app and the group container modules aim 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 Module package — contributes into the App package's group. */ +const ordersStack = (group: string) => ({ + manifest: { + id: ORDERS_ID, + name: 'Multi-Package Orders', + namespace: 'crm', + version: '1.0.0', + type: 'module' as const, + dependencies: { [CORE_ID]: '^1.0.0' }, + navigationContributions: [{ + app: APP, + group, + items: [{ id: 'nav_orders', type: 'object' as const, objectName: 'crm_order', label: 'Orders' }], + }], + }, + objects: [{ + name: 'crm_order', + label: 'Order', + sharingModel: 'private' as const, + fields: { name: { name: 'name', type: 'text' as const, label: 'Order Number', required: true } }, + }], +}); + +/** Compose exactly as `examples/app-multi-package/objectstack.config.ts` does. */ +const artifact = (group: string): AnyRec => + composeStacks([ordersStack(group), coreStack()], { manifest: 'preserve' }) as unknown as AnyRec; + +/** + * The composed artifact as `compile.ts` actually hands it to this check — + * through the SAME `normalizeStackInput` + `ObjectStackDefinitionSchema` + * parse the command runs, and `result.data` is the object it passes. + * + * ⚠️ Load-bearing, and the one thing a raw `composeStacks` reading cannot + * establish. The check walks `parsed.packages[].manifest`, and if the parse + * reshaped, renamed or dropped any part of that path the derivation would + * silently see an empty artifact — reporting nothing, which is + * indistinguishable from a clean stack. This is asserted rather than assumed + * because the alternative was a 57-package build of the CLI's dependency + * closure to spawn one `os build`. + */ +const parsedArtifact = (group: string): AnyRec => { + const normalized = normalizeStackInput(artifact(group) as Record, { + 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; +}; + +/** + * The artifact's packages, through the SHIPPED derivation. + * + * ⛔ Not a local re-spelling of the id rule. A second copy here would let this + * 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. + */ +const packagesOf = (parsed: AnyRec) => artifactPackagesOf(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 () => { + // Without this, a composition change that stopped emitting `packages[]` + // would make every assertion in this file vacuously true: the derivation + // would see no contributions and report nothing, which is also what "clean" + // looks like. Asserted first, and on the CLEAN leg, so the file cannot go + // green by measuring an empty artifact. + const parsed = artifact(GROUP); + const packages = packagesOf(parsed); + expect(packages.map((p) => p.id).sort()).toEqual([CORE_ID, ORDERS_ID]); + + const { apps, contributions } = collectNavGroupInputs(parsed, packages); + expect(apps.map((a) => a.name)).toContain(APP); + expect(contributions).toHaveLength(1); + expect(contributions[0]).toMatchObject({ app: APP, group: GROUP, packageId: ORDERS_ID }); + }); + + it('the correctly spelled group id reports NOTHING', async () => { + expect(await findNavGroupDiagnostics(artifact(GROUP), packagesOf(artifact(GROUP)))).toEqual([]); + }); + + 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 + // 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'); + expect(await findNavGroupDiagnostics(typod)).toEqual( + await findNavGroupDiagnostics(typod, packagesOf(typod)), + ); + expect(await findNavGroupDiagnostics(typod)).toHaveLength(1); + expect(await findNavGroupDiagnostics(artifact(GROUP))).toEqual([]); + }); + + it('ONE typo\'d group id prints one diagnostic naming the package, the app, the group and the items', async () => { + const parsed = artifact('sales_grp'); + const found = await findNavGroupDiagnostics(parsed, packagesOf(parsed)); + + expect(found).toHaveLength(1); + expect(found[0]).toMatchObject({ + code: 'nav_contribution_group_missing', + severity: 'warning', + app: APP, + packageId: ORDERS_ID, + group: 'sales_grp', + relocated: ['nav_orders'], + }); + // The four facts the ruling required, in the printed text — this is the + // string an author actually reads, and `severity: 'warning'` is what says + // the build still succeeds. + expect(found[0].message).toContain(ORDERS_ID); + expect(found[0].message).toContain(APP); + expect(found[0].message).toContain('sales_grp'); + expect(found[0].message).toContain('nav_orders'); + expect(found[0].fix).toContain('sales_grp'); + }); + + it('reports the mis-aim ONCE, not once per surface the composed artifact carries it on', async () => { + // `manifest: 'preserve'` is ADDITIVE: it flattens every collection to the + // top level AND still picks a singular `manifest` by the default `'last'` + // rule. So the contribution is reachable twice in a composed artifact — + // once under `packages[]`, once under the picked top-level manifest — and a + // walk that read both would report the same author error twice, the second + // time with no package id to act on. + const parsed = artifact('sales_grp'); + expect(await findNavGroupDiagnostics(parsed, packagesOf(parsed))).toHaveLength(1); + }); + + it('a contribution into an app NO package here ships is not a finding — the cross-artifact case stays supported', async () => { + // The bound that keeps this a report rather than option A. A package may + // legally contribute into an app shipped by a different artifact installed + // separately — that is precisely why the merge is a read-time fold — so the + // only mis-aims a build can judge are the ones it can see both halves of. + const parsed = artifact(GROUP); + (parsed as AnyRec).packages = ((parsed.packages ?? []) as unknown[]).slice(); + const orders = ordersStack(GROUP); + const found = await findNavGroupDiagnostics( + { packages: [] }, + [{ id: ORDERS_ID, body: { ...orders.manifest } as AnyRec }], + ); + expect(found).toEqual([]); + }); + + it('a SINGLE-package stack is judged too — the same defect, without an artifact', async () => { + // `packages[]` is absent from an ordinary `defineStack` project, so the + // artifact walk skips it entirely. A stack that declares an app and + // contributes into it has the identical mis-aim, and the top-level manifest + // is where its contributions live. + const single: AnyRec = { + manifest: { + id: 'com.example.single', + name: 'Single', + navigationContributions: [{ + app: APP, + group: 'sales_grp', + items: [{ id: 'nav_orders', type: 'object' as const, objectName: 'crm_order', label: 'Orders' }], + }], + }, + apps: coreStack().apps, + }; + const found = await findNavGroupDiagnostics(single, []); + expect(found).toHaveLength(1); + expect(found[0]).toMatchObject({ app: APP, group: 'sales_grp', packageId: 'com.example.single' }); + }); + + it('survives the command\'s own parse — what `compile.ts` passes in is what this reads', async () => { + // The end-to-end statement this file can make without spawning the CLI: + // run the fixture through the real `normalizeStackInput` + + // `ObjectStackDefinitionSchema` chain `os build` runs, hand `result.data` + // to the derivation exactly as the command does, and require the SAME + // finding the raw composed object produces. + // + // Both directions matter. The typo'd leg proves the parse does not hide the + // path this check walks; the clean leg proves it does not invent findings. + const typod = parsedArtifact('sales_grp'); + const found = await findNavGroupDiagnostics(typod, packagesOf(typod)); + expect(found).toHaveLength(1); + expect(found[0]).toMatchObject({ app: APP, packageId: ORDERS_ID, group: 'sales_grp', relocated: ['nav_orders'] }); + + const clean = parsedArtifact(GROUP); + expect(packagesOf(clean).map((pkg) => pkg.id).sort()).toEqual([CORE_ID, ORDERS_ID]); + expect(await findNavGroupDiagnostics(clean, packagesOf(clean))).toEqual([]); + }); + + it('naming an id that exists but is not a `type: "group"` node reaches the same diagnostic', async () => { + // `findNavGroup` requires BOTH `id` and `type === 'group'`, so pointing at + // the app's `nav_accounts` object entry is the same authoring error as + // pointing at nothing — and the build must not let the near-miss through + // just because the id resolves to something. + const parsed = artifact('nav_accounts'); + const found = await findNavGroupDiagnostics(parsed, packagesOf(parsed)); + expect(found).toHaveLength(1); + expect(found[0].group).toBe('nav_accounts'); + }); +}); diff --git a/packages/cli/src/utils/nav-contribution-groups.ts b/packages/cli/src/utils/nav-contribution-groups.ts new file mode 100644 index 0000000000..d4b626b38c --- /dev/null +++ b/packages/cli/src/utils/nav-contribution-groups.ts @@ -0,0 +1,179 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `os build`'s compile-time half of the #14553 ruling — the group id a + * `navigationContributions[]` entry names, checked where an AI author sees it + * first. + * + * ## Why the check lives here and not in `composeStacks` + * + * Maintainer ruling, 2026-09-02 (verbatim: 「同意」): "when the contributing + * package and the target app are composed into one artifact (`composeStacks`), + * `os build` checks the group id at compile time and reports the same + * diagnostic loudly there". The ruled location is the build STEP, and it is + * also the only one that can be right: `composeStacks` is a spec-level + * composition used by `os dev`, `os validate` and every embedder, whereas the + * question "did this author aim a contribution at a group that exists?" is a + * report, not a composition rule. Nothing here refuses — the runtime still + * relocates, deliberately (option A was weighed and not taken) — so a + * composition function is the wrong place to raise it. + * + * ## Why not an `@objectstack/lint` authoring rule + * + * `runAuthoringRules` hands a rule ONE stack (`run(stack, ctx)`), and every + * member of that table reads one. This question is cross-package by + * construction: the group id is declared by package A's app and named by + * package B's manifest, so the per-package walk in `compile.ts` sees neither + * half on its own and only the artifact does. Adding an artifact-aware member + * to a single-stack rule table is a larger platform change than the ruling + * asked for, and `compile.ts` already owns the artifact layer (`artifactPackages`, + * the capability preflight, the docs sweep). + * + * ## Why the predicate is imported and not written here + * + * {@link checkNavContributionGroups} is `@objectstack/objectql`'s, the same + * function `SchemaRegistry.applyNavContributions` folds through. A second copy + * of "does this group resolve?" would let the build call an aim fine that the + * runtime relocates — the exact silent divergence the card is about, one layer + * up. Loaded LAZILY, and only for a stack that actually declares a + * contribution: `os build`'s cold path should not pull the data engine in to + * judge two empty arrays. + * + * ## Why the findings ride the declared `warnings` key, and why BOTH commands + * ## compute them + * + * The first cut gave `os build --json` a new top-level + * `navigationGroupDiagnostics` key. Two standing pins caught it — + * `build-json-advisory-parity.e2e.test.ts` and + * `build-json-undeclared-key-parity.e2e.test.ts`, both titled "adds NO new + * top-level key to the payload — this fills a declared key, it is not a new + * surface". #11643 and #11727 each faced the same choice and filled + * `warnings`; that payload's shape is MIRRORED from `os validate --json` so a + * consumer reads one shape per class from either command rather than learning + * two. + * + * A third pin in the same file settles the other half: the ONLY permitted + * residue between the two payloads is the structural advisory set, and + * "nothing rides in build that validate does not also report". Folding these + * into build's `warnings` alone would therefore have turned THAT pin red. So + * both commands compute them — which is the better answer on its own terms + * too: an author running `os validate` sees the mis-aim exactly as one running + * `os build` does. + */ + +import type { NavContributionGroupDiagnostic } from '@objectstack/objectql'; + +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. + * + * 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. + */ +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 }; + }); +} + +/** + * The apps and the contributions visible in ONE compilation unit. + * + * ⚠️ The top-level manifest is read only when the stack carries no `packages[]`. + * `composeStacks(…, { manifest: 'preserve' })` is additive: it flattens every + * collection to the top level AND still picks a singular `manifest` by the + * default `'last'` rule, so on an artifact the top-level + * `manifest.navigationContributions` is a COPY of one package's — reading both + * would report that package's mis-aim twice, once without a package id. + */ +export function collectNavGroupInputs( + parsed: AnyRec, + packages: readonly CompiledPackage[], +): { + apps: Array<{ name?: unknown; navigation?: unknown }>; + contributions: Array<{ app?: unknown; group?: unknown; items?: unknown; packageId?: string }>; +} { + const apps: Array<{ name?: unknown; navigation?: unknown }> = []; + const seenApps = new Set(); + const addApps = (list: unknown) => { + for (const entry of asArray(list)) { + const app = asRec(entry); + if (!app || typeof app.name !== 'string' || seenApps.has(app.name)) continue; + seenApps.add(app.name); + apps.push({ name: app.name, navigation: app.navigation }); + } + }; + + const contributions: Array<{ app?: unknown; group?: unknown; items?: unknown; packageId?: string }> = []; + const addContributions = (list: unknown, packageId?: string) => { + for (const entry of asArray(list)) { + const c = asRec(entry); + if (!c) continue; + contributions.push({ + app: c.app, + group: c.group, + items: c.items, + ...(packageId === undefined ? {} : { packageId }), + }); + } + }; + + // The flattened top level — present on every stack, artifact or not. + addApps(parsed.apps); + + if (packages.length > 0) { + for (const pkg of packages) { + // An assembled package body IS its own manifest (`compile.ts`' + // `packageBodyAsStack`), so both collections read off the top of it. + addApps(pkg.body.apps); + // `id` is the artifact's own package key (`manifest.id`, falling back to + // `name`), which is the string the runtime registers the contribution + // under — so the build names the package the same way the fold does. + addContributions(pkg.body.navigationContributions, pkg.id); + } + } else { + const manifest = asRec(parsed.manifest); + const packageId = typeof manifest?.id === 'string' + ? manifest.id + : (typeof manifest?.name === 'string' ? manifest.name : undefined); + addContributions(manifest?.navigationContributions, packageId); + } + + return { apps, contributions }; +} + +/** + * Every contribution in this compilation unit whose `group` names no group in + * the target app — the same finding, in the same words, the runtime fold + * raises when it relocates one. + * + * Returns `[]` without loading `@objectstack/objectql` when the stack declares + * no contributions at all, which is the overwhelming majority of builds. + */ +export async function findNavGroupDiagnostics( + parsed: AnyRec, + packages: readonly CompiledPackage[] = artifactPackagesOf(parsed), +): Promise { + const { apps, contributions } = collectNavGroupInputs(parsed, packages); + if (contributions.length === 0 || apps.length === 0) return []; + const { checkNavContributionGroups } = await import('@objectstack/objectql/core'); + return checkNavContributionGroups(apps, contributions); +} diff --git a/packages/objectql/src/core.ts b/packages/objectql/src/core.ts index 394f5b9297..6b25f2bf14 100644 --- a/packages/objectql/src/core.ts +++ b/packages/objectql/src/core.ts @@ -24,6 +24,26 @@ export { } from './registry.js'; export type { ObjectContributor, SchemaRegistryOptions } from './registry.js'; +// [#14553] The navigation-contribution group diagnostic. Belongs on the LEAN +// entry as well as the barrel: it is a pure derivation over authored metadata +// with no imports of its own, so it costs this entry nothing and stays clear of +// the ADR-0076 D2 boundary ratchet. `os build` reaches it from here rather +// than through the batteries-included barrel, which would pull +// `@objectstack/metadata-protocol` into the compile path for a check that +// reads two arrays. +export { + NAV_CONTRIBUTION_GROUP_MISSING, + findNavGroup, + navContributionGroupDiagnostic, + formatNavContributionGroupDiagnostic, + checkNavContributionGroups, +} from './nav-contribution-diagnostics.js'; +export type { + NavContributionGroupDiagnostic, + NavGroupHostApp, + NavGroupContribution, +} from './nav-contribution-diagnostics.js'; + // Search-normalization companion column (#2486 — pinyin recall) export { SEARCH_COMPANION_FIELD, diff --git a/packages/objectql/src/index.ts b/packages/objectql/src/index.ts index 9a6dc785f3..fa6c3bbe71 100644 --- a/packages/objectql/src/index.ts +++ b/packages/objectql/src/index.ts @@ -66,6 +66,25 @@ export { } from './registry.js'; export type { InjectedColumnProvenance } from './registry.js'; +// [#14553] The navigation-contribution group diagnostic (ADR-0029 D7, +// ADR-0112 D6c). Exported because `os build` is the SECOND door that has to +// answer "does this group id resolve?" — over a composed artifact, at compile +// time, where an AI author sees the mis-aim first — and the whole point of the +// ruling is that the two doors report the same finding in the same words. A +// consumer asks these; it does not re-derive the walk. +export { + NAV_CONTRIBUTION_GROUP_MISSING, + findNavGroup, + navContributionGroupDiagnostic, + formatNavContributionGroupDiagnostic, + checkNavContributionGroups, +} from './nav-contribution-diagnostics.js'; +export type { + NavContributionGroupDiagnostic, + NavGroupHostApp, + NavGroupContribution, +} from './nav-contribution-diagnostics.js'; + // Search-normalization companion column (#2486 — pinyin recall). Shared by // the registry's compile-time provisioning seam, the engine's `$search` // expansion, and plugin-pinyin-search's populate hooks. diff --git a/packages/objectql/src/nav-contribution-diagnostics.ts b/packages/objectql/src/nav-contribution-diagnostics.ts new file mode 100644 index 0000000000..45477a98c8 --- /dev/null +++ b/packages/objectql/src/nav-contribution-diagnostics.ts @@ -0,0 +1,228 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * ADR-0029 D7 / ADR-0130 — the diagnostic for a `navigationContributions[]` + * entry whose `group` names no group in the target app (#14553). + * + * ## What the platform does, and why this file exists + * + * `SchemaRegistry.applyNavContributions` RELOCATES such a contribution to the + * app's top level and carries on. That is deliberate and unchanged: the merge + * is a read-time fold precisely so registration order does not matter + * (`registerAppNavContribution` does not require the target app to exist yet), + * and a contribution into an OPTIONAL group must keep working. Refusing would + * trade both away — Option A of the card, not taken. + * + * What was wrong is that the relocation was INVISIBLE. The only trace was one + * `this.log(...)` line gated at `info`/`debug`, so a deployment running at + * `OS_REGISTRY_LOG=warn` — the level `OS_REGISTRY_LOG` exists to select, and + * the level this package's own vitest config pins — saw nothing at all while + * the information architecture silently changed underneath it. For a module + * split (hotcrm's 17-node navigation conversion) that is the WORSE of the two + * failures the card weighed: a dropped entry is missing and someone notices; a + * relocated one is present, passes a smoke test, and sits one level up from + * where its author put it. A typo'd group id is exactly what an AI author + * emits, so the failure had to become visible without becoming a refusal. + * + * Maintainer ruling, 2026-09-02 (verbatim: 「同意」) — option B: keep the + * relocation, make the trace a real diagnostic emitted at `warn`, and check the + * same condition at COMPILE time when both halves are composed into one + * artifact, where an AI author sees it first. + * + * ## One derivation, two halves — the reason this is a module and not two + * ## call sites + * + * The runtime half (`SchemaRegistry.applyNavContributions`) and the + * compile-time half (`os build`, `packages/cli/src/commands/compile.ts`) must + * answer "does this group id resolve?" the SAME way. Two copies of the + * predicate is the drift this card is about, one layer up: a build that says + * "fine" over a fold that relocates is indistinguishable, to the author, from + * the silence being fixed here. So {@link findNavGroup} is the single + * resolution — the registry's own private lookup now delegates to it — and + * {@link navContributionGroupDiagnostic} is the single message, formatted once + * by {@link formatNavContributionGroupDiagnostic} so both doors print the same + * line. + * + * ## Why the code is lowercase, and why no ledger entry + * + * ADR-0112 D6c, by name: "Diagnostics codes are not error codes." The ADR + * classifies a record that travels as payload of a success, describes an + * ARTIFACT rather than a request, carries a severity that can be `warning`, and + * is never routed to `error.code` — all four true here — as a separate + * vocabulary that "stays lowercase and out of the ledger". This entry is + * shaped after the ADR-0038 `BuildIssue` family that D6c names first + * (`{ severity, artifact, ref, code, message, fix }`, + * `metadata-protocol/src/build-probes.ts`), which is also what lets one stream + * carry the runtime and the build finding. + * + * ⛔ So: **no `ERROR_CODE_LEDGER` registration.** What it does need is a + * CLASSIFICATION row, and which gate wants one was measured rather than + * assumed — the first guess was wrong in a way worth recording. + * + * `check:dispatcher-error-vocabulary` delegates a lowercase literal in an + * object-literal `code:` position to `check:error-code-casing`, whose + * `EXEMPT_FILES` list carries the D6/D6b/D6c discrimination for + * `build-probes.ts` and `metadata-diagnostics.ts`. That delegation does NOT + * reach this file: the code is exported as a named constant and REFERENCED at + * the stamp (`code: NAV_CONTRIBUTION_GROUP_MISSING`), which is the + * `objlitconst` shape, and every lowercase pattern that gate delegates needs a + * quoted literal beside the token. So `check:error-code-casing` passes this + * file with no entry — it never sees the value — and + * `check:dispatcher-error-vocabulary` is the gate that reports it. + * + * ⇒ The row lives in `packages/runtime/src/dispatcher-error-vocabulary.ts`, + * verdict `foreign-vocabulary`, door `none`, carrying the four-test D6c + * argument above. ⛔ Do not add this file to `EXEMPT_FILES`: that list exempts + * files from a gate this one does not trip, so an entry there would assert a + * discrimination nothing is making and hide the real row's absence if it were + * ever deleted. + */ + +/** + * The diagnostics code for a contribution relocated past a missing group. + * + * Exported so consumers branch on the constant instead of re-spelling the + * literal — the D6c vocabulary is a contract even though it is not the error + * catalog's. + */ +export const NAV_CONTRIBUTION_GROUP_MISSING = 'nav_contribution_group_missing'; + +/** One relocated-contribution finding (ADR-0038 BuildIssue family; ADR-0112 D6c). */ +export interface NavContributionGroupDiagnostic { + /** Always {@link NAV_CONTRIBUTION_GROUP_MISSING}. */ + readonly code: typeof NAV_CONTRIBUTION_GROUP_MISSING; + /** Never `error`: the platform relocates and carries on — no refusal (option A not taken). */ + readonly severity: 'warning'; + /** The app whose navigation tree was changed. */ + readonly app: string; + /** The contributing package, when the registration carried one. */ + readonly packageId?: string; + /** The `group` id that resolved to no `type: 'group'` node in {@link app}. */ + readonly group: string; + /** Ids of the items moved to the app's top level, in contribution order. */ + readonly relocated: readonly string[]; + readonly message: string; + readonly fix: string; +} + +/** The shape this module needs from an app — its name and its nav tree. */ +export interface NavGroupHostApp { + readonly name?: unknown; + readonly navigation?: unknown; +} + +/** The shape this module needs from one contribution. */ +export interface NavGroupContribution { + readonly app?: unknown; + readonly group?: unknown; + readonly items?: unknown; + readonly packageId?: string; +} + +/** How an item with no `id` is named in {@link NavContributionGroupDiagnostic.relocated}. */ +const UNNAMED_ITEM = '(unnamed)'; + +/** + * Depth-first search for a `type: 'group'` nav item by id. + * + * ⚠️ BOTH conditions, and the pair is load-bearing: an `object`-type nav item + * that happens to share the id is not a container, so naming it is the same + * authoring error as naming nothing and must reach the same diagnostic. The + * registry's fold and `os build` call THIS function, so the two doors cannot + * disagree about what "the group exists" means. + */ +export function findNavGroup(items: unknown, groupId: string): Record | undefined { + if (!Array.isArray(items)) return undefined; + for (const item of items) { + if (!item || typeof item !== 'object') continue; + const node = item as Record; + if (node.id === groupId && node.type === 'group') return node; + const found = findNavGroup(node.children, groupId); + if (found) return found; + } + return undefined; +} + +/** The ids a contribution's items are reported under. */ +function relocatedIds(items: unknown): string[] { + if (!Array.isArray(items)) return []; + return items.map((item) => { + const id = (item as { id?: unknown } | null)?.id; + return typeof id === 'string' && id !== '' ? id : UNNAMED_ITEM; + }); +} + +/** + * Build the diagnostic for one contribution already known to name a missing + * group. Callers decide reachability (see {@link checkNavContributionGroups} + * and the registry fold); this only words the finding. + */ +export function navContributionGroupDiagnostic(input: { + app: string; + group: string; + packageId?: string; + items?: unknown; +}): NavContributionGroupDiagnostic { + const relocated = relocatedIds(input.items); + const who = input.packageId ?? '(unknown package)'; + const what = relocated.length === 1 ? '1 navigation item' : `${relocated.length} navigation items`; + return { + code: NAV_CONTRIBUTION_GROUP_MISSING, + severity: 'warning', + app: input.app, + ...(input.packageId === undefined ? {} : { packageId: input.packageId }), + group: input.group, + relocated, + message: + `Package "${who}" contributes ${what} [${relocated.join(', ')}] into group ` + + `"${input.group}" of app "${input.app}", but that app declares no such group — ` + + `the item(s) were RELOCATED to the app's top level. Nothing is lost and nothing ` + + `is refused, but the app's information architecture is not the one that was authored.`, + fix: + `Correct the group id to one the app declares as a \`type: "group"\` navigation node, ` + + `or declare "${input.group}" in app "${input.app}"'s own navigation.`, + }; +} + +/** One line carrying the whole finding — the text both doors print. */ +export function formatNavContributionGroupDiagnostic(d: NavContributionGroupDiagnostic): string { + return `[Registry] [${d.code}] ${d.message} Fix: ${d.fix}`; +} + +/** + * The COMPILE-TIME half: every contribution in one composed artifact whose + * `group` names no group in the target app. + * + * ⚠️ A contribution whose target app is NOT among `apps` yields nothing, and + * that silence is a decision rather than an oversight. A package may legally + * contribute into an app shipped by a DIFFERENT artifact installed separately — + * that is what makes the merge a read-time fold in the first place — so the + * only artifacts this can judge are the ones carrying both halves. Reporting + * the absent-app case here would refuse the supported cross-artifact case at + * build time, which is Option A wearing a warning's clothes. + */ +export function checkNavContributionGroups( + apps: readonly NavGroupHostApp[], + contributions: readonly NavGroupContribution[], +): NavContributionGroupDiagnostic[] { + const byName = new Map(); + for (const app of apps) { + if (typeof app?.name === 'string' && app.name !== '') byName.set(app.name, app); + } + + const found: NavContributionGroupDiagnostic[] = []; + for (const c of contributions) { + if (typeof c?.app !== 'string' || typeof c?.group !== 'string' || c.group === '') continue; + if (!Array.isArray(c.items) || c.items.length === 0) continue; + const target = byName.get(c.app); + if (!target) continue; + if (findNavGroup(target.navigation, c.group)) continue; + found.push(navContributionGroupDiagnostic({ + app: c.app, + group: c.group, + ...(c.packageId === undefined ? {} : { packageId: c.packageId }), + items: c.items, + })); + } + return found; +} diff --git a/packages/objectql/src/registry-nav-contribution-group-semantics.test.ts b/packages/objectql/src/registry-nav-contribution-group-semantics.test.ts index 6d75fcad01..d6887ba18b 100644 --- a/packages/objectql/src/registry-nav-contribution-group-semantics.test.ts +++ b/packages/objectql/src/registry-nav-contribution-group-semantics.test.ts @@ -24,24 +24,35 @@ * platform does a third thing: a contribution naming a missing group is * **relocated to the app's top level**. It does not vanish (so "silent drop", * the failure mode the card feared, is not what happens) and it does not fail - * (no throw, no `code`, no `console.warn`) — the only trace is ONE - * `console.log` line emitted by the registry's own `log()`, which is gated at - * `logLevel` `info`/`debug`. The shipped default IS `info`, so a default - * deployment prints it once at boot; a deployment running at `warn` — which is - * what `OS_REGISTRY_LOG` exists to select, and what this very package's - * `vitest.config.ts` selects — emits NOTHING at all. + * — no throw, and no refusal was ever added. * * For the conversion this is the worse half of the two outcomes the card * considered. A dropped entry is missing and someone notices; a relocated entry * is present, looks fine, and has silently changed the information architecture * — which is the exact property the 17-node conversion exists to preserve. A - * typo'd `group` therefore converts a nested entry into a top-level one and - * nothing red ever appears. + * typo'd `group` therefore converts a nested entry into a top-level one. * - * ⛔ This card is measurement only — no runtime behaviour is changed here. The - * pins below record what the platform does TODAY. If the PM rules that a - * missing group must refuse (or at minimum `console.warn`), the pin in - * `PROPOSITION 2` is what changes, in that card, deliberately. + * ## What #14553 changed, and what it deliberately did not + * + * Maintainer ruling, 2026-09-02 (verbatim: 「同意」) — option B. The + * RELOCATION is unchanged and its pin below is unchanged with it: the fold + * stays order-independent (`registerAppNavContribution` still does not require + * the target app to exist yet) and contributions into optional groups keep + * working. Option A — refuse at install, with the registration-order rule that + * implies — was weighed and NOT taken. + * + * What inverted is VISIBILITY. Until #14553 the only trace was one + * `console.log` line emitted by the registry's own `log()`, gated at + * `logLevel` `info`/`debug`: the shipped default IS `info`, so a default + * deployment printed it once at boot, while a deployment running at `warn` — + * which is what `OS_REGISTRY_LOG` exists to select, and what this very + * package's `vitest.config.ts` selects — emitted NOTHING while its information + * architecture changed. The trace is now a real diagnostic: an ADR-0038 + * BuildIssue-family record (ADR-0112 D6c — a diagnostics code, lowercase and + * out of the error ledger) carried on the app via `getAppNavDiagnostics` and + * announced through `console.warn`, so it survives `OS_REGISTRY_LOG=warn`. + * The `PROPOSITION 2 (visibility)` pin below is that inversion, asserted at + * three levels rather than assumed at one. * * ## The log assertions set `logLevel` explicitly, and must * @@ -56,6 +67,7 @@ import { describe, it, expect, afterEach } from 'vitest'; import { ObjectKernel } from '@objectstack/core'; import { SchemaRegistry } from './registry.js'; +import { NAV_CONTRIBUTION_GROUP_MISSING } from './nav-contribution-diagnostics.js'; import { ObjectQLPlugin } from './plugin.js'; import type { ObjectQL } from './engine.js'; @@ -174,7 +186,14 @@ describe('#14454 item 2 — `navigationContributions[].group` semantics across a // // ⚠️ This is the reading that matters for a 17-node conversion: the entry // is PRESENT, so no smoke test misses it, but it sits one level up from - // where the author put it — the information architecture changed, silently. + // where the author put it — the information architecture changed. + // + // ⛔ THIS PIN DOES NOT MOVE, and #14553 is the card that says so out loud. + // The ruling upgraded the TRACE (see the visibility pin) and changed the + // fold by not one line: refusing here would have introduced the + // registration-order constraint the read-time fold exists to avoid, and + // would have broken every package contributing into an OPTIONAL group. + // If this assertion ever goes red, the fold was changed — not the log. const nav = await mergedNav(contributorPackage('cpq', { group: 'group_that_does_not_exist' })); // Not dropped… @@ -183,7 +202,14 @@ describe('#14454 item 2 — `navigationContributions[].group` semantics across a expect(idsOf(groupOf(nav, 'sales_group')?.children)).toEqual(['nav_accounts']); }); - it('PROPOSITION 2 (visibility) — the ONLY trace is a `console.log` gated at `info`; nothing throws, nothing warns', () => { + it('PROPOSITION 2 (visibility) — [#14553 INVERTED] the relocation is a warn-level diagnostic, carried on the app; still nothing throws', () => { + // ⚠️ THIS PIN INVERTED IN #14553 and the direction is the whole point. + // Before: `logs` held one line at `info` and `warns` was EMPTY at every + // level, so the assertion `expect(atWarn.warns).toEqual([])` recorded the + // defect. It now asserts the opposite at the same level, over the same + // fixture, so the file reads as one continuous measurement rather than as + // a rewritten expectation. + // // Driven directly on a `SchemaRegistry` rather than through the kernel so // the level under test is the one this assertion names — the package's // vitest config pins `OS_REGISTRY_LOG=warn` for every registry constructed @@ -216,30 +242,163 @@ describe('#14454 item 2 — `navigationContributions[].group` semantics across a } finally { console.log = ol; console.warn = ow; console.error = oe; } - return { logs, warns, errors, threw, nav: app?.navigation ?? [] }; + return { + logs, warns, errors, threw, + nav: app?.navigation ?? [], + carried: registry.getAppNavDiagnostics('crm_app'), + }; }; - // At the SHIPPED default level, one line — and it is honest about what it did. - const atInfo = read('info'); - expect(atInfo.threw).toBeUndefined(); - expect(atInfo.logs).toHaveLength(1); - expect(atInfo.logs[0]).toContain('targets missing group "group_that_does_not_exist"'); - expect(atInfo.logs[0]).toContain('appending at top level'); - expect(atInfo.logs[0]).toContain('com.acme.crm.cpq'); - // ⚠️ Not a warning and not an error — the two channels an operator filters - // for. This is the visibility gap, pinned. - expect(atInfo.warns).toEqual([]); - expect(atInfo.errors).toEqual([]); - - // …and at `warn` — the level `OS_REGISTRY_LOG` selects, and the level this - // package's own test harness runs at — the relocation is COMPLETELY silent - // while still happening. + // ── The level the card is about ────────────────────────────────── + // `warn` is what `OS_REGISTRY_LOG` exists to select, what this package's + // own harness runs at, and what a quiet production boot runs at. It used to + // emit NOTHING while relocating; it now emits the diagnostic, on the + // channel an operator filters FOR. const atWarn = read('warn'); expect(atWarn.threw).toBeUndefined(); - expect(atWarn.logs).toEqual([]); - expect(atWarn.warns).toEqual([]); + expect(atWarn.warns).toHaveLength(1); + // Every fact the ruling named — the contributing package, the target app, + // the missing group id, and the relocated items — asserted individually, so + // a message rewrite that drops one goes red on that one rather than on a + // whole-string comparison nobody can read. + expect(atWarn.warns[0]).toContain(NAV_CONTRIBUTION_GROUP_MISSING); + expect(atWarn.warns[0]).toContain('com.acme.crm.cpq'); + expect(atWarn.warns[0]).toContain('crm_app'); + expect(atWarn.warns[0]).toContain('group_that_does_not_exist'); + expect(atWarn.warns[0]).toContain('nav_cpq'); + expect(atWarn.warns[0]).toContain('RELOCATED'); + // Still not an ERROR and still no throw: a diagnostic was added, a refusal + // was not. Option A stays untaken, and this is the assertion that says so. expect(atWarn.errors).toEqual([]); + expect(atWarn.logs).toEqual([]); + // …and the items are exactly where the unchanged fold puts them. expect(idsOf(atWarn.nav)).toEqual(['sales_group', 'nav_cpq']); + + // ── Carried on the app, not only printed ───────────────────────── + // The ruling asked for a diagnostic the app CARRIES, so `os doctor`, a boot + // report or a test can ask the app what happened to it instead of scraping + // a log line. + expect(atWarn.carried).toHaveLength(1); + expect(atWarn.carried[0]).toMatchObject({ + code: NAV_CONTRIBUTION_GROUP_MISSING, + severity: 'warning', + app: 'crm_app', + packageId: 'com.acme.crm.cpq', + group: 'group_that_does_not_exist', + relocated: ['nav_cpq'], + }); + + // ── The shipped default behaves the same ───────────────────────── + // `info` is BELOW `warn` on the ladder, so a default deployment sees the + // diagnostic too — once, not once per channel. + const atInfo = read('info'); + expect(atInfo.threw).toBeUndefined(); + expect(atInfo.warns).toHaveLength(1); + expect(atInfo.warns[0]).toBe(atWarn.warns[0]); + expect(atInfo.errors).toEqual([]); + + // ── And silence is still available, without losing the record ──── + // A deployment that asked for silence gets it. The diagnostic is still + // CARRIED, which is what separates "quiet" from "not measured": muting the + // log must not destroy the app's own verdict. + const atSilent = read('silent'); + expect(atSilent.logs).toEqual([]); + expect(atSilent.warns).toEqual([]); + expect(atSilent.errors).toEqual([]); + expect(atSilent.carried).toHaveLength(1); + expect(idsOf(atSilent.nav)).toEqual(['sales_group', 'nav_cpq']); + }); + + it('[#14553] the diagnostic is emitted ONCE per mis-aim, however many times the app is read', () => { + // The merge is a READ-time fold — it runs on every `getApp` — so a + // diagnostic emitted per fold would print once per request. That is the + // other half of the #12015 discipline: a line that fires on every read is + // as unreadable as one that never fires, and it would make this diagnostic + // the thing operators filter OUT. + // + // The de-duplication is per REGISTRY, not per process. A module-level memo + // would make the second registry in a process silent — which is exactly the + // shape the pin above reads (three fresh registries, same mis-aim each). + const registry = new SchemaRegistry({ multiTenant: false, collisionPolicy: 'error' }); + registry.logLevel = 'warn'; + registry.registerApp( + { name: 'crm_app', label: 'CRM', navigation: [{ id: 'sales_group', type: 'group', label: 'Sales', children: [] }] }, + 'com.acme.crm', + ); + registry.registerAppNavContribution( + { app: 'crm_app', group: 'group_that_does_not_exist', items: [{ id: 'nav_cpq', type: 'object', objectName: 'crm_cpq', label: 'cpq' }] }, + 'com.acme.crm.cpq', + ); + + const warns: string[] = []; + const ow = console.warn; + const capture = (...a: unknown[]) => { warns.push(a.map(String).join(' ')); }; + + console.warn = capture; + try { + registry.getApp('crm_app'); + registry.getApp('crm_app'); + registry.getApp('crm_app'); + } finally { + console.warn = ow; + } + expect(warns).toHaveLength(1); + // …and the carried record is deduplicated with it, so a long-running + // process does not accumulate one entry per read of the same app. + expect(registry.getAppNavDiagnostics('crm_app')).toHaveLength(1); + + // A DIFFERENT mis-aim on the same app is a different finding and speaks — + // the floor under the de-duplication, without which "once" could be + // satisfied by a memo that silences everything after the first line. + registry.registerAppNavContribution( + { app: 'crm_app', group: 'another_missing_group', items: [{ id: 'nav_quote', type: 'object', objectName: 'crm_quote', label: 'quote' }] }, + 'com.acme.crm.quote', + ); + console.warn = capture; + try { + registry.getApp('crm_app'); + } finally { + console.warn = ow; + } + expect(warns).toHaveLength(2); + expect(registry.getAppNavDiagnostics('crm_app')).toHaveLength(2); + }); + + it('[#14553] a contribution that RESOLVES raises no diagnostic — what is reported is the mis-aim, not the mechanism', () => { + // The floor under every assertion above. Without it, a diagnostic that + // fired on EVERY contribution would satisfy all of them, and the platform + // would have traded a silent relocation for a warning nobody can act on. + const registry = new SchemaRegistry({ multiTenant: false, collisionPolicy: 'error' }); + registry.logLevel = 'warn'; + registry.registerApp( + { name: 'crm_app', label: 'CRM', navigation: [{ id: 'sales_group', type: 'group', label: 'Sales', children: [] }] }, + 'com.acme.crm', + ); + registry.registerAppNavContribution( + { app: 'crm_app', group: 'sales_group', items: [{ id: 'nav_cpq', type: 'object', objectName: 'crm_cpq', label: 'cpq' }] }, + 'com.acme.crm.cpq', + ); + // …and a group-LESS contribution, which lands at the top level BY DESIGN + // and must never be reported as a mis-aim. + registry.registerAppNavContribution( + { app: 'crm_app', items: [{ id: 'nav_help', type: 'object', objectName: 'crm_help', label: 'help' }] }, + 'com.acme.crm.help', + ); + + const warns: string[] = []; + const ow = console.warn; + console.warn = (...a: unknown[]) => { warns.push(a.map(String).join(' ')); }; + let nav: NavItem[] = []; + try { + nav = (registry.getApp('crm_app') as { navigation?: NavItem[] }).navigation ?? []; + } finally { + console.warn = ow; + } + + expect(warns).toEqual([]); + expect(registry.getAppNavDiagnostics('crm_app')).toEqual([]); + expect(idsOf(groupOf(nav, 'sales_group')?.children)).toEqual(['nav_cpq']); + expect(idsOf(nav)).toEqual(['sales_group', 'nav_help']); }); it('PROPOSITION 2 (authoring door) — the authoring gate cannot catch it either: `group` reaches no cross-reference check', async () => { @@ -250,6 +409,14 @@ describe('#14454 item 2 — `navigationContributions[].group` semantics across a // observable consequence is that a typo'd group id survives BOTH doors: it // installs, and it relocates. // + // ⛔ STILL TRUE AFTER #14553, and that is the point of keeping it. The + // ruling put the diagnostic on the FOLD (where the relocation happens) and + // on `os build` (where an author sees it first) — not on registration, + // which by design does not know whether the target app exists yet, let + // alone which groups it declares. So the install door is as quiet as it + // ever was, deliberately, and this pin is what would notice a refusal + // creeping in here. + // // Asserted at the install door, where the consequence is observable: the // contribution is recorded verbatim, group id and all, with no diagnostic. const kernel = new ObjectKernel({ logger: { level: 'silent' }, gracefulShutdown: false }); diff --git a/packages/objectql/src/registry.ts b/packages/objectql/src/registry.ts index 6f4c2b2f08..c0699cf0d8 100644 --- a/packages/objectql/src/registry.ts +++ b/packages/objectql/src/registry.ts @@ -42,6 +42,17 @@ import { applyProtection } from '@objectstack/spec/shared'; // same string the artifact loader ordered them by, or a co-owner would be // admitted — or refused — under a key nothing else in the path uses. import { artifactPackageId } from '@objectstack/core'; +// [#14553] The ONE resolution of "does this nav group id exist?" and the ONE +// wording of the diagnostic when it does not. `os build` calls the same two +// (`checkNavContributionGroups`), so the compile-time door and this read-time +// fold cannot disagree about what a missing group is — the drift that would +// make the build's silence indistinguishable from the silence being fixed. +import { + findNavGroup as resolveNavGroup, + navContributionGroupDiagnostic, + formatNavContributionGroupDiagnostic, + type NavContributionGroupDiagnostic, +} from './nav-contribution-diagnostics.js'; /** * Reserved namespaces that do not get FQN prefix applied. @@ -1644,6 +1655,23 @@ export class SchemaRegistry { console.log(msg); } + /** + * [#14553] A diagnostic an operator filters FOR — emitted at every level down + * to `warn`, silent only where the deployment has asked for silence + * (`error`, `silent`). + * + * The level ladder is `debug < info < warn < error < silent`, so this is the + * mirror of {@link log}: that one speaks only ABOVE `warn` and is therefore + * the wrong channel for anything a `warn`-level deployment must not miss. + * `OS_REGISTRY_LOG=warn` is what the objectql vitest config selects and what + * a quiet production boot selects, and a relocation nobody can see there was + * the defect this method exists to end. + */ + private warn(msg: string): void { + if (this._logLevel === 'silent' || this._logLevel === 'error') return; + console.warn(msg); + } + /** * Debug-only diagnostic: emitted solely when `logLevel === 'debug'`, so it * stays out of the default (`'info'`) boot log. Use for expected-but-noisy @@ -4226,10 +4254,14 @@ export class SchemaRegistry { if (!Array.isArray(group.children)) group.children = []; group.children.push(...c.items); } else { - this.log( - `[Registry] Navigation contribution from "${c.packageId ?? '(unknown)'}" targets ` + - `missing group "${c.group}" in app "${app.name}" — appending at top level.`, - ); + // [#14553] The relocation itself is UNCHANGED — the fold stays + // order-independent and contributions into optional groups keep + // working (option A, a refusal plus a registration-order rule, was + // weighed and not taken). What changed is that it is no longer + // silent: this used to be one `this.log(...)` gated at `info`, so a + // deployment at `OS_REGISTRY_LOG=warn` watched its information + // architecture change with nothing in the log at all. + this.recordNavGroupMiss(String(app.name), c); nav.push(...c.items); } } else { @@ -4239,16 +4271,77 @@ export class SchemaRegistry { return cloned; } - /** Depth-first search for a `type: 'group'` nav item by id. */ + /** + * Depth-first search for a `type: 'group'` nav item by id. + * + * [#14553] Delegates rather than implements: `os build` asks the identical + * question at compile time over a composed artifact, and two copies of this + * walk is exactly the drift that would let the build call an authoring error + * fine while this fold relocates it. + */ private findNavGroup(items: any[], groupId: string): any | undefined { - for (const item of items) { - if (item && item.id === groupId && item.type === 'group') return item; - if (item && Array.isArray(item.children)) { - const found = this.findNavGroup(item.children, groupId); - if (found) return found; - } - } - return undefined; + return resolveNavGroup(items, groupId); + } + + // ========================================== + // Navigation-contribution diagnostics (#14553) + // ========================================== + + /** + * Diagnostics raised by the fold, keyed `app` → `packageId|group` → entry. + * + * ⚠️ PER REGISTRY and DE-DUPLICATED, both deliberate. `applyNavContributions` + * runs on EVERY read of an app (that is what makes it a fold rather than a + * mutation), so an undeduplicated list would grow without bound and an + * undeduplicated `console.warn` would print once per request — which is the + * #12015 discipline's other failure: a diagnostic that fires on every read is + * as unreadable as one that never fires. Keyed on the triple the author has + * to act on, the line is emitted once per process per distinct mis-aim. + * + * ⛔ Not a module-level `Set` (the shape `warnStrippedLegacyApiMethods` + * uses): registries are constructed per kernel and per test, and a + * process-global memo would make the SECOND registry that hits the same + * mis-aim silent — including, measurably, the pin that reads this + * diagnostic at two log levels in a row. + */ + private navGroupDiagnostics = new Map>(); + + /** + * Record — and, the first time, announce — one relocated contribution. + * + * The entry is kept so a caller that is not reading the log + * ({@link getAppNavDiagnostics}: `os doctor`, a boot report, a test) can ask + * the app what happened to it. + */ + private recordNavGroupMiss( + appName: string, + c: { packageId?: string; group?: string; items: any[] }, + ): void { + const diagnostic = navContributionGroupDiagnostic({ + app: appName, + group: String(c.group), + ...(c.packageId === undefined ? {} : { packageId: c.packageId }), + items: c.items, + }); + const perApp = this.navGroupDiagnostics.get(appName) ?? new Map(); + const key = `${c.packageId ?? ''}|${c.group}`; + if (perApp.has(key)) return; + perApp.set(key, diagnostic); + this.navGroupDiagnostics.set(appName, perApp); + this.warn(formatNavContributionGroupDiagnostic(diagnostic)); + } + + /** + * Navigation-contribution diagnostics raised for `appName` so far, in the + * order they were first raised (empty when the app's contributions all + * resolved, or when nothing has read the app yet). + * + * ⚠️ Populated BY the read-time fold, so it answers for the reads that have + * happened — call `getApp(appName)` first if you need the app's own verdict + * rather than the process's history. + */ + getAppNavDiagnostics(appName: string): NavContributionGroupDiagnostic[] { + return [...(this.navGroupDiagnostics.get(appName)?.values() ?? [])]; } // ========================================== diff --git a/packages/runtime/src/dispatcher-error-vocabulary.ts b/packages/runtime/src/dispatcher-error-vocabulary.ts index 3690a65db6..049924963d 100644 --- a/packages/runtime/src/dispatcher-error-vocabulary.ts +++ b/packages/runtime/src/dispatcher-error-vocabulary.ts @@ -309,6 +309,30 @@ export const UNREGISTERED_CODE_SITES: readonly UnregisteredCodeSite[] = [ // (no source site — the producer is tenant code; see SANDBOX_AUTHORED_LIMB) // ── foreign vocabularies: spelled `code`, not an ADR-0112 error.code ──── + // [#14553] The navigation-contribution relocation diagnostic. + { + code: 'nav_contribution_group_missing', + file: 'packages/objectql/src/nav-contribution-diagnostics.ts', + shape: 'objlitconst', + door: 'none', + verdict: 'foreign-vocabulary', + why: + 'ADR-0112 D6c by name, on all four of its tests: it ships as PAYLOAD of a success (carried on ' + + 'the served app, and in the `warnings` list of `os build --json` AND `os validate --json`, ' + + 'from runs that exit 0), it describes an ARTIFACT rather than a request (which app, which ' + + 'package, which group id), its severity is `warning` and cannot be anything else, and no ' + + 'path routes it to ' + + '`error.code` — the producer is a read-time FOLD (`applyNavContributions`) and a compile ' + + 'step, neither of which throws. The card that added it added the diagnostic and ' + + 'deliberately added NO refusal: the contribution is still relocated to the app\'s top ' + + 'level and still installs, so there is no failing request for a catalog to govern. Hence ' + + 'lowercase and out of the ledger, ' + + 'exactly as D6c prescribes for `metadata-diagnostics.ts` and `build-probes.ts`, whose ' + + '`{ severity, artifact, ref, code, message, fix }` family this record is shaped after. ' + + 'Reported here rather than delegated to `check:error-code-casing` because the constant is ' + + 'REFERENCED (`objlitconst`), not quoted at the stamp — that gate\'s lowercase patterns all ' + + 'need a literal beside the token, so this position is this gate\'s to classify.', + }, { code: 'MODULE_NOT_FOUND', file: 'packages/types/src/node.ts',