feat(lint): null-guard 闸门覆盖 requiredWhen,其余各面按绑定全量性定案 (#4811)

.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.<f>` / `previous.<f>`,从不解析裸标识符,而引擎无
+  条件绑定这两个根。真正的阻碍还是全量性:`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` 里时还能忍;现在报错正文要告诉作者改哪个字段,就不能忍了。

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.<f>`/`previous.<f>`, 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);