diff --git a/.changeset/adr-0072-nav-target-refs.md b/.changeset/adr-0072-nav-target-refs.md new file mode 100644 index 0000000000..412d28afb8 --- /dev/null +++ b/.changeset/adr-0072-nav-target-refs.md @@ -0,0 +1,28 @@ +--- +'@objectstack/lint': minor +--- + +Nav targets that are not object names (`page` / `report` / `dashboard`) are now checked at author time — closing a hole *inside* an existing check. + +`defineStack`'s `validateCrossReferences` already validates these three. But each arm is gated on the collection being non-empty: + +```ts +if (nav.type === 'page' && typeof nav.pageName === 'string' + && pageNames.size > 0 && !pageNames.has(nav.pageName)) { … } +``` + +So a stack that declares **no `pages` at all** has its page-nav check silently switched off, and `{ type: 'page', pageName: 'anything' }` sails through. That is exactly the state a stack is in when the target was never written — the most likely way to reach this bug, not the least. + +Note the asymmetry the guard creates. The `object` arm of the same block has no size gate: it errors unless the item carries `requiresObject`, an **explicit** opt-in to "another package provides this". Objects have to say so out loud; pages, reports and dashboards got an implicit exemption that depends on an unrelated property of the stack. + +`validateNavTargetRefs` joins `REFERENCE_INTEGRITY_RULES` (16 → 17), so it runs on `validate`, `lint` and `compile` with no CLI rewiring. It reports **warning**, not error, and that ceiling is deliberate: `validate-object-references` can say ERROR for an unresolved *object* because it resolves against the curated `PLATFORM_PROVIDED_OBJECT_NAMES` registry and knows which cross-package names are real. No such registry exists for pages, reports or dashboards, so "unresolved" cannot honestly be distinguished from "provided by a package we cannot see". Fixing the guard by tightening the parse-time throw was the other option and was rejected: a throw has no escape hatch for a legitimately cross-package page, and ADR-0072 D1's rule is that one dead finding costs more than a missed one. When `defineStack`'s check *is* live it still hard-fails first; this rule is what speaks when that check has switched itself off, and it says so in the message. + +**Three nav types are deliberately NOT covered, each verified rather than assumed:** + +- **`action`** — already owned by `validate-action-name-refs`, which walks app navigation explicitly. Adding it here would double-report. +- **`component`** — a verified NON-rule. An unregistered `componentRef` does *not* fail silently: `ComponentNavView` renders a named diagnostic ("Component not registered … Ensure the plugin that provides this surface is installed and has called `registerAppComponent()`"), and the registry exists precisely so plugin-provided surfaces may legitimately be absent. Flagging it would break valid plugin nav and prescribe a fix for something already reported better at runtime. +- **`url`** — external by definition. + +Both NON-rules are pinned by tests, so "completing" the module by adding them fails there first. + +**Scope honesty:** all 35 authored nav page/report/dashboard targets in this repo resolve, so this closes a latent hole rather than a shipped bug. The rule was proven to go red and then green through the real `validateReferenceIntegrity` entry point on a known-bad stack, not only in unit tests — a green check that has never been made to fail is the recurring defect this campaign keeps finding in its own instruments. diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index 3edb952416..3bc31b9758 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -205,6 +205,13 @@ export { } from './validate-object-references.js'; export type { ObjectRefFinding, ObjectRefSeverity } from './validate-object-references.js'; +// [ADR-0072] The non-object nav targets (page/report/dashboard). Restores the +// coverage `defineStack`'s cross-reference block switches off when the stack +// declares none of that collection. `action` and `component` are deliberately +// NOT members — see the module doc for the verification behind each. +export { validateNavTargetRefs, NAV_TARGET_UNRESOLVED } from './validate-nav-target-refs.js'; +export type { NavTargetRefFinding, NavTargetRefSeverity } from './validate-nav-target-refs.js'; + export { validateSearchableFields, SEARCHABLE_FIELD_UNKNOWN, diff --git a/packages/lint/src/reference-integrity-suite.test.ts b/packages/lint/src/reference-integrity-suite.test.ts index 7ae1b17586..5e79bbe482 100644 --- a/packages/lint/src/reference-integrity-suite.test.ts +++ b/packages/lint/src/reference-integrity-suite.test.ts @@ -21,6 +21,7 @@ describe('reference-integrity suite — membership', () => { 'validatePageFieldBindings', 'validateChartBindings', 'validateNavAccess', + 'validateNavTargetRefs', 'validateTranslationReferences', 'validateFlowTemplatePaths', 'validateAiSurfaceAffinity', diff --git a/packages/lint/src/reference-integrity-suite.ts b/packages/lint/src/reference-integrity-suite.ts index 2f77eb1c86..7fb13532ce 100644 --- a/packages/lint/src/reference-integrity-suite.ts +++ b/packages/lint/src/reference-integrity-suite.ts @@ -60,6 +60,7 @@ import { validateActionNameRefs } from './validate-action-name-refs.js'; import { validatePageFieldBindings } from './validate-page-field-bindings.js'; import { validateChartBindings } from './validate-chart-bindings.js'; import { validateNavAccess } from './validate-nav-access.js'; +import { validateNavTargetRefs } from './validate-nav-target-refs.js'; import { validateTranslationReferences } from './validate-translation-references.js'; import { validateFlowTemplatePaths } from './validate-flow-template-paths.js'; import { validateAiSurfaceAffinity } from './validate-ai-surface-affinity.js'; @@ -111,6 +112,13 @@ export const REFERENCE_INTEGRITY_RULES: readonly ReferenceIntegrityRule[] = [ { name: 'validatePageFieldBindings', run: validatePageFieldBindings }, { name: 'validateChartBindings', run: validateChartBindings }, { name: 'validateNavAccess', run: validateNavAccess }, + // Nav targets that are NOT object names — page/report/dashboard. Restores the + // coverage `defineStack`'s own cross-reference block switches off whenever the + // stack declares none of that collection (`pageNames.size > 0 && …`), which is + // exactly the state a stack is in when the target was never written. + // `action` is deliberately absent (validateActionNameRefs owns it) and so is + // `component` (an unregistered ref renders a named diagnostic, not silence). + { name: 'validateNavTargetRefs', run: validateNavTargetRefs }, { name: 'validateTranslationReferences', run: validateTranslationReferences }, { name: 'validateFlowTemplatePaths', run: validateFlowTemplatePaths }, { name: 'validateAiSurfaceAffinity', run: validateAiSurfaceAffinity }, diff --git a/packages/lint/src/validate-nav-target-refs.test.ts b/packages/lint/src/validate-nav-target-refs.test.ts new file mode 100644 index 0000000000..8b594470fd --- /dev/null +++ b/packages/lint/src/validate-nav-target-refs.test.ts @@ -0,0 +1,139 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Tests for the non-object nav-target rule (ADR-0072). + * + * Two of these are the load-bearing ones: + * + * 1. The **empty-collection** case. That is the whole reason this rule exists: + * `defineStack`'s own check is gated on `pageNames.size > 0`, so a stack with + * no `pages` has its page-nav validation switched off. If someone "optimises" + * this rule by skipping stacks with no pages, that test goes red. + * 2. The **deliberate NON-members** (`component`, `action`). Each was verified, + * not assumed — `component` renders a named "Component not registered" + * diagnostic rather than failing silently, and `action` is already owned by + * `validate-action-name-refs`. Flagging either would be a false prescription + * or a duplicate report, and these tests are where that attempt fails first. + */ + +import { describe, expect, it } from 'vitest'; + +import { validateNavTargetRefs, NAV_TARGET_UNRESOLVED } from './validate-nav-target-refs.js'; + +const app = (items: unknown[]) => ({ apps: [{ name: 'ops', navigation: items }] }); + +describe('validateNavTargetRefs — the gap defineStack leaves', () => { + it('flags a page target when the stack declares NO pages (the size>0 hole)', () => { + const [f] = validateNavTargetRefs(app([{ id: 'n1', type: 'page', pageName: 'missing_page' }])); + expect(f.rule).toBe(NAV_TARGET_UNRESOLVED); + expect(f.severity).toBe('warning'); + expect(f.path).toBe('apps[0].navigation[0].pageName'); + expect(f.where).toBe('app "ops" · nav "n1"'); + // The message must say WHY nothing else caught it, or the author has no + // way to know this rule is the only thing speaking. + expect(f.message).toContain('NO pages at all'); + expect(f.message).toContain('size > 0'); + }); + + it('flags a page target when pages exist but the name is wrong', () => { + const findings = validateNavTargetRefs({ + ...app([{ id: 'n1', type: 'page', pageName: 'typo' }]), + pages: [{ name: 'real_page' }], + }); + expect(findings).toHaveLength(1); + expect(findings[0].message).not.toContain('NO pages at all'); + }); + + it('is silent when the target resolves', () => { + expect(validateNavTargetRefs({ + ...app([{ id: 'n1', type: 'page', pageName: 'real_page' }]), + pages: [{ name: 'real_page' }], + })).toEqual([]); + }); + + it.each([ + ['report', 'reportName', 'reports'], + ['dashboard', 'dashboardName', 'dashboards'], + ])('covers %s targets too', (type, prop, collection) => { + expect(validateNavTargetRefs(app([{ id: 'x', type, [prop]: 'nope' }]))).toHaveLength(1); + expect(validateNavTargetRefs({ + ...app([{ id: 'x', type, [prop]: 'ok' }]), + [collection]: [{ name: 'ok' }], + })).toEqual([]); + }); + + it('walks nested children — an `object` item carries them too, not just a group', () => { + const findings = validateNavTargetRefs(app([ + { id: 'grp', type: 'object', objectName: 'task', children: [ + { id: 'deep', type: 'page', pageName: 'missing' }, + ] }, + ])); + expect(findings).toHaveLength(1); + expect(findings[0].path).toBe('apps[0].navigation[0].children[0].pageName'); + }); + + it('walks the `areas[]` container, not just `navigation`', () => { + const findings = validateNavTargetRefs({ + apps: [{ name: 'ops', areas: [{ name: 'a', items: [{ id: 'n', type: 'page', pageName: 'missing' }] }] }], + }); + expect(findings).toHaveLength(1); + expect(findings[0].path).toBe('apps[0].areas[0].items[0].pageName'); + }); + + it('skips interpolated targets — they resolve at render time (ADR-0072 D1)', () => { + expect(validateNavTargetRefs(app([ + { id: 'n', type: 'page', pageName: '${ctx.page}' }, + { id: 'm', type: 'page', pageName: '{dynamic}' }, + ]))).toEqual([]); + }); +}); + +describe('the deliberate NON-members — each verified, not assumed', () => { + it('does NOT flag `component` — an unregistered ref is reported loudly at runtime', () => { + // ComponentNavView renders "Component not registered … Ensure the plugin + // that provides this surface is installed and has called + // registerAppComponent()". The registry exists so plugin surfaces MAY be + // absent; flagging this would break valid plugin nav and duplicate a + // better runtime message. + expect(validateNavTargetRefs(app([ + { id: 'n', type: 'component', componentRef: 'nobody:nothing' }, + ]))).toEqual([]); + }); + + it('does NOT flag `action` — validate-action-name-refs already walks app nav', () => { + expect(validateNavTargetRefs(app([ + { id: 'n', type: 'action', actionDef: { actionName: 'ghost_action' } }, + ]))).toEqual([]); + }); + + it('does NOT flag `url` — external by definition', () => { + expect(validateNavTargetRefs(app([ + { id: 'n', type: 'url', url: 'https://example.com' }, + ]))).toEqual([]); + }); +}); + +describe('robustness', () => { + it('never throws on junk or partial stacks', () => { + for (const junk of [ + undefined, null, 42, 'x', [], {}, + { apps: 'nope' }, { apps: [null, 7] }, + { apps: [{ name: 'a', navigation: 'nope' }] }, + { apps: [{ navigation: [null, 3, { type: 'page' }] }] }, + { apps: [{ name: 'a', areas: 'nope' }] }, + app([{ type: 'page' }]), // no pageName at all + ]) { + expect(() => validateNavTargetRefs(junk)).not.toThrow(); + } + }); + + it('emits nothing for a stack with no apps', () => { + expect(validateNavTargetRefs({ pages: [{ name: 'p' }] })).toEqual([]); + }); + + it('every finding carries a usable hint', () => { + for (const f of validateNavTargetRefs(app([{ id: 'n', type: 'page', pageName: 'x' }]))) { + expect(f.hint.length).toBeGreaterThan(20); + } + }); +}); diff --git a/packages/lint/src/validate-nav-target-refs.ts b/packages/lint/src/validate-nav-target-refs.ts new file mode 100644 index 0000000000..9daec0b113 --- /dev/null +++ b/packages/lint/src/validate-nav-target-refs.ts @@ -0,0 +1,175 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [ADR-0072 — reference resolvability] App-navigation targets that are not + * object names: `page`, `report`, `dashboard`. + * + * ## The hole this closes is inside an EXISTING check, not a missing one + * + * `defineStack`'s `validateCrossReferences` already validates these three + * (`stack.zod.ts`, the "Validate app navigation → object/dashboard/page/report + * references" block). But each of the three is guarded on the collection being + * non-empty: + * + * ```ts + * if (nav.type === 'page' && typeof nav.pageName === 'string' + * && pageNames.size > 0 && !pageNames.has(nav.pageName)) { … } + * ``` + * + * So a stack that declares **no `pages` at all** has its page-nav check + * silently switched off, and `{ type: 'page', pageName: 'anything' }` sails + * through. That is precisely the state a stack is in when the target was never + * written — the most likely way to get here, not the least. + * + * Note the asymmetry the guard creates. The `object` arm of the same block has + * no size gate: it errors unless the item carries `requiresObject`, an + * EXPLICIT opt-in to "another package provides this". Objects therefore say so + * out loud; pages, reports and dashboards get an implicit exemption that + * depends on an unrelated property of the stack. + * + * This rule restores the coverage with the ADR-0072 severity posture rather + * than by tightening the parse-time throw — a throw has no escape hatch for a + * legitimately cross-package page, and ADR-0072 D1's rule is that one dead + * finding costs more than a missed one. + * + * ## Severity: warning, and why it is not error + * + * `validate-object-references` can say ERROR for an unresolved *object* + * because it resolves against a curated `PLATFORM_PROVIDED_OBJECT_NAMES` + * registry — it knows which cross-package names are real. No such registry + * exists for pages, reports or dashboards, so "unresolved" genuinely cannot be + * distinguished from "provided by a package we cannot see from here". + * Advisory is the honest ceiling. When `defineStack`'s own check is live (the + * collection is non-empty) it still hard-fails first; this rule is what speaks + * when that check has switched itself off. + * + * ## Deliberately NOT covered — each verified, not assumed + * + * - **`action`** (`actionDef.actionName`) — already owned by + * `validate-action-name-refs`, which walks app navigation explicitly. Adding + * it here would double-report the same finding. + * - **`component`** (`componentRef`) — verified a NON-rule. An unregistered ref + * does NOT fail silently: `ComponentNavView` renders a named diagnostic + * ("Component not registered … Ensure the plugin that provides this surface + * is installed and has called `registerAppComponent()`"), and the registry + * exists precisely so plugin-provided surfaces may legitimately be absent. + * Flagging it would break valid plugin nav and prescribe a fix for something + * already reported better at runtime. + * - **`url`** — external by definition; nothing to resolve against. + */ + +import type { ReferenceIntegrityFinding } from './reference-integrity-suite.js'; + +export type NavTargetRefSeverity = 'error' | 'warning'; +export type NavTargetRefFinding = ReferenceIntegrityFinding; + +/** Emitted when a nav item targets a page/report/dashboard the stack cannot resolve. */ +export const NAV_TARGET_UNRESOLVED = 'nav-target-unresolved'; + +type AnyRec = Record; + +const isRec = (v: unknown): v is AnyRec => !!v && typeof v === 'object' && !Array.isArray(v); + +function asArray(v: unknown): AnyRec[] { + if (Array.isArray(v)) return v.filter(isRec); + if (isRec(v)) return Object.entries(v).map(([name, def]) => (isRec(def) ? { name, ...def } : { name })); + return []; +} + +function strName(v: unknown): string | undefined { + return typeof v === 'string' && v.length > 0 ? v : undefined; +} + +/** + * An interpolated target resolves at render time — the same conservative + * exemption `validate-object-references` and `validate-dashboard-action-refs` + * use to keep false positives near zero (ADR-0072 D1). + */ +const isInterpolated = (s: string): boolean => s.includes('${') || s.includes('{'); + +/** nav `type` → [target property, stack collection, human noun]. */ +const NAV_TARGETS: ReadonlyArray = [ + ['page', 'pageName', 'pages', 'page'], + ['report', 'reportName', 'reports', 'report'], + ['dashboard', 'dashboardName', 'dashboards', 'dashboard'], +]; + +function namesOf(collection: unknown): Set { + const out = new Set(); + for (const entry of asArray(collection)) { + const n = strName(entry.name); + if (n) out.add(n); + } + return out; +} + +export function validateNavTargetRefs(stack: unknown): NavTargetRefFinding[] { + const findings: NavTargetRefFinding[] = []; + if (!isRec(stack)) return findings; + + const apps = asArray(stack.apps); + if (apps.length === 0) return findings; + + const declared = new Map>(); + for (const [, , collection] of NAV_TARGETS) { + declared.set(collection, namesOf((stack as AnyRec)[collection])); + } + + for (const [ai, app] of apps.entries()) { + const appName = strName(app.name) ?? `#${ai}`; + + const walk = (items: unknown, basePath: string): void => { + if (!Array.isArray(items)) return; + for (const [ni, raw] of items.entries()) { + if (!isRec(raw)) continue; + const nav = raw; + const navPath = `${basePath}[${ni}]`; + + for (const [type, prop, collection, noun] of NAV_TARGETS) { + if (nav.type !== type) continue; + const target = strName(nav[prop]); + if (!target || isInterpolated(target)) continue; + const known = declared.get(collection)!; + if (known.has(target)) continue; + + const emptyCollection = known.size === 0; + findings.push({ + severity: 'warning', + rule: NAV_TARGET_UNRESOLVED, + where: `app "${appName}" · nav "${strName(nav.id) ?? strName(nav.label) ?? `#${ni}`}"`, + path: `${navPath}.${prop}`, + message: + `Navigation targets ${noun} '${target}', which this stack does not declare in ` + + `\`${collection}\`. ` + + (emptyCollection + ? `The stack declares NO ${collection} at all, so \`defineStack\`'s own ` + + `cross-reference check skipped this entry entirely (it is gated on ` + + `\`${collection === 'pages' ? 'pageNames' : collection === 'reports' ? 'reportNames' : 'dashboardNames'}.size > 0\`) — ` + + `nothing else will report it. ` + : '') + + `The entry renders in the sidebar and resolves to nothing when clicked. If another ` + + `package provides this ${noun}, this is expected and advisory only.`, + hint: + `Declare the ${noun} in \`${collection}\`, correct the name, or remove the nav entry ` + + `if the ${noun} is gone.`, + }); + } + + // Recurse: an `object` nav item carries `children` too, not just a + // `group` — the same reason `stack.zod.ts` does not gate its recursion + // on the item type. + if (Array.isArray(nav.children)) walk(nav.children, `${navPath}.children`); + } + }; + + walk(app.navigation, `apps[${ai}].navigation`); + // `areas[]` is the other nav container; it was once skipped wholesale in + // `stack.zod.ts`, so an areas-based app got no nav validation at all. + for (const [ari, area] of asArray(app.areas).entries()) { + walk(area.items, `apps[${ai}].areas[${ari}].items`); + walk(area.navigation, `apps[${ai}].areas[${ari}].navigation`); + } + } + + return findings; +}