feat(objectql)!: hook 的 condition 求不出值时 fail loud —— 抛错并中断该次操作 (#4775)

.changeset/hook-condition-fail-loud.md

@@ -0,0 +1,86 @@
+---
+"@objectstack/objectql": major
+---
+
+feat(objectql)!: a hook `condition` the platform cannot evaluate now ABORTS the operation (#4775)
+
+**Breaking.** A declarative hook whose `condition` cannot be evaluated used to
+emit a `logger.warn` and `return false` — the hook simply did not fire. Existing
+hooks that have been getting by on that silent skip will now **fail the write**.
+That is the point of the change, not a side effect: those conditions were never
+enforcing anything, and the failure is how you find out.
+
+## What changed
+
+"The condition said no" and "the platform could not work out what the condition
+says" used to collapse into one outcome, and that one outcome carries **opposite**
+risks depending on the hook:
+
+- a `before*` guard ("hold this write when the condition is met") swallowed into
+  `false` **lets through** a write it was declared to stop;
+- an `after*` audit ("leave a trace when the condition is met") swallowed into
+  `false` **drops** a row nobody will go looking for, because nobody knows it
+  should exist.
+
+So an unevaluable condition is `declared ≠ enforced`, and it is now resolved the
+way #4649 already resolved it for validation predicates one module over: reject
+loudly, naming the hook and the key that would not resolve. The rejection is a
+`HookConditionError` (exported), carrying `hook` / `object` / `event` /
+`condition` / `reason` / `fault` / `missingKey` machine-readably.
+
+`before*` and `after*` take the **same** direction, knowingly: a typo in an
+`afterUpdate` audit condition fails the write it was only watching. One rule, one
+answer — the platform does not grow a hidden second rule that makes the failure
+direction depend on the event name.
+
+A condition that never **compiled** aborts too. Its old treatment
+(`condition ignored`) was the worse half of the swallow: the gate disappeared
+entirely, so a declared guard let every write through and an audit fired on all
+of them. It is reported at invocation rather than at bind time, so one broken
+hook cannot wedge boot for an app nobody is writing to.
+
+## What did NOT change
+
+- A condition that evaluates **FALSE** is still just a skip, and the write still
+  succeeds. Only *unevaluable* is new.
+- `onError` (`abort` / `log`) is untouched and is deliberately **not** in this
+  path. It governs a handler that threw; the condition gate runs before the
+  handler is ever reached. Routing a condition fault through it would let
+  `onError: 'log'` resurrect the exact silent skip this change abolishes, and
+  would mint a third set of semantics for one word. `retryPolicy` and `async`
+  are outside it for the same reason.
+
+## Predicate (`multi: true`) bulk writes (#4800)
+
+A bulk write matches N rows and fires the hook **once**, so `previous` is unbound
+and `record` is the bare payload — there is no single prior record, and
+materialising declared fields to `null` would state something false about all N.
+Fail loud takes **no exception** here, but the message is a diagnosis rather than
+a riddle: it names the hook, says *this is a predicate bulk write and there is no
+single prior record*, and gives the route that works (rewrite without `previous`,
+or target the write at one record by id).
+
+It deliberately does **not** offer "use a record-change flow trigger instead":
+that trigger subscribes to these same lifecycle hooks, so on a bulk write it
+fires once with `previous` undefined too — verified against
+`trigger-record-change` and the engine, not assumed. Pointing at it would have
+made this very message the next `declared ≠ delivered`.
+
+An **undeclared** key on a bulk write still gets the ordinary typo message — that
+one really is a misspelling, and calling it a batch problem would send the author
+to fix a field that is spelled correctly.
+
+## Migrating
+
+Run your app and watch for `HookConditionError`. Each one names the hook and the
+key. The usual causes, in order of frequency:
+
+- **a misspelled or retired field** — fix the condition, or declare the field;
+- **an unguarded `null` comparison** (`record.spent > record.budget`) — guard
+  with `!= null`. Note `has(x)` does **not** do this: a declared field holding
+  `null` is still PRESENT, so `has(x)` is `true` and the ordering comparison
+  still faults;
+- **`previous` on a bulk write** — rewrite without `previous`, or write by id;
+- **a bare identifier** (`done == true`) — hook conditions are `record`-scoped,
+  so write `record.done == true`. Flow/automation conditions, which flatten
+  fields to top level, are a different surface and are unaffected.

packages/objectql/src/cel-fault.ts

@@ -0,0 +1,150 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * Read a CEL fault the way the AUTHOR needs to read it.
+ *
+ * Shared by the two surfaces that evaluate a CEL expression "against the
+ * record" and reject the write when they cannot: object-level validation
+ * predicates (`validation/rule-validator.ts`, #4649) and declarative hook
+ * `condition`s (`hook-wrappers.ts`, #4775). Both were told to reuse ONE error
+ * shape, and the only durable way to keep two messages worded alike is to stop
+ * writing them twice — the same argument that put {@link
+ * ./declared-fields.js#materializeDeclaredFields} in front of both evaluators.
+ *
+ * This module has no opinion about what a caller does with a fault. It answers
+ * three questions and hands back a sentence:
+ *
+ *   1. What broke, in one line? (`summary`)
+ *   2. Did the expression read a key the record does not carry, and which?
+ *      (`missingKey` — after materialisation this can only mean an UNDECLARED
+ *      key, i.e. an author typo or a retired field)
+ *   3. Did it name a ROOT that is not in scope at all? (`unknownVariable` —
+ *      the fault an unbound `previous` produces, which is a different
+ *      diagnosis from a missing key on a bound root)
+ *   4. Did it compare/order a `null`? (`nullOverload` — the other way a
+ *      predicate over a TOTAL record still faults)
+ */
+
+/** The `{ kind, message }` failure `@objectstack/formula` resolves. */
+export interface CelFault {
+  kind: string;
+  message: string;
+}
+
+/** `No such key: <key>` is cel-js's word for "the expression 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 an expression 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 expression 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;
+
+/**
+ * `Unknown variable: <name>` is what cel-js says when a ROOT identifier is not
+ * in the scope at all — as opposed to `No such key`, which means the root
+ * resolved and the key under it did not.
+ *
+ * Both evaluators bind exactly two roots, `record` and `previous`, and bind
+ * `previous` only when the record's prior state is actually in hand. So this
+ * fault has one meaning worth spelling out: the expression asked for a binding
+ * that this operation does not have. Kept apart from {@link missingKeyOf}
+ * because the two need OPPOSITE advice — a missing key says "you named a field
+ * that isn't declared", an unknown variable says "the field is fine; the thing
+ * you hung it off isn't available here".
+ */
+const UNKNOWN_VARIABLE_RE = /Unknown variable:\s*([A-Za-z_$][\w$]*)/;
+
+/**
+ * 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.
+ */
+export function faultSummary(error: CelFault): string {
+  const first = String(error.message ?? '').split('\n')[0]!.trim();
+  return `${error.kind}: ${first || 'unknown error'}`;
+}
+
+/** The key a `No such key: <key>` fault names, or `undefined`. */
+export function missingKeyOf(error: CelFault): string | undefined {
+  return NO_SUCH_KEY_RE.exec(String(error.message ?? ''))?.[1];
+}
+
+/** The root identifier an `Unknown variable: <name>` fault names, or `undefined`. */
+export function unknownVariableOf(error: CelFault): string | undefined {
+  return UNKNOWN_VARIABLE_RE.exec(String(error.message ?? ''))?.[1];
+}
+
+/** True when the fault is the null-comparison overload fault (and not a
+ *  missing key, which is always the more specific diagnosis). */
+export function isNullOverloadFault(error: CelFault): boolean {
+  const raw = String(error.message ?? '');
+  return !missingKeyOf(error) && NULL_OVERLOAD_RE.test(raw) && /null/.test(raw);
+}
+
+/** What the two surfaces call the thing that faulted, so one helper can write
+ *  both sentences without either side inventing its own phrasing. */
+export interface CelFaultSubject {
+  /** How the expression is named in prose: `'predicate'`, `'condition'`, … */
+  what: string;
+  /** How to fix an undeclared key, e.g. `"fix the rule's condition, or declare the field"`. */
+  undeclaredKeyFix: string;
+}
+
+export interface CelFaultDescription {
+  /** `kind: first line` — safe to put in an API error. */
+  summary: string;
+  /** The undeclared key the expression read, when that is the fault. */
+  missingKey?: string;
+  /** The unbound ROOT the expression named, when that is the fault. */
+  unknownVariable?: string;
+  /** True when the fault is the `null` ordering/arithmetic overload. */
+  nullOverload: boolean;
+  /** A trailing sentence explaining the fault, or `''` when we have nothing
+   *  more specific to say than {@link CelFaultDescription.summary}. Starts with
+   *  a leading space so it appends directly onto a message. */
+  detail: string;
+}
+
+/**
+ * Turn a raw CEL fault into the facts + the sentence an author can act on.
+ * The wording is deliberately identical across surfaces; only `what` and the
+ * fix clause differ.
+ */
+export function describeCelFault(error: CelFault, subject: CelFaultSubject): CelFaultDescription {
+  const summary = faultSummary(error);
+  const missingKey = missingKeyOf(error);
+  const unknownVariable = missingKey ? undefined : unknownVariableOf(error);
+  const nullOverload = isNullOverloadFault(error);
+  let detail = '';
+  if (missingKey) {
+    detail =
+      ` The ${subject.what} reads '${missingKey}', which this object does not declare` +
+      ` — ${subject.undeclaredKeyFix}.`;
+  } else if (unknownVariable) {
+    detail =
+      ` The ${subject.what} reads '${unknownVariable}', which is not bound for this operation` +
+      ` — the scope holds 'record', plus 'previous' only when the record's prior state is in hand.`;
+  } else if (nullOverload) {
+    detail =
+      ` The ${subject.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 {
+    summary,
+    ...(missingKey ? { missingKey } : {}),
+    ...(unknownVariable ? { unknownVariable } : {}),
+    nullOverload,
+    detail,
+  };
+}

packages/objectql/src/core.ts

@@ -56,7 +56,7 @@ export { applyInMemoryAggregation, bucketDateValue } from './in-memory-aggregati
 // Hook binder & wrappers (declarative-metadata → engine glue)
 export { bindHooksToEngine } from './hook-binder.js';
 export type { BindHooksOptions, BindHooksResult } from './hook-binder.js';
-export { wrapDeclarativeHook } from './hook-wrappers.js';
+export { wrapDeclarativeHook, HookConditionError } from './hook-wrappers.js';
 export type { WrapDeclarativeOptions } from './hook-wrappers.js';
 
 // Validation

packages/objectql/src/hook-binder.test.ts

@@ -3,7 +3,7 @@
 import { describe, it, expect, vi } 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';
 
 function makeEngine() {
@@ -377,21 +377,33 @@ describe('wrapDeclarativeHook', () => {
     expect(calls).toEqual(['done']); // awaited despite async=true
   });
 
-  it('logs and treats invalid condition formulas as skipping', async () => {
-    const warn = vi.fn();
+  it('rejects the operation when the condition formula does not compile', async () => {
+    // [#4775] This test used to accept EITHER outcome ("ignored at compile time
+    // (handler runs) or evaluated false (skipped) … just assert we didn't
+    // crash"). That latitude was the defect: "condition ignored" DELETED the
+    // gate, so a hook declared to run conditionally ran on every write, and the
+    // only trace was a `warn`. A condition that cannot compile can never be
+    // evaluated, so the hook can neither run nor be skipped honestly — the
+    // operation is rejected instead, naming the hook.
+    const error = vi.fn();
     const calls: string[] = [];
     const meta: Hook = {
       name: 'badcond', object: 'a', events: ['beforeInsert'], priority: 100,
       condition: '(((not valid syntax',
       handler: () => { calls.push('ran'); },
     };
     const wrapped = wrapDeclarativeHook(meta, meta.handler as any, {
-      logger: { debug: () => {}, info: () => {}, warn, error: () => {} },
+      logger: { debug: () => {}, info: () => {}, warn: () => {}, error },
     });
-    await wrapped(makeCtx());
-    expect(warn).toHaveBeenCalled();
-    // Either ignored at compile time (handler runs) or evaluated false
-    // (skipped). Both are valid; just assert we didn't crash.
-    expect(calls.length === 0 || calls[0] === 'ran').toBe(true);
+
+    const err = await wrapped(makeCtx()).then(() => null, (e) => e);
+
+    expect(err).toBeInstanceOf(HookConditionError);
+    expect(err.reason).toBe('uncompilable');
+    expect(err.message).toContain("Hook 'badcond'");
+    expect(calls).toEqual([]);
+    // Reported at bind time too, at error level — an operator sees the broken
+    // hook before the first write trips over it.
+    expect(error).toHaveBeenCalled();
   });
 });