fix(objectql)!: a field-level requiredWhen / readonlyWhen that cannot be evaluated refuses the write (ADR-0137 D2)

.changeset/19727-field-rule-predicate-fault-refuses-submit.md

@@ -0,0 +1,79 @@
+---
+'@objectstack/objectql': minor
+'@objectstack/lint': patch
+---
+
+fix(objectql)!: a field-level `requiredWhen` / `readonlyWhen` predicate that cannot be evaluated now REFUSES the write, naming the field and the rule, instead of letting it through (ADR-0137 D2)
+
+Clause-②: no (narrowing)
+
+**BREAKING**: shipped as `minor` under the launch-window convention
+(`check-changeset-no-major` refuses `major` until GA). The banner and the ADR-0087
+disposition below carry the breaking change, not the level.
+
+**Writes that used to save now fail.** ADR-0137 D2 says: "At submit time, a
+field-rule predicate that cannot be evaluated refuses the write and names the
+field and the rule. Nothing is persisted." The server now enforces that on the
+two arms that let such a write through:
+
+- **`requiredWhen`**: a predicate that faults used to be logged
+  (`requiredWhen for '<field>' failed to evaluate — skipped`), and the record
+  saved with the field empty. It now refuses the insert or update. This covers
+  every fault, including a `parent`-scoped rule whose master-detail header
+  could not be resolved for the write.
+- **`readonlyWhen`**: a predicate that faults used to be logged
+  (`failed to evaluate — change allowed through`), and the field the author
+  declared frozen was written. It now refuses the update. On a bulk update, a
+  fault in any matched row refuses the whole write, and the refusal names that
+  row. One case is unchanged: a predicate that faults because the header it
+  reads as `parent` could not be resolved still holds the lock, as before.
+
+The refusal is the same `ValidationError` a broken validation rule has thrown
+since #4649: `VALIDATION_FAILED`, served as `400`. Its entry for the field
+carries `code: 'rule_violation'` and
+`constraint: { rule: 'requiredWhen' | 'readonlyWhen', reason: 'unevaluable', fault }`,
+with `missingKey` or `hint: 'null-comparison'` when the fault is one of those.
+The message names the field and the rule. It is refused before anything is
+written, on insert, single-id update and bulk update alike. The operator also
+gets a `warn` line saying the write was rejected.
+
+The refusal applies to the whole submit. A `requiredWhen` whose predicate
+faults refuses the write even when the write supplies the field, because the
+rule has no verdict to judge that value against.
+
+**What starts refusing.** A stored predicate that faults on the writes it
+judges:
+
+- a key the object does not declare, usually a typo (`record.statsu`);
+- an ordering comparison or arithmetic over a `null` (`record.amount > 100`
+  where `amount` is empty). Guard it with `!= null`. `has(x)` is true for a
+  declared field holding null, so it does not guard this;
+- a column read through a lookup (`record.account.tier`). The field level never
+  reads the related record, so the reference holds a bare id there. The refusal
+  says so, and names the reference and its target object;
+- an envelope with no evaluable `source`: blank, or `ast`-only.
+
+Nothing in this repository's own metadata is affected. A census of every
+`requiredWhen` / `readonlyWhen` under `packages/`, `examples/` and `apps/`
+found none that faults on a write it judges. How many stored predicates in a
+deployment fault is unknown, and ADR-0137 names that as the point: the loud
+state is what finds them.
+
+**Fix.** Read the refusal. It names the field, the rule, and the key or
+overload that faulted. Then correct the predicate: fix the key's spelling,
+guard the null operand with `!= null`, or move a check that reads through a
+lookup into a `validations[]` `script` rule, whose condition does read one hop
+through a reference.
+
+Unchanged: a predicate that evaluates is judged exactly as before, in both
+directions. So is the ADR-0113 legacy-row rule for an evaluated `requiredWhen`.
+Option-level `visibleWhen` is not a field-rule predicate, so D2 does not reach
+it, and it stays fail-open. The render side is not touched here (ADR-0137 D3
+keeps its directions for display).
+
+`@objectstack/lint`: the build-time messages for a field `requiredWhen` no
+longer say the server "skips" a faulting predicate. The unbound-root message,
+the `parent`-without-a-master message and the null-guard message now say the
+server refuses the write.
+
+<!-- adr-0087: not-required (no-migration-prescription) Nothing authored moves: `packages/spec` is untouched, `requiredWhen` and `readonlyWhen` keep their schema, and no stored shape is refused, so `objectstack migrate meta` has nothing to rewrite and the ledger has no row to gain. What changes is the runtime's answer to a predicate that cannot run, and the repair is specific to each broken predicate. There is no mechanical FROM to TO rewrite. The other categories are closed on facts: both packages publish (not `unpublished`); no ADR-0087 id covers a runtime fault direction (not `registered` / `already-registered`); and the change is runtime behaviour, not a TypeScript declaration (not `runtime-interface-only` / `type-surface-only`). -->

