From 9e6de652c5cb26a45f2536aefef05a46e4b472bc Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 08:00:40 +0000 Subject: [PATCH] feat(lint): check nav page/report/dashboard targets (ADR-0072) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `defineStack`'s `validateCrossReferences` already validates these three nav target kinds. But each arm is gated on the collection being non-empty: 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, not the least. The hole is INSIDE an existing check, not a missing one. 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 keyed on an unrelated property of the stack. `validateNavTargetRefs` joins REFERENCE_INTEGRITY_RULES (16 -> 17), so it runs on `validate` / `lint` / `compile` with no CLI rewiring. ## Warning, not error — and why `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 from here". Advisory is the honest ceiling. Tightening `defineStack`'s 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. Where defineStack's check IS live it still hard-fails first; this rule speaks when that check has switched itself off, and the message says so. ## Three nav types deliberately NOT covered — each verified, not assumed action already owned by validate-action-name-refs, which walks app navigation explicitly; adding it here would double-report component 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 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. The suite's membership pin caught the new rule on first run, which is what it is for: adding a member is a deliberate two-place edit, not something that slips in unreviewed. Verification: 14 new tests; lint 776/776; full suite 132/132; API surface unchanged; i18n ratchet unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WnqGjQFQMqd5k81LYV8SCY --- .changeset/adr-0072-nav-target-refs.md | 28 +++ packages/lint/src/index.ts | 7 + .../src/reference-integrity-suite.test.ts | 1 + .../lint/src/reference-integrity-suite.ts | 8 + .../lint/src/validate-nav-target-refs.test.ts | 139 ++++++++++++++ packages/lint/src/validate-nav-target-refs.ts | 175 ++++++++++++++++++ 6 files changed, 358 insertions(+) create mode 100644 .changeset/adr-0072-nav-target-refs.md create mode 100644 packages/lint/src/validate-nav-target-refs.test.ts create mode 100644 packages/lint/src/validate-nav-target-refs.ts 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; +}