fix(automation,objectql): conditional & record-change flows silently skipping

.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.

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);

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<string, unknown> | null = null;
            const updateSchema = this._registry.getObject(object);
            if (hookContext.input.id) {
                await this.encryptSecretFields(object, hookContext.input.data as Record<string, unknown>, opCtx.context, hookContext.input.options);
                validateRecord(updateSchema, hookContext.input.data as Record<string, unknown>, '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<string, unknown> | 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<string, unknown>, 'update', { previous, logger: this.logger });
+               evaluateValidationRules(updateSchema as any, hookContext.input.data as Record<string, unknown>, 'update', { previous: priorRecord, logger: this.logger });
                result = await driver.update(object, hookContext.input.id as string, hookContext.input.data as Record<string, unknown>, hookContext.input.options as any);
            } else if (options?.multi && driver.updateMany) {
                await this.encryptSecretFields(object, hookContext.input.data as Record<string, unknown>, 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

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<string, unknown>();
+        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<string, unknown>();
+        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<string, unknown>();
         // These should all return false safely

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<string, unknown> = {};
                 for (const [key, value] of variables) {
                     // Convert "step.result" keys into nested object paths.