diff --git a/.changeset/hook-condition-previous-binding.md b/.changeset/hook-condition-previous-binding.md new file mode 100644 index 0000000000..0a73a374be --- /dev/null +++ b/.changeset/hook-condition-previous-binding.md @@ -0,0 +1,55 @@ +--- +'@objectstack/objectql': minor +--- + +**A declarative hook `condition` can now express a TRANSITION: the CEL scope binds `previous` alongside `record` (#4784).** + +The condition gate evaluated against a single root — `{ record }`. Both published skill +docs, however, taught the `previous` form: `objectstack-formula` §5 ("Update hook +condition — `previous` vs `record`") gives +`P\`previous.status != 'escalated' && record.status == 'escalated'\``, and its legacy +migration table maps `OLD.x` → `previous.x` and `ISCHANGED(x)` → `previous.x != record.x`. +Written into a hook, any of those aborted the expression with `No such key: previous`, +which the gate swallowed into `false` — the hook simply never ran, leaving one WARN line. +Declared ≠ delivered. + +It became load-bearing with #4770. `record` now means the record's **state** (stored ⊕ +payload), so `record.done == true` is true on *every* update of an already-done row — not +only the one that completed it. `showcase_audit_task_completion`'s own description says +"after a task transitions to done", and there was no way to write that. Now there is: + +```ts +condition: P`previous.done != true && record.done == true` +``` + +`previous` is built exactly as the validation side builds it (#4649), through the shared +`materializeDeclaredFields` helper, so one CEL expression means one thing on both +surfaces: + +- **the stored pre-write row**, made **total over the object's DECLARED fields** — a + column the driver never returned reads as `null` instead of aborting the expression; +- **declared fields only** — `previous.dnoe` stays unevaluable, so a typo is still + reported rather than quietly answered; +- **copied, never mutated in place.** `ctx.previous` is the engine's own pre-image object, + observed by every after-hook; the materialised `null`s do not leak into it. + +**Where `previous` is NOT bound** — verbatim the rule `validation/rule-validator.ts` +already applies, so referencing it there makes the condition unevaluable: + +- **insert events** (`beforeInsert` / `afterInsert`) — there is no prior state. Write + insert conditions over `record` alone. +- **predicate (`multi: true`) bulk updates** — one write matches N rows and the hook fires + once, so there is no single prior record. Binding `{}` or `null` would answer + `previous.x == null` with a fabricated fact about rows nobody read. + +**Cost: none.** No new demand-driven fetch was introduced. `previous` rides on the prior +row `engine.update` already reads whenever an afterUpdate hook is registered — the same +one that feeds `ctx.previous` and record-change flow triggers. A condition that never +mentions `previous` reads nothing extra, pinned by test. + +**What you may see after upgrading:** hooks whose condition referenced `previous` never +fired before and start firing now. That is the declaration finally being honoured — review +any hook carrying a `previous.*` condition before you upgrade. + +**Unchanged, deliberately:** a condition that is *still* unevaluable is logged at WARN and +treated as `false`. Whether that should fail loudly instead is tracked separately. diff --git a/examples/app-showcase/src/data/hooks/index.ts b/examples/app-showcase/src/data/hooks/index.ts index fa289a1b96..7905a07e34 100644 --- a/examples/app-showcase/src/data/hooks/index.ts +++ b/examples/app-showcase/src/data/hooks/index.ts @@ -38,13 +38,20 @@ export const NormalizeTaskTitleHook = { description: 'Trims leading/trailing whitespace from the task title before every write.', }; -/** afterUpdate (gated) — log a line whenever a task flips to done. */ +/** + * afterUpdate (gated) — log a line on the update that flips a task to done. + * + * The condition compares against `previous` on purpose (#4784). Since #4770 + * `record` means the record's STATE, so `record.done == true` alone would audit + * every later edit of an already-done task — while this hook's own description + * says "transitions to done". The transition is the two-root form. + */ export const AuditTaskCompletionHook = { name: 'showcase_audit_task_completion', label: 'Audit Task Completion', object: 'showcase_task', events: ['afterUpdate'] as LifecycleEvent[], - condition: "record.done == true", + condition: "previous.done != true && record.done == true", body: { language: 'js' as const, source: "var r = ctx.result || ctx.input || {}; ctx.log.info('task completed: ' + (r.title || r.id || 'unknown'));", diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 3250ba6d0f..dc146fdd6e 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -4495,6 +4495,18 @@ export class ObjectQL implements IObjectQLEngine { // record-change flow triggers work: their start-condition gate reads // `previous.*` (e.g. `status == "done" && previous.status != "done"`), // which silently fails when `previous` is absent. + // + // [#4784] It is ALSO what supplies the `previous` binding to a + // declarative hook `condition` (`hook-wrappers.ts`), which is how a + // TRANSITION is expressed there: `previous.done != true && + // record.done == true`. Note this needs NO second demand-driven + // fetch — the existing gate already fetches whenever an afterUpdate + // hook exists, and afterUpdate is the event whose context carries + // `previous`. Deliberately: adding a "does the condition reference + // `previous`?" analysis on top would be dead code today. If this + // gate is ever NARROWED (e.g. scoped per object), hook conditions + // reading `previous` must be counted into the new demand test — + // pinned by `hook-condition-previous-scope.test.ts`. let priorRecord: Record | null = null; const updateSchema = this._registry.getObject(object); const mediaValueShapeStrict = await this.mediaValueShapeStrictFor(updateSchema); diff --git a/packages/objectql/src/hook-condition-previous-scope.test.ts b/packages/objectql/src/hook-condition-previous-scope.test.ts new file mode 100644 index 0000000000..d6adac38f1 --- /dev/null +++ b/packages/objectql/src/hook-condition-previous-scope.test.ts @@ -0,0 +1,477 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4784] A declarative hook `condition` evaluates against `record` AND + * `previous` — the same two bindings a validation predicate reads. + * + * Before this fix the scope carried a single root (`{ record }`), so the + * `previous.*` form BOTH published skill docs teach — `skills/objectstack-formula` + * §5 and its `ISCHANGED(x)` → `previous.x != record.x` migration row — faulted + * with `No such key: previous` and was swallowed into `false`. + * + * It matters more since #4770: `record` now means the record's STATE, so + * `record.done == true` is true on EVERY update of an already-done row. The + * transition ("just became done"), which is what an audit hook actually means, + * is expressible only by comparing against `previous`. + * + * Semantics copied verbatim from `validation/rule-validator.ts`: + * - total over the object's DECLARED fields (shared `materializeDeclaredFields`), + * so a column the driver never returned reads as `null` rather than faulting; + * - 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. + */ + +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 type { Hook, HookContext } from '@objectstack/spec/data'; + +/** `archived` is declared but never written by any test here — the + * declared-but-absent column materialisation exists for. */ +const TASK_FIELDS = { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + status: { name: 'status', label: 'Status', type: 'text' as const }, + done: { name: 'done', label: 'Done', type: 'boolean' as const }, + archived: { name: 'archived', label: 'Archived', type: 'boolean' as const }, +}; + +const taskObject = { name: 'hook_task', label: 'Task', fields: TASK_FIELDS }; + +const qlStub = { + getObject: (name: string) => (name === 'hook_task' ? taskObject : undefined), +}; + +function makeCtx(overrides: Partial = {}): HookContext { + return { + object: 'hook_task', + event: 'afterUpdate', + input: { id: 't1', data: { done: true } }, + previous: { id: 't1', title: 'Ship it', status: 'todo', done: false }, + ql: qlStub, + ...overrides, + } as unknown as HookContext; +} + +function makeHook(condition: string, events: string[] = ['afterUpdate']): Hook { + return { + name: 'audit', object: 'hook_task', events, priority: 100, + condition, + handler: () => {}, + } as unknown as Hook; +} + +function captureLogger() { + const warn = vi.fn(); + return { + warn, + logger: { debug: () => {}, info: () => {}, warn, error: () => {} }, + conditionWarnings: () => + warn.mock.calls.filter(([msg]) => String(msg).includes('condition evaluation failed')), + }; +} + +/** The transition form the skill docs teach, and #4784's headline shape. */ +const TRANSITION = 'previous.done != true && record.done == true'; + +describe('[#4784] hook condition binds `previous` alongside `record`', () => { + it('fires on the update that FLIPS the field, not on later updates of a done record', async () => { + const calls: string[] = []; + const { logger, conditionWarnings } = captureLogger(); + const wrapped = wrapDeclarativeHook( + makeHook(TRANSITION), + (async () => { calls.push('ran'); }) as any, + { logger }, + ); + + // The flip: stored done=false, this write sets done=true. + await wrapped(makeCtx()); + expect(calls).toEqual(['ran']); + + // A later, unrelated update of the (already done) record. + await wrapped(makeCtx({ + previous: { id: 't1', title: 'Ship it', status: 'in_progress', done: true }, + input: { id: 't1', data: { status: 'review' } }, + } as any)); + expect(calls).toEqual(['ran']); + + expect(conditionWarnings()).toEqual([]); + }); + + it('is TOTAL over declared fields — a column the driver never returned reads as null', async () => { + const calls: string[] = []; + const { logger, conditionWarnings } = captureLogger(); + // `archived` is declared; this driver's prior row simply has no such key. + // Guarded with `!= null`, never `has()` — a materialised null is a PRESENT + // key, so `has(previous.archived)` is uniformly true (#4649/#4770). + const wrapped = wrapDeclarativeHook( + makeHook('previous.archived == null && record.done == true'), + (async () => { calls.push('ran'); }) as any, + { logger }, + ); + + await wrapped(makeCtx()); + + expect(calls).toEqual(['ran']); + expect(conditionWarnings()).toEqual([]); + }); + + it('an UNDECLARED key on `previous` stays unevaluable — typos stay reportable', async () => { + const calls: string[] = []; + const { logger, conditionWarnings } = captureLogger(); + const wrapped = wrapDeclarativeHook( + makeHook('previous.dnoe != true'), + (async () => { calls.push('ran'); }) as any, + { logger }, + ); + + await wrapped(makeCtx()); + + expect(calls).toEqual([]); + const warns = conditionWarnings(); + expect(warns).toHaveLength(1); + expect(String(warns[0]![1]?.error)).toMatch(/No such key: dnoe/); + }); + + it('never leaks materialised nulls back into the engine\'s ctx.previous', async () => { + const calls: string[] = []; + const wrapped = wrapDeclarativeHook( + makeHook('previous.archived == null'), + (async () => { calls.push('ran'); }) as any, + ); + const ctx = makeCtx(); + await wrapped(ctx); + + expect(calls).toEqual(['ran']); + // The after-hooks that run next observe THIS object. A fabricated + // `archived: null` here would be a column the row never had. + expect(Object.keys(ctx.previous as object).sort()).toEqual(['done', 'id', 'status', 'title']); + expect((ctx.previous as any).archived).toBeUndefined(); + }); + + it('leaves `previous` UNBOUND on insert — verbatim the validation side', async () => { + const calls: string[] = []; + const { logger, conditionWarnings } = 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 }, + ); + + await wrapped(makeCtx({ + event: 'beforeInsert', + previous: undefined, + input: { data: { done: true } }, + } as any)); + + expect(calls).toEqual([]); + expect(conditionWarnings()).toHaveLength(1); + }); + + it('a `record`-only condition on insert is unaffected by the added binding', async () => { + const calls: string[] = []; + const { logger, conditionWarnings } = captureLogger(); + const wrapped = wrapDeclarativeHook( + makeHook('record.done == true', ['beforeInsert']), + (async () => { calls.push('ran'); }) as any, + { logger }, + ); + + await wrapped(makeCtx({ + event: 'beforeInsert', + previous: undefined, + input: { data: { done: true } }, + } as any)); + + expect(calls).toEqual(['ran']); + expect(conditionWarnings()).toEqual([]); + }); + + it('fabricates nothing on a predicate bulk update — `previous` stays unbound', async () => { + const calls: string[] = []; + const { logger } = captureLogger(); + // A `multi: true` update matched N rows and fires the hook ONCE; there is + // no single prior record. Binding `{}` or `null` would make + // `previous.done != true` answer for rows nobody read. + const wrapped = wrapDeclarativeHook( + makeHook(TRANSITION), + (async () => { calls.push('ran'); }) as any, + { logger }, + ); + + await wrapped(makeCtx({ previous: undefined, input: { data: { done: true } } } as any)); + + expect(calls).toEqual([]); + }); + + it('a delete-shaped context evaluates `previous` against the pre-image', async () => { + const calls: string[] = []; + const { logger, conditionWarnings } = captureLogger(); + const wrapped = wrapDeclarativeHook( + makeHook('previous.done != true', ['beforeDelete']), + (async () => { calls.push('ran'); }) as any, + { logger }, + ); + + await wrapped(makeCtx({ event: 'beforeDelete', input: { id: 't1', options: {} } } as any)); + + expect(calls).toEqual(['ran']); + expect(conditionWarnings()).toEqual([]); + }); + + it('a context with no engine still binds `previous` (merge only, no materialisation)', async () => { + const calls: string[] = []; + const { logger, conditionWarnings } = captureLogger(); + const wrapped = wrapDeclarativeHook( + makeHook(TRANSITION), + (async () => { calls.push('ran'); }) as any, + { logger }, + ); + + await wrapped(makeCtx({ ql: undefined } as any)); + + expect(calls).toEqual(['ran']); + expect(conditionWarnings()).toEqual([]); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * Through a REAL engine, over an in-memory driver that — like a SQL driver — + * stores only the columns a write actually touched. + * ──────────────────────────────────────────────────────────────────────────── */ + +function makeMemoryDriver() { + const stores = new Map>>(); + /** Every read the engine performs, so "no extra fetch" is measurable. */ + const reads = { findOne: 0, find: 0 }; + const storeFor = (obj: string) => { + let s = stores.get(obj); + if (!s) { s = new Map(); stores.set(obj, s); } + return s; + }; + let nextId = 0; + const matchesWhere = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) continue; + const expected = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + if ((row[k] ?? null) !== (expected ?? null)) return false; + } + return true; + }; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {} as any, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find(object: string, ast: any) { + reads.find += 1; + return Array.from(storeFor(object).values()).filter((r) => matchesWhere(r, ast?.where)); + }, + async findOne(object: string, ast: any) { + reads.findOne += 1; + for (const r of storeFor(object).values()) if (matchesWhere(r, ast?.where)) return r; + return null; + }, + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + // Only written columns are stored: a declared-but-unwritten column is + // absent from every row this driver ever returns. + const row: Record = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const cur = s.get(id); + if (!cur) return null; + const updated = { ...cur, ...data, id }; + s.set(id, updated); + return updated; + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + if (id && storeFor(object).has(id)) return this.update(object, id, data); + return this.create(object, data); + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count(object: string, ast: any) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + async updateMany(object: string, ast: any, data: Record) { + const rows = await this.find(object, ast); + for (const r of rows) storeFor(object).set(r.id as string, { ...r, ...data, id: r.id }); + return rows.length; + }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, stores, reads }; +} + +describe('[#4784] transition condition over a real engine', () => { + let engine: ObjectQL; + let audited: string[]; + let warn: ReturnType; + let reads: { findOne: number; find: number }; + + const conditionWarnings = () => + warn.mock.calls.filter(([msg]) => String(msg).includes('condition evaluation failed')); + + async function boot(hooks: Hook[]) { + engine = new ObjectQL(); + const mem = makeMemoryDriver(); + reads = mem.reads; + engine.registerDriver(mem.driver, true); + await engine.init(); + engine.registry.registerObject(taskObject as any); + + audited = []; + warn = vi.fn(); + bindHooksToEngine(engine, hooks, { + packageId: 'app:showcase', + logger: { debug: () => {}, info: () => {}, warn, error: () => {} }, + }); + } + + beforeEach(async () => { + await boot([{ + // `showcase_audit_task_completion`'s own description says "after a task + // transitions to done". Since #4770 `record.done == true` says "IS done"; + // only this shape says "just BECAME done". + name: 'showcase_audit_task_completion', + object: 'hook_task', + events: ['afterUpdate'], + priority: 90, + condition: TRANSITION, + handler: (ctx: any) => { audited.push(String(ctx.input?.id ?? '?')); }, + } as unknown as Hook]); + }); + + it('audits exactly the update that completes the task', async () => { + const row: any = await engine.insert('hook_task', { title: 'Ship it', status: 'todo', done: false }); + + await engine.update('hook_task', { done: true }, { where: { id: row.id } } as any); + expect(audited).toEqual([row.id]); + + // Two further updates of a record that is ALREADY done — the transition + // happened once, so the audit happens once. + await engine.update('hook_task', { status: 'in_progress' }, { where: { id: row.id } } as any); + await engine.update('hook_task', { title: 'Ship it (v2)' }, { where: { id: row.id } } as any); + + expect(audited).toEqual([row.id]); + expect(conditionWarnings()).toEqual([]); + }); + + it('does not audit an update that leaves the task incomplete', async () => { + const row: any = await engine.insert('hook_task', { title: 'Draft', status: 'todo', done: false }); + await engine.update('hook_task', { status: 'in_progress' }, { where: { id: row.id } } as any); + + expect(audited).toEqual([]); + expect(conditionWarnings()).toEqual([]); + }); + + it('completes a task whose `done` column was never written — no fault', async () => { + // The driver stored no `done` column at all; `previous.done` must read as + // the materialised null, not abort the expression. + const row: any = await engine.insert('hook_task', { title: 'Untouched', status: 'todo' }); + await engine.update('hook_task', { done: true }, { where: { id: row.id } } as any); + + expect(audited).toEqual([row.id]); + expect(conditionWarnings()).toEqual([]); + }); + + it('does not leak materialised nulls into what the after-hook observes', async () => { + const seen: Array> = []; + await boot([{ + name: 'observe_previous', + object: 'hook_task', + events: ['afterUpdate'], + priority: 90, + condition: 'previous.archived == null && record.done == true', + handler: (ctx: any) => { seen.push(ctx.previous); }, + } as unknown as Hook]); + + const row: any = await engine.insert('hook_task', { title: 'Ship it', status: 'todo' }); + await engine.update('hook_task', { done: true }, { where: { id: row.id } } as any); + + expect(seen).toHaveLength(1); + // `archived` is DECLARED and was materialised for the condition. The + // hook's own view of `previous` must not have gained it. + expect(Object.keys(seen[0]!).sort()).toEqual(['id', 'status', 'title']); + }); +}); + +describe('[#4784] a condition that never mentions `previous` costs zero extra fetches', () => { + /** + * The demand-driven prior fetch (`engine.ts`, the `needsPriorRecord(...) || + * afterUpdate hooks exist` gate) is the ONE mechanism that decides whether a + * prior row is read; #4784 adds no second one. These two pins are what a + * future narrowing of that gate has to keep true. + */ + async function bootWith(hooks: Hook[]) { + const engine = new ObjectQL(); + const mem = makeMemoryDriver(); + engine.registerDriver(mem.driver, true); + await engine.init(); + engine.registry.registerObject(taskObject as any); + bindHooksToEngine(engine, hooks, { + packageId: 'app:pin', + logger: { debug: () => {}, info: () => {}, warn: () => {}, error: () => {} }, + }); + return { engine, reads: mem.reads }; + } + + it('reads no prior row at all for a before-hook condition over `record` only', async () => { + const { engine, reads } = await bootWith([{ + name: 'record_only_guard', + object: 'hook_task', + events: ['beforeUpdate'], + priority: 100, + condition: 'record.status == "todo"', + handler: () => {}, + } as unknown as Hook]); + + const row: any = await engine.insert('hook_task', { title: 'A', status: 'todo', done: false }); + const before = reads.findOne; + await engine.update('hook_task', { status: 'in_progress' }, { where: { id: row.id } } as any); + + // No afterUpdate hook, no prior-needing validation rule → nothing to fetch. + expect(reads.findOne - before).toBe(0); + }); + + it('reads the SAME number of rows whether or not the condition mentions `previous`', async () => { + const mkHook = (condition: string): Hook => ({ + name: 'audit', object: 'hook_task', events: ['afterUpdate'], priority: 90, + condition, handler: () => {}, + } as unknown as Hook); + + const plain = await bootWith([mkHook('record.done == true')]); + const plainRow: any = await plain.engine.insert('hook_task', { title: 'A', status: 'todo', done: false }); + const plainBefore = plain.reads.findOne; + await plain.engine.update('hook_task', { done: true }, { where: { id: plainRow.id } } as any); + const plainCost = plain.reads.findOne - plainBefore; + + const withPrev = await bootWith([mkHook(TRANSITION)]); + const prevRow: any = await withPrev.engine.insert('hook_task', { title: 'A', status: 'todo', done: false }); + const prevBefore = withPrev.reads.findOne; + await withPrev.engine.update('hook_task', { done: true }, { where: { id: prevRow.id } } as any); + const prevCost = withPrev.reads.findOne - prevBefore; + + // Both pay the one fetch the afterUpdate gate already made; `previous` + // rides along on it. + expect(plainCost).toBe(1); + expect(prevCost).toBe(plainCost); + }); +}); diff --git a/packages/objectql/src/hook-wrappers.ts b/packages/objectql/src/hook-wrappers.ts index 4c067940f1..aef2e1b46d 100644 --- a/packages/objectql/src/hook-wrappers.ts +++ b/packages/objectql/src/hook-wrappers.ts @@ -49,12 +49,23 @@ const noopLogger = { * 4. timeout → abort if handler runs too long * 5. onError → swallow when set to 'log' * - * The condition formula is evaluated against the record-shaped view of the - * context built by {@link pickRecordPayload}: for a write, the stored record - * overlaid with this write's payload, made total over the object's declared - * fields (#4770 — the same shape a validation predicate reads). Read events - * typically have no record yet, so a condition on a `beforeFind` will simply - * skip when no data is present. + * The condition formula is evaluated against two bindings, the same two a + * validation predicate reads (#4784): + * + * - `record` — the record-shaped view built by {@link pickRecordPayload}: + * for a write, the stored record overlaid with this write's payload, made + * total over the object's declared fields (#4770). Read events typically + * have no record yet, so a condition on a `beforeFind` will simply skip + * when no data is present. + * - `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. + * + * `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 + * already-done row; "just became done" is `previous.done != true && + * record.done == true`. */ export function wrapDeclarativeHook( meta: Hook, @@ -73,7 +84,7 @@ export function wrapDeclarativeHook( }); // Pre-compile condition once so each invocation is cheap. - let conditionFn: ((record: any) => boolean) | undefined; + let conditionFn: ((record: any, previous: Record | undefined) => boolean) | undefined; if (meta.condition) { // Accept either string shorthand or full Expression envelope. const expr: Expression = typeof meta.condition === 'string' @@ -82,8 +93,14 @@ export function wrapDeclarativeHook( if (expr.source && expr.source.trim()) { const check = ExpressionEngine.compile(expr); if (check.ok) { - conditionFn = (record: any) => { - const r = ExpressionEngine.evaluate(expr, { record: record ?? {} }); + conditionFn = (record: any, previous: Record | undefined) => { + // `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. + const r = ExpressionEngine.evaluate(expr, { record: record ?? {}, previous }); if (!r.ok) { logger.warn('[hook] condition evaluation failed; treating as false', { hook: meta.name, @@ -180,7 +197,7 @@ export function wrapDeclarativeHook( // 1. Condition gate if (conditionFn) { const record = pickRecordPayload(ctx); - if (!conditionFn(record)) { + if (!conditionFn(record, pickPreviousPayload(ctx))) { logger.debug('[hook] skipped by condition', { hook: meta.name, object: ctx.object, @@ -404,3 +421,55 @@ function pickRecordPayload(ctx: HookContext): any { } return input; } + +/** + * The `previous` CEL binding: the record's state BEFORE this write (#4784). + * + * ## Why the binding exists at all + * + * #4770 made `record` mean the record's STATE (stored ⊕ payload) rather than + * this write's diff. That is the right meaning, but it left every TRANSITION + * inexpressible: `record.done == true` is true on every update of an + * already-done row, not only on the one that completed it — while the audit + * hooks that motivated it (`showcase_audit_task_completion`, whose own + * description says "after a task transitions to done") mean the transition. + * Comparing against `previous` is the only way to say that, and the surface + * next door — validation predicates — has bound `previous` all along. Two + * surfaces evaluating CEL "over the record" with different scopes is a drift + * an author cannot be expected to hold in their head, so the scopes are the + * same: `record` + `previous`, built by the same helper. + * + * ## Total, and copied + * + * Made total over the object's declared fields for the same reason `record` + * is ({@link materializeDeclaredFields}, #4649/#4770): whether a driver + * returns a column the write never touched is a storage detail the author + * cannot see, and `previous.x` on a row missing `x` aborts the WHOLE + * expression with `No such key` — which #4775 turns into a rejected write. + * An UNDECLARED key still faults, so a typo stays reportable. + * + * The copy is load-bearing: `ctx.previous` is the engine's own pre-image + * object, handed to every after-hook. Materialising in place would give those + * 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 + * + * `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; + * - **predicate (`multi: true`) bulk update** — the engine matched N rows + * and fires the hook ONCE, so there is no single prior record to bind; + * `previous` stays unbound rather than being invented. + * 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. + */ +function pickPreviousPayload(ctx: HookContext): Record | undefined { + if (isInsertEvent(ctx.event)) return undefined; + const prior = ctx.previous; + if (!prior || typeof prior !== 'object' || Array.isArray(prior)) return undefined; + return materializeDeclaredFields({ ...(prior as Record) }, declaredFieldsFor(ctx)); +} diff --git a/skills/objectstack-automation/SKILL.md b/skills/objectstack-automation/SKILL.md index 217817594f..d008ff0979 100644 --- a/skills/objectstack-automation/SKILL.md +++ b/skills/objectstack-automation/SKILL.md @@ -576,9 +576,18 @@ Time-word cheat sheet across surfaces (do not mix them up): | Surface | Event-time record | Pre-event record | Live record | |:--------|:------------------|:-----------------|:------------| | Flow condition / `{…}` template | `record` (trigger snapshot) | `previous` | — (use a `get_record` node) | -| Object hook (`ctx`) | `ctx.record` (write payload) | `ctx.previous` | — | +| Object hook **handler** (`ctx`) | `ctx.input` (write payload); `ctx.result` after the write | `ctx.previous` | — (query via `ctx.ql`) | +| Object hook **`condition`** (CEL) | `record` (stored ⊕ payload) | `previous` | — | | Approval `expression` approver | `trigger.*` | `vars.previous` | `current.*` | +**There is no `ctx.record`.** `HookContext` declares `input` / `result` / +`previous` / `session` / `ql` (plus `object` / `event`) — a handler reads the +write payload as `ctx.input`. The bare `record` / `previous` roots are the +**condition**'s CEL scope, not the handler's context object: `record` is the +stored row overlaid with this write's payload (#4770) and `previous` is the +pre-write row (#4784), both made total over the object's declared fields. See +`objectstack-formula` §5 for where `previous` is bound and where it is not. + ### Node Config (`ApprovalNodeConfigSchema`) | Field | Purpose | diff --git a/skills/objectstack-data/references/data-hooks.md b/skills/objectstack-data/references/data-hooks.md index 689df53e38..97eda5a254 100644 --- a/skills/objectstack-data/references/data-hooks.md +++ b/skills/objectstack-data/references/data-hooks.md @@ -217,8 +217,8 @@ async: true #### `condition` — Declarative Filtering Skip handler execution if the condition is false. Author it as a **CEL -predicate** over `record` (use `P\`...\`` from `@objectstack/spec`; SQL-style -`=` / `AND` / `IN (...)` is not CEL): +predicate** over `record` and `previous` (use `P\`...\`` from +`@objectstack/spec`; SQL-style `=` / `AND` / `IN (...)` is not CEL): ```typescript // Only run for high-value accounts @@ -229,6 +229,10 @@ condition: P`record.status in ['pending', 'in_review']` // Complex conditions condition: P`record.type == 'enterprise' && record.region == 'APAC' && record.is_active == true` + +// A TRANSITION — fires only on the update that completes the task, +// not on later updates of an already-done record (#4784) +condition: P`previous.done != true && record.done == true` ``` **`record` here is the RECORD, not this write's payload (#4770).** The condition @@ -240,9 +244,17 @@ in neither). So: `record.done == true` works on an update that only sets `status`. (This is unlike `ctx.input` / `ctx.result` inside the handler, which stay partial — see Gotcha 1.) -- A condition describes the record's **state**, not the diff. `record.done == - true` fires on every update of an already-done record, not only on the update - that set it. +- `record` describes the record's **state**, not the diff. `record.done == true` + fires on every update of an already-done record, not only on the update that + set it. **For the transition, compare against `previous`** (#4784): + `previous.done != true && record.done == true`. `previous` is the stored + pre-write row, made total over the same declared fields, and it is the same + binding a validation predicate reads. +- **`previous` is UNBOUND where there is no prior state**, and a reference to an + unbound root makes the whole condition unevaluable. That means: insert events + (`beforeInsert` / `afterInsert`) — write those over `record` alone — and + predicate (`multi: true`) bulk updates, where one write matches N rows and the + hook fires once, so there is no single prior record to bind. - **Guard optional values with `!= null`, never with `has(...)`.** A declared field holding `null` is *present*, so `has(record.spent)` is uniformly true and `has(record.spent) && record.spent > record.budget` still faults on diff --git a/skills/objectstack-formula/SKILL.md b/skills/objectstack-formula/SKILL.md index d8f505e4a6..fe85199992 100644 --- a/skills/objectstack-formula/SKILL.md +++ b/skills/objectstack-formula/SKILL.md @@ -99,7 +99,7 @@ scope as CEL — you do **not** learn three languages. | Concept | CEL | |:---|:---| | Current record field | `record.first_name` | -| Previous record (update hooks) | `previous.status` | +| Previous record (update hooks, validation rules) | `previous.status` — §5 | | Hook input payload | `input.amount` | | Identity context | `os.user.id`, `os.org.id`, `os.org.tier`, `os.env` | | Equality | `==` / `!=` | @@ -274,6 +274,34 @@ P`previous.status != 'escalated' && record.status == 'escalated'` ISCHANGED-style logic does not exist as a function; use explicit `previous` comparison. +`record` is the record's **state**, not this write's diff (#4770): stored row ⊕ +payload, so `record.status == 'escalated'` is true on *every* update of an +already-escalated record. Comparing against `previous` is the only way to say +"just became". Hook `condition`s and validation predicates bind the same two +roots (#4784) — one scope, one meaning, whichever surface reads it. + +**Where `previous` is bound, and where it is not:** + +| Surface / event | `previous` | +|:---|:---| +| Update hook `condition` (single-record write), validation rule on update | the stored pre-write row | +| Insert events (`beforeInsert` / `afterInsert`), validation rule on insert | **unbound** — there is no prior state | +| Predicate bulk update (`multi: true`) hook `condition` | **unbound** — one write matches N rows and the hook fires once, so there is no single prior record | + +Referencing `previous` where it is unbound makes the whole expression +unevaluable — so write insert-event conditions over `record` alone, and keep +transition conditions to single-record writes. + +**`previous` is total over the object's declared fields.** A declared column the +driver never returned reads as `null`, not as a fault. Guard with `!= null`, +**never** with `has(...)`: a materialised `null` is a *present* key, so +`has(previous.spent)` is uniformly true for a declared field and tells you +nothing about its value. + +**Cost:** none of its own. `previous` rides on the prior row the engine already +fetches for update hooks; a condition that never mentions it adds no fetch at +all. + --- ## Mechanical translation table (legacy → CEL)