Skip to content

Commit c351a84

Browse files
hotlongclaude
andauthored
fix(objectql,cli): a navigation contribution relocated past a missing group now says so, at warn and at build time (#14920)
* feat(objectql,cli): make a relocated navigation contribution a real diagnostic `SchemaRegistry.applyNavContributions` relocates a contribution whose `group` names no group in the target app to the app's top level. That stays — the read-time fold is order-independent by design and contributions into optional groups must keep working — but the only trace was one `log()` call gated at `info`/`debug`, so at `OS_REGISTRY_LOG=warn` an app's information architecture changed in complete silence. The trace is now an ADR-0038 BuildIssue-family record (ADR-0112 D6c: a diagnostics code, lowercase and out of the error ledger) naming the contributing package, the app, the missing group id and the relocated items. It is carried on the app (`getAppNavDiagnostics`) and announced through a new level-aware `warn()`, once per registry per distinct mis-aim. `os build` answers the same question at compile time over a composed artifact, through the same predicate, and reports it loudly without refusing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UHvF5hyiZjnCyExFnfQB8m * test(cli): pin the nav-group check through the command's own parse chain Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UHvF5hyiZjnCyExFnfQB8m * chore: classify the new diagnostic code, changeset, and gate repairs - `packages/runtime/src/dispatcher-error-vocabulary.ts`: classify `nav_contribution_group_missing` as `foreign-vocabulary` / door `none`. ADR-0112 D6c on all four of its tests — payload of a success, describes an artifact, severity `warning`, never routed to `error.code` — so it is lowercase and stays OUT of the error-code ledger. The gate reports it here rather than delegating to `check:error-code-casing` because the constant is referenced (`objlitconst`), not quoted at the stamp. - `content/docs/permissions/system-context.mdx`: line rot repaired by `check-system-context-census --fix` after the registry edit shifted anchors. - `packages/cli/src/utils/nav-contribution-groups.test.ts`: module-top side-effect load of the dist-resolved dependency, per `check:test-source-alias` — a first load inside a clocked window is the measured cliff that gate exists to stop. - changeset: objectql + cli patch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UHvF5hyiZjnCyExFnfQB8m * chore(runtime): keep the tracker id out of the classification row's prose `check:doc-authoring` — a runtime string reaches authors, operators and generated surfaces, none of whom can resolve `#NNNN`. The anchor moves to an adjacent comment, where the reader who can resolve it is already looking. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UHvF5hyiZjnCyExFnfQB8m * docs(ui): state what happens when a nav contribution names an anchor that is not there The Setup app page explains the shell/anchor mechanism — the shell publishes empty group anchors and capability plugins contribute into them — but stopped short of the failure the anchor id makes possible. A contributor cannot see the shell's ids at authoring time, so a typo is undetectable from its own source, and the platform relocates rather than refusing: the menu renders, a smoke test passes, and the entry has moved one level up. Documents the rule and the diagnostic that now reports it: the `warn`-level `nav_contribution_group_missing`, the per-app `getAppNavDiagnostics` reader, and the `os build --json` `navigationGroupDiagnostics` key — following the `bodyExtractionWarnings` precedent, which documents a build-only JSON key on the page that owns the behaviour rather than in a CLI schema dump. No hand-written page was falsified by the code change; this fills a gap rather than correcting an error. The `packages/spec` describe() for the key has the same gap and is filed separately — that string is embedded in ~14 generated artifacts, so it belongs in its own PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UHvF5hyiZjnCyExFnfQB8m * docs(ui): home the mis-aimed-anchor section under Navigation It landed mid-intro on the first pass, which orphaned the intro's closing paragraph under an H3 and put that H3 ahead of the page's first H2. It now sits at the end of `## Navigation`, directly after the anchor table — where a reader has just learned the anchor ids and would ask what happens if one is wrong. Content unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UHvF5hyiZjnCyExFnfQB8m * fix(cli): fill the declared `warnings` key instead of adding a payload key Two standing pins caught the first cut and both are right: `build-json-advisory-parity.e2e.test.ts` and `build-json-undeclared-key-parity.e2e.test.ts`, each 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`; that payload's shape is mirrored from `os validate --json` so a consumer reads one shape per class from either command. `navigationGroupDiagnostics` is gone and the findings ride `warnings`. A third pin in the same file settles what that implies: the only permitted residue between the two payloads is the structural advisory set, and "nothing rides in build that validate does not also report". So `os validate` computes the same list — which is the better answer on its own terms, since an author running `validate` should see a mis-aimed contribution exactly as one running `build` does. `findNavGroupDiagnostics` now derives the artifact's package entries itself (`artifactPackagesOf`), so both commands reach the check through one call that needs only the parsed stack, and the test uses that shipped derivation rather than a second copy of the id rule. `...navGroupWarnings` is APPENDED after `structuralWarnings` in validate, and that position is load-bearing: #12047's `the order lives at ONE site` pin matches the five existing members as contiguous source text, which is how it proves the order is defined once rather than re-spelled per exit. Appending keeps that pin guarding exactly what it was written to guard — no gate was loosened to get green. Reproduced first: 2 failed / 12 passed on the two key-set pins, naming `navigationGroupDiagnostics`; 14 passed after. The #12047 pin was then caught locally by the same discipline and is green with the other three (27 passed across the four files). Full cli unit project: 165 files / 2176 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UHvF5hyiZjnCyExFnfQB8m * docs: correct four places whose prose the payload-shape fix falsified All four were self-inflicted by the previous commit, which deleted the `navigationGroupDiagnostics` key and made `os validate` compute the list — and all four are exactly the class the docs-drift bot says it cannot detect, since a page stating a rule by its inputs shares no identifier with the emitter. 1. `content/docs/ui/setup-app.mdx` documented the deleted key. That page is what a contributor reads before authoring an anchor id, so it is the worst place in the repo for the sentence to be wrong. It now says the findings ride the declared `warnings` key and that `os validate` reports them too. 2. The changeset named the deleted key. This one propagates: the changeset is this PR's input to the release notes, so a wrong key here would have become a wrong key in a published release. 3. `nav-contribution-diagnostics.ts` claimed this file is registered in `check-error-code-casing`'s `EXEMPT_FILES`. It is not, and the reason is the discovery the previous commits recorded: the code is REFERENCED at the stamp (`objlitconst`), so that gate's lowercase delegation never reaches this position and `dispatcher-error-vocabulary` classifies it instead. The docstring was the pre-discovery version and sent the next reader to the wrong file to look for a row that is not in it. It now describes what was actually done, and says not to add an `EXEMPT_FILES` entry — that list exempts files from a gate this one does not trip. 4. The vocabulary row's own `why` prose carried the dead key. Corrected to the `warnings` list of both commands; the four-test D6c argument is untouched. The historical note in `nav-contribution-groups.ts` keeps the old key name on purpose — it describes what the first cut did and why the pins rejected it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UHvF5hyiZjnCyExFnfQB8m --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 7bc5d37 commit c351a84

15 files changed

Lines changed: 1275 additions & 47 deletions

File tree

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
---
2+
"@objectstack/objectql": patch
3+
"@objectstack/cli": patch
4+
---
5+
6+
fix(objectql,cli): a navigation contribution relocated past a missing group now says so, at `warn` and at build time
7+
8+
A package that injects navigation into another package's app names the target
9+
container by id (`navigationContributions[].group`). When that id matches no
10+
`type: "group"` node in the target app, `SchemaRegistry.applyNavContributions`
11+
appends the items at the app's **top level** and continues.
12+
13+
That relocation is unchanged, deliberately. The merge is a read-time fold
14+
precisely so registration order does not matter — `registerAppNavContribution`
15+
does not require the target app to exist yet — and a package contributing into
16+
an *optional* group has to keep working. Refusing would trade both away.
17+
18+
What changes is that it is no longer invisible. The only trace used to be one
19+
log line gated at `info`/`debug`, so a deployment running at
20+
`OS_REGISTRY_LOG=warn` watched its information architecture change in complete
21+
silence: a typo'd group id — exactly what an AI author emits — turned a nested
22+
menu entry into a top-level one, and because the entry was still *present*, no
23+
smoke test noticed. That is worse than a dropped entry, which someone notices.
24+
25+
The trace is now a real diagnostic naming the contributing package, the target
26+
app, the missing group id and the relocated items:
27+
28+
- **At runtime.** An ADR-0038 `BuildIssue`-family record (ADR-0112 D6c — a
29+
diagnostics code, lowercase and out of the error ledger) is carried on the
30+
app and reachable as `registry.getAppNavDiagnostics(appName)`, and announced
31+
through `console.warn`, so it survives `OS_REGISTRY_LOG=warn` and reaches
32+
`os doctor` / boot output. Emitted once per registry per distinct mis-aim:
33+
the fold runs on every read of the app, and a line printed per request is as
34+
unreadable as one never printed. A deployment that asks for `silent` still
35+
gets silence, and still keeps the record.
36+
- **At authoring time.** `os build` and `os validate` answer the same question
37+
over a composed artifact, through the same predicate, and report the same
38+
finding where an author sees it first — in the text output and in `--json`
39+
under the existing `warnings` key, beside the authoring-rule advisories and
40+
capability hints. A contribution aimed at an app no package in the artifact
41+
ships is not reported: contributing into an app another artifact installs is
42+
the supported case, and is why the merge is a fold.
43+
44+
**Nothing is refused.** No new failure, no ordering constraint, no change to
45+
what installs or to what `os build` accepts — a diagnostic was added and a
46+
refusal was not.
47+
48+
`examples/app-multi-package` now demonstrates the mechanism it was missing: the
49+
App package publishes a `sales_group` container and the Orders module
50+
contributes its nav entry into it, which is what a module split converts an
51+
app's own navigation into.

‎content/docs/permissions/system-context.mdx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -253,7 +253,7 @@ should recognise it instead of re-deriving it.
253253
rule-materialised grant that the next reconcile silently restores.
254254

255255
5. **`applySystemFields` does not read this flag.** It is named as if it did.
256-
`packages/objectql/src/registry.ts:464` is **schema-side column
256+
`packages/objectql/src/registry.ts:475` is **schema-side column
257257
provisioning** — which columns an object carries — and consumes
258258
`ExecutionContext.isSystem` zero times. The write-time ownership behaviour
259259
people attribute to it is row 2, in `plugin-security`.

‎content/docs/ui/setup-app.mdx‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,37 @@ A few notable entries:
7979
live in `plugin-audit`, but they are not contributed as Setup nav
8080
entries.)
8181

