From 076f94de2d6a4a577573f34c1d3fc03cb04c4054 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 15:32:38 +0000 Subject: [PATCH 1/4] fix(trigger-record-change): materialize declared fields on the seeded flow record (#4953) Services half of the maintainer's 2026-08-06 ruling on #4953 item 1. The flow-trigger `record` / `previous` CEL roots were sparse -- a declared field this write's payload/after-row didn't mention was an absent key, so `record.x != null` in a flow's start/edge condition faulted instead of evaluating, exactly the mechanism the issue measured on @marcbachmann/cel-js. `buildContext` now layers the prior row under the payload/after-row (so an untouched field keeps its real persisted value) and materializes both roots over the object's declared fields via a structural-mirror `materializeDeclaredFields` (keeping this package's zero build-time dependency on objectql), gated on the same groundTruth rule `evaluateValidationRules` uses so a still-missing field is only defaulted to null when the persisted state is actually in hand. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RDTnVvsgA6cUZ4xFVtPZRy --- .changeset/flow-trigger-record-materialize.md | 44 +++++ packages/lint/src/validate-null-guards.ts | 59 ++++-- packages/objectql/src/declared-fields.ts | 29 +-- .../trigger-record-change/package.json | 1 + .../src/record-change-integration.test.ts | 93 +++++++++ .../src/record-change-trigger.test.ts | 183 +++++++++++++++++- .../src/record-change-trigger.ts | 147 ++++++++++++-- .../trigger-record-change/vitest.config.ts | 31 +++ pnpm-lock.yaml | 3 + 9 files changed, 537 insertions(+), 53 deletions(-) create mode 100644 .changeset/flow-trigger-record-materialize.md create mode 100644 packages/triggers/trigger-record-change/vitest.config.ts diff --git a/.changeset/flow-trigger-record-materialize.md b/.changeset/flow-trigger-record-materialize.md new file mode 100644 index 0000000000..c05458902e --- /dev/null +++ b/.changeset/flow-trigger-record-materialize.md @@ -0,0 +1,44 @@ +--- +"@objectstack/trigger-record-change": patch +--- + +fix(trigger-record-change): the seeded flow record is total over the object's declared fields — no more fault on an untouched field (#4953) + +A record-change flow's `record` / `previous` CEL roots used to be **sparse**: +`record` was seeded as `{ ...(inputData ?? {}), ...after }` with no fallback to +the prior row, so a declared field this write's payload didn't mention — and the +driver's after-row didn't echo back either — was simply an ABSENT key. CEL is +strict about that: `record.x != null` on a record missing the key `x` doesn't +evaluate to `false`, it **faults** (`No such key: x`), while `has(record.x)` +silently answers `false` for the same reason — reading as "the field genuinely +has no value" when the truth is "this evaluation point never got told". A +`record-before-*` trigger's `record` was hit hardest: with no `after` row at all +(the write hasn't landed yet), it was literally just the incoming patch. + +This is the services-lane half of the maintainer's 2026-08-06 ruling on #4953 +item 1 ("server-side unified, cross-process deferred"). The engine-core half +(field `readonlyWhen`, PR #6454) already materializes; this closes the other +named server seam so both are now total, matching the sibling seams +(`rule-validator.ts`'s object validation / `requiredWhen`, `hook-wrappers.ts`'s +declarative hook `condition`s). + +`record-change-trigger.ts`'s `buildContext` now: + +- layers the prior row (`ctx.previous`, fetched unconditionally ahead of + dispatch for by-id writes since #7867) as the BASE of `record`, so a field + this write didn't touch keeps its real persisted value instead of vanishing — + this runs for every dispatch, before- and after-hooks alike, not just the + after-row merge #1872 already covered; +- then makes both `record` and `previous` total over the object's DECLARED + fields (a structural mirror of `@objectstack/objectql`'s + `materializeDeclaredFields`, keeping this package's zero build-time + dependency on objectql), filling whatever is STILL missing with an explicit + `null` — but only once the record's persisted state is actually in hand + (insert: always; update/delete: only when the prior row was fetched), so a + write whose prior row genuinely could not be read is left sparse rather than + fabricating a value that might contradict the stored row. + +`has()` semantics are unchanged: once a declared field is present (materialized +or not), `has()` still answers whether the KEY is declared/present, not whether +the value is empty — `!= null` is still the way to test emptiness, same contract +`declared-fields.ts` has documented since #4649. diff --git a/packages/lint/src/validate-null-guards.ts b/packages/lint/src/validate-null-guards.ts index 1d32445f10..da79781947 100644 --- a/packages/lint/src/validate-null-guards.ts +++ b/packages/lint/src/validate-null-guards.ts @@ -71,7 +71,7 @@ * Surface ledger (each verdict traced to the code that decides it, so the next * author does not have to re-derive it — #4811). `binding` is the measured * shape TODAY; `verdict` is whether this gate runs there. Those are two - * questions, and since #6454 they have come apart on one row: + * questions, and since #6454 they have come apart on two rows: * * | surface | binding | evidence | verdict | * |:-------------------------------|:--------|:------------------------------------------------------------|:--------| @@ -80,7 +80,7 @@ * | field `requiredWhen` | TOTAL | same `merged` in `evaluateValidationRules` — fail-OPEN, so an unguarded predicate enforces NOTHING in silence | covered (#4811) | * | field `readonlyWhen` | TOTAL | `rule-validator.ts` `readonlyWhenBindings` materialises BOTH roots (#4953 clause 1, landed in #6454) | excluded — NOT on totality; see below | * | action `visible` / `disabled` | sparse | evaluated client-side; no materialization exists in `objectui` | excluded (decided — #4953 clause 2) | - * | flow / edge `condition` | sparse | `record-change-trigger.ts` seeds `{...(inputData ?? {}), ...after}` — #4953 clause 1's other half, not yet wired | excluded (not yet) | + * | flow / edge `condition` | TOTAL | `record-change-trigger.ts` `buildContext` layers `previous` under the payload/after-row then runs BOTH `record` and `previous` through a structural-mirror `materializeDeclaredFields` (#4953 clause 1's other half, services lane) | excluded — NOT on totality; see below | * | sharing-rule `condition` | n/a | compiled to a SQL filter; `NULL > x` is three-valued, never faults | excluded | * | field `expression` (`Field.formula`) | n/a | product judgement, not a wiring gap — see below | excluded | * @@ -98,10 +98,14 @@ * (`stripReadonlyWhenFields` merging `{...previous, ...data}` raw) * describes code that no longer exists. What keeps the row excluded is * clause 3 of the same ruling: the gate widens once BOTH server-side seams - * are total, and the other one — flow trigger-record seeding, services - * lane — is not wired yet. Widening this face alone would also mean the - * `binding` column had stopped being the thing that decides coverage, - * which is the property #4811 bought. + * are total. The other one — flow trigger-record seeding, services lane — + * is ALSO wired now (#4953 clause 1's other half; see the `flow / edge + * condition` row below), so both server-side seams the ruling named are + * total as of that landing. The actual gate widening clause 3 promises is + * tracked separately (#4811) rather than folded into either seam's own + * PR — this row (and the one below) stay excluded here on purpose, so the + * `binding` column keeps meaning what #4811 needs it to mean: a fact about + * the surface, not a verdict this module renders on itself. * * Two facts to carry into that widening; neither is bookkeeping: * @@ -142,20 +146,35 @@ * The ruling's replacement action for this face is the MIRROR of this gate * — flag `!= null` on a sparse binding — and it is an evaluation owed by * the devx / objectui lanes, never a widening of `checkNullGuards`. - * - **Flow / edge `condition`.** #4811 excluded these for flattened-scope - * ambiguity ("a bare identifier may be a flow variable"). That reason does - * not actually apply to this module — {@link findUnguardedNullableOperands} - * only ever resolves `record.` / `previous.` and never a bare - * identifier, and the engine binds `record` / `previous` unconditionally. - * The real blocker is totality: the trigger seeds the record as - * `{...(inputData ?? {}), ...after}` — spelled `inputDoc` here until #5671 - * dropped that alias read — so a declared column the write never mentioned - * is an ABSENT key, and the `!= null` this gate prescribes would fault. - * Since #4953 that sparseness is a NOT-YET rather than a decision: clause 1 - * puts this seam under the same server-side totality guarantee as - * `readonlyWhen`, and only the services-lane wiring is outstanding. When it - * lands, this row and the `readonlyWhen` row flip together — which is - * exactly what clause 3 asks for. + * - **Flow / edge `condition` — the row where `binding` and `verdict` came + * apart, same shape as `readonlyWhen` above.** #4811 originally excluded + * these for flattened-scope ambiguity ("a bare identifier may be a flow + * variable"). That reason does not actually apply to this module — + * {@link findUnguardedNullableOperands} only ever resolves `record.` / + * `previous.` and never a bare identifier, and the engine binds + * `record` / `previous` unconditionally. The real blocker was totality: + * the trigger used to seed the record as `{...(inputData ?? {}), ...after}` + * — spelled `inputDoc` here until #5671 dropped that alias read — so a + * declared column the write never mentioned was an ABSENT key, and the + * `!= null` this gate prescribes would fault. + * + * #4953 (services half) closed that gap: `record-change-trigger.ts` + * `buildContext` now layers `previous` under the payload/after-row (so an + * untouched field reads its REAL persisted value instead of going + * missing) and runs the result through a structural-mirror + * `materializeDeclaredFields` for whatever is still absent — gated on the + * same `groundTruth` rule `evaluateValidationRules` uses (insert always; + * update/delete only once the prior row was fetched), so a write whose + * prior row genuinely cannot be read is left sparse rather than + * fabricating a `null` over an unknown real value. Both `record` and + * `previous` are covered. The totality criterion above is therefore + * SATISFIED here too, and the evidence this row used to carry (the raw + * `{...(inputData ?? {}), ...after}` seed) describes code that no longer + * exists. + * + * What keeps the row excluded is the SAME clause-3 reason the + * `readonlyWhen` row states: the gate's actual widening is tracked + * separately (#4811), not folded into this seam's own PR. * (The flattened-scope ambiguity is real for a *bare-identifier* checker — * flow inputs shadow record fields, and a node's `outputVariable` can * overwrite either — but that is a different, unbuilt pass.) diff --git a/packages/objectql/src/declared-fields.ts b/packages/objectql/src/declared-fields.ts index bf16e9fe82..dc2fe80041 100644 --- a/packages/objectql/src/declared-fields.ts +++ b/packages/objectql/src/declared-fields.ts @@ -6,20 +6,27 @@ * Shared by the SERVER-side places that evaluate a CEL expression against "the * record": object-level validation predicates + field `requiredWhen` + * option `visibleWhen` (`validation/rule-validator.ts`, #1871 / #4649), - * declarative hook `condition`s (`hook-wrappers.ts`, #4770), and the field + * declarative hook `condition`s (`hook-wrappers.ts`, #4770), the field * `readonlyWhen` strips on the write path (`validation/rule-validator.ts` - * `readonlyWhenBindings`, #4953). They used to disagree — a predicate saw a - * total record while a hook condition saw only the fields the current write - * happened to carry — which is precisely the drift this module exists to - * prevent: an author cannot be expected to know that the same `record.done == - * true` means two different things depending on which surface reads it. + * `readonlyWhenBindings`, #4953, PR #6454), and — since #4953's services + * half — the flow-trigger record seeded in + * `packages/triggers/trigger-record-change/src/record-change-trigger.ts`. + * They used to disagree — a predicate saw a total record while a hook + * condition saw only the fields the current write happened to carry — which + * is precisely the drift this module exists to prevent: an author cannot be + * expected to know that the same `record.done == true` means two different + * things depending on which surface reads it. * - * Two bindings are still sparse, and the difference between them matters: + * The flow-trigger seam is a STRUCTURAL MIRROR of this exact function, not an + * import of it: `trigger-record-change` keeps zero build-time dependency on + * `@objectstack/objectql` (the same reason it re-declares `FlowTriggerBinding` + * locally), so it carries its own copy with the identical algorithm and + * contract — see that file's own `materializeDeclaredFields` doc comment for + * the duplication rationale. This doc comment stays the canonical statement + * of the RULE; the copy defers to it rather than re-deriving. + * + * One binding is still sparse, and by decision rather than by gap: * - * - The flow trigger record (`packages/triggers/trigger-record-change`) is a - * server seam the same ruling puts on this list; it is simply not wired yet - * (services lane, #4953 item 1's other half). Do not read its absence as a - * decision. * - objectui's action `visible` / `disabled` binds whatever record the client * already fetched. That one is a DECISION (#4953 item 2): making it total * would mean every REST read padding out all declared columns, so it stays diff --git a/packages/triggers/trigger-record-change/package.json b/packages/triggers/trigger-record-change/package.json index d038124708..2257167f8c 100644 --- a/packages/triggers/trigger-record-change/package.json +++ b/packages/triggers/trigger-record-change/package.json @@ -23,6 +23,7 @@ }, "devDependencies": { "@objectstack/driver-sql": "workspace:*", + "@objectstack/formula": "workspace:*", "@objectstack/objectql": "workspace:*", "@objectstack/service-automation": "workspace:*", "@types/node": "^26.1.2", diff --git a/packages/triggers/trigger-record-change/src/record-change-integration.test.ts b/packages/triggers/trigger-record-change/src/record-change-integration.test.ts index a53b9c5e0c..37568dd0db 100644 --- a/packages/triggers/trigger-record-change/src/record-change-integration.test.ts +++ b/packages/triggers/trigger-record-change/src/record-change-integration.test.ts @@ -161,6 +161,30 @@ const objectDef = (name: string) => ({ }, }); +/** + * #4953 (services half) — a `record-before-*` flow's `record` used to be + * JUST the incoming patch (`inputData`, see `record-change-trigger.ts` + * `buildContext`), never merged with the prior row and never materialized + * over the object's declared fields. `tag` here is untouched by the update + * below (only `note` is written), so BEFORE the fix `record.tag != null` + * read a record with no `tag` key at all and FAULTED — the flow's own + * error-isolation try/catch (`RecordChangeTrigger.start`'s handler) swallows + * that fault, so the only observable symptom was "the flow silently never + * fires", never a visible error. + */ +const beforeHookObjectDef = (name: string) => ({ + name, + label: name, + fields: { + tag: { name: 'tag', label: 'Tag', type: 'text' }, + note: { name: 'note', label: 'Note', type: 'text' }, + // Declared, but never given a value anywhere below — the "genuinely no + // value" case `materializeDeclaredFields` fills with an explicit `null` + // rather than leaving it an absent (fault-on-access) key. + nickname: { name: 'nickname', label: 'Nickname', type: 'text' }, + }, +}); + /** * #3760 — the fail-open this trigger was the dominant carrier of. * @@ -398,4 +422,73 @@ describe('record-change trigger — end-to-end (#1491)', () => { await sleep(200); expect((await data.findOne('wid5', { where: { id: lowId } }))?.alerted).toBe('yes'); }, 15000); + + /** + * #4953 (services half) — a REAL-engine (SqlDriver) measurement of the + * `record-before-*` seam. `record-before-write` dispatches on `beforeUpdate` + * BEFORE the driver has anything to echo back (`ctx.result` is unset), so + * `hydrateComputedFields` never runs either — this leg is entirely on + * `buildContext`'s own prior-row fold + `materializeDeclaredFields` to be + * total, with no assist from the after-row merge #1872 fixed. + */ + it('a record-before-write start condition sees an UNTOUCHED declared field\'s real value, and a NEVER-SET one as null — not a fault (#4953)', async () => { + const kernel = new ObjectKernel({ logLevel: 'silent' }); + await kernel.use(new ObjectQLPlugin()); + await kernel.use(new AutomationServicePlugin()); + await kernel.use(new RecordChangeTriggerPlugin()); + await kernel.bootstrap(); + + const objectql = kernel.getService('objectql') as any; + const data = kernel.getService('data') as any; + const automation = kernel.getService('automation'); + + await attachSqlite(objectql); + objectql.registry.registerObject(beforeHookObjectDef('bfw'), 'test', 'test'); + objectql.registry.registerObject( + { name: 'bfw_audit', label: 'bfw audit', fields: { seen_tag: { name: 'seen_tag', label: 'T', type: 'text' } } }, + 'test', 'test', + ); + await objectql.syncSchemas(); + + automation.registerFlow('before_write_probe', { + name: 'before_write_probe', label: 'Before-update probe', type: 'record_change', + nodes: [ + { + id: 'start', type: 'start', label: 'Start', + // `record-before-update` ONLY (not `-write`, which would also bind + // `beforeInsert` — the insert leg would trivially satisfy this same + // condition too and double the audit count, muddying the measurement). + config: { + objectName: 'bfw', triggerType: 'record-before-update', + // `tag` is NOT in THIS write's payload (only `note` is written) — + // BEFORE the fix this read a record with no `tag` key and + // FAULTED. `nickname` is declared but never given a value + // ANYWHERE (not on insert, not on this update): a materialized + // `null`, not a fabrication. + condition: "record.tag != null && record.nickname == null", + }, + }, + { + id: 'log', type: 'create_record', label: 'Log', + config: { objectName: 'bfw_audit', fields: { seen_tag: '{record.tag}' } }, + }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [{ id: 'e1', source: 'start', target: 'log' }, { id: 'e2', source: 'log', target: 'end' }], + } as any); + + const created = await data.insert('bfw', { tag: 'keep', note: 'x' }, { context: { userId: 'u_trigger' } }); + const id = Array.isArray(created) ? created[0]?.id : created?.id ?? created; + await sleep(200); + + // Update touches ONLY `note` — `tag` never appears in this write's payload. + await data.update('bfw', { id, note: 'y' }, { context: { userId: 'u_trigger' } }); + await sleep(200); + + const audit: any[] = await data.find('bfw_audit', {}); + expect(audit).toHaveLength(1); + // The condition read `record.tag`'s REAL persisted value ('keep', folded + // from the prior row) — not a fabricated null, and not a fault. + expect(audit[0]?.seen_tag).toBe('keep'); + }, 15000); }); diff --git a/packages/triggers/trigger-record-change/src/record-change-trigger.test.ts b/packages/triggers/trigger-record-change/src/record-change-trigger.test.ts index a12c49662e..ef8042429f 100644 --- a/packages/triggers/trigger-record-change/src/record-change-trigger.test.ts +++ b/packages/triggers/trigger-record-change/src/record-change-trigger.test.ts @@ -3,6 +3,7 @@ import { describe, it, expect, vi } from 'vitest'; import type { AutomationContext } from '@objectstack/spec/contracts'; import type { HookContext } from '@objectstack/spec/data'; +import { ExpressionEngine } from '@objectstack/formula'; import { RecordChangeTrigger, triggerTypeToHookEvent, @@ -13,6 +14,13 @@ import { } from './record-change-trigger.js'; import { RecordChangeTriggerPlugin } from './plugin.js'; +/** CEL-evaluate `source` against `record` — the SAME engine the automation + * service and rule-validator use, so a `record.a != null` fault/pass here is + * the real fault a flow's start/edge condition would hit, not a stand-in. */ +function evalCel(source: string, ctx: { record?: unknown; previous?: unknown }) { + return ExpressionEngine.evaluate({ dialect: 'cel', source }, ctx); +} + // ─── Test doubles ─────────────────────────────────────────────────── interface RegisteredHook { @@ -311,7 +319,15 @@ describe('RecordChangeTrigger', () => { // put back (`data` sits first in the read, so it wins either way), so the // NEGATIVE case is the one carrying the weight — it goes red the moment any // alias limb returns. - it('seeds the record from input.data when result is absent (e.g. before-hooks)', async () => { + // + // [#4953, services half] The expected object grew an `_id` and `assignee` + // that `hookCtx()`'s default `previous` carries and `data` does not touch — + // a before-hook's `record` now layers the prior row as its BASE (so an + // untouched declared field keeps its real persisted value instead of going + // missing/`null`), and `data`'s own `status` still wins over `previous`'s + // stale one. That `status` win is what this test still pins: the payload + // key read is `data`, never `doc`. + it('seeds the record from input.data OVER previous when result is absent (e.g. before-hooks)', async () => { const { engine, hooks } = fakeEngine(); const trigger = new RecordChangeTrigger(engine, silentLogger()); let captured: AutomationContext | undefined; @@ -322,7 +338,7 @@ describe('RecordChangeTrigger', () => { await hooks[0].handler(hookCtx({ event: 'beforeUpdate', result: undefined })); - expect(captured?.record).toEqual({ status: 'done' }); + expect(captured?.record).toEqual({ _id: 't1', status: 'done', assignee: 'u1' }); }); it('does NOT read a `doc` alias off input — no engine path produces that key', async () => { @@ -763,3 +779,166 @@ describe('RecordChangeTrigger computed-field hydration guards (#3426 follow-up)' expect(findOne).toHaveBeenCalledTimes(2); }); }); + +// ─── declared-field materialization (#4953, services half) ────────── +// +// Measured mechanism (matches the issue's own repro, run here through the +// SAME `@objectstack/formula` CEL engine the automation service and +// rule-validator use — not a stand-in): +// +// evalCel('record.a != null', { record: { a: null } }) → { ok: true, value: false } +// evalCel('record.a != null', { record: {} }) → { ok: false, error: 'No such key: a' } +// +// So whether a declared field is a present key (even holding `null`) decides +// whether a flow's `record.x != null` start/edge condition evaluates at all. +describe('RecordChangeTrigger materializes declared fields (#4953, services half)', () => { + /** fakeEngine + a `getObject` returning the given field map (the ONE + * method actually wired on the real ObjectQL engine — `getObjectConfig` + * is not, see the interface doc on `RecordChangeDataEngine`). */ + function fakeEngineWithSchema(fields: Record) { + const base = fakeEngine(); + const getObject = vi.fn().mockReturnValue({ fields }); + const engine: RecordChangeDataEngine = { ...base.engine, getObject }; + return { engine, hooks: base.hooks, getObject }; + } + + it('RED before the fix, reproduced directly: the OLD raw merge faults on an untouched declared field', () => { + // What `buildContext` used to hand CEL for this exact scenario — the + // driver's after-row + payload, with NO prior-row fold-in and no + // materialization. `a` is declared on the object but this write never + // mentions it and the driver didn't echo it back. + const oldShapeRecord = { ...{ b: 'new' }, ...{ id: 'r1', b: 'new' } }; // { id, b } — no `a` + const res = evalCel('record.a != null', { record: oldShapeRecord }); + expect(res.ok).toBe(false); + expect(String((res as { error?: { message?: string } }).error?.message)).toMatch(/no such key/i); + }); + + it('GREEN after the fix: an untouched declared field resolves to its REAL persisted value, not null (no fabrication)', async () => { + const { engine, hooks } = fakeEngineWithSchema({ a: { type: 'text' }, b: { type: 'text' } }); + const trigger = new RecordChangeTrigger(engine, silentLogger()); + let captured: AutomationContext | undefined; + + trigger.start(binding({ event: 'record-after-update' }), async (ctx) => { captured = ctx; }); + // Update touches only `b`. The driver's after-row echoes `id` + `b` + // ONLY (the #1872 gap) — but `a`'s real value ('orig') is known from + // `previous`, the row the engine fetched ahead of dispatch (#7867). + await hooks[0].handler(hookCtx({ + event: 'afterUpdate', + input: { id: 'r1', data: { b: 'new' } }, + result: { id: 'r1', b: 'new' }, + previous: { id: 'r1', a: 'orig', b: 'old' }, + })); + + const record = captured?.record as Record; + // Folded from `previous`, NOT materialized to null — materialization + // only fills what is GENUINELY unknown, never overrides a known value. + expect(record.a).toBe('orig'); + expect(record.b).toBe('new'); // the write's own value still wins + + const res = evalCel('record.a != null', { record }); + expect(res).toEqual({ ok: true, value: true }); + }); + + it('GREEN after the fix: a field never set anywhere materializes to null (has() true, != null false — no fault)', async () => { + const { engine, hooks } = fakeEngineWithSchema({ a: { type: 'text' }, b: { type: 'text' }, c: { type: 'text' } }); + const trigger = new RecordChangeTrigger(engine, silentLogger()); + let captured: AutomationContext | undefined; + + trigger.start(binding({ event: 'record-after-update' }), async (ctx) => { captured = ctx; }); + // Neither `previous` nor this write ever mentions `c` — it genuinely + // has no value, so `null` here is a materialised absence, not a + // fabrication. + await hooks[0].handler(hookCtx({ + event: 'afterUpdate', + input: { id: 'r1', data: { b: 'new' } }, + result: { id: 'r1', b: 'new' }, + previous: { id: 'r1', a: 'x', b: 'old' }, + })); + + const record = captured?.record as Record; + expect(record.c).toBeNull(); + expect(evalCel('record.c != null', { record })).toEqual({ ok: true, value: false }); + // The documented, PINNED consequence (#4649 contract, unchanged here): + // has() on a declared field is uniformly true once materialised — it + // guards an undeclared KEY, never an empty value. + expect(evalCel('has(record.c)', { record })).toEqual({ ok: true, value: true }); + }); + + it('does NOT materialize on update when the prior row is unavailable (no fabrication without ground truth)', async () => { + const { engine, hooks } = fakeEngineWithSchema({ a: { type: 'text' }, b: { type: 'text' } }); + const trigger = new RecordChangeTrigger(engine, silentLogger()); + let captured: AutomationContext | undefined; + + trigger.start(binding({ event: 'record-after-update' }), async (ctx) => { captured = ctx; }); + await hooks[0].handler(hookCtx({ + event: 'afterUpdate', + input: { id: 'r1', data: { b: 'new' } }, + result: { id: 'r1', b: 'new' }, + previous: undefined, // prior row not in hand + })); + + const record = captured?.record as Record; + expect('a' in record).toBe(false); // left sparse, not fabricated + const res = evalCel('record.a != null', { record }); + expect(res.ok).toBe(false); // fails closed, same policy as the validation seam + }); + + it('materializes unconditionally on insert (absence genuinely means "no value")', async () => { + const { engine, hooks } = fakeEngineWithSchema({ a: { type: 'text' }, b: { type: 'text' } }); + const trigger = new RecordChangeTrigger(engine, silentLogger()); + let captured: AutomationContext | undefined; + + trigger.start(binding({ event: 'record-after-create' }), async (ctx) => { captured = ctx; }); + await hooks[0].handler(hookCtx({ + event: 'afterInsert', + input: { data: { b: 'new' } }, + result: { id: 'r1', b: 'new' }, + previous: undefined, + })); + + const record = captured?.record as Record; + expect(record.a).toBeNull(); + expect(evalCel('record.a != null', { record })).toEqual({ ok: true, value: false }); + }); + + it('materializes the `previous` root too, WITHOUT mutating the shared ctx.previous (no cross-binding leakage)', async () => { + const { engine, hooks } = fakeEngineWithSchema({ a: { type: 'text' }, b: { type: 'text' } }); + const trigger = new RecordChangeTrigger(engine, silentLogger()); + let captured: AutomationContext | undefined; + // The SAME object the engine would hand to every OTHER flow binding + // sharing this write's HookContext. + const sharedPrevious: Record = { id: 'r1', b: 'old' }; // `a` never set + + trigger.start(binding({ event: 'record-after-update' }), async (ctx) => { captured = ctx; }); + await hooks[0].handler(hookCtx({ + event: 'afterUpdate', + input: { id: 'r1', data: { b: 'new' } }, + result: { id: 'r1', b: 'new' }, + previous: sharedPrevious, + })); + + expect((captured?.previous as Record).a).toBeNull(); + // The shared object itself must be untouched — a second binding on the + // same write must not see a materialised null it never asked for. + expect('a' in sharedPrevious).toBe(false); + }); + + it('is a no-op when the engine has no getObject (structural fallback, no crash)', async () => { + const { engine, hooks } = fakeEngine(); // no getObject surface at all + const trigger = new RecordChangeTrigger(engine, silentLogger()); + let captured: AutomationContext | undefined; + + trigger.start(binding({ event: 'record-after-update' }), async (ctx) => { captured = ctx; }); + await hooks[0].handler(hookCtx({ + event: 'afterUpdate', + input: { id: 'r1', data: { b: 'new' } }, + result: { id: 'r1', b: 'new' }, + previous: { id: 'r1', a: 'orig', b: 'old' }, + })); + + const record = captured?.record as Record; + // previous is still folded in as the base layer (that part doesn't + // need field declarations) — only the null-fill step is skipped. + expect(record.a).toBe('orig'); + }); +}); diff --git a/packages/triggers/trigger-record-change/src/record-change-trigger.ts b/packages/triggers/trigger-record-change/src/record-change-trigger.ts index 96d5bd783f..a8a085d034 100644 --- a/packages/triggers/trigger-record-change/src/record-change-trigger.ts +++ b/packages/triggers/trigger-record-change/src/record-change-trigger.ts @@ -44,13 +44,25 @@ export interface RecordChangeDataEngine { ): void; unregisterHooksByPackage?(packageId: string): number; /** - * Optional object-existence probe (the ObjectQL engine's `getObject`). - * When present, {@link RecordChangeTrigger.start} uses it to call out a - * flow whose `objectName` matches no registered object — a hook filtered - * to a name nobody writes never fires, with zero output at any layer - * (2026-07-17 third-party eval). + * Optional object-schema accessor (the ObjectQL engine's `getObject`, + * `IObjectQLEngine.getObject` — confirmed WIRED on the concrete engine, + * unlike {@link getObjectConfig} below). Two independent consumers: + * + * 1. {@link RecordChangeTrigger.start} probes existence to call out a + * flow whose `objectName` matches no registered object — a hook + * filtered to a name nobody writes never fires, with zero output at + * any layer (2026-07-17 third-party eval). + * 2. {@link RecordChangeTrigger.buildContext} reads `.fields` off the + * result to make the seeded `record` / `previous` CEL roots TOTAL + * over the object's DECLARED fields (#4953 services half) — see + * {@link materializeDeclaredFields}. + * + * Typed loosely (not `ServiceObject`) so this plugin keeps its zero + * build-time dependency on objectql; a fixture that returns only what a + * probe needs (e.g. `{ name }`) is still a valid implementation for + * consumer (1) and simply contributes no fields to consumer (2). */ - getObject?(name: string): unknown; + getObject?(name: string): { fields?: Record } | undefined; /** * Optional record re-read (the ObjectQL engine's `findOne`). When present, * {@link RecordChangeTrigger} uses it to hydrate the seeded `record` with @@ -66,14 +78,26 @@ export interface RecordChangeDataEngine { options: { where?: Record; fields?: string[]; context?: unknown }, ): Promise | null | undefined>; /** - * Optional object-config accessor (the ObjectQL engine's `getObjectConfig`). - * When present, {@link RecordChangeTrigger} uses it to SKIP the hydration - * re-read for objects that declare no `formula` field — the only thing the - * re-read adds (`summary` fields are stored on write, not read-time - * computed). Returns the object's field map; typed loosely so this plugin - * keeps its zero build-time dependency on objectql. Absent (or unsure) ⇒ the + * Optional object-config accessor (`getObjectConfig`). When present, + * {@link RecordChangeTrigger} uses it to SKIP the hydration re-read for + * objects that declare no `formula` field — the only thing the re-read + * adds (`summary` fields are stored on write, not read-time computed). + * Returns the object's field map; typed loosely so this plugin keeps its + * zero build-time dependency on objectql. Absent (or unsure) ⇒ the * trigger re-reads unconditionally (prior behavior — correctness over the * optimization). + * + * ⚠️ Measured while wiring #4953 (services half): the concrete ObjectQL + * engine does NOT implement a method named `getObjectConfig` — only + * `getObject` (above) exists there. So on the real engine this optional + * hook is always absent and {@link objectHasFormulaField} always takes + * its `true` fallback; the schema-gate skip it describes is unreachable + * in production (harmless — "correctness over the optimization" is + * exactly the documented fallback — but it is dead code, not a working + * gate). Out of scope here (a hydration-perf question, not a + * materialization one); filed separately rather than folded into this + * fix. Left as-is, and deliberately NOT reused for #4953's field lookup — + * {@link getObject} is the one actually wired. */ getObjectConfig?(object: string): { fields?: Record } | undefined; } @@ -91,6 +115,37 @@ export interface TriggerLogger { error?(msg: string, ...args: unknown[]): void; } +/** + * Make a record TOTAL over an object's DECLARED fields — a structural mirror + * of `@objectstack/objectql`'s `materializeDeclaredFields` + * (`packages/objectql/src/declared-fields.ts`), duplicated here for the same + * reason {@link FlowTriggerBinding} / {@link FlowTrigger} above are: this + * package stays free of a build-time dependency on objectql (`@objectstack/ + * objectql` is a devDependency only — tests wire the real engine, production + * gets it structurally via {@link RecordChangeDataEngine}). SAME algorithm, + * SAME contract as the canonical copy — this is not a second materialisation + * pattern, it is the one #4953 (readonlyWhen, PR #6454) established, applied + * at the one seam objectql itself cannot reach (a server package one hop + * further out). See the canonical doc comment for the full rationale + * (why `undefined` counts as absent, why scope is declared-fields-only, why + * `has()` becomes uniformly true afterwards). + * + * Mutates `record` in place (matching the canonical copy's contract) and + * returns it. Callers that must not mutate a shared object — this file's own + * `ctx.previous`, observed by every OTHER binding sharing the same + * HookContext — pass a shallow copy in. + */ +function materializeDeclaredFields( + record: Record, + fields: Record | undefined | null, +): Record { + if (!fields || typeof fields !== 'object') return record; + for (const name of Object.keys(fields)) { + if (record[name] === undefined) record[name] = null; + } + return record; +} + const TRIGGER_PREFIX = 'com.objectstack.trigger.record-change'; /** @@ -277,11 +332,13 @@ export class RecordChangeTrigger implements FlowTrigger { /** * Build the flow execution context from an ObjectQL hook context. The new * record comes from `ctx.result` (after-hooks) or falls back to the - * mutation input payload / previous row; the old record from `ctx.previous`, - * which the engine binds ahead of every dispatch. + * mutation input payload, layered over the prior row; the old record from + * `ctx.previous`, which the engine binds ahead of every dispatch. * * Async because the seeded `record` is hydrated with read-time computed - * fields (see {@link hydrateComputedFields}) via a data-engine re-read. + * fields (see {@link hydrateComputedFields}) via a data-engine re-read, + * AND — since #4953 (services half) — made total over the object's + * declared fields (see the `materializeDeclaredFields` call below). */ private async buildContext(binding: FlowTriggerBinding, ctx: HookContext): Promise { // objectql lifecycle hooks carry the written row under `input.data` (insert / @@ -311,8 +368,21 @@ export class RecordChangeTrigger implements FlowTrigger { // producer of `__previous` is therefore ignored by design; the way to hand // this consumer a pre-image is to bind the declared `ctx.previous`. const previous = ctx.previous as Record | undefined; + const priorBase = previous && typeof previous === 'object' ? previous : undefined; + + const object = binding.object ?? ctx.object; const inputData = input.data && typeof input.data === 'object' ? input.data : undefined; + // #4953 (services half) — the prior row is now the BASE layer, not just + // the before-hook fallback: a field this write's payload/after-row + // doesn't mention still needs its REAL persisted value (from `previous`) + // rather than going missing, or #1871/#4649's `materializeDeclaredFields` + // below would default it to `null` and FABRICATE a value that + // contradicts the stored row (`declared-fields.ts`'s own warning) — + // wrong for a field that simply was not touched by this write. `after` + // (this write's own post-write echo) is trusted last / most, `previous` + // least, matching `readonlyWhenBindings`' `{ ...previous, ...data }` + // shape in `rule-validator.ts` (#6454). const record: Record = after && typeof after === 'object' ? // #1872 — overlay the after-row on the input payload so fields the @@ -320,21 +390,58 @@ export class RecordChangeTrigger implements FlowTrigger { // stored as an array column) stay visible to the flow's start // condition and `{record.}` interpolation. The after-row // wins for every field it DOES return (id, DB-computed values). - { ...(inputData ?? {}), ...after } - : inputData ?? (previous && typeof previous === 'object' ? previous : {}); + { ...(priorBase ?? {}), ...(inputData ?? {}), ...after } + : { ...(priorBase ?? {}), ...(inputData ?? {}) }; const session = (ctx.session ?? {}) as { userId?: string; organizationId?: string }; - const object = binding.object ?? ctx.object; - // Hydrate read-time computed fields (formula virtuals) onto the seeded // record so the flow's start condition and every `{record.}` // template resolve them — the raw hook row never carries them (#3426). + // Runs BEFORE materialization below: hydration only fills keys `record` + // LACKS (`{...full, ...record}`, record wins), so a field materialized + // to `null` first would shadow the real value hydration's re-read could + // have supplied. const hydrated = await this.hydrateComputedFields(object, ctx.event, record, ctx); + // #4953 (services half) — make BOTH CEL roots (`record` AND `previous`) + // TOTAL over the object's DECLARED fields, exactly as the server's other + // materialised seams already are (`materializeDeclaredFields`, + // `packages/objectql/src/declared-fields.ts`; `rule-validator.ts` + // `readonlyWhenBindings`, PR #6454). Without this, `record.x != null` / + // `previous.x != null` in a flow's start condition, edge condition, or a + // `{record.x}` template fault with `No such key: x` whenever the driver + // didn't echo `x` back — while `has(record.x)` silently answers `false` + // for the exact same reason, which reads as "the field genuinely has no + // value" when the truth is "this evaluation point never got told". + // + // Only once the record's persisted state is actually IN HAND — same + // `groundTruth` gate `evaluateValidationRules` uses. On insert there is + // nothing to know yet (absence genuinely means "no value"): every + // insert-type event materializes unconditionally. On update/delete it is + // knowable only once the prior row was fetched (`ctx.previous`, `#7867` + // makes that unconditional for by-id writes) — defaulting a + // still-missing field to `null` without that would not materialise an + // absent value, it would FABRICATE one that contradicts the stored row, + // so a write whose prior row genuinely could not be read is left as-is + // and the rare unevaluable predicate fails closed (a fault), same policy + // as the validation seam. + const isInsertEvent = ctx.event === 'beforeInsert' || ctx.event === 'afterInsert'; + const groundTruth = isInsertEvent || priorBase !== undefined; + const fields = groundTruth ? this.engine.getObject?.(object)?.fields : undefined; + if (fields) materializeDeclaredFields(hydrated, fields); + // COPIED before materialising, never mutated in place: `previous` here + // is the engine's shared `ctx.previous` — the SAME HookContext reference + // is handed to every OTHER flow binding on this write (see the class doc + // on `hydrationCache`), so writing into it would leak materialised + // `null`s into bindings that haven't run yet. Same rule + // `readonlyWhenBindings` follows for the identical reason. + const materializedPrevious = + priorBase && fields ? materializeDeclaredFields({ ...priorBase }, fields) : previous; + return { record: hydrated, - previous, + previous: materializedPrevious, object, event: binding.event, userId: session.userId, diff --git a/packages/triggers/trigger-record-change/vitest.config.ts b/packages/triggers/trigger-record-change/vitest.config.ts new file mode 100644 index 0000000000..5391c0911c --- /dev/null +++ b/packages/triggers/trigger-record-change/vitest.config.ts @@ -0,0 +1,31 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { defineConfig } from 'vitest/config'; +import path from 'path'; + +/** + * `check:test-source-alias` (#7668/#7778/#7849) — a unit test must be a + * verdict about the SOURCE in the checkout, not a sibling package's build + * artifact. This package had no config at all until #4953 (services half) + * added a `@objectstack/formula` devDependency for + * `record-change-trigger.test.ts` (CEL-evaluating the seeded record through + * the SAME engine the automation service and rule-validator use, to prove + * the materialization fix without a stand-in). That import resolves through + * `exports` to `dist/` with no config, so it is aliased to source here — + * exactly the fix that gate's own header prescribes, not a widening of its + * `KNOWN_UNALIASED_TEST_IMPORTS` registry entry for this package (which stays + * unchanged: `@objectstack/core`, `@objectstack/driver-sql`, + * `@objectstack/objectql`, `@objectstack/service-automation` are untouched by + * this file and remain that registry's problem to eventually retire). + */ +export default defineConfig({ + test: { + globals: true, + environment: 'node', + }, + resolve: { + alias: { + '@objectstack/formula': path.resolve(__dirname, '../../formula/src/index.ts'), + }, + }, +}); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 4e6827155c..51d9cc9b74 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -2616,6 +2616,9 @@ importers: '@objectstack/driver-sql': specifier: workspace:* version: link:../../drivers/driver-sql + '@objectstack/formula': + specifier: workspace:* + version: link:../../formula '@objectstack/objectql': specifier: workspace:* version: link:../../objectql From c7a89cc0f110d275d2f7d94f26d3ad9c1973c9f7 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 16:04:58 +0000 Subject: [PATCH 2/4] fix(trigger-record-change): drift guard, doc gap, and gate fixes (PM follow-up) Three follow-ups on #4953's services-half PR, all addressed before leaving draft: 1. Drift guard for the duplicated materializeDeclaredFields: published it from @objectstack/objectql's ./core entry, and added a parity test that runs the local mirror and the canonical copy over a shared case table, plus a demonstration case proving the comparison actually discriminates a synthetic divergence (not a tautology). 2. Doc gap: no doc described whether a flow condition's record/previous is total or sparse over declared fields. Added that guarantee to content/docs/automation/flows.mdx, mirroring validation.mdx's existing language for the sibling seam. 3. Two gates the derived list could not name (triggered by what the change IS, not by changed paths): check:slot-lookup (2 new untyped getService(...) erasures in the new integration test -- typed via IObjectQLEngine/IDataEngine instead) and check:type-check-debt's TEST_DEBT re-measure (this package hides its own tests from tsc, so its own `typecheck` script cannot see them; the re-measure runs tsc INCLUDING tests and found 2 real errors: a TS6059 from a relative cross-package import -- resolved by switching to the new published export instead -- and a generic-inference narrowing issue in a test helper). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RDTnVvsgA6cUZ4xFVtPZRy --- content/docs/automation/flows.mdx | 15 ++ packages/objectql/src/core.ts | 6 + ...materialize-declared-fields-parity.test.ts | 181 ++++++++++++++++++ .../src/record-change-integration.test.ts | 41 +++- .../src/record-change-trigger.test.ts | 2 +- .../src/record-change-trigger.ts | 10 +- 6 files changed, 250 insertions(+), 5 deletions(-) create mode 100644 packages/triggers/trigger-record-change/src/materialize-declared-fields-parity.test.ts diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index 5a332f7a81..964318194a 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -1249,6 +1249,21 @@ true`. Compare against the stored shape (`record.done == 1`) or normalize the value before the condition. + +**`record` / `previous` are *total* over the object's declared fields** — the +same guarantee [validation predicates](/docs/data-modeling/validation#has-x-is-not-a-null-guard) +carry. A field this write didn't touch (and, for `record`, a field the driver +didn't echo back) still reads as an explicit `null` rather than dropping out +of scope, so `record.discount != null` and `previous.stage != previous_stage` +evaluate cleanly instead of aborting with `No such key`. `has(record.x)` is +therefore uniformly `true` for a *declared* field the moment it's present at +all — including when its value is `null` — so it answers "is `x` declared on +this object", never "does `x` have a value". Guard emptiness with +`record.x != null` / `isBlank(record.x)`, same as everywhere else CEL runs +(see the [`has()` gotcha](/docs/data-modeling/formulas#cel-primer) above). +`has()` still earns its keep against a genuinely *undeclared* key. + + ## Run a flow via API Flows of any type can be launched over HTTP — this is what an external system, diff --git a/packages/objectql/src/core.ts b/packages/objectql/src/core.ts index 96d8450cec..f2a1a1a3d2 100644 --- a/packages/objectql/src/core.ts +++ b/packages/objectql/src/core.ts @@ -69,6 +69,12 @@ export { ValidationError, validateRecord } from './validation/record-validator.j export type { FieldValidationError } from './validation/record-validator.js'; export { evaluateValidationRules, needsPriorRecord, legalNextStates } from './validation/rule-validator.js'; export type { EvaluateRulesOptions } from './validation/rule-validator.js'; +// #4953 — published so a package that duplicates this algorithm for its own +// zero-build-dependency reasons (`@objectstack/trigger-record-change`'s +// structural mirror, `record-change-trigger.ts`) has a TEST-TIME way to +// verify its copy still agrees, instead of the two silently drifting behind +// one doc comment's word. +export { materializeDeclaredFields } from './declared-fields.js'; export { InMemoryHookMetricsRecorder, noopHookMetricsRecorder, diff --git a/packages/triggers/trigger-record-change/src/materialize-declared-fields-parity.test.ts b/packages/triggers/trigger-record-change/src/materialize-declared-fields-parity.test.ts new file mode 100644 index 0000000000..a13d642f0b --- /dev/null +++ b/packages/triggers/trigger-record-change/src/materialize-declared-fields-parity.test.ts @@ -0,0 +1,181 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4953 (services half), PM follow-up — a drift guard for the duplicated + * `materializeDeclaredFields`. + * + * `record-change-trigger.ts` carries a STRUCTURAL MIRROR of + * `@objectstack/objectql`'s `materializeDeclaredFields` + * (`packages/objectql/src/declared-fields.ts`), duplicated rather than + * imported so this package keeps its zero BUILD-TIME dependency on objectql + * (`@objectstack/objectql` is a devDependency only — see that package's own + * `package.json`). A doc comment saying "same algorithm, see the canonical + * copy" is a convention, not a mechanism: nothing stops the two from + * silently diverging if objectql's copy changes and this one doesn't (or + * vice versa) — a flow condition would then evaluate a field differently + * from a validation rule for the SAME field on the SAME object, with + * nothing red anywhere. This file is the mechanism. + * + * ## Why the canonical import is `@objectstack/objectql/core`, not a relative path + * + * `@objectstack/objectql`'s `package.json` `exports` map did not publish + * `declared-fields.ts` before this PR (it was an internal module, imported by + * relative path from objectql's OWN other modules, e.g. + * `validation/rule-validator.ts`). Two ways to reach it from a sibling + * package's TEST file: + * + * 1. Publish it from objectql's `./core` entry (already the home of other + * internal-tooling exports like `evaluateValidationRules`) and import + * the bare specifier `@objectstack/objectql/core` — a devDependency + * already. This is what this file does. + * 2. A relative import straight into `packages/objectql/src/`. MEASURED + * and rejected: `tsc --noEmit` over this package's tests (the + * `check:type-check-debt` TEST_DEBT re-measure, which — unlike this + * package's own `pnpm typecheck` — does NOT exclude `*.test.ts`) reports + * **TS6059**, because the package's `tsconfig.json` sets + * `rootDir: "./src"` and a file physically outside that directory + * cannot be part of its program. That is a repo-wide, mechanically + * enforced rule, not a style call this file gets to make locally. + * + * (1)'s residual risk — `@objectstack/objectql` resolves through `exports` to + * `dist/`, so the comparison is "does the local copy match objectql's LAST + * BUILD", not its live source — is the SAME risk this package already + * accepts for every other `@objectstack/objectql` import its tests make (it + * is grandfathered in `scripts/check-test-source-alias.mjs`'s + * `KNOWN_UNALIASED_TEST_IMPORTS`, unlike `@objectstack/formula`, which THIS + * package's own `vitest.config.ts` aliases to source for exactly this + * reason). Adding one more named export from an already-dist-resolved + * package does not create a new category of risk, and turbo's + * `test` `dependsOn: ["^build"]` means the risk is dormant in every path CI + * actually runs — only a bare `vitest run` against an unbuilt tree could see + * it stale, the same as every other `@objectstack/objectql` symbol this + * package's tests already read. + * + * ## What "demonstrate it's a real tripwire" means here + * + * A parity check that only ever compares two calls to functions that happen + * to behave identically proves nothing about whether the COMPARISON itself + * would catch a real divergence — it could pass just as easily if both + * sides were broken in the same way, or if a copy-paste bug made both + * variables reference the SAME function. The last `it` below manufactures a + * SYNTHETIC divergence (a hand-written third implementation with one + * deliberately wrong line) and asserts the comparison disagrees with it — + * proving the harness discriminates a real difference, not just tautology. + */ +import { describe, it, expect } from 'vitest'; +import { materializeDeclaredFields as localMaterialize } from './record-change-trigger.js'; +// The published `@objectstack/objectql/core` entry — see the file doc above +// for why this is a bare specifier and not a relative source import. +import { materializeDeclaredFields as canonicalMaterialize } from '@objectstack/objectql/core'; + +/** One shared table of cases, run through BOTH implementations. */ +const CASES: Array<{ + name: string; + record: Record; + fields: Record | undefined | null; +}> = [ + { + name: 'fills a genuinely-missing declared field with null', + record: { b: 'x' }, + fields: { a: { type: 'text' }, b: { type: 'text' } }, + }, + { + name: 'an already-present value (including a falsy one) is left untouched', + record: { a: 0, b: '', c: false }, + fields: { a: { type: 'number' }, b: { type: 'text' }, c: { type: 'boolean' } }, + }, + { + name: 'an own key holding `undefined` counts as absent, same as a missing key', + record: { a: undefined, b: 'x' }, + fields: { a: { type: 'text' }, b: { type: 'text' } }, + }, + { + name: 'a key already holding null stays null (not re-materialized into something else)', + record: { a: null }, + fields: { a: { type: 'text' } }, + }, + { + name: 'scope is declared-fields-only — an undeclared key on the record is untouched', + record: { undeclared: 'x' }, + fields: { a: { type: 'text' } }, + }, + { + name: 'no fields declared at all → no-op', + record: { a: 'x' }, + fields: {}, + }, + { + name: 'fields is undefined → no-op (record returned as-is)', + record: { a: 'x' }, + fields: undefined, + }, + { + name: 'fields is null → no-op (record returned as-is)', + record: { a: 'x' }, + fields: null, + }, + { + name: 'empty record, several declared fields → all materialize to null', + record: {}, + fields: { a: {}, b: {}, c: {} }, + }, +]; + +describe('materializeDeclaredFields parity: local mirror vs @objectstack/objectql canonical (#4953)', () => { + for (const { name, record, fields } of CASES) { + it(`agree: ${name}`, () => { + // Independent copies — each function mutates its argument in + // place, and the two must not be run over the SAME object. + const localInput = { ...record }; + const canonicalInput = { ...record }; + + const localResult = localMaterialize(localInput, fields); + const canonicalResult = canonicalMaterialize(canonicalInput, fields); + + expect(localResult).toEqual(canonicalResult); + }); + } + + it('agree on the RETURN-VALUE IDENTITY contract too (both return the same object they mutated)', () => { + const localInput = { a: 'x' }; + const canonicalInput = { a: 'x' }; + expect(localMaterialize(localInput, { a: {}, b: {} })).toBe(localInput); + expect(canonicalMaterialize(canonicalInput, { a: {}, b: {} })).toBe(canonicalInput); + }); + + // ── the harness is a real tripwire, not a tautology ───────────────── + it('DEMONSTRATION: a synthetic divergence is actually caught by this comparison', () => { + // A deliberately WRONG third implementation: defaults a missing + // declared field to the STRING `'null'` instead of the value + // `null` — the kind of one-character regression a future edit to + // either copy could introduce unnoticed. + function brokenMaterialize( + record: Record, + fields: Record | undefined | null, + ): Record { + if (!fields || typeof fields !== 'object') return record; + for (const name of Object.keys(fields)) { + if (record[name] === undefined) record[name] = 'null'; // <- the injected bug + } + return record; + } + + const fields = { a: {}, b: {} }; + // Widened explicitly: the canonical signature is generic + // (`>(record: T, …): T`, preserving + // the exact input shape) precisely so a caller who DOES know its + // object's real field set gets it back typed — `rule-validator.ts` + // relies on that. An inline `{ b: 'x' }` literal would otherwise infer + // `T = { b: string }`, and `.a` below would be a compile error despite + // being genuinely present at runtime after materialization. + const canonicalResult = canonicalMaterialize({ b: 'x' } as Record, fields); + const brokenResult = brokenMaterialize({ b: 'x' }, fields); + + // If this assertion ever failed to hold, the parity `it`s above + // would not be trustworthy evidence of anything — they would pass + // regardless of what either real implementation did. + expect(brokenResult).not.toEqual(canonicalResult); + expect(brokenResult.a).toBe('null'); + expect(canonicalResult.a).toBeNull(); + }); +}); diff --git a/packages/triggers/trigger-record-change/src/record-change-integration.test.ts b/packages/triggers/trigger-record-change/src/record-change-integration.test.ts index 37568dd0db..ed7b4ad005 100644 --- a/packages/triggers/trigger-record-change/src/record-change-integration.test.ts +++ b/packages/triggers/trigger-record-change/src/record-change-integration.test.ts @@ -28,8 +28,31 @@ import { ObjectKernel } from '@objectstack/core'; import { ObjectQLPlugin } from '@objectstack/objectql'; import { SqlDriver } from '@objectstack/driver-sql'; import { AutomationServicePlugin, type AutomationEngine } from '@objectstack/service-automation'; +import type { IDataEngine, IObjectQLEngine } from '@objectstack/spec/contracts'; import { RecordChangeTriggerPlugin } from './plugin.js'; +/** + * `check:slot-lookup` (#4251) — a NEW `kernel.getService(...)` site must carry + * the slot's real contract type, never an `as any` erasure (this file's other + * lookups predate the ratchet and are grandfathered by count, not by file — + * see `scripts/slot-lookup-baseline.json`). + * + * `IObjectQLEngine` covers everything the `objectql` slot's PUBLISHED contract + * promises. Two things the test below also calls are real, public methods on + * the concrete `ObjectQL` engine but are deliberately NOT part of that + * contract — `syncSchemas()` (a boot-time operation, not a slot consumer's + * concern) and `registry.registerObject` (the registry's TEST-time seam; + * `bulk-write-per-row-context.test.ts` in this same package casts the same + * gap separately as `TestObjectRegistry`). Naming both here, once, keeps the + * lookup itself fully typed rather than reaching back for `any`. + */ +type TestObjectQLEngine = IObjectQLEngine & { + syncSchemas(): Promise; + registry: IObjectQLEngine['registry'] & { + registerObject(schema: unknown, packageId?: string, namespace?: string): void; + }; +}; + const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); /** @@ -432,14 +455,26 @@ describe('record-change trigger — end-to-end (#1491)', () => { * total, with no assist from the after-row merge #1872 fixed. */ it('a record-before-write start condition sees an UNTOUCHED declared field\'s real value, and a NEVER-SET one as null — not a fault (#4953)', async () => { - const kernel = new ObjectKernel({ logLevel: 'silent' }); + // `{ logger: { level: 'silent' } }`, not this file's OTHER `{ logLevel: + // 'silent' }` — `ObjectKernelConfig` declares only `logger` + // (`packages/core/src/kernel.ts`); the sibling spelling is an untyped + // excess property `tsc` never catches on this test-hiding package (a + // pre-existing `check:type-check-debt` TEST_DEBT site, not this PR's to + // sweep) and, measurably, does NOTHING — `createLogger(config.logger)` + // never reads it, so every sibling `it` below actually logs at its + // default level despite the option. + const kernel = new ObjectKernel({ logger: { level: 'silent' } }); await kernel.use(new ObjectQLPlugin()); await kernel.use(new AutomationServicePlugin()); await kernel.use(new RecordChangeTriggerPlugin()); await kernel.bootstrap(); - const objectql = kernel.getService('objectql') as any; - const data = kernel.getService('data') as any; + // Typed via the slot's contract (`check:slot-lookup` ratchet, #4251) — + // see the `TestObjectQLEngine` doc comment at the top of this file for + // why it is `IObjectQLEngine` PLUS the two concrete-engine members the + // published contract omits, rather than `as any`. + const objectql = kernel.getService('objectql'); + const data = kernel.getService('data'); const automation = kernel.getService('automation'); await attachSqlite(objectql); diff --git a/packages/triggers/trigger-record-change/src/record-change-trigger.test.ts b/packages/triggers/trigger-record-change/src/record-change-trigger.test.ts index ef8042429f..ad8c963436 100644 --- a/packages/triggers/trigger-record-change/src/record-change-trigger.test.ts +++ b/packages/triggers/trigger-record-change/src/record-change-trigger.test.ts @@ -17,7 +17,7 @@ import { RecordChangeTriggerPlugin } from './plugin.js'; /** CEL-evaluate `source` against `record` — the SAME engine the automation * service and rule-validator use, so a `record.a != null` fault/pass here is * the real fault a flow's start/edge condition would hit, not a stand-in. */ -function evalCel(source: string, ctx: { record?: unknown; previous?: unknown }) { +function evalCel(source: string, ctx: { record?: Record; previous?: Record }) { return ExpressionEngine.evaluate({ dialect: 'cel', source }, ctx); } diff --git a/packages/triggers/trigger-record-change/src/record-change-trigger.ts b/packages/triggers/trigger-record-change/src/record-change-trigger.ts index a8a085d034..14df1189f2 100644 --- a/packages/triggers/trigger-record-change/src/record-change-trigger.ts +++ b/packages/triggers/trigger-record-change/src/record-change-trigger.ts @@ -134,8 +134,16 @@ export interface TriggerLogger { * returns it. Callers that must not mutate a shared object — this file's own * `ctx.previous`, observed by every OTHER binding sharing the same * HookContext — pass a shallow copy in. + * + * Exported (module-scope only — NOT re-exported from `index.ts`, so this + * stays off the package's published API) so + * `materialize-declared-fields-parity.test.ts` can run it head-to-head + * against the canonical copy in `@objectstack/objectql`'s + * `declared-fields.ts` and fail the moment the two disagree — a duplicated + * algorithm with nothing checking it stays duplicated is exactly the + * declared-vs-enforced gap this platform treats as a bug. */ -function materializeDeclaredFields( +export function materializeDeclaredFields( record: Record, fields: Record | undefined | null, ): Record { From 6c8cd0fc0fbb6c1ea42e31eaf7a69d464723b824 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 16:05:33 +0000 Subject: [PATCH 3/4] chore(objectql): changeset for the new materializeDeclaredFields export Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RDTnVvsgA6cUZ4xFVtPZRy --- .../objectql-export-materialize-declared-fields.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 .changeset/objectql-export-materialize-declared-fields.md diff --git a/.changeset/objectql-export-materialize-declared-fields.md b/.changeset/objectql-export-materialize-declared-fields.md new file mode 100644 index 0000000000..829642862e --- /dev/null +++ b/.changeset/objectql-export-materialize-declared-fields.md @@ -0,0 +1,12 @@ +--- +"@objectstack/objectql": patch +--- + +feat(objectql): publish `materializeDeclaredFields` from `@objectstack/objectql/core` (#4953) + +Was an internal-only module (`declared-fields.ts`) shared by `rule-validator.ts` and +`hook-wrappers.ts` via relative import. Published from the `./core` entry so a package +that structurally mirrors the algorithm for its own reasons (`@objectstack/trigger-record-change`, +which keeps zero build-time dependency on `objectql`) has a test-time way to verify its +copy still agrees with the canonical one, instead of the two silently drifting behind a +doc comment's word. No behavior change to the function itself. From d3d1e1c77e9be464b5a90f42c96461f78b990434 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 16:13:07 +0000 Subject: [PATCH 4/4] fix(docs): flows.mdx linked a validation.mdx anchor that does not exist check:doc-anchors caught it (ESLint was the reported job name, not the real failure -- the fourth such mismatch today in this repo). The has(x)-is-not- a-null-guard callout added in validation.mdx has no heading of its own (it sits under "Basic Structure"), so the inferred #has-x-is-not-a-null-guard fragment never existed. Verified real heading ids with the gate's own headingIds() rather than hand-deriving a slug; dropped the fragment and linked the page, naming the callout in prose instead. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RDTnVvsgA6cUZ4xFVtPZRy --- content/docs/automation/flows.mdx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index 964318194a..cd7571d86b 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -1251,8 +1251,8 @@ value before the condition. **`record` / `previous` are *total* over the object's declared fields** — the -same guarantee [validation predicates](/docs/data-modeling/validation#has-x-is-not-a-null-guard) -carry. A field this write didn't touch (and, for `record`, a field the driver +same guarantee [validation predicates](/docs/data-modeling/validation) +carry (see the `has(x)` is not a null guard callout under "Basic Structure"). A field this write didn't touch (and, for `record`, a field the driver didn't echo back) still reads as an explicit `null` rather than dropping out of scope, so `record.discount != null` and `previous.stage != previous_stage` evaluate cleanly instead of aborting with `No such key`. `has(record.x)` is