fix(objectql): bind previous to null on insert so a rule that reads it answers with its own message

.changeset/23910-validation-previous-insert.md

@@ -0,0 +1,30 @@
+---
+'@objectstack/objectql': minor
+---
+
+Clause-②: yes (widening)
+
+On insert, `previous` is now bound to `null` in validation rules and in declarative hook
+conditions, so a rule or condition that reads it decides the insert — the binding a flow's start
+condition already gets on its create leg. Until now `previous` was unbound on insert: any
+reference to it faulted `Unknown variable: previous`, and the write was refused (validation) or
+aborted (hook) as `unevaluable`. So this widens what an insert can pass: a rule shaped
+`previous != null && …` ("judge updates only") now ADMITS the inserts it used to refuse with that
+fault, and a field's `requiredWhen: previous == null` now requires the field on insert instead of
+refusing every insert. `previous == null || …` judges an insert, `previous != null && …` skips
+one. A column read off `previous` on insert (`previous.stage`) still has no value and still
+refuses the write; the refusal now names that read and the guard to add.
+
+A violating insert under a rule that reads `previous` now answers with the rule's own message.
+The platform's own `validity_window_order` on `sys_user_permission_set` is the measured case: an
+inverted grant window answers `400 VALIDATION_FAILED` with "Valid Until must be later than Valid
+From …", not a CEL fault.
+
+The binding is the same for every predicate a write evaluates — a `script` / `cross_field`
+condition, a `conditional`'s `when`, a field's `requiredWhen` and an option's `visibleWhen` —
+and for a hook's `condition` on `beforeInsert` / `afterInsert`; the validate-only preview judges
+an insert the same way. Only the CEL binding is `null`: a hook's `ctx.previous` is unchanged. No
+shipped rule's verdict changes: every rule and hook this repository ships that reads `previous`
+refused and admitted the same inserts before and after; only the refusal's reason moved. A write
+to an existing row whose prior state is not in hand keeps `previous` unbound and still fails
+closed, so `previous == null` never reads an update as an insert.

content/docs/data-modeling/formulas.mdx

@@ -208,7 +208,7 @@ Keep them pure, dependency-free, and AI-readable.
 | Binding | Source | Available in |
 |:---|:---|:---|
 | `record` | the row being evaluated | formulas, validation, sharing, visibility |
-| `previous` | row before update — on a `multi: true` write, that row's own pre-write state, in `before*` and `after*` hooks alike and in record-change triggers: since ADR-0058 Addendum II both phases dispatch once per matched row | hooks, validation on update |
+| `previous` | the row before the update — on a `multi: true` write, that row's own pre-write state, in `before*` and `after*` hooks alike and in record-change triggers: since ADR-0058 Addendum II both phases dispatch once per matched row. `null` on insert, so `previous == null` judges an insert; a column read off it there (`previous.status`) has no value and faults | validation, hook conditions and flows — `null` on insert |
 | `input` | hook payload | hooks |
 | `current_user` | the authenticated subject — the canonical binding (ADR-0068). `user`, `ctx.user` and `os.user` are aliases of the **same** object | predicates with identity |
 | `os.user` | alias of `current_user` | seed, predicates with identity |

packages/objectql/src/engine-after-hook-abort-unit.test.ts