packages/lint/src/validate-expressions.test.ts

@@ -985,10 +985,11 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => {
     });
 
     // #4977 — the same gate, extended to the slot the same issue gave a server
-    // `parent` binding. `requiredWhen` stays FAIL-OPEN at runtime, so this build
-    // gate is the only thing that stops an unbindable declaration from shipping
-    // and enforcing nothing forever — which is why the message must name that
-    // consequence and not `readonlyWhen`'s opposite one.
+    // `parent` binding. `requiredWhen` REFUSES the write at runtime when the
+    // predicate cannot be evaluated (ADR-0137 D2; it was fail-OPEN until then),
+    // so this build gate is what stops an unbindable declaration from shipping
+    // and refusing writes in production — which is why the message must name
+    // that consequence and not `readonlyWhen`'s LOCKED one.
     describe('parent-scoped `requiredWhen` needs a resolvable master (#4977)', () => {
       const parentScopeIssues = (obj: Record<string, unknown>) =>
         validateStackExpressions({ objects: [obj] }).filter((i) => /reads `parent`/.test(i.message));
@@ -1006,8 +1007,9 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => {
         expect(issues[0]!.where).toMatch(/field 'description' requiredWhen/);
         expect(issues[0]!.message).toMatch(/declares no `master_detail` relationships/);
         // The CONSEQUENCE clause is what separates this from its `readonlyWhen`
-        // twin: fail-open there, fail-closed here, opposite fixes.
-        expect(issues[0]!.message).toMatch(/the requirement would never be enforced/);
+        // twin: that one LOCKS the field, this one REFUSES the write.
+        expect(issues[0]!.message).toMatch(/writes would be refused/);
+        expect(issues[0]!.message).not.toMatch(/never be enforced/);
         expect(issues[0]!.message).not.toMatch(/locked on every write/);
       });
 
@@ -1883,10 +1885,11 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => {
         expect(m).toMatch(/editable/);
       });
 
