diff --git a/.changeset/null-guard-surface-coverage.md b/.changeset/null-guard-surface-coverage.md new file mode 100644 index 0000000000..a9f92ee9f4 --- /dev/null +++ b/.changeset/null-guard-surface-coverage.md @@ -0,0 +1,62 @@ +--- +"@objectstack/lint": minor +--- + +feat(lint): null-guard 闸门覆盖 `requiredWhen`,其余各面按"绑定是否全量"逐一定案 (#4811) + +#4763 的 null-guard 闸门只接了两面(对象校验规则、生命周期 hook `condition`), +其余各面留作"待定"。本次把"待定"收敛成一条**可判定的判据**,并按它逐面定案 —— +一个只覆盖部分面、又没有任何东西说出这件事的闸门,正是这一族缺陷本身的形状。 + +## 判据:记录绑定是否对已声明字段**全量** + +这不是口味问题,也不是"这个谓词是不是 CEL"。实测 `@marcbachmann/cel-js`,两种绑定 +下的语义**恰好相反**: + +| 谓词 | 全量绑定 `{a: null}` | 稀疏绑定 `{}` | +|:--|:--|:--| +| `has(record.a)` | `true` ← 陷阱 | `false` ← 真守卫 | +| `record.a < record.b` | FAULT `no such overload` | FAULT `No such key: a` | +| `record.a != null` | `false` ← **修法有效** | FAULT `No such key: a` | + +即:全量绑定下 `has()` 恒真而无用、`!= null` 是解药;稀疏绑定下 `has()` 恰恰是正确的 +守卫,而 `!= null` **自身就会 fault**。把闸门指向一个稀疏绑定的面,等于判红正确的元数据、 +并给出一个会把它改坏的"修法" —— 比不覆盖更糟。所以:**只有绑定全量的面才可以接入。** + +## 纳入:字段 `requiredWhen` + +议题没有列出这一面,而它恰恰是唯一满足判据的:`evaluateValidationRules` 用与对象校验 +规则**同一个** `materializeDeclaredFields` 合并记录来求值 `requiredWhen`。 + +它也是几个已覆盖面里失败得最安静的一个:`requiredWhen` 谓词 fault 时是 **fail-open** —— +`rule-validator.ts` 记一行 `failed to evaluate — skipped` 就跳过,字段于是**从未真正必填**, +写入照常通过。校验规则至少自 #4761 起是 fail-closed 的拒绝。因此报错文案按面区分后果: +"被跳过、字段从未必填"与"写入被 fail-closed 拒绝"是两个相反的故障,作者需要知道自己 +碰到的是哪一个。 + +## 排除,且各自留下可引用的理由 + +- **action `visible` / `disabled`**:谓词确实走真 CEL(裸串经 `ExpressionInputSchema` + 规范成 `{dialect:'cel'}` 信封,渲染器保留它),fault 也确实 fail-closed —— 陷阱在这一面 + 是真的。但绑定是客户端已取到的那条记录(详情读取,或只带列表视图投影列的一行), + `objectui` 这条路径上不存在任何物化步骤。稀疏绑定下 `!= null` 是错的修法。要覆盖它, + 得先决定是否把该绑定做成全量 —— 那是平台契约改动,不是 lint 改动。 +- **flow / edge `condition`**:议题记的理由(扁平作用域下裸标识符可能是 flow 变量)对本 + 模块**不成立** —— 它只解析 `record.` / `previous.`,从不解析裸标识符,而引擎无 + 条件绑定这两个根。真正的阻碍还是全量性:`record-change-trigger.ts` 把记录播种为 + `{ ...inputDoc, ...after }`,没有 `materializeDeclaredFields`,所以写入未提及的已声明列 + 是**缺键**而非 null,`!= null` 会和它本要守卫的比较一样 fault。 +- **字段 `readonlyWhen`**:与 `requiredWhen` 同一个字段、相反的结论 —— 它由 + `stripReadonlyWhenFields` 求值,那里合并的是 `{ ...previous, ...data }`,从不物化。 +- **`Field.formula`**:按产品判断排除,而非按本判据。formula 是 `value` 角色、天然可空, + `guard ? value : null` 是被祝福的写法(#3306)。是否强制守卫会改变"作者被允许写什么", + 该由维护者决定,不是一个接线缺口。 + +判据、实测表与逐面台账写在 `validate-null-guards.ts` 的模块注释里,每条排除在它对应的 +调用点也留了注释,并各配一条断言钉住。 + +## 顺带修正:`field '?'` + +诊断的字段名此前走 `Object.values(fields)`,把**名字键**丢掉了 —— 而名字键正是 +`Field.text({…})` 这种(最常见的)写法产生的形状,于是这类对象上的每条字段级诊断都定位在 +`field '?'`。名字只出现在 `where` 里时还能忍;现在报错正文要告诉作者改哪个字段,就不能忍了。 diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index e637acb5ca..a44fbb1874 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -463,12 +463,19 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { expect(issues.some(i => i.where.includes('requiredWhen') && /bare reference `status`/.test(i.message))).toBe(true); }); + // `qty` carries `required` + `defaultValue` so it can never be null. That + // is not decoration: it mirrors the real `showcase_invoice_line.quantity`, + // and without it `record.qty >= 100` is a genuine #4811 finding — `>=` on a + // nullable declared field, which faults at runtime and makes the + // `requiredWhen` silently unenforced. This case is about bare-vs-qualified + // references (#1928) and the `parent` namespace, so the fixture is pinned + // to the non-null shape rather than the gate being loosened around it. it('accepts record-qualified field rules and the master-detail `parent` namespace', () => { const issues = validateStackExpressions({ objects: [{ name: 'inv_line', fields: { - qty: { type: 'number', readonlyWhen: "parent.status == 'paid'" }, + qty: { type: 'number', required: true, defaultValue: 1, readonlyWhen: "parent.status == 'paid'" }, note: { type: 'text', requiredWhen: 'record.qty >= 100' }, }, }], @@ -919,6 +926,103 @@ describe('null-guard gate (#4763)', () => { }); }); + // #4811 — the one surface the coverage review found to MEET the gate's + // totality criterion: `evaluateValidationRules` evaluates a field's + // `requiredWhen` against the same `materializeDeclaredFields`-merged record + // the object's validation rules see. It is also the quietest failure of the + // three covered surfaces: a faulting `requiredWhen` is fail-OPEN (logged and + // skipped), so the field is simply never required and the write sails through. + describe('field `requiredWhen` — covered since #4811', () => { + const withField = (requiredWhen: string) => + validateStackExpressions({ + objects: [{ ...project, fields: { ...project.fields, note: { type: 'text', requiredWhen } } }], + }); + + it('REJECTS the `has(a) && has(b) && a < b` shape on a requiredWhen predicate', () => { + const issues = withField( + '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 slot … + expect(joined).toContain("field 'note' requiredWhen"); + // … the operands … + expect(joined).toContain('record.end_date'); + expect(joined).toContain('record.start_date'); + // … and the fix, in the runtime's own words (identical to every other + // surface this gate covers — one voice, #4763). + expect(joined).toContain("Guard it with '!= null'"); + expect(joined).toContain('has(x)'); + expect(issues[0].where).toContain("object 'showcase_project' · field 'note' requiredWhen"); + }); + + it('names `has()` explicitly as a non-guard when that is all the author wrote', () => { + const issues = withField('has(record.budget) && record.budget > 100'); + expect(issues).toHaveLength(1); + expect(issues[0].message).toContain('`has(record.budget)` does not guard it'); + }); + + // The consequence clause is per-surface, and getting it wrong sends the + // author to the wrong place. `requiredWhen` is fail-OPEN — `rule-validator` + // logs and skips — so it must NOT borrow the validation rules' "the write + // is rejected fail-closed" wording. + it('reports the fail-OPEN consequence, not the validation rules’ fail-closed one', () => { + const [issue] = withField('has(record.budget) && record.budget > 100'); + expect(issue.message).toContain('SKIPPED fail-open'); + expect(issue.message).toContain('the field is never actually required'); + expect(issue.message).not.toContain('rejected fail-closed'); + }); + + it('leaves the fail-closed wording on the surfaces that really fail closed', () => { + const [issue] = validateStackExpressions({ + objects: [{ ...project, validations: [{ type: 'script', name: 'r', condition: 'record.budget > 1' }] }], + }); + expect(issue.message).toContain('rejected fail-closed'); + expect(issue.message).not.toContain('SKIPPED fail-open'); + }); + + it('ACCEPTS the `!= null` form', () => { + expect( + withField('record.start_date != null && record.end_date != null && record.end_date < record.start_date'), + ).toHaveLength(0); + }); + + it('never flags a required field or one carrying a default', () => { + expect(withField('record.spent > 0')).toHaveLength(0); + }); + + // `has()` over a key the object does not declare is the macro's LEGITIMATE + // use ("was this key in the PATCH?") and must never draw a null-guard + // verdict — a false positive here is worse than a miss. Asserted on the + // null-guard verdict specifically: the independent #1928 field-existence + // pass has its own (pre-existing, correct) opinion about an undeclared + // name on a record-scoped slot, and that is not what this pins. + it('leaves `has()` on an UNDECLARED key alone — its legitimate use', () => { + const nullGuardIssues = withField('has(record.some_transient_key)') + .filter((i) => i.message.includes("Guard it with '!= null'")); + expect(nullGuardIssues).toHaveLength(0); + }); + + // Live-metadata pin: `showcase_invoice_line.description` really does carry + // `requiredWhen: record.quantity >= 100`, and `quantity` is `required: true` + // WITH `defaultValue: 1`, so it can never be null. This predicate must stay + // green — flagging it would be the false positive that is worse than a miss. + it('leaves the real `showcase_invoice_line` requiredWhen alone', () => { + expect( + validateStackExpressions({ + objects: [{ + name: 'showcase_invoice_line', + fields: { + quantity: { type: 'number', required: true, defaultValue: 1 }, + description: { type: 'text', requiredWhen: 'record.quantity >= 100' }, + }, + }], + }), + ).toHaveLength(0); + }); + }); + describe('surfaces deliberately NOT covered', () => { it('leaves sharing-rule conditions alone (compiled to a SQL filter, never faults)', () => { expect( @@ -933,7 +1037,15 @@ describe('null-guard gate (#4763)', () => { ).toHaveLength(0); }); - it('leaves flattened flow conditions alone (a bare id may be a flow variable)', () => { + // #4811 re-measured the reason this one is excluded. It is NOT the + // flattened scope (this gate never resolves a bare identifier — only + // `record.`/`previous.`, and the engine binds both roots + // unconditionally). It is that `record-change-trigger.ts` seeds the flow's + // record as `{ ...inputDoc, ...after }` with no `materializeDeclaredFields`, + // so a declared column the write never mentioned is an ABSENT key — and on + // an absent key the `!= null` this gate prescribes faults exactly like the + // comparison it was meant to guard. + it('leaves flow conditions alone (trigger record is not total over declared fields)', () => { expect( validateStackExpressions({ objects: [project], @@ -943,7 +1055,41 @@ describe('null-guard gate (#4763)', () => { { id: 'start', type: 'start', config: { objectName: 'showcase_project' } }, { id: 'd', type: 'decision', config: { condition: 'record.budget > 100000' } }, ], - edges: [], + edges: [{ id: 'e1', source: 'd', target: 'end', condition: 'record.spent > record.budget' }], + }], + }), + ).toHaveLength(0); + }); + + // Action predicates reach real CEL and fail closed, so the trap bites here + // too — but the record bound is whatever the client fetched (a list row + // carries only the view's projected columns) and nothing materializes it, + // so `!= null` would be the wrong prescription. Excluded until that binding + // is made total; see the ledger in `validate-null-guards.ts`. + it('leaves action `visible` / `disabled` alone (client record is not total)', () => { + expect( + validateStackExpressions({ + objects: [{ + ...project, + actions: [{ + name: 'escalate', + visible: 'has(record.budget) && has(record.spent) && record.spent > record.budget', + disabled: 'record.budget < 1000', + }], + }], + }), + ).toHaveLength(0); + }); + + // Same field as the covered `requiredWhen`, opposite verdict — the split is + // the point. `readonlyWhen` is evaluated by `stripReadonlyWhenFields`, which + // merges `{ ...previous, ...data }` and never materializes. + it('leaves field `readonlyWhen` alone (strip path merges without materializing)', () => { + expect( + validateStackExpressions({ + objects: [{ + ...project, + fields: { ...project.fields, note: { type: 'text', readonlyWhen: 'record.budget > 100' } }, }], }), ).toHaveLength(0); diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index 0dcc3f2d68..805d5e250c 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -28,6 +28,7 @@ import { collectFlowGraphs, resolveFlowNodeExpressions } from '@objectstack/spec import type { FlowNodeParsed } from '@objectstack/spec/automation'; import { findUnguardedNullableOperands, nullGuardMessage } from './validate-null-guards.js'; +import type { NullGuardOutcome } from './validate-null-guards.js'; export interface ExprIssue { where: string; @@ -192,21 +193,30 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { 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. + * The #4763 null-guard gate. Wired to exactly those surfaces whose predicates + * CEL evaluates over a record made **total** for every declared field + * (`materializeDeclaredFields`, #1871/#4649) — today: object validation rules + * (`rule-validator.ts`, fail-closed since #4761), lifecycle hook `condition`s, + * and field `requiredWhen` (#4811). + * + * Totality is the whole criterion, not a detail: on a total binding `has()` is + * uniformly true and `!= null` is the fix, while on a SPARSE one `has()` is a + * genuine guard and `!= null` faults with `No such key` — so pointing this gate + * at a sparse-bound surface would reject correct metadata and prescribe a fix + * that breaks it. `validate-null-guards.ts` carries the measured evidence table + * and the per-surface ledger (action predicates, flow conditions, field + * `readonlyWhen`, sharing rules and `Field.formula` are each excluded there with + * a traced reason). Read it before extending this call. */ const checkNullGuards = ( where: string, subject: string, raw: unknown, objectName: string | undefined, + // What THIS surface's runtime does once the predicate aborts. Passed + // explicitly rather than inferred, because the two possibilities are + // opposite failures and the message has to name the right one (#4811). + outcome: NullGuardOutcome = 'fail-closed', ): void => { if (!objectName) return; const nullableFields = nullableIndex.get(objectName); @@ -216,7 +226,7 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { for (const finding of findUnguardedNullableOperands(source, { nullableFields })) { issues.push({ where, - message: nullGuardMessage(subject, objectName, finding), + message: nullGuardMessage(subject, objectName, finding, outcome), source, severity: 'error', }); @@ -342,6 +352,28 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { for (const edge of graph.edges as unknown as AnyRec[]) { check(`${at} · edge '${edge.id}' (${edge.source}→${edge.target}) condition`, edge.condition, objectName); } + // No `checkNullGuards` on node/edge conditions — and NOT for the reason + // #4811 first recorded (#4811 re-measured it). The stated blocker was the + // flattened scope: a bare identifier like `budget > 100000` might be a flow + // variable rather than a field. That is true of a bare-identifier checker, + // but this gate never resolves a bare identifier — it matches only + // `record.` / `previous.`, and the engine binds both roots + // unconditionally, so it is immune to that ambiguity. + // + // The real blocker is totality. `record-change-trigger.ts` seeds the flow's + // record as `{ ...inputDoc, ...after }` with no `materializeDeclaredFields`, + // so a declared column the write never mentioned is an ABSENT key, not a + // null one — and there `record.x != null` faults (`No such key`) exactly + // like the comparison it was meant to guard. The gate's prescription is + // unsound on this surface until the trigger's record is made total the way + // #4649 made the validation-rule and hook bindings total. + // + // (For the record, the flattened-scope ambiguity is separately real and + // would need its own criterion: flow inputs shadow record fields — + // `if (!variables.has(k))` in `AutomationEngine.execute` — and a node's + // `outputVariable` can overwrite either, so a sound bare-identifier pass + // must subtract flow inputs, every `outputVariable`, screen-collected + // variable names and node ids.) } } @@ -364,30 +396,78 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { } // Field-level formulas (computed fields) reference the same object. const fields = obj.fields; - const fieldList = Array.isArray(fields) + // Paired with the field's NAME. `fields` has two authored shapes and the + // name lives in a different place in each: it is `f.name` in the array + // shape and the KEY in the name-keyed map shape. Walking `Object.values` + // dropped that key, so every diagnostic on a map-shaped object — the shape + // `Field.text({…})` authoring produces, i.e. the common one — was located + // at `field '?'` (#4811). Harmless-looking while the name only appeared in + // `where`; not harmless once a message has to tell the author which field + // to edit. Array entries keep their previous fallback so a nameless one is + // still validated rather than silently skipped. + const fieldList: Array<[string, AnyRec]> = Array.isArray(fields) ? (fields as AnyRec[]) - : (fields && typeof fields === 'object' ? Object.values(fields as AnyRec) as AnyRec[] : []); + .filter((f) => !!f && typeof f === 'object') + .map((f) => [typeof f.name === 'string' ? f.name : '?', f] as [string, AnyRec]) + : (fields && typeof fields === 'object' + ? Object.entries(fields as AnyRec) + .filter(([, def]) => !!def && typeof def === 'object') + .map(([n, def]) => [n, def as AnyRec] as [string, AnyRec]) + : []); // (ADR-0062 D7's `field.columnName`-on-external-objects rejection was removed // with `field.columnName` itself in #2377: the field no longer exists, so there // is no dual-source ambiguity to guard — external column mapping is `external.columnMap`.) - for (const f of fieldList) { + for (const [fname, f] of fieldList) { // Field-level conditional rules are server-enforced (rule-validator) and // record-scoped — a bare ref silently fails the rule (required/readonly // not enforced = data-integrity hole). #1928 class, same as actions. - if (f && typeof f === 'object') { - const fname = (f.name as string) ?? '?'; - for (const key of ['requiredWhen', 'readonlyWhen', 'conditionalRequired', 'visibleWhen'] as const) { - check(`object '${objectName}' · field '${fname}' ${key}`, (f as AnyRec)[key], objectName, 'record'); - } + for (const key of ['requiredWhen', 'readonlyWhen', 'conditionalRequired', 'visibleWhen'] as const) { + check(`object '${objectName}' · field '${fname}' ${key}`, (f as AnyRec)[key], objectName, 'record'); } - if (f && typeof f === 'object' && f.formula) { + // #4811 — `requiredWhen` is the one field-level slot that meets the + // null-guard gate's totality criterion: `evaluateValidationRules` + // evaluates it against the SAME `materializeDeclaredFields`-merged + // record the object's validation rules see. It is also the surface + // where an unguarded predicate hurts most quietly — a faulting + // `requiredWhen` is fail-OPEN (`rule-validator.ts` logs + // "failed to evaluate — skipped"), so the field is simply never + // required and the write sails through. Validation rules at least + // reject fail-closed since #4761. + // + // `readonlyWhen` is deliberately NOT included even though it sits on + // the same field: it is evaluated by `stripReadonlyWhenFields`, which + // builds `{ ...previous, ...data }` and never materializes, so its + // binding is sparse and `!= null` would be the wrong prescription + // there. Same for `conditionalRequired` / `visibleWhen`, which have no + // record-scoped total binding of their own. See the surface ledger in + // `validate-null-guards.ts`. + checkNullGuards( + `object '${objectName}' · field '${fname}' requiredWhen`, + `field '${fname}' requiredWhen`, + f.requiredWhen, + objectName, + 'fail-open', + ); + if (f.formula) { // formulas are `value` role (any return type), still CEL. They are // `record`-scoped — `record.`, never bare — so flag bare refs (#1928). + // + // No `checkNullGuards` here, and unlike the action / flow surfaces this + // is NOT a totality verdict (#4811). A formula is `value`-role and + // natively nullable, and `guard ? value : null` is the blessed shape + // (#3306 rewrites it through `dyn(...)`). Whether unguarded arithmetic + // such as `record.budget - record.spent` should be *forbidden* here is a + // question about what authors are allowed to write — a product decision + // for the maintainer, not a wiring gap for a lint PR to close on its own. + // The convention already leans guarded (`showcase_project.budget_remaining` + // writes `(record.budget == null ? 0 : record.budget) - …`), so the cost of + // deciding later is low. Raise it as its own issue rather than widening + // this call. Ledger: `validate-null-guards.ts`. const res = validateExpression('value', f.formula as string | { dialect?: string; source?: string }, objectName ? { objectName, fields: fieldIndex.get(objectName), fieldTypes: fieldTypeIndex.get(objectName), scope: 'record' } : { scope: 'record' }); - const fieldWhere = `object '${objectName}' · field '${(f.name as string) ?? '?'}' formula`; + const fieldWhere = `object '${objectName}' · field '${fname}' formula`; for (const e of res.errors) issues.push({ where: fieldWhere, message: e.message, source: e.source, severity: 'error' }); for (const w of res.warnings) issues.push({ where: fieldWhere, message: w.message, source: w.source, severity: 'warning' }); } @@ -415,6 +495,19 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { if (typeof action.disabled !== 'boolean') { check(`${where} · action '${name}' disabled`, action.disabled, obj, 'record'); } + // No `checkNullGuards` here, and the reason is measured rather than assumed + // (#4811). These predicates DO reach real CEL — a bare authored string is + // normalized to a `{dialect:'cel'}` envelope by `ExpressionInputSchema` and + // `objectui`'s renderers preserve it — and a fault IS fail-closed, so the + // `has(a) && has(b) && a < b` trap genuinely bites here too: the action + // silently disappears on every record. What blocks the gate is the record + // BINDING: it is whatever the client fetched (a detail read, or a list row + // holding only the view's projected columns), and no materialization step + // exists anywhere on that path. On such a sparse binding `!= null` — the fix + // this gate prescribes — faults with `No such key`, so gating here would + // reject working metadata and hand the author a correction that breaks it. + // Covering this surface means first making the binding total, which is a + // platform contract change, not a lint change. Ledger: `validate-null-guards.ts`. }; for (const action of asArray(stack.actions)) { checkAction('stack', action); diff --git a/packages/lint/src/validate-null-guards.ts b/packages/lint/src/validate-null-guards.ts index 4f95596a49..63c8e7d04f 100644 --- a/packages/lint/src/validate-null-guards.ts +++ b/packages/lint/src/validate-null-guards.ts @@ -42,6 +42,76 @@ * 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. + * + * ## Which surfaces this gate may cover — the totality criterion (#4811) + * + * #4763 wired two surfaces and left the rest "to be decided". The deciding + * property turns out not to be a matter of taste, and it is not "is this + * predicate CEL?" either. It is whether the record bound at evaluation time is + * **TOTAL over the object's declared fields** (`materializeDeclaredFields`, + * #1871/#4649). Measured against `@marcbachmann/cel-js`, the two bindings do + * not merely differ in wording — they inarguably invert: + * + * | predicate | TOTAL binding (`{a: null}`) | SPARSE binding (`{}`) | + * |:-------------------------|:----------------------------|:-----------------------| + * | `has(record.a)` | `true` ← the trap | `false` ← a real guard | + * | `record.a < record.b` | FAULT `no such overload` | FAULT `No such key: a` | + * | `record.a != null` | `false` ← **the fix works** | FAULT `No such key: a` | + * + * So on a total binding `has()` is uniformly true and useless while `!= null` + * is the cure; on a sparse binding `has()` is exactly the right guard and + * `!= null` **is itself a fault**. Running this gate over a sparse-bound + * surface would therefore reject correct metadata and prescribe a "fix" that + * breaks it — strictly worse than not covering the surface at all. Hence: + * + * **This module may only be wired to a surface whose record binding is total.** + * + * Surface ledger (each verdict traced to the code that decides it, so the next + * author does not have to re-derive it — #4811): + * + * | surface | binding | evidence | verdict | + * |:-------------------------------|:--------|:------------------------------------------------------------|:--------| + * | object validation rules | TOTAL | `rule-validator.ts` `materializeDeclaredFields(merged, …)` | covered | + * | lifecycle hook `condition` | TOTAL | `hook-wrappers.ts` `materializeDeclaredFields(…)` | covered | + * | field `requiredWhen` | TOTAL | same `merged` in `evaluateValidationRules` — fail-OPEN, so an unguarded predicate enforces NOTHING in silence | covered (#4811) | + * | field `readonlyWhen` | sparse | `stripReadonlyWhenFields` merges `{...previous, ...data}` and never materializes | excluded | + * | action `visible` / `disabled` | sparse | evaluated client-side; no materialization exists in `objectui` | excluded | + * | flow / edge `condition` | sparse | `record-change-trigger.ts` seeds `{...inputDoc, ...after}` | excluded | + * | sharing-rule `condition` | n/a | compiled to a SQL filter; `NULL > x` is three-valued, never faults | excluded | + * | `Field.formula` | n/a | product judgement, not a wiring gap — see below | excluded | + * + * The three exclusions that are *not* self-evident, spelled out because a + * surface excluded without a reason is indistinguishable from one nobody + * looked at — the failure mode this whole family of issues is about: + * + * - **Action `visible` / `disabled`.** #4811 asked whether the ActionEngine + * materializes declared fields before evaluating. It does not: the record + * is whatever the client already fetched (a record-detail read, or a LIST + * ROW carrying only the view's projected columns), and `objectui` contains + * no materialization step on that path. The predicate does reach real CEL + * (a bare authored string is normalized to a `{dialect:'cel'}` envelope by + * `ExpressionInputSchema`, which the renderers preserve) and a fault IS + * fail-closed — the action silently vanishes — so the *trap* is real here. + * But with a sparse binding the prescription inverts (see the table), so the + * gate cannot be the thing that catches it. Covering this surface requires + * first deciding whether the action-predicate binding should be made total, + * which is a platform contract change, not a lint change. + * - **Flow / edge `condition`.** #4811 excluded these for flattened-scope + * ambiguity ("a bare identifier may be a flow variable"). That reason does + * not actually apply to this module — {@link findUnguardedNullableOperands} + * only ever resolves `record.` / `previous.` and never a bare + * identifier, and the engine binds `record` / `previous` unconditionally. + * The real blocker is totality: the trigger seeds the record as + * `{...inputDoc, ...after}`, so a declared column the write never mentioned + * is an ABSENT key, and the `!= null` this gate prescribes would fault. + * (The flattened-scope ambiguity is real for a *bare-identifier* checker — + * flow inputs shadow record fields, and a node's `outputVariable` can + * overwrite either — but that is a different, unbuilt pass.) + * - **`Field.formula`.** Excluded by product judgement, not by this criterion: + * a formula is `value`-role and natively nullable, and `guard ? value : null` + * is the blessed shape (#3306 rewrites it via `dyn(...)`). Making guarded + * arithmetic mandatory there would change what authors are *allowed to + * write*, which is a decision for the maintainer, not a wiring gap to close. */ import { Environment } from '@marcbachmann/cel-js'; @@ -354,6 +424,29 @@ export function findUnguardedNullableOperands( return findings; } +/** + * What the surface actually DOES once the predicate aborts (#4811). + * + * Both outcomes are bugs, but they are *opposite* bugs, and an author needs to + * be told which one they have: "your write will be refused" and "your rule will + * never fire" send you to different places. Never guess one — read the + * surface's runtime and name what it really implements. + */ +export type NullGuardOutcome = + /** Validation rules + hook conditions: the fault propagates, the write is refused (#4761). */ + | 'fail-closed' + /** Field `requiredWhen`: `rule-validator.ts` logs and skips, so nothing is enforced (#4811). */ + | 'fail-open'; + +const OUTCOME_CLAUSE: Record = { + 'fail-closed': + 'so the rule enforces nothing and the write is rejected fail-closed (#4649/#4763)', + 'fail-open': + 'so the predicate is SKIPPED fail-open — the field is never actually required, the write ' + + 'proceeds unchecked, and the only trace is a `requiredWhen … failed to evaluate — skipped` ' + + 'log line (#4649/#4811)', +}; + /** * 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 @@ -361,11 +454,15 @@ export function findUnguardedNullableOperands( * * @param subject How the site names itself, e.g. `validation rule 'end_after_start'`. * @param objectName The object whose field list decided nullability. + * @param finding The unguarded operand to report. + * @param outcome What this surface's runtime does with the abort. Defaults to + * `fail-closed` — what both #4763 surfaces do. */ export function nullGuardMessage( subject: string, objectName: string | undefined, finding: NullGuardFinding, + outcome: NullGuardOutcome = 'fail-closed', ): string { const owner = objectName ? `'${objectName}'` : 'this object'; const hasNote = finding.hasOnlyGuard @@ -375,7 +472,7 @@ export function nullGuardMessage( `${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). ` + + `${OUTCOME_CLAUSE[outcome]}. ` + `The predicate compares a value that is null. ${NULL_GUARD_HINT}` ); }