@@ -179,7 +179,7 @@ describe('#22782 — an aborting after* hook rolls its write back', () => {
  * condition can abort its write too, and the door's unit rolls the row back.
  */
 describe('#22856 — an after* hook carrying a condition rolls its write back when the condition cannot be evaluated', () => {
-  /** `previous` is unbound on an insert, so this condition cannot be evaluated there. */
+  /** `previous` is `null` on an insert, so a column read off it cannot be evaluated there. */
   const unbindable = 'previous.name != record.name';
   /** A key `case` does not declare: unevaluable on every event. */
   const undeclared = 'record.stauts == "x"';

packages/objectql/src/hook-condition-previous-scope.test.ts

@@ -20,14 +20,17 @@
  *   - an UNDECLARED key stays unevaluable, so typos remain reportable;
  *   - copied, never mutated in place — the after-hooks that run next observe
  *     the engine's own `ctx.previous`;
- *   - absent (unbound identifier) whenever the prior state is not in hand:
- *     insert, and a predicate bulk update.
+ *   - `null` on an insert event (#23910): there is no prior row, so
+ *     `previous == null` judges the insert, and a column read off it
+ *     (`previous.done`) still faults loudly;
+ *   - absent (unbound identifier) on any other context whose prior state is
+ *     not in hand — an unknown row is never bound as `null`.
  */
 
 import { describe, it, expect, vi, beforeEach } from 'vitest';
 import { ObjectQL } from './engine.js';
 import { bindHooksToEngine } from './hook-binder.js';
-import { wrapDeclarativeHook } from './hook-wrappers.js';
+import { wrapDeclarativeHook, HookConditionError } from './hook-wrappers.js';
 import type { Hook, HookContext } from '@objectstack/spec/data';
 
 /** `archived` is declared but never written by any test here — the
@@ -150,27 +153,55 @@ describe('[#4784] hook condition binds `previous` alongside `record`', () => {
     expect((ctx.previous as any).archived).toBeUndefined();
   });
 
-  it('leaves `previous` UNBOUND on insert — verbatim the validation side', async () => {
+  it('binds `previous` to NULL on insert — the validation side\'s rule — so `previous == null` judges the insert', async () => {
+    const calls: string[] = [];
+    const { logger, conditionWarnings } = captureLogger();
+    // [#23910] An insert has no prior row, and that is a fact the condition
+    // may read: `previous` is bound to `null` on an insert event, exactly as a
+    // validation rule's scope and a flow's create leg bind it. Until then it
+    // was unbound, so `previous == null` faulted `Unknown variable: previous`
+    // and #4775 aborted the insert.
+    const wrapped = wrapDeclarativeHook(
+      makeHook('previous == null && record.done == true', ['beforeInsert', 'afterInsert']),
+      (async () => { calls.push('ran'); }) as any,
+      { logger },
+    );
+
+    const before = makeCtx({ event: 'beforeInsert', previous: undefined, input: { data: { done: true } } } as any);
+    await wrapped(before);
+    await wrapped(makeCtx({ event: 'afterInsert', previous: undefined, input: { data: { done: true } } } as any));
+    expect(calls).toEqual(['ran', 'ran']);
+    expect(conditionWarnings()).toEqual([]);
+    // Only the CEL binding is null: the engine's own `ctx.previous`, which the
+    // after-hooks observe, is untouched.
+    expect(before.previous).toBeUndefined();
+  });
+
+  it('NEGATIVE — a COLUMN read off the null `previous` on insert still faults loudly, naming the read', async () => {
     const calls: string[] = [];
     const { logger } = captureLogger();
-    // `rule-validator.ts` binds `previous` only for `mode: 'update'` with a
-    // prior record in hand; on insert it passes `undefined`, which omits the
-    // identifier from the CEL scope. Referencing it on an insert event is
-    // therefore an author error, reported — not silently answered.
     const wrapped = wrapDeclarativeHook(
       makeHook(TRANSITION, ['beforeInsert']),
       (async () => { calls.push('ran'); }) as any,
       { logger },
     );
 
-    // #4775: an unbound `previous` is unevaluable, and unevaluable rejects the
-    // insert rather than being swallowed into `false`.
-    await expect(wrapped(makeCtx({
-      event: 'beforeInsert',
-      previous: undefined,
-      input: { data: { done: true } },
-    } as any))).rejects.toThrow(/Unknown variable: previous/);
+    // #4775: unevaluable rejects the insert rather than being swallowed into
+    // `false` — `previous` has no columns on an insert.
+    let err: any = null;
+    try {
+      await wrapped(makeCtx({
+        event: 'beforeInsert',
+        previous: undefined,
+        input: { data: { done: true } },
+      } as any));
+    } catch (e) {
+      err = e;
+    }
 
+    expect(err).toBeInstanceOf(HookConditionError);
+    expect(err.reason).toBe('unevaluable');
+    expect(String(err.message)).toContain("'previous.done'");
     expect(calls).toEqual([]);
   });
 

packages/objectql/src/hook-wrappers.ts

@@ -22,6 +22,7 @@ import { ExpressionEngine } from '@objectstack/formula';
 import { noopHookMetricsRecorder, type HookMetricsRecorder, type HookMetricOutcome } from './hook-metrics.js';
 import { materializeDeclaredFields } from './declared-fields.js';
 import { describeCelFault, type CelFault } from './cel-fault.js';
+import { nullPreviousReadDetail, ruleScope, type PreviousBinding } from './validation/rule-validator.js';
 import { hookRunAs, deriveHookApi, type HookRunAs } from './hook-run-as.js';
 
 /**
@@ -281,8 +282,9 @@ export function declaredHookCondition(meta: Pick<Hook, 'condition'>): (Expressio
  *     the platform can no longer paper over.
  *   - `previous` — the record's pre-write state, built by
  *     {@link pickPreviousPayload}: `ctx.previous`, made total over the same
- *     declared fields. Absent (an unbound CEL identifier) whenever the prior
- *     state is not in hand, exactly as on the validation side.
+ *     declared fields; `null` on an insert event, which has no prior row; and
+ *     absent (an unbound CEL identifier) whenever an existing row's prior
+ *     state is not in hand — exactly as on the validation side (#23910).
  *
  * `previous` is what makes a TRANSITION expressible. Since #4770 `record` is
  * the record's STATE, so `record.done == true` is true on every update of an
@@ -313,18 +315,17 @@ export function wrapDeclarativeHook(
     const check = ExpressionEngine.compile(expr);
     if (check.ok) {
       conditionFn = (ctx: HookContext) => {
-        // `previous` is passed through as-is: `undefined` means the binding
-        // is OMITTED from the CEL scope (see `buildScope` in
-        // @objectstack/formula), which is exactly what the validation side
-        // does when no prior record is in hand. Binding it to an empty
-        // object instead would answer `previous.x == null` with a
-        // fabricated "yes" for a record whose prior state is unknown.
+        // [#23910] The scope is the validation side's, built by the same
+        // `ruleScope`: `previous` is `null` on an insert event, the prior row
+        // on any other write that carries one, and OMITTED from the CEL scope
+        // (`undefined`, see `buildScope` in @objectstack/formula) when an
+        // existing row's prior state is not in hand — never invented.
         const record = pickRecordPayload(ctx);
         const previous = pickPreviousPayload(ctx);
-        const r = ExpressionEngine.evaluate<boolean>(expr, { record: record ?? {}, previous });
+        const r = ExpressionEngine.evaluate<boolean>(expr, ruleScope(record ?? {}, previous));
         if (!r.ok) {
           // [#4775] Fail LOUD. Not `false` — see `HookConditionError`.
-          throw unevaluableConditionError(meta, ctx, source, r.error);
+          throw unevaluableConditionError(meta, ctx, source, r.error, previous);
         }
         return Boolean(r.value);
       };
@@ -991,11 +992,20 @@ function unevaluableConditionError(
   ctx: HookContext,
   source: string,
   error: CelFault,
+  previous?: PreviousBinding,
 ): HookConditionError {
-  const { summary, missingKey, detail } = describeCelFault(error, {
+  const described = describeCelFault(error, {
     what: 'condition',
     undeclaredKeyFix: "fix the hook's condition, or declare the field",
   });
+  const { summary } = described;
+  // [#23910] A column read off an insert's `null` previous gets the validation
+  // side's sentence (`nullPreviousReadDetail`): "this object does not declare"
+  // would be false for a declared column. Like the validation side, a sentence
+  // worded for the fault carries no generic `missingKey`.
+  const nullRead = nullPreviousReadDetail(error, source, previous, declaredFieldsFor(ctx), 'condition');
+  const detail = nullRead ?? described.detail;
+  const missingKey = nullRead === undefined ? described.missingKey : undefined;
   const head = `Hook '${meta.name}' could not evaluate its condition (${summary}) — operation aborted.`;
   return new HookConditionError(`${head}${detail}`, {
     hook: meta.name,
@@ -1182,26 +1192,31 @@ function pickRecordPayload(ctx: HookContext): any {
  * handlers columns the row never had (#4649 left the same note on the
  * validation side).
  *
- * ## When it is ABSENT — verbatim the validation side's rule
+ * ## When it is `null`, and when it is ABSENT — verbatim the validation side's rule
  *
- * `rule-validator.ts` binds `previous` only for `mode: 'update'` with a prior
- * record actually fetched, and passes `undefined` otherwise, which omits the
- * identifier from the CEL scope. Same here:
- *   - **insert** — there is no prior state, so `previous` is unbound and any
- *     reference to it is an author error, reported as such;
- *   - **any update-shaped context whose prior row is not in hand** — with no
- *     row there is no single prior record to bind, so `previous` stays unbound
- *     rather than being invented. A predicate (`multi: true`) bulk write is no
- *     longer one of these: since #5574 (ADR-0058 Addendum II, D1/D2) it
- *     dispatches BOTH phases per matched row — `after*` since #5038 — each
- *     context binding that row's own pre-image, so a transition condition
- *     reads exactly as it does on a single-record write.
- * Binding `null`/`{}` instead would make `previous.x == null` answer "yes"
- * for a record whose prior state is simply unknown — a fabricated fact, the
- * one thing materialisation is careful never to do.
+ * `rule-validator.ts` decides the binding once per evaluation
+ * (`previousBinding`) and both surfaces build their scope with the same
+ * `ruleScope` (#23910). Same rule here:
+ *   - **insert** (`beforeInsert` / `afterInsert`) — `null`. There is no prior
+ *     row, and that is a FACT a condition may read: `previous == null` is how
+ *     a condition says "this write creates the row", the binding a flow's
+ *     create leg and a validation rule's insert already get. A COLUMN read off
+ *     it (`previous.done`) still has no value, faults, and aborts the
+ *     operation (#4775), with a sentence naming the read and the guard to add.
+ *     Only the CEL binding is `null`: `ctx.previous`, the engine's pre-image,
+ *     is not touched.
+ *   - **any other context whose prior row is not in hand** — unbound. The row
+ *     exists and its state is unknown, so `previous` stays unbound rather than
+ *     being invented: `null` there would answer `previous == null` with "this
+ *     is an insert", a fabricated fact, the one thing materialisation is
+ *     careful never to do. A predicate (`multi: true`) bulk write is no longer
+ *     one of these: since #5574 (ADR-0058 Addendum II, D1/D2) it dispatches
+ *     BOTH phases per matched row — `after*` since #5038 — each context
+ *     binding that row's own pre-image, so a transition condition reads
+ *     exactly as it does on a single-record write.
  */
-function pickPreviousPayload(ctx: HookContext): Record<string, unknown> | undefined {
-  if (isInsertEvent(ctx.event)) return undefined;
+function pickPreviousPayload(ctx: HookContext): PreviousBinding {
+  if (isInsertEvent(ctx.event)) return null;
   const prior = ctx.previous;
   if (!prior || typeof prior !== 'object' || Array.isArray(prior)) return undefined;
   return materializeDeclaredFields({ ...(prior as Record<string, unknown>) }, declaredFieldsFor(ctx));