diff --git a/.changeset/object-graph-null-entry-guard.md b/.changeset/object-graph-null-entry-guard.md new file mode 100644 index 0000000000..851c7d6b73 --- /dev/null +++ b/.changeset/object-graph-null-entry-guard.md @@ -0,0 +1,10 @@ +--- +'@objectstack/lint': patch +'@objectstack/metadata-protocol': patch +--- + +A junk entry in `stack.objects` no longer crashes the reference-integrity rules, and a probe rule that throws is reported instead of read as "nothing wrong". + +`indexObjectGraph` is the first statement of every rule that resolves a field path, and it read each `stack.objects` member without checking it was a record — so a `null` entry (an empty YAML list item, a partial editor write) threw `TypeError: Cannot read properties of null (reading 'name')` before any rule's own per-object guard could run. Because these rules also run inside the runtime publish gate, that was an exception on a write path rather than a missed finding. The seam now drops non-record entries — silently, matching every sibling collection reader in the package — and the valid objects beside them are judged exactly as before. + +On the publish receipt, `runBuildProbes`' object plane wrapped its rule call in a catch that produced an empty finding list, so a crashed rule was indistinguishable from a clean object while `checked.objects` had already counted it. A rule that throws now surfaces as a `runtime`-layer `object_field_ref_rule_failed` error carrying the thrown message, so an unverified object never reads as a verified one. Probes still never fail the publish they verify. diff --git a/packages/lint/src/object-graph.test.ts b/packages/lint/src/object-graph.test.ts index b8c435b527..c36877d727 100644 --- a/packages/lint/src/object-graph.test.ts +++ b/packages/lint/src/object-graph.test.ts @@ -194,3 +194,43 @@ describe('filter-walk — walkFilterFieldKeys across the three authored shapes', expect(keys('a string')).toEqual([]); }); }); + +describe('object-graph — a non-record entry in `stack.objects` (#15494)', () => { + // The seam is the FIRST statement of every rule that resolves a field path, + // so an unguarded read here threw before any member's own `if (!isRec(obj)) + // continue` could run — on the runtime publish door that is an exception on + // a write path, not a skipped finding. Measured on `origin/main` at + // 615fac3a0, `validateObjectFieldRefs({ objects: [null] })`: + // TypeError: Cannot read properties of null (reading 'name') + // at indexObjectGraph (src/object-graph.ts:159:30) + // The entry is SKIPPED rather than reported: this module decides no + // severities by contract, and a junk member is a shape defect the schema + // owns — see `asArray`'s note for the three reasons and the measurement. + + it('drops a null entry instead of throwing, and still indexes the rest', () => { + const valid = { name: 'crm_lead', fields: { name: { type: 'text' } } }; + expect(() => indexObjectGraph({ objects: [null] })).not.toThrow(); + const g = indexObjectGraph({ objects: [null, valid, undefined, 'junk', 42, []] }); + expect([...g.keys()]).toEqual(['crm_lead']); + expect(resolveFieldPath(g, 'crm_lead', 'name')).toMatchObject({ kind: 'ok' }); + }); + + it('drops a non-record FIELD entry too — the same read, one level down', () => { + // `graphObjectOf` walks `obj.fields` through the identical helper, so + // `fields: [null]` crashed at the same statement for the same reason. + const g = indexObjectGraph({ + objects: [{ name: 'crm_lead', fields: [null, { name: 'amount', type: 'currency' }] }], + }); + expect(resolveFieldPath(g, 'crm_lead', 'amount')).toMatchObject({ kind: 'ok' }); + }); + + it('reads a name-keyed map whose VALUE is not a record as a nameless object', () => { + // `{ a: 'junk' }` used to spread the string's indices into the record; the + // verdict was already `no-field-map`, and it still is — the entry keeps + // its key so an object declaring nothing stays distinguishable from one + // this stack never defined (skip 2 vs. skip 1). + const g = indexObjectGraph({ objects: { a: 'junk', b: { fields: { n: { type: 'text' } } } } }); + expect(resolveFieldPath(g, 'a', 'n')).toMatchObject({ kind: 'unknowable', reason: 'no-field-map' }); + expect(resolveFieldPath(g, 'b', 'n')).toMatchObject({ kind: 'ok' }); + }); +}); diff --git a/packages/lint/src/object-graph.ts b/packages/lint/src/object-graph.ts index 35b0a44a80..9e31675a9d 100644 --- a/packages/lint/src/object-graph.ts +++ b/packages/lint/src/object-graph.ts @@ -115,11 +115,51 @@ export interface GraphObject { /** object name → its resolvable surface, or `null` (skip 2). */ export type ObjectGraph = ReadonlyMap; -/** Coerce a collection (array or name-keyed map) to an array of records. */ +/** A plain record — not `null`, not an array. */ +function isRec(v: unknown): v is AnyRec { + return !!v && typeof v === 'object' && !Array.isArray(v); +} + +/** + * Coerce a collection (array or name-keyed map) to an array of records, + * DROPPING every member that is not one. + * + * The drop is the whole point, and it is a SKIP rather than a finding. + * + * This seam is the first statement of every rule that resolves a field path, + * so an entry it cannot read decides the fate of the entire family: an + * unguarded read here threw `TypeError: Cannot read properties of null` out of + * `indexObjectGraph` before any member's own per-object guard could run, which + * on the runtime publish door is an exception on a WRITE path rather than the + * silent miss this family exists to end. These rules are pure + * `(stack) => Finding[]` (ADR-0019) and run on the RAW `lint` path as well as + * the parsed one, so `objects` here is whatever the author's files deserialised + * to — a YAML list item left empty is `null`, and nothing upstream of the raw + * path has judged the shape. + * + * Skipping, not reporting, for three reasons that all point the same way: + * + * 1. It is what the rest of the family already does. Every sibling `asArray` + * in this package that spells the defensive read at all drops the member + * silently (`validate-nav-target-refs.ts`, `validate-flow-node-writes.ts`, + * `validate-hook-body-writes.ts`, `validate-page-visualization-bindings.ts` + * and the rest); not one of them emits a finding about it. Driving the + * whole `AUTHORING_RULES` table over `{ objects: [null, validObject] }` + * measured 28 rules judging it in silence and none reporting the junk + * entry — the seam was the outlier, not the reporters. + * 2. Each member of this family ALREADY answers the question three lines + * below the call, with `if (!isRec(obj)) continue` in its own per-object + * loop. A report from here would contradict the guard the same rule is + * about to run. + * 3. This module decides no severities and holds no rule ids by contract (see + * the module note). A junk `objects` member is a SHAPE defect — the + * schema's subject, not reference integrity's — and reporting it here + * would emit the same finding once per member for one bad entry. + */ function asArray(v: unknown): AnyRec[] { - if (Array.isArray(v)) return v as AnyRec[]; - if (v && typeof v === 'object') { - return Object.entries(v as AnyRec).map(([name, def]) => ({ name, ...(def as AnyRec) })); + if (Array.isArray(v)) return v.filter(isRec); + if (isRec(v)) { + return Object.entries(v).map(([name, def]) => ({ name, ...(isRec(def) ? def : {}) })); } return []; } diff --git a/packages/lint/src/reference-integrity-suite.test.ts b/packages/lint/src/reference-integrity-suite.test.ts index fba734b3bd..7d97fc3c5b 100644 --- a/packages/lint/src/reference-integrity-suite.test.ts +++ b/packages/lint/src/reference-integrity-suite.test.ts @@ -7,6 +7,9 @@ import { } from './reference-integrity-suite.js'; import { validateObjectReferences } from './validate-object-references.js'; import { validateTranslationReferences } from './validate-translation-references.js'; +import { validateObjectFieldRefs } from './validate-object-field-refs.js'; +import { validateListViewFieldRefs } from './validate-list-view-field-refs.js'; +import { validateDatasetReferences } from './validate-dataset-references.js'; describe('reference-integrity suite — membership', () => { // Deliberately a written-out list: adding a rule to the suite should be a @@ -412,3 +415,73 @@ describe('reference-integrity suite — every member actually runs', () => { expect(validateReferenceIntegrity({})).toEqual([]); }); }); + +describe('reference-integrity — a non-record entry in `stack.objects` (#15494)', () => { + /** + * One case per rule that resolves through the shared `indexObjectGraph` + * seam. Enumerated from the source rather than written from memory — + * `git grep -l indexObjectGraph packages/lint/src` names four rules: + * `validateObjectFieldRefs`, `validateListViewFieldRefs`, + * `validateDatasetReferences` and `validateWidgetBindings`. The first three + * are the suite members and are the table below. + * + * ⛔ `validateWidgetBindings` is deliberately ABSENT, and not because it is + * fixed. It is not a suite member (it runs on `os doctor` via + * `AUTHORING_RULES`), and it carries a SECOND, independent null dereference + * of its own — `validate-widget-bindings.ts:465`, in the aggregate-coherence + * pass that runs BEFORE it ever reaches this seam — so the seam guard cannot + * reach it. Measured after this change: + * THROW validateWidgetBindings { objects: [null] } + * TypeError: Cannot read properties of null (reading 'name') + * at validateWidgetBindings (src/validate-widget-bindings.ts:465:18) + * That file is held by another in-flight change, so the repair is filed as + * #15552 rather than ridden here — together with the wider inventory the same + * measurement turned up: 13 of 42 `AUTHORING_RULES` entries throw on this + * input through five more unguarded readers of `stack.objects`, three of them + * inside this very suite (`validate-object-references.ts`, + * `indexObjectSearchTargets`, `indexObjectFields`). So the suite ENTRY POINT + * still throws on `{ objects: [null] }` after this change; what this file + * pins is the seam, per member, and no more than that. + * + * Each case asserts BOTH halves: the junk entry is not a crash, and the + * valid object beside it is still judged — a guard that returned early would + * satisfy the first half while silently deleting the rule. + */ + const validObject = { + name: 'crm_lead', + fields: { name: { type: 'text', label: 'Name' }, amount: { type: 'currency', label: 'Amount' } }, + // `nope` exists nowhere on the object — one dangling name per position, so + // each member below has something of its own to report. + highlightFields: ['name', 'nope'], + listViews: { all: { type: 'grid', columns: ['name', 'nope'] } }, + }; + // `validateDatasetReferences` returns before the seam when a stack declares + // no datasets, so the table's stack carries one — without it that member's + // case would pass without ever reaching the code under test. + const datasets = [ + { name: 'lead_ds', object: 'crm_lead', dimensions: [{ field: 'nope' }], measures: [] }, + ]; + + const members: ReadonlyArray<[string, (s: Record) => Array<{ rule: string; path: string }>, string, string]> = [ + ['validateObjectFieldRefs', validateObjectFieldRefs, 'object-field-ref-unknown', 'objects[1].highlightFields[1]'], + ['validateListViewFieldRefs', validateListViewFieldRefs, 'list-view-field-unknown', 'objects[1].listViews.all.columns[1]'], + ['validateDatasetReferences', validateDatasetReferences, 'dataset-field-unknown', 'datasets[0].dimensions[0].field'], + ]; + + for (const [name, run, rule, path] of members) { + it(`${name}: a lone junk entry is skipped, not thrown`, () => { + expect(() => run({ objects: [null], datasets })).not.toThrow(); + expect(() => run({ objects: [undefined, 'junk', 7], datasets })).not.toThrow(); + }); + + it(`${name}: the valid object beside a junk entry is still judged`, () => { + const findings = run({ objects: [null, validObject], datasets }); + const hit = findings.find((f) => f.rule === rule); + expect(hit, `${name} kept judging past the junk entry`).toBeDefined(); + // The path still counts the junk entry: the guard drops it from the + // GRAPH, while each member's own loop keeps walking the raw array, so + // reported positions stay stable against the author's file. + expect(hit!.path).toBe(path); + }); + } +}); diff --git a/packages/metadata-protocol/src/build-probes-rule-failure.test.ts b/packages/metadata-protocol/src/build-probes-rule-failure.test.ts new file mode 100644 index 0000000000..ceaf49bab3 --- /dev/null +++ b/packages/metadata-protocol/src/build-probes-rule-failure.test.ts @@ -0,0 +1,135 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #15494 — the object probe plane must never convert a rule CRASH into + * "nothing wrong". + * + * ## What this pins, and the state it replaces + * + * `runBuildProbes`' object plane re-runs `validateObjectFieldRefs` over each + * published object's ACTIVE body and counts it in `checked.objects`. The call + * was wrapped in `catch { findings = [] }`, so a rule that threw produced the + * byte-identical receipt a genuinely clean object produces: the count went up, + * the issue list stayed empty. That is the one reading this plane exists to + * make impossible — it was added (#15254) precisely because a count that + * cannot go up is indistinguishable from a plane that found nothing wrong, and + * the silent catch reinstated the same ambiguity one layer in. + * + * The crash that motivated the card is a null entry in `stack.objects` + * dereferenced by the shared `indexObjectGraph` seam, repaired in + * `@objectstack/lint` in the same change. This file pins the OTHER half, which + * outlives that bug: whatever the next rule failure is, the receipt says the + * object was not checked, and says why. + * + * ## Why the rule is mocked rather than provoked + * + * With the seam repaired there is no longer a published body that makes the + * real rule throw — which is the point of the repair. Reaching the branch + * therefore means substituting a throwing rule, and `build-probes.ts` imports + * `@objectstack/lint` LAZILY (`await import`) at call time, so `vi.doMock` + * plus a fresh module graph per test is exact: nothing else in the file, and + * no other suite, sees a mocked lint package. + * + * ## The `adr0112-ok:` marks below + * + * `object_field_ref_rule_failed` is a build-probe diagnostics code shipped + * inside a 200 receipt (ADR-0112 D6c), not an `error.code` from the closed + * catalog — the same vocabulary as every other probe code, for which + * `check:error-code-casing` exempts `build-probes.ts` and + * `packages/objectql/src/build-probes.test.ts` whole. The marks here are the + * narrower per-literal spelling of that same exemption, and they are written + * on the literal's OWN line deliberately: a multi-line comment above the + * literal was measured to move it out of the gate's recognition window + * entirely, which reads as a suppression while actually being a blind spot. + */ + +import { describe, expect, it, vi, afterEach } from 'vitest'; +import type { ProbeEngine } from './build-probes.js'; + +const OBJECT_BODY = { + name: 'crm_lead', + fields: { name: { type: 'text', label: 'Name' } }, + highlightFields: ['name'], +}; + +const getItem = async (type: string, name: string) => + type === 'object' && name === 'crm_lead' ? OBJECT_BODY : undefined; + +/** + * The probes' single engine read. The object plane never calls it, but the + * double still honours the caller's `limit` by presence rather than ignoring + * it — a `find` double that answers more rows than it was asked for is how a + * limit regression rides through a green suite (`check:objectql-double-limit`). + */ +const engine: ProbeEngine = { + find: async (_object: string, query: unknown) => { + const rows = [{ id: 'r1' }, { id: 'r2' }]; + const limit = (query as { limit?: unknown } | undefined)?.limit; + return typeof limit === 'number' ? rows.slice(0, limit) : rows; + }, +}; + +afterEach(() => { + vi.doUnmock('@objectstack/lint'); + vi.resetModules(); +}); + +async function probeWith(validateObjectFieldRefs: (stack: Record) => unknown) { + vi.resetModules(); + vi.doMock('@objectstack/lint', () => ({ validateObjectFieldRefs })); + const { runBuildProbes } = await import('./build-probes.js'); + return runBuildProbes({ + engine, + getItem, + published: [{ type: 'object', name: 'crm_lead' }], + }); +} + +describe('runBuildProbes — a throwing object rule is reported, never swallowed', () => { + it('surfaces the crash as a runtime-layer error naming the object and the thrown message', async () => { + const report = await probeWith(() => { + throw new TypeError("Cannot read properties of null (reading 'name')"); + }); + + // The count still goes up — the object WAS reached; what failed is the + // judgement. Reporting one without the other is the ambiguity again. + expect(report.checked.objects).toBe(1); + expect(report.issues).toHaveLength(1); + expect(report.issues[0]).toMatchObject({ + layer: 'runtime', + severity: 'error', + code: 'object_field_ref_rule_failed', // adr0112-ok: D6c build-probe diagnostics code + artifact: { type: 'object', name: 'crm_lead' }, + }); + // The thrown message rides the receipt: without it the report says a + // rule failed and gives nobody a way to find out which defect. + expect(report.issues[0].message).toContain("Cannot read properties of null (reading 'name')"); + expect(report.issues[0].message).toContain('crm_lead'); + // ⛔ The one reading that must be impossible. + expect(report.issues, 'a crash must not read as zero findings').not.toEqual([]); + }); + + it('reports a non-Error throw too — the message is whatever was thrown', async () => { + const report = await probeWith(() => { + throw 'rule exploded'; + }); + expect(report.issues[0]).toMatchObject({ code: 'object_field_ref_rule_failed' }); // adr0112-ok: D6c build-probe diagnostics code + expect(report.issues[0].message).toContain('rule exploded'); + }); + + it('a clean rule still produces the clean receipt — the contrast case', async () => { + // Without this the test above would pass just as well against a probe + // that reported a failure for every object. + const report = await probeWith(() => []); + expect(report.checked.objects).toBe(1); + expect(report.issues).toEqual([]); + }); + + it('a rule that finds a dangling reference still reports THAT, not a failure', async () => { + const report = await probeWith(() => [ + { path: 'objects.crm_lead.highlightFields[0]', message: 'no such field', hint: 'add it' }, + ]); + expect(report.issues).toHaveLength(1); + expect(report.issues[0].code).toBe('object_field_ref_unknown'); + }); +}); diff --git a/packages/metadata-protocol/src/build-probes.ts b/packages/metadata-protocol/src/build-probes.ts index 015f56c22a..287ab83e98 100644 --- a/packages/metadata-protocol/src/build-probes.ts +++ b/packages/metadata-protocol/src/build-probes.ts @@ -23,7 +23,17 @@ export interface RuntimeBuildIssue { artifact: { type: string; name: string }; /** What it exercised, when narrower than the artifact (e.g. a widget). */ ref?: { type: string; name: string; member?: string }; - /** 'seed_not_applied' | 'view_read_failed' | 'empty_query' | 'widget_query_failed' | 'probes_unavailable' */ + /** + * 'seed_not_applied' | 'view_read_failed' | 'object_field_ref_unknown' + * | 'object_field_ref_rule_failed' | 'empty_query' | 'widget_query_failed' + * | 'probes_unavailable' + * + * [#15494] `object_field_ref_rule_failed` is the odd one out and belongs + * here anyway: it reports that a PROBE could not run, not that an artifact + * is broken. A probe plane that can only ever emit findings has no way to + * say "I was unable to judge this", and the absence of that vocabulary is + * what let a thrown rule read as a clean object. + */ code: string; message: string; fix?: string; @@ -251,7 +261,40 @@ export async function runBuildProbes(opts: RunBuildProbesOptions): Promise = []; try { findings = validateObjectFieldRefs({ objects: [{ ...body, name: p.name }] }) ?? []; - } catch { + } catch (e) { + // [#15494] A rule that THREW is not an object that came back + // clean — but `findings = []` made the two indistinguishable + // on the receipt, and `checked.objects` had already counted + // this object, so the reading was "inspected, nothing wrong": + // the exact shape a genuinely clean publish produces. This + // plane exists to be the second, non-differential reading of + // the ACTIVE body; a silent catch turned it into the very + // false clean it was added to prevent. + // + // The crash that motivated this — a non-record entry in + // `stack.objects` dereferenced in the shared + // `indexObjectGraph` seam — is repaired in `@objectstack/lint` + // in the same change. This half is the standing guarantee that + // outlives that one bug: whatever the NEXT rule failure is, + // the receipt says the object was not checked and why. Error + // severity, because an unverified object on a publish receipt + // is a gap in the verification contract (ADR-0038), not a + // property of the artifact — the `fix` says so, so nobody + // hunts the object for a defect it may not have. + // + // Probes still never fail the publish they verify + // (`protocol.ts`), so this is louder reporting, not a new + // refusal. + issues.push({ + layer: 'runtime', + severity: 'error', + artifact: { type: 'object', name: p.name }, + code: 'object_field_ref_rule_failed', + message: + `Object "${p.name}" was NOT checked: the object field-reference rule threw — ` + + `${String((e as Error)?.message ?? e)}. Its field-name lists are unverified.`, + fix: `This is a defect in the lint rule, not necessarily in object "${p.name}". Report it with the message above; the object published, but nothing judged its field-name lists.`, + }); findings = []; } for (const f of findings) {