Skip to content

Commit 11b7a99

Browse files
committed
feat(objectql)!: fail loud when a hook condition cannot be evaluated (#4775)
A declarative hook whose `condition` could not be evaluated emitted a `logger.warn` and returned `false`, collapsing "the condition said no" and "the platform could not work out what the condition says" into one outcome — which carries opposite risks per hook kind: a `before*` guard swallowed into `false` lets through a write it was declared to stop, and an `after*` audit drops a row nobody will go looking for. Resolve it the way #4649 already resolved it for validation predicates: reject loudly, naming the hook and the key that would not resolve. `before*` and `after*` take the same direction, knowingly. A condition that never compiled aborts too, reported at invocation so one broken hook cannot wedge boot. The fault wording is shared with `rule-validator.ts` through a new `cel-fault.ts`, so two evaluators that reject a write for the same reason cannot describe it in two dialects. Predicate (`multi: true`) bulk writes get a dedicated diagnosis (#4800/B1) rather than a bare `No such key: previous`: the hook is named, the batch is explained, and the route that works is given. It deliberately does not name a record-change flow trigger as a way out — that trigger binds these same lifecycle hooks and receives the same unbound `previous` on a bulk write (verified against the engine and `trigger-record-change`). `onError`, `retryPolicy` and `async` are untouched and stay outside this gate: condition evaluation happens before the handler exists to fail, and routing it through `onError` would let `onError: 'log'` resurrect the silent skip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Br2xsJsczFsTR9bvbh2Ny
1 parent 8bd437f commit 11b7a99

9 files changed

Lines changed: 1041 additions & 80 deletions
Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
---
2+
"@objectstack/objectql": major
3+
---
4+
5+
feat(objectql)!: a hook `condition` the platform cannot evaluate now ABORTS the operation (#4775)
6+
7+
**Breaking.** A declarative hook whose `condition` cannot be evaluated used to
8+
emit a `logger.warn` and `return false` — the hook simply did not fire. Existing
9+
hooks that have been getting by on that silent skip will now **fail the write**.
10+
That is the point of the change, not a side effect: those conditions were never
11+
enforcing anything, and the failure is how you find out.
12+
13+
## What changed
14+
15+
"The condition said no" and "the platform could not work out what the condition
16+
says" used to collapse into one outcome, and that one outcome carries **opposite**
17+
risks depending on the hook:
18+
19+
- a `before*` guard ("hold this write when the condition is met") swallowed into
20+
`false` **lets through** a write it was declared to stop;
21+
- an `after*` audit ("leave a trace when the condition is met") swallowed into
22+
`false` **drops** a row nobody will go looking for, because nobody knows it
23+
should exist.
24+
25+
So an unevaluable condition is `declared ≠ enforced`, and it is now resolved the
26+
way #4649 already resolved it for validation predicates one module over: reject
27+
loudly, naming the hook and the key that would not resolve. The rejection is a
28+
`HookConditionError` (exported), carrying `hook` / `object` / `event` /
29+
`condition` / `reason` / `fault` / `missingKey` machine-readably.
30+
31+
`before*` and `after*` take the **same** direction, knowingly: a typo in an
32+
`afterUpdate` audit condition fails the write it was only watching. One rule, one
33+
answer — the platform does not grow a hidden second rule that makes the failure
34+
direction depend on the event name.
35+
36+
A condition that never **compiled** aborts too. Its old treatment
37+
(`condition ignored`) was the worse half of the swallow: the gate disappeared
38+
entirely, so a declared guard let every write through and an audit fired on all
39+
of them. It is reported at invocation rather than at bind time, so one broken
40+
hook cannot wedge boot for an app nobody is writing to.
41+
42+
## What did NOT change
43+
44+
- A condition that evaluates **FALSE** is still just a skip, and the write still
45+
succeeds. Only *unevaluable* is new.
46+
- `onError` (`abort` / `log`) is untouched and is deliberately **not** in this
47+
path. It governs a handler that threw; the condition gate runs before the
48+
handler is ever reached. Routing a condition fault through it would let
49+
`onError: 'log'` resurrect the exact silent skip this change abolishes, and
50+
would mint a third set of semantics for one word. `retryPolicy` and `async`
51+
are outside it for the same reason.
52+
53+
## Predicate (`multi: true`) bulk writes (#4800)
54+
55+
A bulk write matches N rows and fires the hook **once**, so `previous` is unbound
56+
and `record` is the bare payload — there is no single prior record, and
57+
materialising declared fields to `null` would state something false about all N.
58+
Fail loud takes **no exception** here, but the message is a diagnosis rather than
59+
a riddle: it names the hook, says *this is a predicate bulk write and there is no
60+
single prior record*, and gives the route that works (rewrite without `previous`,
61+
or target the write at one record by id).
62+
63+
It deliberately does **not** offer "use a record-change flow trigger instead":
64+
that trigger subscribes to these same lifecycle hooks, so on a bulk write it
65+
fires once with `previous` undefined too — verified against
66+
`trigger-record-change` and the engine, not assumed. Pointing at it would have
67+
made this very message the next `declared ≠ delivered`.
68+
69+
An **undeclared** key on a bulk write still gets the ordinary typo message — that
70+
one really is a misspelling, and calling it a batch problem would send the author
71+
to fix a field that is spelled correctly.
72+
73+
## Migrating
74+
75+
Run your app and watch for `HookConditionError`. Each one names the hook and the
76+
key. The usual causes, in order of frequency:
77+
78+
- **a misspelled or retired field** — fix the condition, or declare the field;
79+
- **an unguarded `null` comparison** (`record.spent > record.budget`) — guard
80+
with `!= null`. Note `has(x)` does **not** do this: a declared field holding
81+
`null` is still PRESENT, so `has(x)` is `true` and the ordering comparison
82+
still faults;
83+
- **`previous` on a bulk write** — rewrite without `previous`, or write by id;
84+
- **a bare identifier** (`done == true`) — hook conditions are `record`-scoped,
85+
so write `record.done == true`. Flow/automation conditions, which flatten
86+
fields to top level, are a different surface and are unaffected.

packages/objectql/src/cel-fault.ts

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Read a CEL fault the way the AUTHOR needs to read it.
5+
*
6+
* Shared by the two surfaces that evaluate a CEL expression "against the
7+
* record" and reject the write when they cannot: object-level validation
8+
* predicates (`validation/rule-validator.ts`, #4649) and declarative hook
9+
* `condition`s (`hook-wrappers.ts`, #4775). Both were told to reuse ONE error
10+
* shape, and the only durable way to keep two messages worded alike is to stop
11+
* writing them twice — the same argument that put {@link
12+
* ./declared-fields.js#materializeDeclaredFields} in front of both evaluators.
13+
*
14+
* This module has no opinion about what a caller does with a fault. It answers
15+
* three questions and hands back a sentence:
16+
*
17+
* 1. What broke, in one line? (`summary`)
18+
* 2. Did the expression read a key the record does not carry, and which?
19+
* (`missingKey` — after materialisation this can only mean an UNDECLARED
20+
* key, i.e. an author typo or a retired field)
21+
* 3. Did it name a ROOT that is not in scope at all? (`unknownVariable` —
22+
* the fault an unbound `previous` produces, which is a different
23+
* diagnosis from a missing key on a bound root)
24+
* 4. Did it compare/order a `null`? (`nullOverload` — the other way a
25+
* predicate over a TOTAL record still faults)
26+
*/
27+
28+
/** The `{ kind, message }` failure `@objectstack/formula` resolves. */
29+
export interface CelFault {
30+
kind: string;
31+
message: string;
32+
}
33+
34+
/** `No such key: <key>` is cel-js's word for "the expression read something the
35+
* record does not carry" — the single most useful fact to put in front of the
36+
* author, since after materialisation it can only mean an UNDECLARED key. */
37+
const NO_SUCH_KEY_RE = /No such key:\s*([A-Za-z_$][\w$]*)/;
38+
39+
/**
40+
* The OTHER way an expression written against a total record still faults: an
41+
* ordering comparison (`<`, `>`, `<=`, `>=`) or arithmetic over a value that is
42+
* `null`. CEL has no overload for it, so the whole expression aborts.
43+
*
44+
* This one deserves its own sentence because the obvious guard does not work:
45+
* `has(x)` is TRUE for a declared field holding `null` (CEL asks whether the key
46+
* is PRESENT, not whether it has a usable value), so `has(a) && has(b) && a < b`
47+
* still faults the moment either is null — on any driver that returns its NULL
48+
* columns, which is most of them. Such a rule never enforced anything on those
49+
* rows; #4649 is what makes that visible instead of silent.
50+
*/
51+
const NULL_OVERLOAD_RE = /no such overload/i;
52+
53+
/**
54+
* `Unknown variable: <name>` is what cel-js says when a ROOT identifier is not
55+
* in the scope at all — as opposed to `No such key`, which means the root
56+
* resolved and the key under it did not.
57+
*
58+
* Both evaluators bind exactly two roots, `record` and `previous`, and bind
59+
* `previous` only when the record's prior state is actually in hand. So this
60+
* fault has one meaning worth spelling out: the expression asked for a binding
61+
* that this operation does not have. Kept apart from {@link missingKeyOf}
62+
* because the two need OPPOSITE advice — a missing key says "you named a field
63+
* that isn't declared", an unknown variable says "the field is fine; the thing
64+
* you hung it off isn't available here".
65+
*/
66+
const UNKNOWN_VARIABLE_RE = /Unknown variable:\s*([A-Za-z_$][\w$]*)/;
67+
68+
/**
69+
* One-line summary of a CEL fault. The engine appends a source excerpt and a
70+
* caret line to `message`, which is right for a log and wrong for an API error,
71+
* so only the first line travels.
72+
*/
73+
export function faultSummary(error: CelFault): string {
74+
const first = String(error.message ?? '').split('\n')[0]!.trim();
75+
return `${error.kind}: ${first || 'unknown error'}`;
76+
}
77+
78+
/** The key a `No such key: <key>` fault names, or `undefined`. */
79+
export function missingKeyOf(error: CelFault): string | undefined {
80+
return NO_SUCH_KEY_RE.exec(String(error.message ?? ''))?.[1];
81+
}
82+
83+
/** The root identifier an `Unknown variable: <name>` fault names, or `undefined`. */
84+
export function unknownVariableOf(error: CelFault): string | undefined {
85+
return UNKNOWN_VARIABLE_RE.exec(String(error.message ?? ''))?.[1];
86+
}
87+
88+
/** True when the fault is the null-comparison overload fault (and not a
89+
* missing key, which is always the more specific diagnosis). */
90+
export function isNullOverloadFault(error: CelFault): boolean {
91+
const raw = String(error.message ?? '');
92+
return !missingKeyOf(error) && NULL_OVERLOAD_RE.test(raw) && /null/.test(raw);
93+
}
94+
95+
/** What the two surfaces call the thing that faulted, so one helper can write
96+
* both sentences without either side inventing its own phrasing. */
97+
export interface CelFaultSubject {
98+
/** How the expression is named in prose: `'predicate'`, `'condition'`, … */
99+
what: string;
100+
/** How to fix an undeclared key, e.g. `"fix the rule's condition, or declare the field"`. */
101+
undeclaredKeyFix: string;
102+
}
103+
104+
export interface CelFaultDescription {
105+
/** `kind: first line` — safe to put in an API error. */
106+
summary: string;
107+
/** The undeclared key the expression read, when that is the fault. */
108+
missingKey?: string;
109+
/** The unbound ROOT the expression named, when that is the fault. */
110+
unknownVariable?: string;
111+
/** True when the fault is the `null` ordering/arithmetic overload. */
112+
nullOverload: boolean;
113+
/** A trailing sentence explaining the fault, or `''` when we have nothing
114+
* more specific to say than {@link CelFaultDescription.summary}. Starts with
115+
* a leading space so it appends directly onto a message. */
116+
detail: string;
117+
}
118+
119+
/**
120+
* Turn a raw CEL fault into the facts + the sentence an author can act on.
121+
* The wording is deliberately identical across surfaces; only `what` and the
122+
* fix clause differ.
123+
*/
124+
export function describeCelFault(error: CelFault, subject: CelFaultSubject): CelFaultDescription {
125+
const summary = faultSummary(error);
126+
const missingKey = missingKeyOf(error);
127+
const unknownVariable = missingKey ? undefined : unknownVariableOf(error);
128+
const nullOverload = isNullOverloadFault(error);
129+
let detail = '';
130+
if (missingKey) {
131+
detail =
132+
` The ${subject.what} reads '${missingKey}', which this object does not declare` +
133+
` — ${subject.undeclaredKeyFix}.`;
134+
} else if (unknownVariable) {
135+
detail =
136+
` The ${subject.what} reads '${unknownVariable}', which is not bound for this operation` +
137+
` — the scope holds 'record', plus 'previous' only when the record's prior state is in hand.`;
138+
} else if (nullOverload) {
139+
detail =
140+
` The ${subject.what} compares a value that is null. Guard it with '!= null'` +
141+
` — 'has(x)' does NOT do that: a declared field holding null is still PRESENT, so has(x) is true.`;
142+
}
143+
return {
144+
summary,
145+
...(missingKey ? { missingKey } : {}),
146+
...(unknownVariable ? { unknownVariable } : {}),
147+
nullOverload,
148+
detail,
149+
};
150+
}

packages/objectql/src/core.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ export { applyInMemoryAggregation, bucketDateValue } from './in-memory-aggregati
5656
// Hook binder & wrappers (declarative-metadata → engine glue)
5757
export { bindHooksToEngine } from './hook-binder.js';
5858
export type { BindHooksOptions, BindHooksResult } from './hook-binder.js';
59-
export { wrapDeclarativeHook } from './hook-wrappers.js';
59+
export { wrapDeclarativeHook, HookConditionError } from './hook-wrappers.js';
6060
export type { WrapDeclarativeOptions } from './hook-wrappers.js';
6161

6262
// Validation

0 commit comments

Comments
 (0)