From ac3b83b3d888ff7badcad4b17813486d08de553b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 06:51:26 +0000 Subject: [PATCH] fix(lint): the seven system-field exemption lists derive from the spec's declarations (#4330) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five rules each carried their own hand-copy of "registry-injected columns present on almost every object but absent from authored fields", and they had already drifted — two more copies (validate-translation-references' extended list, validate-hook-body-writes' union) appeared while the issue was open. Same shape #3786 removed from the audit-provenance family. One module now owns the answer: system-fields.ts derives SYSTEM_FIELDS from FIELD_GROUP_SYSTEM_FIELDS (@objectstack/spec/data) + SystemFieldName (@objectstack/spec/system), and all seven field-resolving rules consume it. A pin test holds the boundary both ways: exactly the two declarations' union, and none of the rule-local exemptions. The two judgement calls the issue asked for, made deliberately: - The derived union is a superset of the four identical copies (adds is_deleted) and of flow-template-paths' set (adds user_id). Both are the permissive direction the rules' own comments argue for: over-inclusion costs at worst a missed warning, under-inclusion a false one. - name / owner / record_type (and the legacy physical spellings _id / space) are NOT spec system columns — name is an ordinary authored field on most objects — so they stay rule-local next to each rule's reason (IMPLICIT_FIELDS / IMPLICIT_HEADS extensions) instead of widening every field-existence rule in the package. Closes #4330 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01D8QhQz2V6VH1SX5SQ1nAgk --- .changeset/lint-system-fields-derived.md | 35 +++++++++++++++ packages/lint/src/system-fields.test.ts | 39 ++++++++++++++++ packages/lint/src/system-fields.ts | 45 +++++++++++++++++++ .../lint/src/validate-flow-template-paths.ts | 32 ++++++------- .../lint/src/validate-hook-body-writes.ts | 29 ++++++------ .../lint/src/validate-page-field-bindings.ts | 20 ++------- .../lint/src/validate-react-page-props.ts | 15 +------ .../lint/src/validate-searchable-fields.ts | 21 ++------- .../src/validate-translation-references.ts | 21 +++++---- packages/lint/src/validate-widget-bindings.ts | 22 ++------- 10 files changed, 173 insertions(+), 106 deletions(-) create mode 100644 .changeset/lint-system-fields-derived.md create mode 100644 packages/lint/src/system-fields.test.ts create mode 100644 packages/lint/src/system-fields.ts diff --git a/.changeset/lint-system-fields-derived.md b/.changeset/lint-system-fields-derived.md new file mode 100644 index 0000000000..a1078c8d5a --- /dev/null +++ b/.changeset/lint-system-fields-derived.md @@ -0,0 +1,35 @@ +--- +"@objectstack/lint": patch +--- + +fix(lint): the seven system-field exemption lists derive from the spec's declarations (#4330) + +Five rules in `@objectstack/lint` each carried their own hand-copy of +"registry-injected columns present on almost every object but absent from +authored `fields`" — and they had already drifted from one another (two more +copies had appeared by the time the fix landed). This is the shape #3786 +removed from the audit-provenance family, rebuilt one package over: the same +list, maintained in parallel, each under a comment asking to be kept in sync +with one of the others. + +The package now has one module, `system-fields.ts`, whose `SYSTEM_FIELDS` is +DERIVED from the spec's two declarations — `FIELD_GROUP_SYSTEM_FIELDS` +(`@objectstack/spec/data`) and `SystemFieldName` (`@objectstack/spec/system`) +— and all seven field-resolving rules consume it. A pin test holds the +boundary in both directions: the set contains exactly the two declarations' +union, and none of the rule-local exemptions. + +Two deliberate behavior consequences, both in the permissive direction the +rules' own comments argue for (over-inclusion costs at worst a missed +warning; under-inclusion costs a false one): + +- `widget-bindings`, `page-field-bindings` and `react-page-props` now also + exempt `is_deleted`; +- `flow-template-paths` now also exempts `user_id`. + +Names that are NOT system columns in the spec's sense (`name`, `owner`, +`record_type`, and the legacy physical spellings `_id` / `space`) stay +rule-local next to the reason each rule exempts them, instead of widening +every rule: `name` in particular is an ordinary authored field on most +objects, and exempting it package-wide would stop the field-existence rules +from catching a reference to a field the object genuinely does not have. diff --git a/packages/lint/src/system-fields.test.ts b/packages/lint/src/system-fields.test.ts new file mode 100644 index 0000000000..c8409d770f --- /dev/null +++ b/packages/lint/src/system-fields.test.ts @@ -0,0 +1,39 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { FIELD_GROUP_SYSTEM_FIELDS } from '@objectstack/spec/data'; +import { SystemFieldName } from '@objectstack/spec/system'; +import { SYSTEM_FIELDS } from './system-fields.js'; + +describe('SYSTEM_FIELDS (#4330)', () => { + it('contains every member of both spec declarations — the derivation is complete', () => { + for (const f of FIELD_GROUP_SYSTEM_FIELDS) { + expect(SYSTEM_FIELDS.has(f), f).toBe(true); + } + for (const f of Object.values(SystemFieldName)) { + expect(SYSTEM_FIELDS.has(f), f).toBe(true); + } + }); + + it('contains nothing BEYOND the two spec declarations — additions go to the spec, not here', () => { + const declared = new Set([ + ...FIELD_GROUP_SYSTEM_FIELDS, + ...Object.values(SystemFieldName), + ]); + for (const f of SYSTEM_FIELDS) { + expect(declared.has(f), f).toBe(true); + } + }); + + it('excludes the rule-local exemptions — they are decisions, not system columns', () => { + // `name` is an ordinary authored field on most objects; `owner` and + // `record_type` are not registry-injected; `_id` and `space` are legacy + // physical spellings. Rules that exempt them do so locally, next to their + // reason (see the module note). Putting any of them here would silently + // stop every consumer from catching a reference to a genuinely missing + // field — this test is where that accidental widening fails. + for (const f of ['name', 'owner', 'record_type', '_id', 'space']) { + expect(SYSTEM_FIELDS.has(f), f).toBe(false); + } + }); +}); diff --git a/packages/lint/src/system-fields.ts b/packages/lint/src/system-fields.ts new file mode 100644 index 0000000000..8a01bbabb5 --- /dev/null +++ b/packages/lint/src/system-fields.ts @@ -0,0 +1,45 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The ONE answer to "which columns does the registry inject on (almost) every + * object without them appearing in authored `fields`?" (#4330). + * + * Every field-resolving rule in this package needs that answer: a reference to + * `created_at` or `owner_id` is authored against a real, addressable column + * even though no object declares it, so flagging it would be the false finding + * that makes authors stop trusting the linter (ADR-0072 D1). Before this + * module, five rules each carried their own hand-copied list and had already + * drifted from one another — the exact shape #3786 removed from the + * audit-provenance family, rebuilt one package over. + * + * DERIVED from the spec's two declarations, so it cannot drift from them: + * + * - {@link FIELD_GROUP_SYSTEM_FIELDS} (`@objectstack/spec/data`) — audit + * provenance plus `organization_id` / `tenant_id` / `is_deleted` / + * `deleted_at`; + * - {@link SystemFieldName} (`@objectstack/spec/system`) — the protocol-level + * ids: `id`, `owner_id`, `user_id` and the timestamp/tenant columns. + * + * The union is deliberately generous, because the cost asymmetry is the same + * in every consumer: over-inclusion costs at worst a missed finding on a + * `systemFields: false` object (rare); under-inclusion costs a false one. + * + * What does NOT belong here: names that are ordinary AUTHORED fields on most + * objects (`name`, `owner`, `record_type`) or legacy physical spellings + * (`_id`, `space`). A rule that deliberately exempts those keeps them in a + * rule-local extension next to its reason — adding them here would silently + * stop every other rule from catching a reference to a field the object + * genuinely does not have. + */ + +import { FIELD_GROUP_SYSTEM_FIELDS } from '@objectstack/spec/data'; +import { SystemFieldName } from '@objectstack/spec/system'; + +/** + * Registry-injected columns addressable at runtime without being authored in + * `fields` — the union of the spec's two system-field declarations. + */ +export const SYSTEM_FIELDS: ReadonlySet = new Set([ + ...FIELD_GROUP_SYSTEM_FIELDS, + ...Object.values(SystemFieldName), +]); diff --git a/packages/lint/src/validate-flow-template-paths.ts b/packages/lint/src/validate-flow-template-paths.ts index a83525df94..6384732e5b 100644 --- a/packages/lint/src/validate-flow-template-paths.ts +++ b/packages/lint/src/validate-flow-template-paths.ts @@ -55,6 +55,8 @@ // - Structured scalar heads (`json` / `composite` / `repeater` / `record`) may // carry legitimate sub-paths — their `.` access is left alone. +import { SYSTEM_FIELDS } from './system-fields.js'; + export type FlowTemplatePathSeverity = 'error' | 'warning'; export interface FlowTemplatePathFinding { @@ -86,24 +88,16 @@ function asArray(v: unknown): AnyRec[] { return []; } -// System/audit columns the platform injects on every object — always -// addressable in a `{record.}` template even though they are not authored -// fields. Mirrors `validateFormLayout`'s system-field set plus the audit/tenant -// columns from `FIELD_GROUP_SYSTEM_FIELDS`. -const SYSTEM_FIELDS: ReadonlySet = new Set([ - 'id', - 'name', - 'owner', - 'owner_id', - 'created_at', - 'created_by', - 'updated_at', - 'updated_by', - 'organization_id', - 'tenant_id', - 'is_deleted', - 'deleted_at', - 'record_type', +// Path heads addressable in a `{record.}` template without being authored +// fields: the package-shared registry-injected columns (`system-fields.ts`, +// #4330) plus three heads this rule has always exempted. `name`, `owner` and +// `record_type` are NOT registry-injected system columns (`name` in particular +// is an ordinary authored field on most objects), so they stay rule-local — +// see the shared module's note — instead of widening every field-existence +// rule in the package. +const IMPLICIT_HEADS: ReadonlySet = new Set([ + ...SYSTEM_FIELDS, + 'name', 'owner', 'record_type', ]); // Field types that address ANOTHER object — a `.` hop through one is @@ -332,7 +326,7 @@ export function validateFlowTemplatePaths(stack: AnyRec): FlowTemplatePathFindin // A trailing numeric segment is an array index (#1872), not a hop. const nextIsIdentifier = hasSubPath && !/^\d+$/.test(rest[1]); - const isKnown = fieldTypes.has(head) || SYSTEM_FIELDS.has(head); + const isKnown = fieldTypes.has(head) || IMPLICIT_HEADS.has(head); if (!isKnown) { if (seenUnknown.has(head)) continue; diff --git a/packages/lint/src/validate-hook-body-writes.ts b/packages/lint/src/validate-hook-body-writes.ts index 8a8f571d68..9be99cc694 100644 --- a/packages/lint/src/validate-hook-body-writes.ts +++ b/packages/lint/src/validate-hook-body-writes.ts @@ -43,6 +43,8 @@ import { createRequire } from 'node:module'; import type ts from 'typescript'; import { findClosestMatches, formatSuggestion } from '@objectstack/spec/shared'; +import { SYSTEM_FIELDS } from './system-fields.js'; + // The TypeScript compiler must NOT be imported at module top level: it is // ~9 MB of CJS, and @objectstack/lint sits on the kernel boot path — while // this gate only parses when a hook actually carries an L2 JS body. Same @@ -175,18 +177,17 @@ const API_WRITE_METHODS: ReadonlyMap = new Map([ const INPUT_ENVELOPE_KEYS: ReadonlySet = new Set(['id', 'options', 'ast', 'data']); /** - * Registry-injected columns present on (almost) every object but absent from - * `object.fields` — always legitimately writable by automation. The UNION of - * the sets the other field-resolving rules carry, because the cost asymmetry - * is the same everywhere: over-inclusion is at worst a missed finding, - * under-inclusion is a false one. + * Columns always legitimately writable by automation without appearing in + * `object.fields`: the package-shared registry-injected columns + * (`system-fields.ts`, #4330) plus the UNION of its sibling rules' local + * exemptions (`_id`/`name`/`space` from validate-translation-references, + * `name`/`owner`/`record_type` from validate-flow-template-paths) — because + * the cost asymmetry is the same everywhere: over-inclusion is at worst a + * missed finding, under-inclusion is a false one. */ -const SYSTEM_FIELDS: ReadonlySet = new Set([ - 'id', '_id', 'name', 'space', - 'owner', 'owner_id', 'user_id', - 'created_at', 'created_by', 'updated_at', 'updated_by', - 'organization_id', 'tenant_id', - 'is_deleted', 'deleted_at', 'record_type', +const IMPLICIT_FIELDS: ReadonlySet = new Set([ + ...SYSTEM_FIELDS, + '_id', 'name', 'space', 'owner', 'record_type', ]); type AnyRec = Record; @@ -414,7 +415,7 @@ export function validateHookBodyWrites(stack: AnyRec): HookBodyWriteFinding[] { // missing on EVERY named target (a multi-target body may branch per // object, so a partial miss is not statically wrong). if (!inputJudgeable) continue; - if (SYSTEM_FIELDS.has(w.field)) continue; + if (IMPLICIT_FIELDS.has(w.field)) continue; if (targetSets.some((s) => s!.has(w.field))) continue; reported.add(dedupeKey); @@ -438,7 +439,7 @@ export function validateHookBodyWrites(stack: AnyRec): HookBodyWriteFinding[] { // ctx.api write → the named object. const known = objectFields!.get(w.object); if (!known) continue; // object declared by another package — cannot judge - if (SYSTEM_FIELDS.has(w.field) || known.has(w.field)) continue; + if (IMPLICIT_FIELDS.has(w.field) || known.has(w.field)) continue; reported.add(dedupeKey); findings.push({ @@ -468,7 +469,7 @@ function unionCandidates(targetSets: ReadonlyArray | undefined>): st /** Did-you-mean (declared + system columns as candidates) plus the fix. */ function fixHint(field: string, declared: string[]): string { - const suggestion = formatSuggestion(findClosestMatches(field, [...declared, ...SYSTEM_FIELDS])); + const suggestion = formatSuggestion(findClosestMatches(field, [...declared, ...IMPLICIT_FIELDS])); return ( (suggestion ? `${suggestion} ` : '') + `Fix the field name, or declare '${field}' on the object. Only the literal write patterns in ` + diff --git a/packages/lint/src/validate-page-field-bindings.ts b/packages/lint/src/validate-page-field-bindings.ts index 2153346aa6..4bc236fbfe 100644 --- a/packages/lint/src/validate-page-field-bindings.ts +++ b/packages/lint/src/validate-page-field-bindings.ts @@ -59,22 +59,10 @@ export interface PageFieldFinding { } import { walkPageComponents, type AnyRec } from './page-walk.js'; - -/** - * Registry-injected fields present on (almost) every object but NOT declared in - * `object.fields`. Copied verbatim from `validate-widget-bindings` so the two - * rules agree on what counts as a field. Deliberately generous: over-inclusion - * costs at worst a missed warning on a `systemFields: false` object; under- - * inclusion costs a false one, and a false finding is what makes authors stop - * trusting the linter (ADR-0072 D1). Real pages DO reference these — e.g. - * `sys_user.page.ts` lists `created_at` in a related-list's columns. - */ -const SYSTEM_FIELDS = new Set([ - 'id', - 'created_at', 'created_by', 'updated_at', 'updated_by', - 'owner_id', 'organization_id', 'tenant_id', 'user_id', - 'deleted_at', -]); +// Real pages DO reference registry-injected columns — e.g. `sys_user.page.ts` +// lists `created_at` in a related-list's columns — so the shared set is load- +// bearing here, not merely defensive. +import { SYSTEM_FIELDS } from './system-fields.js'; function asArray(v: unknown): AnyRec[] { if (Array.isArray(v)) return v as AnyRec[]; diff --git a/packages/lint/src/validate-react-page-props.ts b/packages/lint/src/validate-react-page-props.ts index a2b052d4fd..8e0ddf9333 100644 --- a/packages/lint/src/validate-react-page-props.ts +++ b/packages/lint/src/validate-react-page-props.ts @@ -28,6 +28,8 @@ import { createRequire } from 'node:module'; import type ts from 'typescript'; import { REACT_BLOCKS, chartAggregateResultKeys } from '@objectstack/spec/ui'; +import { SYSTEM_FIELDS } from './system-fields.js'; + // The TypeScript compiler must NOT be imported at module top level: it is // ~9 MB of CJS (~70 ms+ to parse, worse on container cold starts), and // @objectstack/lint sits on the kernel boot path — while this gate only runs @@ -202,19 +204,6 @@ export const REACT_CHART_AXIS_UNKNOWN = 'react-chart-axis-unknown'; const CHART_FUNCTIONS = ['count', 'sum', 'avg', 'min', 'max'] as const; -/** - * Registry-injected fields present on (almost) every object but absent from - * `object.fields`. Same set as `validate-page-field-bindings`, for the same - * reason: over-inclusion costs at worst a missed finding, under-inclusion - * costs a false one. - */ -const SYSTEM_FIELDS = new Set([ - 'id', - 'created_at', 'created_by', 'updated_at', 'updated_by', - 'owner_id', 'organization_id', 'tenant_id', 'user_id', - 'deleted_at', -]); - /** * Both `objects` and an object's `fields` are authored either as an array of * `{ name }` records or as a name-keyed map — normalize to the array form, the diff --git a/packages/lint/src/validate-searchable-fields.ts b/packages/lint/src/validate-searchable-fields.ts index e5197ed9e9..afbcae7184 100644 --- a/packages/lint/src/validate-searchable-fields.ts +++ b/packages/lint/src/validate-searchable-fields.ts @@ -57,9 +57,9 @@ * 2. An object that declares no field map at all — external objects and * datasource-introspected schemas whose columns are resolved at runtime. * 3. Registry-injected system columns, which are searchable at runtime but - * never appear in authored `fields`. Derived from the spec's own - * declarations rather than hand-copied: this package already carries five - * slightly-different copies of that list (#3786's lesson, unlearned). + * never appear in authored `fields` — the package-shared `SYSTEM_FIELDS` + * (`system-fields.ts`), derived from the spec's own declarations rather + * than hand-copied (#4330). * * Dotted paths are NOT skipped here, unlike every sibling rule. Elsewhere * `owner_id.name` is left alone because the query engine resolves the traversal; @@ -68,8 +68,7 @@ * one wrong spelling most likely to be borrowed from `select`/`sort`. */ -import { FIELD_GROUP_SYSTEM_FIELDS } from '@objectstack/spec/data'; -import { SystemFieldName } from '@objectstack/spec/system'; +import { SYSTEM_FIELDS } from './system-fields.js'; export const SEARCHABLE_FIELD_UNKNOWN = 'searchable-field-unknown'; @@ -92,18 +91,6 @@ export interface SearchableFieldFinding { type AnyRec = Record; -/** - * Registry-injected columns that are addressable at runtime without being - * authored in `fields`. DERIVED from the spec's two declarations — - * `FIELD_GROUP_SYSTEM_FIELDS` (audit provenance + tenant + soft-delete) and - * `SystemFieldName` (the protocol-level ids) — so it cannot drift from them the - * way five hand-copied variants in this package already have. - */ -const SYSTEM_FIELDS: ReadonlySet = new Set([ - ...FIELD_GROUP_SYSTEM_FIELDS, - ...Object.values(SystemFieldName), -]); - /** Coerce a collection (array or name-keyed map) to an array of records. */ function asArray(v: unknown): AnyRec[] { if (Array.isArray(v)) return v as AnyRec[]; diff --git a/packages/lint/src/validate-translation-references.ts b/packages/lint/src/validate-translation-references.ts index 2433f9f1ed..475bb405f1 100644 --- a/packages/lint/src/validate-translation-references.ts +++ b/packages/lint/src/validate-translation-references.ts @@ -67,6 +67,7 @@ import { hasPlatformObjectPrefix, isPlatformProvidedObjectName } from '@objectstack/spec/system'; import { walkPageComponents } from './page-walk.js'; +import { SYSTEM_FIELDS } from './system-fields.js'; export const TRANSLATION_TARGET_UNKNOWN = 'translation-target-unknown'; export const TRANSLATION_OPTION_KEY_UNKNOWN = 'translation-option-key-unknown'; @@ -163,15 +164,17 @@ function listNames(names: Iterable, max = 12): string { } /** - * Audit/system fields every object carries implicitly. A bundle translating - * them is authoring against a real field, so they must not read as orphans. - * Mirrors `validate-page-field-bindings`' list. + * Fields a bundle may translate without reading as orphans: the package-shared + * registry-injected columns (`system-fields.ts`, #4330) plus three exemptions + * this rule has carried since it landed — `_id` and `space` are legacy + * physical spellings older bundles still key, and `name` is an ordinary + * authored field on most objects. None of the three is a system column in the + * spec's sense, so they stay rule-local (see the shared module's note) instead + * of widening every field-existence rule in the package. */ -const SYSTEM_FIELDS: ReadonlySet = new Set([ - 'id', '_id', 'name', - 'created_at', 'created_by', 'updated_at', 'updated_by', - 'owner_id', 'organization_id', 'tenant_id', 'user_id', - 'is_deleted', 'deleted_at', 'space', +const IMPLICIT_FIELDS: ReadonlySet = new Set([ + ...SYSTEM_FIELDS, + '_id', 'name', 'space', ]); /** Everything a bundle may legally name under one object. */ @@ -502,7 +505,7 @@ export function validateTranslationReferences(stack: AnyRec): TranslationRefFind const fieldPath = `${objPath}.fields.${fieldName}`; const field = facts.fields.get(fieldName); if (!field) { - if (SYSTEM_FIELDS.has(fieldName)) continue; + if (IMPLICIT_FIELDS.has(fieldName)) continue; orphan( `${inLocale} · object "${objectName}" · field "${fieldName}"`, fieldPath, diff --git a/packages/lint/src/validate-widget-bindings.ts b/packages/lint/src/validate-widget-bindings.ts index 8ea759e28b..f8c1b1b7a4 100644 --- a/packages/lint/src/validate-widget-bindings.ts +++ b/packages/lint/src/validate-widget-bindings.ts @@ -3,6 +3,8 @@ import { isIncoherentAggregate } from '@objectstack/spec/data'; import { ChartTypeSchema } from '@objectstack/spec/ui'; +import { SYSTEM_FIELDS } from './system-fields.js'; + /** * Build-time dashboard widget binding diagnostics (issues #1719, #1721). * @@ -221,27 +223,11 @@ const DATE_RANGE_FILTER_NAME = 'dateRange'; * Default field of the built-in date range when `dateRange.field` is omitted. * MUST track objectui `dashboard-filters.ts` `DATE_RANGE_DEFAULT_FIELD` — the * runtime this check shadows. `created_at` is a registry-injected system field - * (below), so a bare `dateRange` never false-positives. + * (the package-shared `SYSTEM_FIELDS`, `system-fields.ts`), so a bare + * `dateRange` never false-positives. */ const DATE_RANGE_DEFAULT_FIELD = 'created_at'; -/** - * Registry-injected fields present on (almost) every object but NOT declared in - * `object.fields`, so a dashboard filter targeting one must not be flagged as a - * missing column. Superset of the objectql registry's `applySystemFields` - * (audit columns, ownership, tenant, soft-delete) and spec's `SystemFieldName`. - * Deliberately generous: the cost of over-inclusion is at worst a missed error - * on a `systemFields: false` object (rare); the cost of under-inclusion is a - * false build failure on the ubiquitous `dateRange` → `created_at` default. The - * near-zero-false-positive bias mirrors ADR-0032's field-ref validator. - */ -const SYSTEM_FIELDS = new Set([ - 'id', - 'created_at', 'created_by', 'updated_at', 'updated_by', - 'owner_id', 'organization_id', 'tenant_id', 'user_id', - 'deleted_at', -]); - interface DashFilterDef { /** Stable filter name — the key widgets bind against in `filterBindings`. */ name: string;