From 8e69fef69998d8335be6a1c44ad31691d961c7cb Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 08:08:20 +0000 Subject: [PATCH 1/2] =?UTF-8?q?feat(lint,docs):=20reject=20un-guarded=20nu?= =?UTF-8?q?llable=20comparisons=20in=20CEL=20predicates=20=E2=80=94=20`has?= =?UTF-8?q?(x)`=20is=20not=20a=20null=20guard=20(#4763)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CEL's `has(x)` asks whether the KEY is present. Since #4649 every predicate reads a record that is total over the object's declared fields, so a declared column holding NULL is still present and `has(record.end_date)` is uniformly true. The idiom that reads like a guard — has(record.start_date) && has(record.end_date) && record.end_date < record.start_date — therefore reaches `null < null`, CEL has no overload, and the whole predicate aborts. Before #4761 the abort was swallowed, so a rule of this shape enforced NOTHING on exactly the rows it was written to catch. The fault is fully decidable from the metadata alone, so per PD #12 it belongs at authoring, not at a 400 on production data. New gate (error, no warn-mode escape hatch), in `packages/lint`: - `validate-null-guards.ts` — the decision procedure. Parses the predicate with cel-js and rejects an ordering (`< <= > >=`) or arithmetic (`+ - * / %`, unary `-`) operator applied to an operand that resolves to a declared NULLABLE field (no `required: true`, no `defaultValue`, no default option, not autonumber) and is not dominated by an explicit `!= null` / `== null` / `!isBlank()` test in the same boolean branch. `has()` deliberately does not count. Guard propagation follows `&&` left-to-right, `||` short-circuit, `!` polarity and ternary branches. - Wired into `validateStackExpressions`, already a `gating` authoring rule on `os build` / `os validate` / `os lint` AND the runtime publish gate. - Scope: object validation rules (including predicates nested in a `conditional` rule's `then`/`otherwise`) and lifecycle hook `condition`s — the surfaces CEL actually evaluates over a total record. Sharing rules (compiled to a SQL filter, three-valued, never faults), flattened flow conditions (a bare id may be a flow variable) and `Field.formula` (its own #3306 handling) are deliberately out, not half-covered. - Message names the rule, the operand and the fix, and closes with the sentence lifted verbatim from `unevaluableRuleError` in `rule-validator.ts`, so the publish-time and runtime rejections read identically. `has()` over an UNDECLARED key is untouched — that is its legitimate use. All three example apps pass the new gate unchanged; the pre-#4786 showcase hook shape is pinned as a regression test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018iARDqtrhQgz6fVHDeDkbQ --- .changeset/has-is-not-a-null-guard-lint.md | 42 ++ content/docs/data-modeling/validation.mdx | 4 + packages/lint/package.json | 1 + packages/lint/src/index.ts | 10 + .../lint/src/validate-expressions.test.ts | 208 +++++++++- packages/lint/src/validate-expressions.ts | 143 +++++++ .../lint/src/validate-null-guards.test.ts | 138 +++++++ packages/lint/src/validate-null-guards.ts | 373 ++++++++++++++++++ pnpm-lock.yaml | 3 + skills/objectstack-formula/SKILL.md | 22 ++ 10 files changed, 943 insertions(+), 1 deletion(-) create mode 100644 .changeset/has-is-not-a-null-guard-lint.md create mode 100644 packages/lint/src/validate-null-guards.test.ts create mode 100644 packages/lint/src/validate-null-guards.ts diff --git a/.changeset/has-is-not-a-null-guard-lint.md b/.changeset/has-is-not-a-null-guard-lint.md new file mode 100644 index 0000000000..7f364dc984 --- /dev/null +++ b/.changeset/has-is-not-a-null-guard-lint.md @@ -0,0 +1,42 @@ +--- +"@objectstack/lint": minor +--- + +feat(lint): `has(x)` 不是 null 守卫 —— 发布期直接拒绝未守卫的可空比较 (#4763) + +CEL 的 `has(x)` 问的是**键是否存在**。自 #4649 起,谓词读到的记录对对象声明的每个 +字段都是**全量**的:一个声明了却存 `NULL` 的列同样"存在",所以 +`has(record.end_date)` 对声明字段恒为 `true`,什么也没告诉作者。于是这个读起来 +像守卫的写法根本不是守卫: + +```text +has(record.start_date) && has(record.end_date) && record.end_date < record.start_date +``` + +它会走到 `null < null`,CEL 没有对应重载,整个谓词中断。#4761 之前中断被吞掉 +(规则跳过,一条 WARN),也就是说**这一形状的规则在任何含 null 值的行上从未生效 +过**——它写在元数据里、读起来完全正确、却什么都没有强制执行。#4761 把运行时改成 +fail-closed 之后,当场就在我们自己的两个示例对象里抓到了它。 + +运行时拒绝是兜底,不是该学到这件事的地方:作者会在真实数据(很可能是生产数据) +上收到一个 400,离写下规则可能已经过去几个月。而这个错误**仅凭元数据就可判定** +——谓词的 AST 加上对象声明的字段类型,就足以判断某个操作数是否可能为 null。按 +AGENTS.md PD #12(在创作期拒绝,不要在消费端容忍),它属于发布闸门。 + +**新增闸门(error,直接拒绝,没有降级开关)。** `os build` / `os validate` / +`os lint` 与运行时发布闸门共用的 `validateStackExpressions` 现在会拒绝这样的谓词: +对**声明为可空**的字段(没有 `required: true`、没有 `defaultValue`、没有默认选项、 +不是 autonumber)应用**排序**(`< <= > >=`)或**算术**(`+ - * / %`,含一元 `-`) +运算符,而该操作数没有被同一布尔分支内支配它的 `!= null` / `== null` / `!isBlank()` +显式判空所守卫。`has(x)` **刻意不**计入守卫——这正是本规则存在的理由。错误信息点名 +规则、操作数与修法,收尾句逐字取自 `rule-validator.ts` 的 `unevaluableRuleError`, +两道闸门措辞完全一致。 + +覆盖面(有意划定,而不是含糊地覆盖一半):对象**校验规则**(含 `conditional` 规则 +`then` / `otherwise` 里嵌套的谓词)与**生命周期 hook 的 `condition`** ——即真正由 CEL +在全量记录上求值、会 fail-closed 的两类面。共享规则条件(下推成 SQL 过滤,`NULL > x` +是三值逻辑,不会 fault)、flow 的扁平作用域条件(裸标识符可能是 flow 变量)与 +`Field.formula`(有自己的 #3306 `guard ? value : null` 处理)不在此列。 + +对**未声明**键的 `has()` 完全不受影响——那才是它的正当用途:区分"这次 PATCH 里 +根本没提到这个键"与"显式写了 null"。示例应用无需改动即通过新闸门。 diff --git a/content/docs/data-modeling/validation.mdx b/content/docs/data-modeling/validation.mdx index 27ada7d15d..b0f9d27e84 100644 --- a/content/docs/data-modeling/validation.mdx +++ b/content/docs/data-modeling/validation.mdx @@ -59,6 +59,10 @@ export const Order = ObjectSchema.create({ Condition expressions are **CEL** (evaluated by `@objectstack/formula`), not Salesforce-style formulas. Reference the incoming record via `record.`, use `==`/`!=`, `&&`/`||`, and helpers like `isBlank(x)` and `has(record.field)`. A string condition is accepted as authoring shorthand and normalized to `{ dialect: 'cel', source }` at build time. + +**`has(x)` is not a null guard.** Predicates see a record that is *total* over the object's declared fields, so `has(record.end_date)` is true even when the value is `NULL` — `has(a) && has(b) && a < b` then reaches `null < null`, CEL has no overload, and the whole rule aborts (the write is rejected fail-closed). Write `record.start_date != null && record.end_date != null && record.end_date < record.start_date` instead. Since #4763 the `has()` form is **rejected at build/publish**: an ordering or arithmetic operator over a declared nullable field needs a real `!= null` guard. `has()` over an *undeclared* key — "was this in the PATCH at all?" — is untouched. + + ## Common Properties All validation types share these base properties: diff --git a/packages/lint/package.json b/packages/lint/package.json index 0920b9be8e..c4da5fc478 100644 --- a/packages/lint/package.json +++ b/packages/lint/package.json @@ -25,6 +25,7 @@ "typecheck": "tsc --noEmit" }, "dependencies": { + "@marcbachmann/cel-js": "^8.0.0", "@objectstack/formula": "workspace:*", "@objectstack/sdui-parser": "workspace:*", "@objectstack/spec": "workspace:*", diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index eed219cb90..d9ab277512 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -27,6 +27,16 @@ export type { WidgetBindingFinding, WidgetBindingSeverity } from './validate-wid export { validateStackExpressions } from './validate-expressions.js'; export type { ExprIssue } from './validate-expressions.js'; +// #4763 — `has(x)` reads as a null guard and is not one. The decision procedure +// is exported on its own so other authoring surfaces (cloud graph-lint, the AI +// authoring path) reuse ONE verdict instead of re-deriving it. +export { + findUnguardedNullableOperands, + nullGuardMessage, + NULL_GUARD_HINT, +} from './validate-null-guards.js'; +export type { NullGuardFinding, NullGuardOptions } from './validate-null-guards.js'; + export { validateListViewMode, LIST_VIEW_FILTERS_IN_VIEWS_MODE } from './validate-list-view-mode.js'; // [ADR-0078] The functional-completeness gate. All judgement lives in the shared diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index e0e5e3daf9..e637acb5ca 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -341,7 +341,13 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { close_date: { type: 'date' }, expected: { type: 'formula', formula: 'record.amount * record.probability / 100' }, }, - validations: [{ name: 'future', expression: 'record.close_date >= today()' }], + // The `!= null` guard is load-bearing since #4763: `close_date` is a + // declared NULLABLE field, and an un-guarded `>=` over it faults at + // runtime (`null >= timestamp` has no overload) — the null-guard gate + // rejects that shape at authoring now. Soundness (this block's + // subject) and null-guarding are separate verdicts; the predicate has + // to satisfy both to produce zero issues. + validations: [{ name: 'future', expression: 'record.close_date != null && record.close_date >= today()' }], }], }); expect(issues).toHaveLength(0); @@ -758,3 +764,203 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { }); }); }); + +// ─────────────────────────────────────────────────────────────────────── +// #4763 — `has(x)` reads as a null guard and is not one. +// +// Scope note (constraint of the issue, pinned here so it stays a decision): +// this gate walks the AUTHORED METADATA the stack carries — object validation +// rules and lifecycle-hook conditions. It never reads source files, so the +// deliberately-bad fixtures in `packages/objectql/src/validation/rule-*.test.ts` +// (which pin the runtime's fail-closed behaviour and MUST keep the bad shape) +// are structurally out of its reach. +// ─────────────────────────────────────────────────────────────────────── +describe('null-guard gate (#4763)', () => { + // Mirrors `showcase_project`: dates and money are declared but nullable; + // `status` carries a default option and `name` is required, so neither can + // be null and neither may ever be flagged. + const project = { + name: 'showcase_project', + fields: { + name: { type: 'text', required: true }, + status: { type: 'select', options: [{ value: 'planned', default: true }, { value: 'active' }] }, + start_date: { type: 'date' }, + end_date: { type: 'date' }, + budget: { type: 'currency' }, + spent: { type: 'currency', defaultValue: 0 }, + }, + }; + const withRule = (rule: Record) => + validateStackExpressions({ objects: [{ ...project, validations: [rule] }] }); + + it('REJECTS the `has(a) && has(b) && a < b` shape over nullable declared fields', () => { + const issues = withRule({ + type: 'script', + name: 'end_after_start', + condition: 'has(record.start_date) && has(record.end_date) && record.end_date < record.start_date', + }); + expect(issues.length).toBeGreaterThan(0); + expect(issues.every((i) => (i.severity ?? 'error') === 'error')).toBe(true); + const joined = issues.map((i) => i.message).join('\n'); + // names the rule … + expect(joined).toContain("validation rule 'end_after_start'"); + // … the operand … + expect(joined).toContain('record.end_date'); + expect(joined).toContain('record.start_date'); + // … and the fix, in the runtime's own words. + expect(joined).toContain("Guard it with '!= null'"); + expect(joined).toContain('has(x)'); + expect(issues[0].where).toContain("object 'showcase_project'"); + }); + + it('ACCEPTS the `!= null` form (the fix #4761 landed in the examples)', () => { + expect( + withRule({ + type: 'script', + name: 'end_after_start', + condition: + 'record.start_date != null && record.end_date != null && record.end_date < record.start_date', + }), + ).toHaveLength(0); + }); + + it('ACCEPTS a guarded arithmetic predicate (showcase `spent_within_budget`)', () => { + expect( + withRule({ + type: 'script', + name: 'spent_within_budget', + condition: 'record.budget != null && record.spent != null && record.spent > record.budget * 1.2', + }), + ).toHaveLength(0); + }); + + it('never flags a required field or one with a default (`spent`, `status`, `name`)', () => { + expect( + withRule({ type: 'script', name: 'spend_positive', condition: 'record.spent > 0' }), + ).toHaveLength(0); + }); + + it('reaches the predicates nested in a `conditional` rule’s then/otherwise', () => { + const issues = withRule({ + type: 'conditional', + name: 'budget_sanity', + when: "record.status == 'active'", + then: { type: 'script', name: 'over_budget', condition: 'has(record.budget) && record.budget > 1' }, + }); + expect(issues.length).toBe(1); + expect(issues[0].message).toContain('record.budget'); + expect(issues[0].where).toContain("'budget_sanity' then → 'over_budget'"); + }); + + // Negative-case pin: the real `showcase_account` rule pair. Both use `has()` + // — legitimately, to tell "key absent from the PATCH" apart from "explicit + // null" — and both compare with EQUALITY only. They must stay legal; a rule + // that flags them is too broad. + it('leaves `showcase_account.churn_reason_consistency` alone (legitimate `has()`)', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'showcase_account', + fields: { status: { type: 'select', options: [{ value: 'churned' }] }, churn_reason: { type: 'text' } }, + validations: [{ + type: 'conditional', + name: 'churn_reason_consistency', + when: "record.status == 'churned'", + then: { + type: 'script', + name: 'churn_reason_present', + condition: "!has(record.churn_reason) || record.churn_reason == null || record.churn_reason == ''", + }, + otherwise: { + type: 'script', + name: 'churn_reason_absent', + condition: "has(record.churn_reason) && record.churn_reason != null && record.churn_reason != ''", + }, + }], + }], + }); + expect(issues).toHaveLength(0); + }); + + describe('hook conditions — the third instance the issue named', () => { + const hookStack = (condition: string) => ({ + objects: [project], + hooks: [{ name: 'project_budget_alert', object: 'showcase_project', condition }], + }); + + // Regression pin. `examples/app-showcase/src/data/hooks/index.ts` carried + // `has(record.spent) && has(record.budget) && record.spent > record.budget` + // until #4770/#4786 corrected it. This asserts the bad shape cannot come + // back: it is red today, and would have been red before that fix. + it('REJECTS the pre-#4786 showcase hook shape', () => { + const issues = validateStackExpressions( + hookStack('has(record.spent) && has(record.budget) && record.spent > record.budget'), + ); + expect(issues.length).toBeGreaterThan(0); + expect(issues[0].where).toContain("hook 'project_budget_alert'"); + expect(issues.map((i) => i.message).join('\n')).toContain('record.budget'); + }); + + it('ACCEPTS the corrected shape now on `main`', () => { + expect( + validateStackExpressions( + hookStack('record.spent != null && record.budget != null && record.spent > record.budget'), + ), + ).toHaveLength(0); + }); + + it('applies per target for a multi-object hook', () => { + const issues = validateStackExpressions({ + objects: [project, { name: 'other_obj', fields: { budget: { type: 'currency', required: true } } }], + hooks: [{ name: 'multi', object: ['showcase_project', 'other_obj'], condition: 'record.budget > 1' }], + }); + // Only the object that declares `budget` nullable is flagged. + expect(issues).toHaveLength(1); + expect(issues[0].where).toContain('showcase_project'); + }); + }); + + describe('surfaces deliberately NOT covered', () => { + it('leaves sharing-rule conditions alone (compiled to a SQL filter, never faults)', () => { + expect( + validateStackExpressions({ + objects: [project], + sharingRules: [{ + name: 'big_budget', + object: 'showcase_project', + condition: "record.status == 'active' && record.budget > 100000", + }], + }), + ).toHaveLength(0); + }); + + it('leaves flattened flow conditions alone (a bare id may be a flow variable)', () => { + expect( + validateStackExpressions({ + objects: [project], + flows: [{ + name: 'escalate', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'showcase_project' } }, + { id: 'd', type: 'decision', config: { condition: 'record.budget > 100000' } }, + ], + edges: [], + }], + }), + ).toHaveLength(0); + }); + + it('leaves `Field.formula` expressions alone (blessed `guard ? value : null`, #3306)', () => { + expect( + validateStackExpressions({ + objects: [{ + ...project, + fields: { + ...project.fields, + remaining: { type: 'formula', formula: 'record.budget - record.spent' }, + }, + }], + }), + ).toHaveLength(0); + }); + }); +}); diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index 37d4b928bc..0dcc3f2d68 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -15,12 +15,20 @@ * object validation-rule / formula predicates, and UI action `visible` / * `disabled` predicates. Each error is located (flow/object/action + * node/edge/field) with a corrective message. + * + * Since #4763 it also carries the **null-guard** verdict: an ordering / + * arithmetic operator applied to a nullable declared field that no `!= null` + * test dominates is rejected here, so the `has(a) && has(b) && a < b` trap + * (which reads as a guard and is not one) never reaches a production write. + * See `validate-null-guards.ts` for the decision procedure and its scope. */ import { validateExpression } from '@objectstack/formula'; import { collectFlowGraphs, resolveFlowNodeExpressions } from '@objectstack/spec/automation'; import type { FlowNodeParsed } from '@objectstack/spec/automation'; +import { findUnguardedNullableOperands, nullGuardMessage } from './validate-null-guards.js'; + export interface ExprIssue { where: string; message: string; @@ -88,6 +96,90 @@ function buildFieldTypeIndex(objects: AnyRec[]): Map { + const fields = obj.fields; + if (Array.isArray(fields)) { + return (fields as AnyRec[]) + .filter((f) => f && typeof f === 'object' && typeof f.name === 'string') + .map((f) => [f.name as string, f] as [string, AnyRec]); + } + if (fields && typeof fields === 'object') { + return Object.entries(fields as AnyRec) + .filter(([, def]) => !!def && typeof def === 'object') + .map(([n, def]) => [n, def as AnyRec] as [string, AnyRec]); + } + return []; +} + +/** + * Can this declared field hold `null` when a predicate reads it? (#4763) + * + * Deliberately conservative — this feeds a **build-breaking** verdict, so every + * uncertainty resolves to "not nullable" (no finding). A field is treated as + * always-valued when it is `required`, carries a `defaultValue`, declares a + * default option (`options: [{ …, default: true }]` — the select idiom), or is + * an autonumber the platform populates. + */ +function isNullableField(def: AnyRec): boolean { + if (def.required === true) return false; + if (def.defaultValue !== undefined && def.defaultValue !== null) return false; + if (def.type === 'autonumber') return false; + const options = def.options; + if (Array.isArray(options) && options.some((o) => !!o && typeof o === 'object' && (o as AnyRec).default === true)) { + return false; + } + return true; +} + +/** object name → set of field names that may hold `null` (#4763). */ +function buildNullableFieldIndex(objects: AnyRec[]): Map> { + const idx = new Map>(); + for (const obj of objects) { + const name = typeof obj.name === 'string' ? obj.name : undefined; + if (!name) continue; + const nullable = new Set(); + for (const [fname, def] of fieldEntries(obj)) { + if (isNullableField(def)) nullable.add(fname); + } + idx.set(name, nullable); + } + return idx; +} + +/** The raw CEL source behind a predicate slot (string or `{ dialect, source }`). */ +function celSourceOf(raw: unknown): string | undefined { + if (typeof raw === 'string') return raw; + if (raw && typeof raw === 'object') { + const rec = raw as AnyRec; + // A non-CEL dialect (`js`) has its own null semantics — not ours to judge. + if (typeof rec.dialect === 'string' && rec.dialect !== 'cel') return undefined; + if (typeof rec.source === 'string') return rec.source; + } + return undefined; +} + +/** + * Every predicate a validation rule carries, including the ones nested inside a + * `conditional` rule's `then` / `otherwise` — the trap hides there just as + * happily as at the top level. + */ +function rulePredicates(rule: AnyRec, path: string): Array<{ label: string; raw: unknown }> { + const out: Array<{ label: string; raw: unknown }> = []; + const name = typeof rule.name === 'string' ? rule.name : '?'; + const here = path ? `${path} → '${name}'` : `'${name}'`; + const main = rule.expression ?? rule.predicate ?? rule.condition ?? rule.formula; + if (main != null) out.push({ label: `validation rule ${here}`, raw: main }); + if (rule.when != null) out.push({ label: `validation rule ${here} when-predicate`, raw: rule.when }); + for (const branch of ['then', 'otherwise'] as const) { + const nested = rule[branch]; + if (nested && typeof nested === 'object' && !Array.isArray(nested)) { + out.push(...rulePredicates(nested as AnyRec, `${here} ${branch}`)); + } + } + return out; +} + /** * Validate every predicate in the stack. Returns the list of issues (empty = * clean). Caller decides how to surface / whether to fail the build. @@ -97,6 +189,39 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { const objects = asArray(stack.objects); const fieldIndex = buildFieldIndex(objects); const fieldTypeIndex = buildFieldTypeIndex(objects); + const nullableIndex = buildNullableFieldIndex(objects); + + /** + * The #4763 null-guard gate. Scoped to the surfaces whose predicates are + * EVALUATED by CEL over a record made total for every declared field — + * validation rules (`rule-validator.ts`, fail-closed since #4649/#4761) and + * lifecycle hook `condition`s. Deliberately NOT applied to sharing-rule + * conditions (compiled to a SQL filter, where `NULL > x` is three-valued and + * never faults), flow conditions (flattened scope: a bare identifier may be a + * flow variable, not a field), or `Field.formula` expressions (whose blessed + * `guard ? value : null` shape has its own #3306 handling). Those surfaces are + * tracked separately rather than half-covered. + */ + const checkNullGuards = ( + where: string, + subject: string, + raw: unknown, + objectName: string | undefined, + ): void => { + if (!objectName) return; + const nullableFields = nullableIndex.get(objectName); + if (!nullableFields || nullableFields.size === 0) return; + const source = celSourceOf(raw); + if (!source) return; + for (const finding of findUnguardedNullableOperands(source, { nullableFields })) { + issues.push({ + where, + message: nullGuardMessage(subject, objectName, finding), + source, + severity: 'error', + }); + } + }; const check = ( where: string, @@ -231,6 +356,11 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { check(where, rule.expression ?? rule.predicate ?? rule.condition ?? rule.formula, objectName, 'record'); // `conditional` rules carry a nested `when` predicate (record-scoped). check(`${where} when`, (rule as AnyRec).when, objectName, 'record'); + // #4763 — null-guard gate over every predicate the rule carries, nested + // `then`/`otherwise` branches included. + for (const p of rulePredicates(rule, '')) { + checkNullGuards(`object '${objectName}' · ${p.label}`, p.label, p.raw, objectName); + } } // Field-level formulas (computed fields) reference the same object. const fields = obj.fields; @@ -313,6 +443,13 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { const hookName = (hook.name as string) ?? '?'; if (typeof hook.object === 'string') { check(`hook '${hookName}' (${hook.object}) condition`, hook.condition, hook.object, 'record'); + // #4763 — the third instance the issue found lived on exactly this path. + checkNullGuards( + `hook '${hookName}' (${hook.object}) condition`, + `hook '${hookName}' condition`, + hook.condition, + hook.object, + ); continue; } @@ -339,6 +476,12 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { for (const target of targets) { const mark = issues.length; check(`hook '${hookName}' (${target}) condition`, hook.condition, target, 'record'); + checkNullGuards( + `hook '${hookName}' (${target}) condition`, + `hook '${hookName}' condition`, + hook.condition, + target, + ); for (let i = mark; i < issues.length; i++) { const issue = issues[i]; const key = `${issue.message}\u0000${issue.source ?? ''}`; diff --git a/packages/lint/src/validate-null-guards.test.ts b/packages/lint/src/validate-null-guards.test.ts new file mode 100644 index 0000000000..3315f0e665 --- /dev/null +++ b/packages/lint/src/validate-null-guards.test.ts @@ -0,0 +1,138 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #4763 — `has(x)` reads as a null guard and is not one. These tests pin the +// decision procedure itself; `validate-expressions.test.ts` pins the wiring +// into the gating rule (and with it the publish gate). + +import { describe, it, expect } from 'vitest'; + +import { + findUnguardedNullableOperands, + nullGuardMessage, + NULL_GUARD_HINT, +} from './validate-null-guards.js'; + +const nullableFields = new Set(['start_date', 'end_date', 'budget', 'spent', 'score']); +const find = (source: string) => findUnguardedNullableOperands(source, { nullableFields }); + +describe('findUnguardedNullableOperands — the `has()` trap (#4763)', () => { + it('rejects the `has(a) && has(b) && a < b` shape and names both operands', () => { + const findings = find( + 'has(record.start_date) && has(record.end_date) && record.end_date < record.start_date', + ); + expect(findings.map((f) => f.operand).sort()).toEqual(['record.end_date', 'record.start_date']); + expect(findings.every((f) => f.operator === '<')).toBe(true); + // `has()` was present and still did not count — that is the whole point. + expect(findings.every((f) => f.hasOnlyGuard)).toBe(true); + }); + + it('accepts the `!= null` form', () => { + expect( + find('record.start_date != null && record.end_date != null && record.end_date < record.start_date'), + ).toEqual([]); + }); + + it('accepts a guard written on either side of the null literal', () => { + expect(find('null != record.budget && record.budget > 100')).toEqual([]); + }); + + it('rejects an un-guarded ordering comparison against a literal', () => { + const findings = find('record.score > 100'); + expect(findings).toHaveLength(1); + expect(findings[0]).toMatchObject({ operand: 'record.score', operator: '>', hasOnlyGuard: false }); + }); + + it('rejects an un-guarded operand inside arithmetic', () => { + const findings = find('record.spent != null && record.spent > record.budget * 1.2'); + expect(findings.map((f) => f.operand)).toEqual(['record.budget']); + expect(findings[0].operator).toBe('*'); + }); + + it('accepts arithmetic whose operands are all guarded', () => { + expect( + find('record.budget != null && record.spent != null && record.spent > record.budget * 1.2'), + ).toEqual([]); + }); +}); + +describe('findUnguardedNullableOperands — what stays legal', () => { + it('leaves `has()` over an UNDECLARED key alone (its legitimate use)', () => { + // `churn_reason` is not in `nullableFields` here: not a declared field of + // this object, so nothing about it is decidable and nothing is reported. + expect(find('!has(record.churn_reason) || record.churn_reason == null')).toEqual([]); + expect(find('has(record.some_transient_key)')).toEqual([]); + }); + + it('never flags equality — CEL evaluates a null equality cleanly to false', () => { + expect(find("record.budget == null || record.budget != 0")).toEqual([]); + expect(find("record.score == 100")).toEqual([]); + }); + + it('never flags a NON-nullable declared field', () => { + expect(findUnguardedNullableOperands('record.amount > 100', { nullableFields: new Set() })).toEqual([]); + }); + + it('never flags a nested / cross-object path it cannot judge', () => { + expect(find('record.account.budget > 100')).toEqual([]); + }); + + it('never flags a bare identifier (flow-variable shape)', () => { + expect(find('budget > 100')).toEqual([]); + }); + + it('honours a guard reached through `||` short-circuit (`x == null || x < y`)', () => { + expect(find('record.budget == null || record.budget > 100')).toEqual([]); + }); + + it('honours a guard reached through a ternary', () => { + expect(find('record.budget == null ? false : record.budget > 100')).toEqual([]); + expect(find('record.budget != null ? record.budget > 100 : false')).toEqual([]); + }); + + it('honours `!isBlank(x)` as a real guard', () => { + expect(find('!isBlank(record.budget) && record.budget > 100')).toEqual([]); + }); + + it('does NOT let a guard leak backwards across `&&`', () => { + // The comparison is evaluated BEFORE the guard, so the guard cannot save it. + const findings = find('record.budget > 100 && record.budget != null'); + expect(findings.map((f) => f.operand)).toEqual(['record.budget']); + }); + + it('does NOT accept a guard that only one arm of a `||` proves', () => { + const findings = find('(record.budget != null || record.score != null) && record.budget > 100'); + expect(findings.map((f) => f.operand)).toEqual(['record.budget']); + }); + + it('returns nothing for an unparseable source (syntax is another gate’s verdict)', () => { + expect(find('record.budget >')).toEqual([]); + expect(find('')).toEqual([]); + }); + + it('reports each operand/operator pair once, not per occurrence', () => { + expect(find('record.budget > 1 && record.budget > 2')).toHaveLength(1); + }); +}); + +describe('nullGuardMessage', () => { + it('names the rule, the operand and the `!= null` fix', () => { + const [finding] = find('has(record.end_date) && record.end_date < 5'); + const msg = nullGuardMessage("validation rule 'end_after_start'", 'showcase_project', finding); + expect(msg).toContain("validation rule 'end_after_start'"); + expect(msg).toContain('record.end_date'); + expect(msg).toContain('showcase_project'); + expect(msg).toContain('`<`'); + expect(msg).toContain('has(record.end_date)` does not guard it'); + expect(msg).toContain(NULL_GUARD_HINT); + }); + + it('closes with the runtime rejection wording verbatim (one voice, two gates)', () => { + // Lifted from `unevaluableRuleError` in + // packages/objectql/src/validation/rule-validator.ts — if that text moves, + // this assertion is the tripwire. + expect(NULL_GUARD_HINT).toBe( + "Guard it with '!= null'" + + " — 'has(x)' does NOT do that: a declared field holding null is still PRESENT, so has(x) is true.", + ); + }); +}); diff --git a/packages/lint/src/validate-null-guards.ts b/packages/lint/src/validate-null-guards.ts new file mode 100644 index 0000000000..053fbc59b2 --- /dev/null +++ b/packages/lint/src/validate-null-guards.ts @@ -0,0 +1,373 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `has(x)` is not a null guard — publish-time rejection (#4763). + * + * CEL's `has(x)` asks whether the key is **present**. A declared column holding + * `NULL` is present, and since #4649 every predicate sees a record that is + * TOTAL over the object's declared fields — so `has(record.end_date)` is + * uniformly `true` and tells the author nothing about the value. The idiom that + * reads like a null guard + * + * ```text + * has(record.start_date) && has(record.end_date) && record.end_date < record.start_date + * ``` + * + * therefore reaches `null < null`, CEL has no overload for it, the predicate + * aborts, and (post-#4761) the write is rejected fail-closed. Before #4761 the + * abort was swallowed: the rule was declared, listed in the metadata, and + * enforced **nothing** on exactly the rows it was written to catch. + * + * The fault is fully decidable from the metadata alone — the predicate's AST + * plus the object's declared field types say whether an operand can be null — + * so it belongs at authoring/publish, not at a 400 on production data + * (AGENTS.md PD #12: reject at authoring, do not tolerate at the consumer). + * This module is that decision procedure; `validate-expressions.ts` wires it + * into the gating `validateStackExpressions` rule, which `os build`, + * `os validate`, `os lint` and the runtime publish gate all run. + * + * ## What is rejected + * + * An **ordering** (`< <= > >=`) or **arithmetic** (`+ - * / %`, unary `-`) + * operator applied to an operand that + * + * 1. resolves to a declared field of the object (`record.` / `previous.`), + * 2. that field is nullable — no `required: true`, no `defaultValue`, no + * default option, not an autonumber, and + * 3. is not dominated by an explicit `!= null` / `== null` / `isBlank()` test + * in the same boolean branch. + * + * `has(x)` deliberately does **not** satisfy (3) — that is the entire point. + * `has()` over an **undeclared** key is untouched: it never resolves to a + * declared field, so (1) fails and the legitimate "was this key in the PATCH" + * use stays legal. Equality (`==` / `!=`) is never flagged: CEL evaluates a + * heterogeneous equality cleanly to `false` rather than faulting. + */ + +import { Environment } from '@marcbachmann/cel-js'; +import type { ASTNode } from '@marcbachmann/cel-js'; + +/** + * The corrective sentence, lifted **verbatim** from `unevaluableRuleError` in + * `packages/objectql/src/validation/rule-validator.ts` so the publish-time and + * runtime messages read identically — an author who hits one and then the other + * must not have to reconcile two phrasings of one rule (#4763). + */ +export const NULL_GUARD_HINT = + `Guard it with '!= null'` + + ` — 'has(x)' does NOT do that: a declared field holding null is still PRESENT, so has(x) is true.`; + +/** Ordering + arithmetic operators — the ones with no `null` overload. */ +const FAULTING_BINARY_OPS = new Set(['<', '<=', '>', '>=', '+', '-', '*', '/', '%']); + +/** Roots that bind the object's declared record shape (total since #4649). */ +const DEFAULT_RECORD_ROOTS = ['record', 'previous'] as const; + +export interface NullGuardOptions { + /** Field names of the object that may hold `null` at evaluation time. */ + nullableFields: ReadonlySet; + /** Roots bound to the object's record shape. Defaults to `record`/`previous`. */ + roots?: readonly string[]; +} + +export interface NullGuardFinding { + /** The operand as written, e.g. `record.end_date`. */ + operand: string; + /** The declared field the operand resolves to, e.g. `end_date`. */ + field: string; + /** The operator with no null overload, e.g. `<`. */ + operator: string; + /** + * True when the predicate "guards" this operand with `has()` and nothing + * else — the exact trap #4763 exists to reject. Used only to sharpen the + * message; the verdict is the same either way. + */ + hasOnlyGuard: boolean; +} + +// A check-only parse environment: no evaluation, no stdlib needed, every +// identifier stays `dyn` so any authored predicate parses. Built once. +let parseEnv: Environment | undefined; +function getParseEnv(): Environment { + if (!parseEnv) { + parseEnv = new Environment({ unlistedVariablesAreDyn: true, enableOptionalTypes: true }); + } + return parseEnv; +} + +type AnyNode = { op?: string; args?: unknown }; + +function isNode(v: unknown): v is AnyNode & ASTNode { + return !!v && typeof v === 'object' && typeof (v as AnyNode).op === 'string'; +} + +function isNullLiteral(node: unknown): boolean { + return isNode(node) && node.op === 'value' && (node as AnyNode).args === null; +} + +/** + * `record.` (or `previous.`) → the field name; anything else — + * a bare id, a nested traversal `record.account.region`, an index — → null. + * Deliberately single-segment: a nested path is a lookup traversal whose + * nullability this object's field list cannot decide. + */ +function fieldOf(node: unknown, roots: readonly string[]): { operand: string; field: string } | null { + if (!isNode(node)) return null; + if (node.op !== '.' && node.op !== '.?') return null; + const args = node.args as unknown; + if (!Array.isArray(args) || args.length < 2) return null; + const [recv, seg] = args as [unknown, unknown]; + if (typeof seg !== 'string') return null; + if (!isNode(recv) || recv.op !== 'id') return null; + const root = (recv as AnyNode).args; + if (typeof root !== 'string' || !roots.includes(root)) return null; + return { operand: `${root}.${seg}`, field: seg }; +} + +/** Children of a node, whatever its arg shape. */ +function childNodes(node: AnyNode): unknown[] { + const args = node.args; + if (isNode(args)) return [args]; + if (!Array.isArray(args)) return []; + const out: unknown[] = []; + for (const a of args) { + if (isNode(a)) out.push(a); + else if (Array.isArray(a)) for (const b of a) if (isNode(b)) out.push(b); + } + return out; +} + +function callName(node: AnyNode): string | null { + if (node.op !== 'call') return null; + const args = node.args; + if (!Array.isArray(args) || typeof args[0] !== 'string') return null; + return args[0]; +} + +function callArgs(node: AnyNode): unknown[] { + const args = node.args; + if (!Array.isArray(args) || !Array.isArray(args[1])) return []; + return args[1] as unknown[]; +} + +function union(a: ReadonlySet, b: ReadonlySet): Set { + return new Set([...a, ...b]); +} + +function intersect(a: ReadonlySet, b: ReadonlySet): Set { + const out = new Set(); + for (const v of a) if (b.has(v)) out.add(v); + return out; +} + +/** + * Operands proven non-null **when `node` evaluates true**. + * + * `has(x)` is conspicuously absent, and its absence is the rule: a present key + * is not a non-null value. `isBlank(x)` is absent for the mirrored reason — + * `isBlank` being *true* says nothing about non-nullness (it is true FOR null); + * it appears in {@link falseGuards} instead. + */ +function truthGuards(node: unknown, roots: readonly string[]): Set { + if (!isNode(node)) return new Set(); + switch (node.op) { + case '!=': { + const [l, r] = (node.args as [unknown, unknown]) ?? []; + if (isNullLiteral(r)) { + const f = fieldOf(l, roots); + return f ? new Set([f.operand]) : new Set(); + } + if (isNullLiteral(l)) { + const f = fieldOf(r, roots); + return f ? new Set([f.operand]) : new Set(); + } + return new Set(); + } + case '&&': { + const [l, r] = node.args as [unknown, unknown]; + return union(truthGuards(l, roots), truthGuards(r, roots)); + } + case '||': { + const [l, r] = node.args as [unknown, unknown]; + // Only what BOTH arms prove survives the disjunction. + return intersect(truthGuards(l, roots), truthGuards(r, roots)); + } + case '!_': + return falseGuards(node.args, roots); + case '?:': { + const [, t, f] = node.args as [unknown, unknown, unknown]; + return intersect(truthGuards(t, roots), truthGuards(f, roots)); + } + default: + return new Set(); + } +} + +/** Operands proven non-null **when `node` evaluates false** (the `!`/`||` side). */ +function falseGuards(node: unknown, roots: readonly string[]): Set { + if (!isNode(node)) return new Set(); + switch (node.op) { + case '==': { + const [l, r] = (node.args as [unknown, unknown]) ?? []; + if (isNullLiteral(r)) { + const f = fieldOf(l, roots); + return f ? new Set([f.operand]) : new Set(); + } + if (isNullLiteral(l)) { + const f = fieldOf(r, roots); + return f ? new Set([f.operand]) : new Set(); + } + return new Set(); + } + case '||': { + const [l, r] = node.args as [unknown, unknown]; + return union(falseGuards(l, roots), falseGuards(r, roots)); + } + case '&&': { + const [l, r] = node.args as [unknown, unknown]; + return intersect(falseGuards(l, roots), falseGuards(r, roots)); + } + case '!_': + return truthGuards(node.args, roots); + case 'call': { + // `!isBlank(record.x)` / `isBlank(record.x) ? … : ` — a false + // `isBlank` DOES prove non-null (it is the stdlib's blank-or-null test). + if (callName(node) !== 'isBlank') return new Set(); + const [only] = callArgs(node); + const f = fieldOf(only, roots); + return f ? new Set([f.operand]) : new Set(); + } + default: + return new Set(); + } +} + +/** Collect every `has(record.)` operand appearing anywhere in the tree. */ +function collectHasOperands(node: unknown, roots: readonly string[], out: Set): void { + if (!isNode(node)) return; + if (callName(node) === 'has') { + for (const a of callArgs(node)) { + const f = fieldOf(a, roots); + if (f) out.add(f.operand); + } + } + for (const child of childNodes(node)) collectHasOperands(child, roots, out); +} + +/** + * Find every ordering/arithmetic operand that resolves to a nullable declared + * field and is not dominated by a real null guard. Returns `[]` for anything + * that does not parse (syntax is reported by `validateExpression`, not here) — + * this pass never invents a second syntax verdict. + */ +export function findUnguardedNullableOperands( + source: string, + opts: NullGuardOptions, +): NullGuardFinding[] { + if (typeof source !== 'string' || !source.trim()) return []; + if (opts.nullableFields.size === 0) return []; + const roots = opts.roots ?? DEFAULT_RECORD_ROOTS; + + let ast: ASTNode; + try { + ast = getParseEnv().parse(source).ast; + } catch { + return []; + } + + const hasOperands = new Set(); + collectHasOperands(ast, roots, hasOperands); + + const findings: NullGuardFinding[] = []; + const seen = new Set(); + + const report = (operandNode: unknown, operator: string, guards: ReadonlySet): void => { + const f = fieldOf(operandNode, roots); + if (!f) return; + if (!opts.nullableFields.has(f.field)) return; + if (guards.has(f.operand)) return; + const key = `${f.operand}${operator}`; + if (seen.has(key)) return; + seen.add(key); + findings.push({ + operand: f.operand, + field: f.field, + operator, + hasOnlyGuard: hasOperands.has(f.operand), + }); + }; + + const visit = (node: unknown, guards: ReadonlySet): void => { + if (!isNode(node)) return; + const op = node.op as string; + + if (op === '&&') { + const [l, r] = node.args as [unknown, unknown]; + visit(l, guards); + // Left-to-right: only the LEFT conjunct's proofs reach the right one. + visit(r, union(guards, truthGuards(l, roots))); + return; + } + if (op === '||') { + const [l, r] = node.args as [unknown, unknown]; + visit(l, guards); + // Reaching the right arm means the left one was FALSE. + visit(r, union(guards, falseGuards(l, roots))); + return; + } + if (op === '?:') { + const [c, t, f] = node.args as [unknown, unknown, unknown]; + visit(c, guards); + visit(t, union(guards, truthGuards(c, roots))); + visit(f, union(guards, falseGuards(c, roots))); + return; + } + if (op === 'call' && callName(node) === 'has') { + // `has(x)` itself never faults — and it never counts as a guard either. + return; + } + if (FAULTING_BINARY_OPS.has(op)) { + const [l, r] = node.args as [unknown, unknown]; + report(l, op, guards); + report(r, op, guards); + visit(l, guards); + visit(r, guards); + return; + } + if (op === '-_') { + report(node.args, '-', guards); + visit(node.args, guards); + return; + } + for (const child of childNodes(node)) visit(child, guards); + }; + + visit(ast, new Set()); + return findings; +} + +/** + * The publish-time message for one finding. Names the rule, the operand and the + * `!= null` fix (the three things the author needs), then closes with the + * verbatim runtime sentence so the two gates speak with one voice. + * + * @param subject How the site names itself, e.g. `validation rule 'end_after_start'`. + * @param objectName The object whose field list decided nullability. + */ +export function nullGuardMessage( + subject: string, + objectName: string | undefined, + finding: NullGuardFinding, +): string { + const owner = objectName ? `'${objectName}'` : 'this object'; + const hasNote = finding.hasOnlyGuard + ? ` \`has(${finding.operand})\` does not guard it.` + : ''; + return ( + `${subject} applies \`${finding.operator}\` to \`${finding.operand}\`, which ${owner} declares ` + + `as nullable (no \`required: true\`, no \`defaultValue\`).${hasNote} At runtime the operand is ` + + `null, CEL has no \`${finding.operator}\` overload for null, and the whole predicate aborts — ` + + `so the rule enforces nothing and the write is rejected fail-closed (#4649/#4763). ` + + `The predicate compares a value that is null. ${NULL_GUARD_HINT}` + ); +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index a47d2b9f3a..5c37d5ab33 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -818,6 +818,9 @@ importers: packages/lint: dependencies: + '@marcbachmann/cel-js': + specifier: ^8.0.0 + version: 8.0.0 '@objectstack/formula': specifier: workspace:* version: link:../formula diff --git a/skills/objectstack-formula/SKILL.md b/skills/objectstack-formula/SKILL.md index fe85199992..40870142f6 100644 --- a/skills/objectstack-formula/SKILL.md +++ b/skills/objectstack-formula/SKILL.md @@ -120,6 +120,28 @@ ADR-0068 — spec field docs write predicates like `current_user.positions`. `null`. To check for "value present and non-blank" use the stdlib helper `isBlank()` or compare to `null` explicitly. +Every predicate reads a record that is **total over the object's declared +fields** (#4649), so `has(record.)` is uniformly `true` and +tells you nothing at all. The idiom that reads like a guard is not one: + +```text +# WRONG — both has() calls are true on a NULL row, so this reaches `null < null`, +# CEL has no overload, the predicate aborts and the write is rejected. +has(record.start_date) && has(record.end_date) && record.end_date < record.start_date + +# RIGHT +record.start_date != null && record.end_date != null && record.end_date < record.start_date +``` + +**This is a publish-time rejection, not advice (#4763).** `os build` / +`os validate` / `os lint` and the runtime publish gate reject any validation-rule +or hook predicate that applies an ordering (`< <= > >=`) or arithmetic +(`+ - * / %`) operator to a **declared nullable** field — no `required: true`, +no `defaultValue`, no default option — unless an explicit `!= null` / `== null` / +`!isBlank()` test dominates it in the same boolean branch. `has()` deliberately +does not satisfy that gate. `has()` over an **undeclared** key stays legal: that +is its real use — telling "absent from this PATCH" apart from "explicitly null". + ### Null + string throws CEL has no implicit `null` coercion. `null + 'foo'` throws From eadbea56168a066816a166e32c4472e720fd6dcc Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 08:21:02 +0000 Subject: [PATCH 2/2] fix(lint): spell the dedup key's NUL separator as a backslash-u-0000 escape, not a raw byte (#4763) `check:nul-bytes` was red: `validate-null-guards.ts` carried a literal 0x00 at byte offset 10987, inside the composite dedup key. Byte-identical at runtime -- this is purely how the character is spelled in source. It is not cosmetic. A raw NUL makes grep/ripgrep classify the whole file as binary and silently return ZERO matches, so the file drops out of code search and out of every grep-based lint. git does not warn, because it only inspects the first 8000 bytes to decide binary-ness and this one sits past that. A new gate whose own source is invisible to code search is a bad way to start. The unicode escape rather than the octal one, matching `packages/rest/src/rest-server.ts:1065`: the octal form becomes a legacy-escape error the moment a digit follows it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018iARDqtrhQgz6fVHDeDkbQ --- packages/lint/src/validate-null-guards.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/lint/src/validate-null-guards.ts b/packages/lint/src/validate-null-guards.ts index 053fbc59b2..4f95596a49 100644 --- a/packages/lint/src/validate-null-guards.ts +++ b/packages/lint/src/validate-null-guards.ts @@ -286,7 +286,15 @@ export function findUnguardedNullableOperands( if (!f) return; if (!opts.nullableFields.has(f.field)) return; if (guards.has(f.operand)) return; - const key = `${f.operand}${operator}`; + // NUL separates the composite key's two halves (it can appear in neither a + // field path nor an operator). Written as the `\u0000` ESCAPE, never as a raw + // byte: a raw NUL makes grep/ripgrep treat the whole file as binary and + // silently return ZERO matches, so the file drops out of code search and out + // of every grep-based lint - and git will not warn you, because it only + // inspects the first 8000 bytes to decide binary-ness. Same convention as + // `packages/rest/src/rest-server.ts`. `\u0000` rather than `\0`, which turns + // into a legacy-octal-escape error the moment a digit follows it. + const key = `${f.operand}\u0000${operator}`; if (seen.has(key)) return; seen.add(key); findings.push({