-      it('`requiredWhen` — says the requirement is never enforced, not anything about visibility', () => {
+      it('`requiredWhen` — says the server refuses the write, not anything about visibility', () => {
         const m = messageFor('requiredWhen');
-        expect(m).toMatch(/never enforced/);
-        expect(m).toMatch(/saves with the field empty/);
+        expect(m).toMatch(/server REFUSES a write/);
+        expect(m).not.toMatch(/never enforced/);
+        expect(m).not.toMatch(/saves with the field empty/);
         expect(m).not.toMatch(/VISIBLE/);
         expect(m).not.toMatch(/showing for everyone/);
       });
@@ -2517,9 +2520,9 @@ 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.
+  // the object's validation rules see. It WAS the quietest failure of the
+  // three covered surfaces — a faulting `requiredWhen` was fail-OPEN (logged
+  // and skipped) — until ADR-0137 D2 made it refuse the write like the rest.
   describe('field `requiredWhen` — covered since #4811', () => {
     const withField = (requiredWhen: string) =>
       validateStackExpressions({
@@ -2552,14 +2555,15 @@ describe('null-guard gate (#4763)', () => {
     });
 
     // 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', () => {
+    // author to the wrong place. Since ADR-0137 D2 a faulting `requiredWhen`
+    // REFUSES the write — `rule-validator` no longer logs and skips — so it
+    // takes the validation rules' "the write is rejected fail-closed" wording
+    // and must NOT keep promising that the write goes through.
+    it('reports the fail-CLOSED consequence — a faulting requiredWhen refuses the write (ADR-0137 D2)', () => {
       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');
+      expect(issue.message).toContain('rejected fail-closed');
+      expect(issue.message).not.toContain('SKIPPED fail-open');
+      expect(issue.message).not.toContain('the field is never actually required');
     });
 
     it('leaves the fail-closed wording on the surfaces that really fail closed', () => {

packages/lint/src/validate-expressions.ts

@@ -527,13 +527,14 @@ function rulePredicates(rule: AnyRec, path: string): Array<{ label: string; raw:
  *
  * ## Why it is an error and not a warning
  *
- * Every fault direction available here is silent, and two of the three are
- * the opposite of what the author declared (see the per-slot table below):
- * a `visibleWhen` written to HIDE leaves the field visible to everyone, a
- * `readonlyWhen` written to unlock-under-a-condition locks the field on every
- * write, and a `requiredWhen` simply never fires. None of the three produces
- * a runtime error an author can find; the only signal that exists is this one
- * (#6146).
+ * Every fault direction available here is either silent or the opposite of
+ * what the author declared (see the per-slot table below): a `visibleWhen`
+ * written to HIDE leaves the field visible to everyone, a `readonlyWhen`
+ * written to unlock-under-a-condition locks the field on every write, and a
+ * `requiredWhen` — silent until ADR-0137 D2 — now refuses every write that
+ * reaches the root, because that root is never bound there. Only the last
+ * produces a runtime error at all, and it produces it on the first write in
+ * production; build time is where the author should meet it (#6146).
  *
  * Verdict scope is the field level only. Per-option `visibleWhen` is checked
  * by the loop in the field walk and deliberately NOT passed through here:
@@ -631,14 +632,15 @@ function rulePredicates(rule: AnyRec, path: string): Array<{ label: string; raw:
  *    success, and the value silently never lands. The old sentence told this
  *    author the field would be VISIBLE TO EVERYONE — the opposite failure, and
  *    the opposite troubleshooting direction.
- *  - **`requiredWhen` — fail-OPEN at both ends, and never about visibility.**
- *    Server: the `requiredWhen` block logs `unknownVariableOf`'s name and
- *    `continue`s — #4977 deliberately did not copy #4889's carve-out, so the
- *    required-check is skipped for that write. Client: `fallback: false`, so
- *    the form does not mark the field required either. Both ends agree and
- *    both do nothing: the requirement is never enforced anywhere, and a record
- *    saves with the field empty. "Falls back to VISIBLE" was not merely
- *    imprecise here, it named the wrong property of the field.
+ *  - **`requiredWhen` — the server REFUSES, and never about visibility.**
+ *    Server: a `requiredWhen` that cannot be evaluated refuses the write,
+ *    naming the field and the rule (ADR-0137 D2). It was fail-OPEN until then
+ *    — the block logged `unknownVariableOf`'s name and `continue`d, and a
+ *    record saved with the field empty. Client: `fallback: false`, so the form
+ *    does not mark the field required, and the save it submits is refused. An
+ *    unbound root faults on every write that reaches it. "Falls back to
+ *    VISIBLE" was not merely imprecise here, it named the wrong property of
+ *    the field.
  *
  * `conditionalRequired` also reaches this helper (the field walk still passes
  * it). It is a `retiredKey` in `FieldSchema` — the strict schema rejects it by
@@ -803,9 +805,10 @@ const FIELD_RULE_SLOT_CONSEQUENCE: Record<string, string> = {
     'field editable (`fallback: false`). The server is the one that decides: ' +
     'the field looks writable, the save reports success, and the value silently never lands',
   requiredWhen:
-    'the predicate faults and the requirement is never enforced anywhere — the server logs it ' +
-    'and SKIPS the check (fail-open, #4977 deliberately did not take #4889\'s carve-out) and ' +
-    'the form does not mark the field required either, so a record saves with the field empty',
+    'the predicate faults on every write that reaches that root, and the server REFUSES a write ' +
+    'whose requirement it cannot evaluate (ADR-0137 D2: the refusal names the field and the ' +
+    'rule) — while the form does not mark the field required, so nothing on screen says why the ' +
+    'save failed',
   // Listed rather than left to the `??` below, so the map covers every slot
   // the field walk passes and the default stays unreachable. `FieldSchema`
   // declares this key only as a `retiredKey`, which rejects it by name, so
@@ -1640,13 +1643,13 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] {
       // What they do NOT share is the CONSEQUENCE, and the message has to name
       // the right one or it prescribes the wrong fix (the same reason #4811's
       // null-guard gate passes its outcome in explicitly instead of inferring
-      // it). The two runtimes fail in OPPOSITE directions on an unbindable
-      // `parent`: `readonlyWhen` fails CLOSED (#4889 — an unbound scope root
-      // resolves to LOCKED, so the field becomes unwritable forever), while
-      // `requiredWhen` stays fail-OPEN (#4977's ruling deliberately did not copy
-      // the carve-out), so the requirement silently enforces NOTHING. This gate
-      // is why fail-open is affordable there: the declaration that would rot
-      // unnoticed at runtime cannot ship in the first place.
+      // it). The two runtimes answer an unbindable `parent` differently:
+      // `readonlyWhen` fails CLOSED (#4889 — an unbound scope root resolves to
+      // LOCKED, so the field becomes unwritable forever), while `requiredWhen`
+      // REFUSES the write (ADR-0137 D2 — it was fail-OPEN until then, #4977)
+      // wherever the predicate reads `parent`. Either way the declaration is
+      // unusable, and this gate is where the author meets it instead of the
+      // first write in production.
       //
       // `conditionalRequired` (retired alias) and `visibleWhen` (no
       // server-enforced `parent` binding of its own) keep their verdicts
@@ -1658,7 +1661,7 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] {
       // indexed read here would have disarmed that scan silently.
       for (const [slot, raw, consequence] of [
         ['readonlyWhen', f.readonlyWhen, `the field would be locked on every write`],
-        ['requiredWhen', f.requiredWhen, `the requirement would never be enforced — the predicate faults, the server logs and skips it, and the field stays optional in the database`],
+        ['requiredWhen', f.requiredWhen, `writes would be refused — the predicate faults wherever it reads \`parent\`, and the server refuses a write whose requirement it cannot evaluate`],
       ] as const) {
         const source = celSourceOf(raw);
         if (masters === 1 || !source || !readsParentRoot(source)) continue;
@@ -1679,29 +1682,30 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] {
       // 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.
+      // where an unguarded predicate used to hurt most quietly — a faulting
+      // `requiredWhen` was fail-OPEN, so the field was simply never required
+      // and the write sailed through. Since ADR-0137 D2 it refuses the write
+      // (`rule-validator.ts`: "failed to evaluate … — write rejected"), the
+      // same outcome as a validation rule's since #4761, so it takes the
+      // `'fail-closed'` clause below.
       //
       // `readonlyWhen` is still NOT included even though it sits on the same
       // field — but no longer because its binding is sparse. Since #6454
       // `readonlyWhenBindings` runs both roots through
       // `materializeDeclaredFields`, so `!= null` IS the right prescription
       // there now; what holds the wiring back is clause 3 of the #4953 ruling
       // (both server-side seams first — the flow trigger-record half is
-      // outstanding). When it is wired it needs `'fail-open'`, like
-      // `requiredWhen` above and unlike the validation-rule surface. Same for
-      // `conditionalRequired` / `visibleWhen`, which have no record-scoped
+      // outstanding). When it is wired it needs `'fail-closed'`, like
+      // `requiredWhen` above: a faulting `readonlyWhen` refuses the write since
+      // ADR-0137 D2 too. `conditionalRequired` / `visibleWhen` 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',
+        'fail-closed',
       );
       if (f.expression) {
         // `expression` is the key `FieldSchema` declares for a computed field —