82+
### When a contribution names an anchor that is not there
83+
84+
Aiming at a `group` id the target app does not declare is **not** refused and
85+
the entry is **not** dropped: the items are appended at the app's **top level**
86+
and the merge continues. That is deliberate — the merge is a read-time fold
87+
precisely so registration order does not matter (a contributor may register
88+
before the app it aims at), and a contribution into an *optional* anchor has to
89+
keep working when the plugin owning that anchor is not loaded.
90+
91+
It is, however, **loud**. The relocation emits a
92+
`nav_contribution_group_missing` diagnostic at `warn` — naming the contributing
93+
package, the target app, the missing group id and the relocated items — so it
94+
survives `OS_REGISTRY_LOG=warn` and appears in boot output. The same finding is
95+
carried on the app itself, readable as
96+
`registry.getAppNavDiagnostics(appName)`, and it is raised once per distinct
97+
mis-aim rather than once per read of the app.
98+
99+
`os build` **and `os validate`** answer the same question at compile time
100+
whenever the contributing package and the target app are composed into one
101+
artifact. Both print the finding and both carry it in `--json` under the
102+
existing `warnings` key, beside the authoring-rule advisories and the
103+
capability hints — the payload is deliberately closed, so a new class of
104+
finding fills a declared key rather than adding one. They **report** there;
105+
neither fails the build.
106+
107+
⚠️ The anchor id is the whole contract between a shell and its contributors,
108+
and a contributor cannot see the shell's ids at authoring time. A typo
109+
therefore produces a menu that renders, passes a smoke test, and has silently
110+
moved the entry one level up — which is why the diagnostic exists rather than a
111+
refusal.
112+
82113
## Why a shell + contributions
83114

