diff --git a/.changeset/fix-conditional-flows-skip.md b/.changeset/fix-conditional-flows-skip.md new file mode 100644 index 000000000..581e19ca3 --- /dev/null +++ b/.changeset/fix-conditional-flows-skip.md @@ -0,0 +1,28 @@ +--- +"@objectstack/service-automation": patch +"@objectstack/objectql": patch +--- + +Fix conditional & record-change flows silently skipping. + +Two bugs together caused every flow with a start-node / edge **condition** to +silently skip (record-change triggers fired but the flow body never ran; +audit-style `previous.*` gates and `budget > 100000`-style gates all evaluated +to false): + +- **service-automation — CEL engine unreachable in ESM.** The condition + evaluator loaded the formula engine via a CommonJS `require('@objectstack/formula')`. + In the package's ESM build (`"type": "module"`) that resolves to tsup's + throwing `__require` stub, so **every** CEL evaluation threw and the + swallowing `catch` returned `false`. Replaced with a static top-level import, + which binds correctly in both the ESM and CJS builds. + +- **objectql — prior record not exposed to update hooks.** `HookContext` + documents a `previous` snapshot for update/delete, but `engine.update` never + populated it (the row it fetched for validation was a local var). Record-change + conditions like `status == "done" && previous.status != "done"` therefore had + no `previous` to read. The engine now attaches the pre-update record to + `hookContext.previous` for single-id updates whenever a validation rule needs + it or an `afterUpdate` hook is registered. + +Both paths are covered by new unit tests. diff --git a/packages/objectql/src/engine.test.ts b/packages/objectql/src/engine.test.ts index e5bab78ca..e2db2e7bf 100644 --- a/packages/objectql/src/engine.test.ts +++ b/packages/objectql/src/engine.test.ts @@ -245,6 +245,36 @@ describe('ObjectQL Engine', () => { }); }); + describe('Update hooks — previous-record snapshot', () => { + beforeEach(async () => { + engine.registerDriver(mockDriver, true); + await engine.init(); + vi.mocked(SchemaRegistry.getObject).mockReturnValue({ name: 'task', fields: {} } as any); + }); + + it('attaches the pre-update record to afterUpdate ctx.previous when an afterUpdate hook is registered', async () => { + // Regression: record-change flow triggers read `previous.*` in their + // start condition (e.g. `status == "done" && previous.status != "done"`). + // The engine must expose the pre-update row on the hook context. + vi.mocked(mockDriver.findOne).mockResolvedValue({ id: 't1', status: 'in_review', assignee: 'sam@example.com' }); + vi.mocked(mockDriver.update).mockResolvedValue({ id: 't1', status: 'done', assignee: 'sam@example.com' } as any); + + let captured: any = 'UNSET'; + engine.registerHook('afterUpdate', async (ctx: any) => { captured = ctx.previous; }, { packageId: 'test' } as any); + + await engine.update('task', { id: 't1', status: 'done' }); + + expect(mockDriver.findOne).toHaveBeenCalled(); + expect(captured).toEqual({ id: 't1', status: 'in_review', assignee: 'sam@example.com' }); + }); + + it('does not fetch the prior record when no afterUpdate hook is registered and no rule needs it', async () => { + vi.mocked(mockDriver.update).mockResolvedValue({ id: 't1' } as any); + await engine.update('task', { id: 't1', status: 'done' }); + expect(mockDriver.findOne).not.toHaveBeenCalled(); + }); + }); + describe('Expand Related Records', () => { beforeEach(async () => { engine.registerDriver(mockDriver, true); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 45d0d91d3..cff8b98a0 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -1850,19 +1850,25 @@ export class ObjectQL implements IDataEngine { try { let result; + // Pre-update snapshot. Exposed to after-hooks via `hookContext.previous` + // (the HookContext contract documents `previous` for update/delete) and + // reused for object-level validation rules. Fetched once, only for + // single-id updates, when either a rule needs it (ADR-0020: + // state_machine / cross_field / script — a PATCH carries only changed + // fields) OR an afterUpdate hook is registered. The latter is what makes + // 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. + let priorRecord: Record | null = null; const updateSchema = this._registry.getObject(object); if (hookContext.input.id) { await this.encryptSecretFields(object, hookContext.input.data as Record, opCtx.context, hookContext.input.options); validateRecord(updateSchema, hookContext.input.data as Record, 'update'); - // ADR-0020: object-level rules (state_machine / cross_field / - // script) need the prior record — a PATCH carries only changed - // fields. Fetch it once, only when such a rule is declared. - let previous: Record | null = null; - if (needsPriorRecord(updateSchema as any)) { + if (needsPriorRecord(updateSchema as any) || (this.hooks.get('afterUpdate')?.length ?? 0) > 0) { const priorAst: QueryAST = { object, where: { id: hookContext.input.id }, limit: 1 }; - previous = await driver.findOne(object, priorAst, hookContext.input.options as any); + priorRecord = await driver.findOne(object, priorAst, hookContext.input.options as any); } - evaluateValidationRules(updateSchema as any, hookContext.input.data as Record, 'update', { previous, logger: this.logger }); + evaluateValidationRules(updateSchema as any, hookContext.input.data as Record, 'update', { previous: priorRecord, logger: this.logger }); result = await driver.update(object, hookContext.input.id as string, hookContext.input.data as Record, hookContext.input.options as any); } else if (options?.multi && driver.updateMany) { await this.encryptSecretFields(object, hookContext.input.data as Record, opCtx.context, hookContext.input.options); @@ -1881,6 +1887,7 @@ export class ObjectQL implements IDataEngine { hookContext.event = 'afterUpdate'; hookContext.result = result; + if (priorRecord) hookContext.previous = priorRecord; await this.triggerHooks('afterUpdate', hookContext); // Publish data.record.updated event to realtime service diff --git a/packages/services/service-automation/src/engine.test.ts b/packages/services/service-automation/src/engine.test.ts index 864f7d004..e3cc26132 100644 --- a/packages/services/service-automation/src/engine.test.ts +++ b/packages/services/service-automation/src/engine.test.ts @@ -1332,6 +1332,25 @@ describe('AutomationEngine - Safe Expression Evaluation', () => { expect(engine.evaluateCondition('false', vars)).toBe(false); }); + it('evaluates CEL record-change conditions with bare fields and previous.* snapshot', () => { + // The record-change trigger shape: the new record's fields are seeded at + // top level, plus a `previous` snapshot. Regression guard — a broken + // ExpressionEngine binding (e.g. an ESM `require` stub) made the CEL path + // throw and silently return false, skipping every record-change flow. + const vars = new Map(); + vars.set('status', 'done'); + vars.set('assignee', 'newowner@example.com'); + vars.set('previous', { status: 'in_review', assignee: 'ada@example.com' }); + + expect(engine.evaluateCondition({ dialect: 'cel', source: 'status == "done" && previous.status != "done"' }, vars)).toBe(true); + expect(engine.evaluateCondition({ dialect: 'cel', source: 'assignee != previous.assignee' }, vars)).toBe(true); + + const unchanged = new Map(); + unchanged.set('status', 'done'); + unchanged.set('previous', { status: 'done' }); + expect(engine.evaluateCondition({ dialect: 'cel', source: 'status == "done" && previous.status != "done"' }, unchanged)).toBe(false); + }); + it('should not execute malicious code', () => { const vars = new Map(); // These should all return false safely diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index f0a219154..afb7148c1 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -7,6 +7,13 @@ import type { Logger } from '@objectstack/spec/contracts'; import { FlowSchema, FLOW_STRUCTURAL_NODE_TYPES } from '@objectstack/spec/automation'; import type { Connector } from '@objectstack/spec/integration'; import { ConnectorSchema } from '@objectstack/spec/integration'; +// Static import (not a lazy `require`): the engine ships as ESM ("type":"module"), +// where a CommonJS `require('@objectstack/formula')` resolves to tsup's throwing +// `__require` stub. That threw on every CEL evaluation and the catch below +// silently returned `false`, so EVERY start-node / edge condition (record-change +// `previous.*`, `budget > 100000`, …) skipped its flow. A static import binds the +// engine at module load in both ESM and CJS builds. +import { ExpressionEngine } from '@objectstack/formula'; // ─── Node Executor Interface (Plugin Extension Point) ─────────────── @@ -1287,8 +1294,6 @@ export class AutomationEngine implements IAutomationService { // the equivalent `vars.step.result` CEL identifier path. if (dialect === 'cel' || (isEnvelope && !dialect)) { try { - // eslint-disable-next-line @typescript-eslint/no-require-imports - const { ExpressionEngine } = require('@objectstack/formula') as typeof import('@objectstack/formula'); const vars: Record = {}; for (const [key, value] of variables) { // Convert "step.result" keys into nested object paths.