diff --git a/.changeset/tender-donkeys-smoke.md b/.changeset/tender-donkeys-smoke.md new file mode 100644 index 0000000000..d30019bc61 --- /dev/null +++ b/.changeset/tender-donkeys-smoke.md @@ -0,0 +1,54 @@ +--- +'@objectstack/objectql': minor +--- + +**Validation rules now fail CLOSED when their predicate cannot be evaluated, and the record a predicate reads is total over the object's declared fields (#4649).** + +⚠️ **Behaviour change — read this before upgrading.** A `script` / `cross_field` / +`conditional` validation whose CEL predicate faulted used to be logged at WARN and +**skipped**, so the write went through. The rule stayed declared, appeared in the +metadata and in any "what protects this object" listing, and enforced nothing. Two +changes close that, and they are load-bearing together: + +1. **The merged record is total on UPDATE, not just on INSERT.** Every field the object + declares is present when the predicate runs — `null` when it is in neither the payload + nor the prior record. Previously `previous` was whatever the driver returned, so on a + driver that stores only written columns a predicate referencing a declared column + aborted with `No such key` and the rule was skipped. The `previous` CEL binding is + materialised the same way. Insert and update now behave identically. +2. **A predicate that still cannot be evaluated rejects the write** with + `VALIDATION_FAILED`, naming the rule and — when the fault is a missing key — the key + the predicate read and how to fix it. A validation exists to reject a write; "the rule + could not be checked" must never resolve to "allowed". + +`severity` still governs blocking: an unevaluable `warning` / `info` rule is logged and +does not throw. + +**What you may see after upgrading** + +- **Rules that were never running start running.** A rule skipped because of a missing key + now evaluates and can reject writes it previously let through. This is not a regression — + it is the declaration finally being enforced — but on an existing deployment it can + surface as new `400 VALIDATION_FAILED` responses on writes that used to succeed. Review + each such rule: it is doing what its author wrote. +- **Predicates guarded with `has(...)` may now reject.** `has(x)` asks whether the key is + **present**, and a declared field holding `null` is present — so + `has(a) && has(b) && a < b` still faults on `null < null`. Such a rule never enforced + anything on rows with a null value (on any driver that returns its NULL columns); the + fault used to be swallowed and is now reported. **Guard with `!= null`, not `has(...)`:** + + ```diff + - condition: 'has(record.start_date) && has(record.end_date) && record.end_date < record.start_date' + + condition: 'record.start_date != null && record.end_date != null && record.end_date < record.start_date' + ``` + + The rejection message says this explicitly, and `error.fields[0].constraint` carries + `{ reason: 'unevaluable', missingKey?, hint?: 'null-comparison' }` for machine handling. + `has()` remains correct for asking whether an **undeclared** key exists. +- **A `conditional` rule now always fetches the prior record on update.** Its `when` is + evaluated against the merged record, so without the prior state it read a PATCH as if it + were the whole record. One extra `findOne` per update on objects that declare one. + +**Unchanged, deliberately:** a broken `regex` (`format`), an uncompilable JSON Schema +(`json_schema`), the field-level `requiredWhen` / `readonlyWhen` / option `visibleWhen` +predicates, and a rule that throws all keep their existing fail-open policy. diff --git a/examples/app-crm/src/objects/opportunity.object.ts b/examples/app-crm/src/objects/opportunity.object.ts index 4281f0413f..b952cf4832 100644 --- a/examples/app-crm/src/objects/opportunity.object.ts +++ b/examples/app-crm/src/objects/opportunity.object.ts @@ -109,7 +109,11 @@ export const Opportunity = ObjectSchema.create({ label: 'Close Date Must Be Future', description: 'Prevent back-dating the close_date of an OPEN opportunity. Closed (won/lost) deals legitimately carry a historical close date, so they are exempt.', fields: ['close_date'], - condition: P`has(record.close_date) && record.close_date < now() && record.stage != "closed_won" && record.stage != "closed_lost"`, + // `!= null`, not `has(...)` (#4649): `has(x)` is TRUE for a declared + // column holding NULL, so the old guard let `null < now()` fault and the + // rule silently did nothing on every opportunity created without a close + // date. + condition: P`record.close_date != null && record.close_date < now() && record.stage != "closed_won" && record.stage != "closed_lost"`, message: 'Close Date must be today or a future date.', events: ['insert'], }, diff --git a/examples/app-showcase/src/data/objects/project.object.ts b/examples/app-showcase/src/data/objects/project.object.ts index 72291538d3..bc42b96936 100644 --- a/examples/app-showcase/src/data/objects/project.object.ts +++ b/examples/app-showcase/src/data/objects/project.object.ts @@ -106,7 +106,12 @@ export const Project = ObjectSchema.create({ label: 'End After Start', description: 'Target end date must be on or after the start date.', fields: ['start_date', 'end_date'], - condition: P`has(record.start_date) && has(record.end_date) && record.end_date < record.start_date`, + // Guarded with `!= null`, NOT `has(...)` (#4649). `has(x)` asks whether + // the key is PRESENT — a declared column holding NULL is present, so + // `has(a) && has(b) && a < b` still faults on `null < null` and the rule + // enforced nothing on any project missing a date. It read as a guard and + // was not one. + condition: P`record.start_date != null && record.end_date != null && record.end_date < record.start_date`, message: 'Target End Date must be on or after the Start Date.', }, { diff --git a/packages/objectql/src/validation/rule-fail-closed.test.ts b/packages/objectql/src/validation/rule-fail-closed.test.ts new file mode 100644 index 0000000000..544d6ce756 --- /dev/null +++ b/packages/objectql/src/validation/rule-fail-closed.test.ts @@ -0,0 +1,398 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4649 — a script validation whose predicate could not be evaluated used to be + * SKIPPED with a WARN, so the rule was declared, listed in the metadata, and + * enforced nothing. For a validation that inverts the guarantee: the rule exists + * to reject a write, and its failure mode shipped the write through. + * + * Two halves, and the tests below pin both because either alone reintroduces a + * trap: + * + * 1. The record a predicate reads is TOTAL over the object's declared fields + * (`null` when neither the payload nor the prior record carries the key), on + * UPDATE as well as insert. Without this, every legitimate predicate 422s on + * any driver that stores only written columns — the shape the HotCRM report + * (hotcrm#630) came from. + * 2. What still cannot be evaluated over that total record — a typo'd key, a + * parse error — REJECTS the write, naming the rule and the offending key. + * Without this, a typo'd predicate is back to silently doing nothing. + */ +import { describe, it, expect, vi } from 'vitest'; +import { evaluateValidationRules } from './rule-validator'; +import { ValidationError } from './record-validator'; + +/** + * The HotCRM shape: a lead may only be closed as a duplicate while naming the + * surviving record. `survivor_lead_id` is DECLARED but the driver stores only + * written columns, so the prior record it hands back does not carry the key. + */ +const leadSchema = { + fields: { + status: { name: 'status', label: 'Status', type: 'text' }, + disqualification_reason: { name: 'disqualification_reason', label: 'Reason', type: 'text' }, + survivor_lead_id: { name: 'survivor_lead_id', label: 'Surviving lead', type: 'text' }, + }, + validations: [ + { + type: 'script' as const, + name: 'duplicate_disqualification_requires_survivor', + condition: { + dialect: 'cel', + source: + 'record.disqualification_reason == "duplicate" && record.survivor_lead_id == null', + }, + message: 'A duplicate must name the surviving lead.', + events: ['insert', 'update'] as Array<'insert' | 'update'>, + }, + ], +}; + +/** What a driver that stores only written columns returns: no `survivor_lead_id`. */ +const priorFromSparseDriver = { id: 'lead_1', status: 'open' }; + +describe('#4649 — the merged record is total on UPDATE', () => { + it('ENFORCES the rule when the prior record omits a declared key entirely', () => { + // Before #4649: `No such key: survivor_lead_id` → rule skipped → write + // allowed. The lead could be closed as a duplicate naming nobody. + expect(() => + evaluateValidationRules( + leadSchema, + { status: 'disqualified', disqualification_reason: 'duplicate' }, + 'update', + { previous: priorFromSparseDriver }, + ), + ).toThrow(/A duplicate must name the surviving lead/); + }); + + it('still allows the write when the payload DOES name the survivor', () => { + expect(() => + evaluateValidationRules( + leadSchema, + { status: 'disqualified', disqualification_reason: 'duplicate', survivor_lead_id: 'lead_9' }, + 'update', + { previous: priorFromSparseDriver }, + ), + ).not.toThrow(); + }); + + it('still allows the write when the survivor is already persisted', () => { + expect(() => + evaluateValidationRules( + leadSchema, + { status: 'disqualified', disqualification_reason: 'duplicate' }, + 'update', + { previous: { ...priorFromSparseDriver, survivor_lead_id: 'lead_9' } }, + ), + ).not.toThrow(); + }); + + it('behaves identically on insert (the two paths no longer diverge)', () => { + expect(() => + evaluateValidationRules( + leadSchema, + { status: 'disqualified', disqualification_reason: 'duplicate' }, + 'insert', + {}, + ), + ).toThrow(/A duplicate must name the surviving lead/); + }); + + it('materialises the `previous` binding too, so `previous.` is readable', () => { + const schema = { + fields: { + stage: { name: 'stage', label: 'Stage', type: 'text' }, + locked: { name: 'locked', label: 'Locked', type: 'boolean' }, + }, + validations: [ + { + type: 'script' as const, + name: 'no_reopen_once_locked', + // `previous.locked` is a declared column the sparse driver omitted. + condition: { dialect: 'cel', source: 'previous.locked == true && record.stage == "open"' }, + message: 'A locked record cannot be reopened.', + }, + ], + }; + // Prior carries neither key → before #4649 this faulted; it must now + // evaluate cleanly to "no violation", NOT reject. + expect(() => + evaluateValidationRules(schema, { stage: 'open' }, 'update', { previous: { id: 'r1' } }), + ).not.toThrow(); + // …and still reject when the prior really is locked. + expect(() => + evaluateValidationRules(schema, { stage: 'open' }, 'update', { previous: { id: 'r1', locked: true } }), + ).toThrow(/locked record cannot be reopened/); + }); + + it('does not mutate the caller\'s prior record (it is the engine\'s hookContext.previous)', () => { + const previous = { ...priorFromSparseDriver }; + try { + evaluateValidationRules( + leadSchema, + { status: 'disqualified', disqualification_reason: 'duplicate' }, + 'update', + { previous }, + ); + } catch { + /* expected */ + } + expect(previous).toEqual(priorFromSparseDriver); + expect('survivor_lead_id' in previous).toBe(false); + }); + + it('materialises DECLARED fields only — an undeclared key is not invented', () => { + const schema = { + fields: { a: { name: 'a', type: 'text' } }, + validations: [ + { + type: 'script' as const, + name: 'reads_undeclared', + condition: { dialect: 'cel', source: 'record.not_a_field == null' }, + message: 'unreachable', + }, + ], + }; + // If materialisation covered arbitrary keys this would evaluate TRUE and + // report the author's message; instead the typo is reported as a typo. + expect(() => evaluateValidationRules(schema, { a: '1' }, 'update', { previous: { a: '0' } })) + .toThrow(/could not be evaluated/); + }); +}); + +describe('#4649 — an unevaluable predicate fails CLOSED', () => { + const typoSchema = { + fields: { status: { name: 'status', label: 'Status', type: 'text' } }, + validations: [ + { + type: 'script' as const, + name: 'status_guard', + condition: { dialect: 'cel', source: 'record.stauts == "bad"' }, // typo + message: 'Status must not be bad.', + }, + ], + }; + + it('rejects the write and names the rule AND the missing key', () => { + let caught: ValidationError | null = null; + try { + evaluateValidationRules(typoSchema, { status: 'ok' }, 'update', { previous: { status: 'ok' } }); + } catch (err) { + caught = err as ValidationError; + } + expect(caught).toBeInstanceOf(ValidationError); + expect(caught!.message).toContain('status_guard'); + expect(caught!.message).toContain('stauts'); + const [field] = caught!.fields; + expect(field.code).toBe('rule_violation'); + expect(field.constraint).toMatchObject({ + rule: 'status_guard', + reason: 'unevaluable', + missingKey: 'stauts', + }); + }); + + it('does not present the author\'s message — the rule never said no', () => { + expect(() => + evaluateValidationRules(typoSchema, { status: 'ok' }, 'update', { previous: { status: 'ok' } }), + ).toThrow(/could not be evaluated/); + expect(() => + evaluateValidationRules(typoSchema, { status: 'ok' }, 'update', { previous: { status: 'ok' } }), + ).not.toThrow(/Status must not be bad/); + }); + + it('still logs the fault — the operator keeps the WARN, worded as a rejection', () => { + const warn = vi.fn(); + expect(() => + evaluateValidationRules(typoSchema, { status: 'ok' }, 'update', { + previous: { status: 'ok' }, + logger: { warn }, + }), + ).toThrow(ValidationError); + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0]![0])).toMatch(/write rejected/); + expect(String(warn.mock.calls[0]![0])).not.toMatch(/— skipped/); + }); + + it('respects severity: a warning-severity broken rule logs and does NOT block', () => { + const warn = vi.fn(); + const advisory = { + ...typoSchema, + validations: [{ ...typoSchema.validations[0]!, severity: 'warning' as const }], + }; + expect(() => + evaluateValidationRules(advisory, { status: 'ok' }, 'update', { + previous: { status: 'ok' }, + logger: { warn }, + }), + ).not.toThrow(); + expect(warn).toHaveBeenCalled(); + }); + + it('rejects a parse-error predicate too, on insert as on update', () => { + const broken = { + fields: { a: { name: 'a', type: 'text' } }, + validations: [ + { type: 'script' as const, name: 'unparseable', condition: 'record.a &&', message: 'm' }, + ], + }; + expect(() => evaluateValidationRules(broken, { a: '1' }, 'insert', {})) + .toThrow(/rule 'unparseable' could not be evaluated/); + }); + + it('keeps the message single-line (the CEL source excerpt stays in the log)', () => { + let msg = ''; + try { + evaluateValidationRules(typoSchema, { status: 'ok' }, 'update', { previous: { status: 'ok' } }); + } catch (err) { + msg = (err as Error).message; + } + expect(msg).not.toContain('\n'); + }); +}); + +describe('#4649 — `has()` semantics over a materialised field, pinned', () => { + /** + * A materialised field is a PRESENT key holding `null`, so `has()` is TRUE — + * CEL's own rule, and what the insert path has always done. The app-side + * mitigation (hotcrm#630 wraps field reads in `has(...)`) must keep working, + * which it does as long as it pairs `has()` with a null test; a bare + * `!has(x)` is an emptiness test that never fires, and this pins that so the + * behaviour cannot drift under the apps relying on it. + */ + const schema = (source: string) => ({ + fields: { + status: { name: 'status', type: 'text' }, + survivor_lead_id: { name: 'survivor_lead_id', type: 'text' }, + }, + validations: [ + { type: 'script' as const, name: 'guarded', condition: { dialect: 'cel', source }, message: 'violated' }, + ], + }); + + const run = (source: string, data: Record, previous?: Record) => + () => evaluateValidationRules(schema(source), data, previous ? 'update' : 'insert', { previous }); + + it('a `has(x) && x == …` predicate evaluates (never faults) and is correct', () => { + // Guarded read over an absent declared key: evaluable, no violation. + expect(run('has(record.survivor_lead_id) && record.survivor_lead_id == "x"', { status: 'a' }, { id: 'r' })) + .not.toThrow(); + // …and fires when the value is really there. + expect(run('has(record.survivor_lead_id) && record.survivor_lead_id == "x"', { survivor_lead_id: 'x' }, { id: 'r' })) + .toThrow(ValidationError); + }); + + it('`has()` is TRUE for a materialised null — identically on insert and update', () => { + // The predicate IS `has(...)`: violated ⇔ has() is true. + expect(run('has(record.survivor_lead_id)', { status: 'a' }, { id: 'r' })).toThrow(ValidationError); + expect(run('has(record.survivor_lead_id)', { status: 'a' })).toThrow(ValidationError); + }); + + it('`has()` is FALSE — has() guards undeclared keys, not empty values', () => { + expect(run('has(record.no_such_column)', { status: 'a' }, { id: 'r' })).not.toThrow(); + }); + + /** + * The trap this pins is the one that cost two example objects in this very + * repo: `has(a) && has(b) && a < b` READS as a null guard and is not one. + * CEL cannot order-compare null, so the predicate aborts — and it aborted on + * every driver that returns its NULL columns, long before #4649. The + * rejection therefore has to teach the fix, not just report the fault. + */ + it('an ordering comparison guarded only by has() is rejected, and the message teaches `!= null`', () => { + let caught: ValidationError | null = null; + try { + evaluateValidationRules( + { + fields: { + start_date: { name: 'start_date', type: 'date' }, + end_date: { name: 'end_date', type: 'date' }, + }, + validations: [{ + type: 'cross_field' as const, + name: 'end_after_start', + fields: ['start_date', 'end_date'], + condition: { + dialect: 'cel', + source: 'has(record.start_date) && has(record.end_date) && record.end_date < record.start_date', + }, + message: 'End must be on or after start.', + }], + }, + { name: 'A project with no dates' }, + 'insert', + {}, + ); + } catch (err) { + caught = err as ValidationError; + } + expect(caught).toBeInstanceOf(ValidationError); + expect(caught!.message).toContain('end_after_start'); + expect(caught!.message).toContain("'!= null'"); + expect(caught!.message).toContain('has(x)'); + expect(caught!.fields[0]!.constraint).toMatchObject({ hint: 'null-comparison' }); + }); + + it('the same rule written with `!= null` evaluates cleanly on the same record', () => { + const schema = { + fields: { + start_date: { name: 'start_date', type: 'date' }, + end_date: { name: 'end_date', type: 'date' }, + }, + validations: [{ + type: 'cross_field' as const, + name: 'end_after_start', + fields: ['start_date', 'end_date'], + condition: { + dialect: 'cel', + source: 'record.start_date != null && record.end_date != null && record.end_date < record.start_date', + }, + message: 'End must be on or after start.', + }], + }; + expect(() => evaluateValidationRules(schema, { name: 'no dates' }, 'insert', {})).not.toThrow(); + expect(() => evaluateValidationRules(schema, { start_date: '2026-06-01', end_date: '2026-01-01' }, 'insert', {})) + .toThrow(/End must be on or after start/); + }); + + it('the emptiness test to write is `== null`, and it works on both paths', () => { + expect(run('record.survivor_lead_id == null', { status: 'a' }, { id: 'r' })).toThrow(ValidationError); + expect(run('record.survivor_lead_id == null', { status: 'a' })).toThrow(ValidationError); + expect(run('record.survivor_lead_id == null', { survivor_lead_id: 'x' }, { id: 'r' })).not.toThrow(); + }); +}); + +describe('#4649 — unchanged neighbours', () => { + it('a broken `regex` on a format rule stays fail-open (out of scope, deliberately)', () => { + const schema = { + fields: { code: { name: 'code', type: 'text' } }, + validations: [ + { type: 'format' as const, name: 'fmt', field: 'code', regex: '([', message: 'bad' }, + ], + }; + expect(() => evaluateValidationRules(schema, { code: 'x' }, 'update', { previous: { code: 'y' } })) + .not.toThrow(); + }); + + it('an uncompilable JSON Schema stays fail-open (out of scope, deliberately)', () => { + const schema = { + fields: { payload: { name: 'payload', type: 'json' } }, + validations: [ + { type: 'json_schema' as const, name: 'js', field: 'payload', schema: { type: 'not-a-type' }, message: 'bad' }, + ], + }; + expect(() => evaluateValidationRules(schema, { payload: { a: 1 } }, 'update', { previous: {} })) + .not.toThrow(); + }); + + it('a broken field-level `requiredWhen` stays fail-open (out of scope, deliberately)', () => { + const schema = { + fields: { + a: { name: 'a', type: 'text', requiredWhen: { dialect: 'cel', source: 'this is (( not valid' } }, + }, + validations: [], + }; + expect(() => evaluateValidationRules(schema, { a: null }, 'update', { previous: { a: null } })) + .not.toThrow(); + }); +}); diff --git a/packages/objectql/src/validation/rule-validator.test.ts b/packages/objectql/src/validation/rule-validator.test.ts index 102454bcc3..c0244aade2 100644 --- a/packages/objectql/src/validation/rule-validator.test.ts +++ b/packages/objectql/src/validation/rule-validator.test.ts @@ -552,7 +552,10 @@ describe('script / cross_field predicates', () => { ).not.toThrow(); }); - it('fails open (no throw) on an un-evaluable predicate', () => { + // #4649 — this used to assert the opposite ("fails open"). A validation that + // could not be checked must not read as a pass; see rule-fail-closed.test.ts + // for the full contract. + it('fails CLOSED on an un-evaluable predicate (#4649)', () => { const schema = { validations: [ { @@ -565,7 +568,7 @@ describe('script / cross_field predicates', () => { }; expect(() => evaluateValidationRules(schema, { a: 1 }, 'update', { previous: { a: 0 } }), - ).not.toThrow(); + ).toThrow(/rule 'broken' could not be evaluated/); }); }); @@ -594,7 +597,7 @@ describe('introspection', () => { expect(needsPriorRecord(undefined)).toBe(false); }); - it('needsPriorRecord recurses into conditional branches', () => { + it('needsPriorRecord is true for any conditional, and recurses into its branches', () => { // conditional wrapping a cross_field → needs prior. const wrapsPrior = { validations: [ @@ -609,7 +612,11 @@ describe('introspection', () => { }; expect(needsPriorRecord(wrapsPrior)).toBe(true); - // conditional wrapping only a format → does not need prior. + // #4649 — a conditional wrapping only a format STILL needs the prior + // record: its `when` is evaluated against the merged record, so without the + // prior state it reads a PATCH as though it were the whole record. This + // assertion was `false` before #4649, which is why a `when` referencing an + // unchanged field used to fault and skip the guard entirely. const wrapsFormat = { validations: [ { @@ -621,7 +628,14 @@ describe('introspection', () => { }, ], }; - expect(needsPriorRecord(wrapsFormat)).toBe(false); + expect(needsPriorRecord(wrapsFormat)).toBe(true); + + // A rule set with no conditional and no prior-dependent rule still needs + // nothing — the fetch is not universal. + const formatOnly = { + validations: [{ type: 'format' as const, name: 'f', message: 'm', field: 'email', format: 'email' }], + }; + expect(needsPriorRecord(formatOnly)).toBe(false); }); }); @@ -821,9 +835,26 @@ describe('conditional enforcement', () => { ).not.toThrow(); }); - it('fails open on an un-evaluable when predicate', () => { + // #4649 — was "fails open". Neither branch ran, so the guard the author + // declared did not happen; that must not read as a pass. + it('fails CLOSED on an un-evaluable when predicate (#4649)', () => { const broken = { validations: [{ ...schema.validations[0], when: { dialect: 'cel', source: 'this is (( not valid' } }] }; - expect(() => evaluateValidationRules(broken, { account_type: 'enterprise', approver: null }, 'insert')).not.toThrow(); + expect(() => evaluateValidationRules(broken, { account_type: 'enterprise', approver: null }, 'insert')) + .toThrow(/could not be evaluated/); + }); + + // The outer conditional's severity still governs an unevaluable `when` — an + // advisory guard stays advisory even when it is broken. + it('an un-evaluable when on a warning-severity conditional does not block', () => { + const broken = { + validations: [{ + ...schema.validations[0], + severity: 'warning' as const, + when: { dialect: 'cel', source: 'this is (( not valid' }, + }], + }; + expect(() => evaluateValidationRules(broken, { account_type: 'enterprise', approver: null }, 'insert')) + .not.toThrow(); }); }); diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index 0d8c7dccd2..fc77b5e74e 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -54,13 +54,53 @@ * - `severity` → only `error` blocks the write. `warning` / `info` * are logged (best-effort) and never throw. * - * ## Fail-open for *broken* rules, fail-closed for *violated* rules + * ## Fail-CLOSED for unevaluable predicates (#4649) * - * A CEL predicate that cannot be evaluated (parse error, references an - * unbound variable, …) is a broken rule, not a violated one — it is logged - * and skipped rather than bricking every write to the object. A predicate - * that evaluates cleanly to "violated", or a transition that is definitively - * illegal, is fail-closed (the write is rejected). + * A validation exists to reject a write, so "the rule could not be checked" + * must never resolve to "the write goes through". Until #4649 a CEL predicate + * that faulted was logged at WARN and **skipped**: the rule stayed declared, + * listed in the metadata, and enforced nothing — on exactly the records whose + * shape triggered the fault. That inverts the guarantee the rule was written + * to give, and the only signal was a line in a log. + * + * Two changes close it, and they are load-bearing together: + * + * 1. **The record a predicate sees is TOTAL** — every field the object + * declares is present, `null` when absent from both the payload and the + * prior record ({@link materializeDeclaredFields}). This already held on + * insert (#1871); #4649 extends it to update and to the `previous` + * binding, so a predicate written against the object's DECLARED shape + * always has a value to read no matter what the driver returned. Without + * it, step 2 would 422 every legitimate predicate on any driver that + * stores only written columns. + * 2. **What still faults is rejected** — a predicate that cannot be evaluated + * over a total record references something the object does not declare (an + * author typo, an unbound variable, a parse error). That is a broken rule, + * and a broken *validation* fails closed: the write is rejected with a + * message naming the rule and the offending key. Without step 1 this alone + * would be unacceptable; without step 2 a typo'd predicate returns to + * silently doing nothing. + * + * `severity` still governs blocking: an unevaluable `warning`/`info` rule is + * logged, never thrown — advisory rules stay advisory. + * + * Deliberately NOT changed here: a broken `regex` (`format`), an uncompilable + * JSON Schema (`json_schema`), the FIELD-level predicates (`requiredWhen` / + * `readonlyWhen` / option `visibleWhen`), and the defensive `catch` around a + * rule that THROWS all keep their existing fail-open policy — #4649 scoped + * itself to the object-level validation predicates it was filed about, and a + * thrown exception is an engine fault the author has no remedy for (rejecting + * on it would brick every write with nothing to fix). Step 1 makes the + * field-level predicates evaluate far more often anyway, since their fault mode + * was the same missing key. + * + * One consequence worth knowing before writing a predicate: because a declared + * field is now always present, `has(record.)` is uniformly TRUE + * (a materialised `null` is a present key holding null — this is CEL's rule, + * and it is what the insert path has always done). `has()` therefore guards + * against an UNDECLARED key, not against an empty value; test emptiness with + * `record.x == null`. Pinned by test so the app-side `has(...)` idiom cannot be + * broken silently from under it. * * ## Prior-record plumbing * @@ -143,7 +183,11 @@ interface ConditionalRule extends BaseRule { */ interface RuleContext { data: Record; + /** Prior state overlaid with the PATCH, made TOTAL over the declared fields + * (#4649) — see {@link materializeDeclaredFields}. */ merged: Record; + /** The prior record, likewise made total (a COPY — the engine's own + * `hookContext.previous` must not gain materialised nulls). */ previous: Record | undefined; mode: Mode; logger: EvaluateRulesOptions['logger']; @@ -408,8 +452,10 @@ function isPreservableUnderAudit(name: string, def: ConditionalFieldDef): boolea /** * A rule needs the prior record if it reasons about the transition or compares * against unchanged fields (`state_machine` / `cross_field` / `script`), or if - * it is a `conditional` whose branches (or `when`) recursively do. `format` and - * `json_schema` only inspect the incoming value, so they never need it. + * it is a `conditional` — its `when` predicate is evaluated against the MERGED + * record, which is only a faithful view of the record with the prior state in + * hand. `format` and `json_schema` only inspect the incoming value, so they + * never need it. */ function ruleNeedsPrior(r: unknown): boolean { if (r == null || typeof r !== 'object') return false; @@ -419,13 +465,48 @@ function ruleNeedsPrior(r: unknown): boolean { } if (type === 'conditional') { const c = r as ConditionalRule; - // `when` is evaluated against the merged record; the branches may need prior - // state. Be conservative and fetch if either branch does. - return ruleNeedsPrior(c.then) || ruleNeedsPrior(c.otherwise); + // #4649 — a `conditional` needs the prior record as soon as it declares a + // `when`, not merely when a BRANCH does. `when` is evaluated against the + // merged record, so without the prior state it reads a PATCH as if it were + // the whole record: a `when` referencing an unchanged field used to fault + // (and skip the rule), and under fail-closed evaluation it would reject + // legitimate partial updates instead. Fetching is what makes the merged + // record total, and totality is what makes fail-closed safe. + return c.when != null || ruleNeedsPrior(c.then) || ruleNeedsPrior(c.otherwise); } return false; } +/** + * Materialise the object's DECLARED-but-absent fields as `null`, in place + * (#1871 for insert, #4649 for update and for the `previous` binding). + * + * CEL is strict about missing keys: `record.x` on a record that does not carry + * the key `x` aborts the whole predicate with `No such key`, which is NOT the + * same as reading `null`. Whether a key is carried is a property of the DRIVER, + * not of the data — a driver that stores only written columns returns a record + * missing every column the write never touched — so without this a predicate's + * evaluability depends on storage internals the author cannot see. + * + * Scope is deliberately the object's **declared fields only**. Materialising + * every key a predicate happens to name would defeat the fail-closed step: a + * typo'd `record.stauts` must stay unevaluable so it is reported, not silently + * read as `null` and quietly answered "no violation". + * + * `undefined` counts as absent (not just a missing key): CEL treats an own key + * holding `undefined` exactly as it treats no key at all. + */ +function materializeDeclaredFields( + record: Record, + fields: Record | undefined, +): Record { + if (!fields) return record; + for (const name of Object.keys(fields)) { + if (record[name] === undefined) record[name] = null; + } + return record; +} + /** Field-level conditional rules (B2): a field is required / read-only when its * CEL predicate is TRUE over the record. */ interface ConditionalFieldOption { @@ -580,20 +661,33 @@ export function evaluateValidationRules( const hasFieldRules = fieldsNeedPrior(fields); if (!hasRules && !hasFieldRules) return; - const previous = opts.previous ?? undefined; + const priorRecord = opts.previous ?? undefined; + // Is the record's persisted state actually in hand? On insert there is + // nothing to know (absence genuinely means "no value"); on update we know it + // only when the engine fetched the prior row. Without it, defaulting a field + // to `null` would not be materialising an absent value — it would be + // FABRICATING one that contradicts the stored row, so we leave the record as + // it is and let the (rare) unevaluable predicate fail closed. `ruleNeedsPrior` + // makes this path unreachable for every rule that reads the merged record. + const groundTruth = mode === 'insert' || priorRecord !== undefined; + // The `previous` CEL binding is made total too (#4649): a predicate reading + // `previous.x` for a declared column the driver did not return would + // otherwise fault — and now that faults are rejections, that would 422 a + // perfectly good rule. Copied, never mutated in place: the same object is the + // engine's `hookContext.previous`, which after-hooks observe. + const previous = mode === 'update' && priorRecord + ? materializeDeclaredFields({ ...priorRecord }, fields) + : priorRecord; // Merged view used by predicate rules: prior state overlaid with the PATCH, // so a rule referencing an unchanged field still sees its persisted value. const merged: Record = { ...(previous ?? {}), ...data }; - // #1871 — on INSERT, a field omitted entirely from the payload is absent from - // the record, so a `record.x == null` predicate sees a missing CEL key (which - // does not equal null) and silently can't match. Default declared-but-absent - // fields to null so an omitted optional reads as null — matching an explicit - // `null` and the UPDATE path (where the prior record already supplies them). - if (mode === 'insert' && fields) { - for (const name of Object.keys(fields)) { - if (!(name in merged)) merged[name] = null; - } - } + // #1871 (insert) / #4649 (update) — a field the payload omits is absent from + // the CEL scope, so `record.x == null` sees a missing key (which does not + // equal null) and aborts the whole predicate. Default declared-but-absent + // fields to null so an omitted optional reads as null, identically on insert + // and update: what a predicate can read is the object's DECLARED shape, not + // whatever subset of columns this driver happened to return. + if (groundTruth) materializeDeclaredFields(merged, fields); const ctx: RuleContext = { data, merged, previous, mode, logger: opts.logger, fields, messages: opts.messages }; const errors: FieldValidationError[] = []; @@ -777,10 +871,88 @@ function checkStateMachine( return null; } +/** `No such key: ` is cel-js's word for "the predicate read something the + * record does not carry" — the single most useful fact to put in front of the + * author, since after materialisation it can only mean an UNDECLARED key. */ +const NO_SUCH_KEY_RE = /No such key:\s*([A-Za-z_$][\w$]*)/; + +/** + * The OTHER way a predicate written against a total record still faults: an + * ordering comparison (`<`, `>`, `<=`, `>=`) or arithmetic over a value that is + * `null`. CEL has no overload for it, so the whole predicate aborts. + * + * This one deserves its own sentence because the obvious guard does not work: + * `has(x)` is TRUE for a declared field holding `null` (CEL asks whether the key + * is PRESENT, not whether it has a usable value), so `has(a) && has(b) && a < b` + * still faults the moment either is null — on any driver that returns its NULL + * columns, which is most of them. Such a rule never enforced anything on those + * rows; #4649 is what makes that visible instead of silent. + */ +const NULL_OVERLOAD_RE = /no such overload/i; + +/** + * One-line summary of a CEL fault. The engine appends a source excerpt and a + * caret line to `message`, which is right for a log and wrong for an API error, + * so only the first line travels. + */ +function faultSummary(error: { kind: string; message: string }): string { + const first = String(error.message ?? '').split('\n')[0]!.trim(); + return `${error.kind}: ${first || 'unknown error'}`; +} + +/** + * The rejection a predicate that CANNOT BE EVALUATED produces (#4649). + * + * Not a violation — the rule never got to say yes or no — but it rejects the + * write all the same, because a validation whose verdict is unknown may not be + * read as "allowed". The message names the rule and, when the fault is a + * missing key, that key plus the two ways to fix it; `constraint` carries the + * same facts machine-readably so a client need not parse prose. + * + * The wire `code` stays `rule_violation`: the field-error catalog (ADR-0114, + * `packages/spec`) is closed and a broken rule is still "a declared rule + * rejected this write" from every consumer's point of view. `constraint.reason` + * is what distinguishes the two for anyone who cares. + */ +function unevaluableRuleError( + ruleName: string, + field: string, + error: { kind: string; message: string }, + what: 'predicate' | 'when-predicate', +): FieldValidationError { + const raw = String(error.message ?? ''); + const summary = faultSummary(error); + const missingKey = NO_SUCH_KEY_RE.exec(raw)?.[1]; + const nullOverload = !missingKey && NULL_OVERLOAD_RE.test(raw) && /null/.test(raw); + let detail = ''; + if (missingKey) { + detail = ` The ${what} reads '${missingKey}', which this object does not declare — fix the rule's condition, or declare the field.`; + } else if (nullOverload) { + detail = + ` The ${what} compares a value that is null. Guard it with '!= null'` + + ` — 'has(x)' does NOT do that: a declared field holding null is still PRESENT, so has(x) is true.`; + } + return { + field, + code: 'rule_violation', + message: + `Validation rule '${ruleName}' could not be evaluated (${summary}) — write rejected.${detail}`, + constraint: { + rule: ruleName, + reason: 'unevaluable', + fault: summary, + ...(missingKey ? { missingKey } : {}), + ...(nullOverload ? { hint: 'null-comparison' } : {}), + }, + }; +} + /** * CEL predicate check (`script` / `cross_field`). The predicate expresses the - * *failure* condition: if it evaluates TRUE the rule is violated. An - * un-evaluable predicate is treated as a broken rule (logged, skipped). + * *failure* condition: if it evaluates TRUE the rule is violated. A predicate + * that cannot be evaluated — over a record already made total for every + * declared field — is a broken rule, and a broken validation is **fail-closed** + * (#4649): it rejects the write rather than waving it through. */ function checkPredicate( rule: PredicateRule, @@ -794,16 +966,20 @@ function checkPredicate( previous: previous ?? undefined, }); + const field = rule.fields?.[0] ?? '_record'; + if (!result.ok) { + // Still logged — the operator needs the fault in the log even though the + // caller now gets it in the response. Note the verb: rejected, not skipped. logger?.warn?.( - `Validation rule '${rule.name}' predicate failed to evaluate (${result.error.kind}: ${result.error.message}) — skipped`, + `Validation rule '${rule.name}' predicate failed to evaluate (${result.error.kind}: ${result.error.message}) — write rejected (#4649)`, ); - return null; + return unevaluableRuleError(rule.name, field, result.error, 'predicate'); } if (result.value === true) { return { - field: rule.fields?.[0] ?? '_record', + field, code: 'rule_violation', message: rule.message, }; @@ -933,9 +1109,12 @@ function checkJsonSchema( /** * Conditional check (`conditional`). Evaluates the `when` predicate against the * merged record, then recurses into `then` (true) or `otherwise` (false) via - * `evaluateRule`. An un-evaluable `when` is a broken rule (logged, fail-open). - * The nested rule supplies the violation (field/code/message); the *outer* - * conditional's `severity` governs whether it blocks (handled by the caller). + * `evaluateRule`. An un-evaluable `when` is **fail-closed** (#4649) for the same + * reason a `script` predicate is: neither branch ran, so the guard the author + * declared did not happen, and a validation that did not happen must not read as + * a pass. The nested rule supplies the violation (field/code/message) when the + * `when` DOES evaluate; the *outer* conditional's `severity` governs whether + * either outcome blocks (handled by the caller). */ function checkConditional( rule: ConditionalRule, @@ -948,9 +1127,9 @@ function checkConditional( if (!result.ok) { ctx.logger?.warn?.( - `Validation rule '${rule.name}' when-predicate failed to evaluate (${result.error.kind}: ${result.error.message}) — skipped`, + `Validation rule '${rule.name}' when-predicate failed to evaluate (${result.error.kind}: ${result.error.message}) — write rejected (#4649)`, ); - return null; + return unevaluableRuleError(rule.name, '_record', result.error, 'when-predicate'); } const branch = result.value === true ? rule.then : rule.otherwise;