84115
The Setup App is a shell of empty group anchors rather than a fixed

‎examples/app-multi-package/src/packages/core/index.ts‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,20 @@ export default defineStack({
4343
name: 'multi_crm',
4444
label: 'Multi-Package CRM',
4545
description: 'Accounts, plus whatever modules this artifact delivers alongside',
46+
// The group is a CONTAINER this package owns and modules aim at
47+
// (ADR-0029 D7). It is the App package's half of the split: a module
48+
// cannot declare a group inside an app it does not own, so the app has
49+
// to publish the container its modules contribute into — which is what
50+
// makes `navigationContributions[].group` resolvable at all.
4651
navigation: [
47-
{ id: 'nav_accounts', type: 'object', objectName: 'crm_account', label: 'Accounts', icon: 'building' },
52+
{
53+
id: 'sales_group',
54+
type: 'group',
55+
label: 'Sales',
56+
children: [
57+
{ id: 'nav_accounts', type: 'object', objectName: 'crm_account', label: 'Accounts', icon: 'building' },
58+
],
59+
},
4860
],
4961
},
5062
],

‎examples/app-multi-package/src/packages/orders/index.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,22 @@ import { defineStack } from '@objectstack/spec';
3232
* legal and is the whole point of the split: cross-package lookups are accepted
3333
* (ADR-0130 §1.5), while a package's own app navigation pointing at a foreign
3434
* object is not — which is why the navigation lives with the App package.
35+
*
36+
* ## Why it also carries a `navigationContributions` entry (#14553)
37+
*
38+
* The other half of that same rule. R3 refuses an app's OWN `navigation` entry
39+
* naming another package's object, so a module split converts every such entry
40+
* into a contribution owned by the module — which is exactly what this one is:
41+
* `crm_order` is reachable from the App's menu without the App package
42+
* knowing the object exists.
43+
*
44+
* ⚠️ `group` names `sales_group`, a container the CORE package declares. A
45+
* module cannot see that id at authoring time, and a typo in it does not fail:
46+
* the runtime RELOCATES the items to the app's top level and says so
47+
* (`nav_contribution_group_missing`, at `warn`), and `os build` reports the
48+
* same finding at compile time. This fixture is where that is measured — keep
49+
* the id spelled correctly here, so a build of this example stays clean and the
50+
* pin that typos it has something to differ from.
3551
*/
3652
export default defineStack({
3753
manifest: {
@@ -46,6 +62,19 @@ export default defineStack({
4662
// it as the topological edge that registers core BEFORE orders (ADR-0130
4763
// D5, ADR-0116's one sorter) — the array order below is not what decides.
4864
dependencies: { 'com.example.multi.core': '^1.0.0' },
65+
66+
// ADR-0029 D7 — `navigationContributions` is a MANIFEST key, not a stack
67+
// collection: it describes what this PACKAGE injects into someone else's
68+
// app, so it travels with the package identity.
69+
navigationContributions: [
70+
{
71+
app: 'multi_crm',
72+
group: 'sales_group',
73+
items: [
74+
{ id: 'nav_orders', type: 'object', objectName: 'crm_order', label: 'Orders', icon: 'shopping-cart' },
75+
],
76+
},
77+
],
4978
},
5079

5180
objects: [

‎packages/cli/src/commands/compile.ts‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,10 @@ import {
4343
errorCodeFields,
4444
} from '../utils/format.js';
4545
import { checkProtocolVersionGap } from '../utils/protocol-version-gap.js';
46+
// [#14553] The compile-time half of the navigation-contribution group ruling.
47+
// Reports; never refuses — the runtime still relocates, deliberately.
48+
import { findNavGroupDiagnostics } from '../utils/nav-contribution-groups.js';
49+
import type { NavContributionGroupDiagnostic } from '@objectstack/objectql';
4650

4751
/**
4852
* The artifact's package entries, as `{ index, id, body }` (ADR-0130 D4).
@@ -178,11 +182,23 @@ export default class Compile extends Command {
178182
let capProviderWarnings: Array<{ token: string; message: string }> = [];
179183
let unknownKeyWarnings: string[] = [];
180184
let docWarnings: DocIssue[] = [];
185+
// [#14553] A member of `warningsSoFar()`, NOT a payload key of its own.
186+
//
187+
// ⛔ The first cut made it a separate top-level key and two standing pins
188+
// refused it by name — `build-json-advisory-parity` and
189+
// `build-json-undeclared-key-parity`, both titled "adds NO new top-level
190+
// key to the payload — this fills a declared key, it is not a new
191+
// surface". #11643 and #11727 each faced this choice and filled
192+
// `warnings`. `os validate` computes the same list, so the parity the
193+
// third pin in that file asserts ("nothing rides in build that validate
194+
// does not also report") holds rather than being weakened to fit.
195+
let navGroupWarnings: NavContributionGroupDiagnostic[] = [];
181196
const warningsSoFar = () => [
182197
...ruleAdvisories,
183198
...docWarnings,
184199
...unknownKeyWarnings,
185200
...capProviderWarnings,
201+
...navGroupWarnings,
186202
];
187203
// [#12125] The ADR-0087 D2 conversion notices, hoisted for the SAME reason
188204
// and under the SAME ruling as the four lists above — one field over. The
@@ -445,6 +461,37 @@ export default class Compile extends Command {
445461
}
446462
}
447463

464+
// 3b-bis. [#14553] Navigation contributions whose `group` names no group
465+
// in the target app. RUNS ON EVERY BUILD, artifact or not — the block
466+
// above is skipped for a single-package stack, but a stack that
467+
// declares an app AND contributes into it has the identical defect
468+
// and `collectNavGroupInputs` reads it from the top-level manifest.
469+
//
470+
// ⛔ REPORTS, NEVER REFUSES. The maintainer ruled option B: the
471+
// runtime keeps relocating the items to the app's top level (the fold
472+
// stays order-independent, contributions into optional groups keep
473+
// working) and the failure becomes VISIBLE instead. Making this exit
474+
// non-zero would be option A wearing a warning's clothes, and would
475+
// narrow what `os build` accepts — which the ruling explicitly does
476+
// not do.
477+
//
478+
// A contribution whose target app is NOT in this compilation unit
479+
// yields nothing: contributing into an app another artifact ships is
480+
// the supported cross-artifact case, and is precisely why the merge
481+
// is a read-time fold. Only the composed case can be judged here.
482+
navGroupWarnings = await findNavGroupDiagnostics(result.data as Record<string, unknown>);
483+
if (navGroupWarnings.length > 0 && !flags.json) {
484+
console.log('');
485+
printWarning(
486+
`Navigation contributions aimed at a group the target app does not declare ` +
487+
`(${navGroupWarnings.length}) — the items still install, RELOCATED to the app's top level`,
488+
);
489+
printBulletList(
490+
navGroupWarnings.map((d) => `[${d.code}] ${d.message} Fix: ${d.fix}`),
491+
{ noun: 'navigation-contribution diagnostic' },
492+
);
493+
}
494+
448495
// 3c. [#3366] Installable-provider preflight. Every capability the app
449496
// DECLARES in `requires: [...]` must have a provider resolvable in the
450497
// active edition. A `requires` entry whose provider has NO installable

‎packages/cli/src/commands/validate.ts‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,11 +31,17 @@ import {
3131
formatZodErrors,
3232
collectMetadataStats,
3333
printMetadataStats,
34+
printWarning,
35+
printBulletList,
3436
emitJson,
3537
isExitSignal,
3638
errorCodeFields,
3739
} from '../utils/format.js';
3840
import { checkProtocolVersionGap } from '../utils/protocol-version-gap.js';
41+
// [#14553] The navigation-contribution group check, shared with `os compile`.
42+
// Reports; never refuses — the runtime still relocates, deliberately.
43+
import { findNavGroupDiagnostics } from '../utils/nav-contribution-groups.js';
44+
import type { NavContributionGroupDiagnostic } from '@objectstack/objectql';
3945

4046
export default class Validate extends Command {
4147
static override description =
@@ -121,12 +127,25 @@ export default class Validate extends Command {
121127
let unknownKeyWarnings: string[] = [];
122128
let docWarnings: DocIssue[] = [];
123129
let structuralWarnings: string[] = [];
130+
// [#14553] Computed HERE as well as in `os compile`, not only there. The
131+
// #11727 residue pin asserts that nothing rides in build's `warnings` that
132+
// validate does not also report, and the two commands being one wall with
133+
// two doors is the #4409 / #4463 discipline this list already follows.
134+
let navGroupWarnings: NavContributionGroupDiagnostic[] = [];
124135
const warningsSoFar = () => [
125136
...ruleAdvisories,
126137
...docWarnings,
127138
...unknownKeyWarnings,
128139
...capProviderWarnings,
129140
...structuralWarnings,
141+
// [#14553] APPENDED, and the position is load-bearing. #12047's
142+
// `the order lives at ONE site` pin matches the five members above as
143+
// CONTIGUOUS source text — that is how it proves the order is defined
144+
// once rather than re-spelled per exit. Slotting a sixth member (or even
145+
// a comment) between them breaks that match, so a new member goes on the
146+
// end and the pin keeps guarding exactly what it was written to guard.
147+
// ⛔ Do not "fix" that pin by loosening its regex.
148+
...navGroupWarnings,
130149
];
131150
// [#12125] The ADR-0087 D2 conversion notices, hoisted for the SAME reason
132151
// and under the SAME ruling as the five lists above — one field over. The
@@ -270,6 +289,26 @@ export default class Validate extends Command {
270289
// an advisory `pnpm add` hint. Mirrors the `os build` gate exactly.
271290
//
272291
// Not a registry rule: it reads `node_modules`, not the stack.
292+
// [#14553] Navigation contributions whose `group` names no group in an
293+
// app this same compilation unit ships. Reports, never refuses: the
294+
// runtime relocates the items to the app's top level deliberately
295+
// (the read-time fold stays order-independent, contributions into
296+
// optional groups keep working), so what was missing was visibility,
297+
// not a gate. A contribution aimed at an app no package here ships is
298+
// NOT reported — that is the supported cross-artifact case.
299+
navGroupWarnings = await findNavGroupDiagnostics(result.data as Record<string, unknown>);
300+
if (navGroupWarnings.length > 0 && !flags.json) {
301+
console.log('');
302+
printWarning(
303+
`Navigation contributions aimed at a group the target app does not declare ` +
304+
`(${navGroupWarnings.length}) — the items still install, RELOCATED to the app's top level`,
305+
);
306+
printBulletList(
307+
navGroupWarnings.map((d) => `[${d.code}] ${d.message} Fix: ${d.fix}`),
308+
{ noun: 'navigation-contribution diagnostic' },
309+
);
310+
}
311+
273312
if (!flags.json) printStep('Checking capability providers (#3366)...');
274313
const capProviderPreflight = preflightRequiredCapabilities({
275314
requires: Array.isArray((config as { requires?: unknown[] }).requires)

0 commit comments

Comments
 (0)