diff --git a/.changeset/update-record-dropped-field-warnings.md b/.changeset/update-record-dropped-field-warnings.md new file mode 100644 index 0000000000..b38d5403e3 --- /dev/null +++ b/.changeset/update-record-dropped-field-warnings.md @@ -0,0 +1,34 @@ +--- +"@objectstack/spec": minor +"@objectstack/objectql": minor +"@objectstack/service-automation": minor +--- + +feat(automation): surface silently-stripped write fields as step warnings (#3407) + +`update_record` used to report an unconditional `success` even when the data +layer legally stripped the requested write fields — static `readonly` (#2948) +or a TRUE `readonlyWhen` predicate (#3042). The only trace was a server-side +logger warn, invisible in the flow run trace: an author saw a clean 3ms +`success` while the DB truth never changed (how #3356's approval stage +write-backs failed unnoticed). + +- **spec**: new `DroppedFieldsEventSchema` / `DroppedFieldsEvent` + (`{ object, fields, reason: 'readonly' | 'readonly_when' }`) in + `data/data-engine.zod.ts`, and a `WriteObservabilityOptions` + (`onFieldsDropped` listener) mixin on `IDataEngine.insert/update` option + params in `contracts/data-engine.ts`. The listener is a TS-contract-level, + in-process-only channel — deliberately NOT part of the serializable Zod + options schemas or the RPC boundary. +- **objectql**: `engine.update()` reports each strip pass's dropped keys + + reason through `options.onFieldsDropped` (all four strip sites: single-id + + bulk × readonly + readonlyWhen). A throwing listener never breaks the write. + System-context writes skip the readonly strip and therefore report nothing, + as before. `insert()` accepts the option for symmetry but strips nothing + today (INSERT is readonly-exempt; FLS write denial throws). +- **service-automation**: `NodeExecutionResult` and `StepLogEntry` gain + advisory `warnings?: string[]`; `update_record` / `create_record` attach one + warning per strip event naming the dropped fields, plus a structured + `droppedFields` output (`{.droppedFields}`) for downstream nodes. + `success` semantics are unchanged — stripping stays legal, it just is no + longer silent. diff --git a/content/docs/kernel/contracts/data-engine.mdx b/content/docs/kernel/contracts/data-engine.mdx index 9e4752f3bf..8814e26dca 100644 --- a/content/docs/kernel/contracts/data-engine.mdx +++ b/content/docs/kernel/contracts/data-engine.mdx @@ -37,9 +37,9 @@ export interface IDataEngine { count(objectName: string, query?: EngineCountOptions): Promise; aggregate(objectName: string, query: EngineAggregateOptions): Promise; - // Mutation - insert(objectName: string, data: any | any[], options?: DataEngineInsertOptions): Promise; - update(objectName: string, data: any, options?: EngineUpdateOptions): Promise; + // Mutation (write ops also accept in-process WriteObservabilityOptions — see `update`) + insert(objectName: string, data: any | any[], options?: DataEngineInsertOptions & WriteObservabilityOptions): Promise; + update(objectName: string, data: any, options?: EngineUpdateOptions & WriteObservabilityOptions): Promise; delete(objectName: string, options?: EngineDeleteOptions): Promise; // AI / Vector Search (optional) @@ -222,6 +222,36 @@ interface EngineUpdateOptions { } ``` +### WriteObservabilityOptions + +The write methods (`insert` / `update`) additionally accept an **in-process** +`onFieldsDropped` listener. The engine invokes it when caller-supplied write +fields are legally stripped from the payload before the driver write — static +`readonly` fields or a TRUE `readonlyWhen` predicate. The write still succeeds; +the listener exists so callers that report per-field success (e.g. the flow +engine's `update_record` step) can surface a warning instead of a silent +success. + +```typescript +interface WriteObservabilityOptions { + onFieldsDropped?: (event: DroppedFieldsEvent) => void; +} + +interface DroppedFieldsEvent { + object: string; // resolved object name + fields: string[]; // caller-supplied fields that were dropped + reason: 'readonly' | 'readonly_when'; // why they were dropped +} +``` + + +`onFieldsDropped` is a **TS-contract-level, in-process-only** channel. It is +deliberately not part of the serializable Zod options schemas: a function is +unrepresentable in JSON Schema and cannot cross the RPC (Virtual Data Engine) +boundary, so remote callers never receive these events. A listener that throws +never breaks the write — the engine catches and logs. + + ### delete Deletes record(s) matching the `where` condition. diff --git a/packages/core/src/contracts/data-engine.ts b/packages/core/src/contracts/data-engine.ts index 19a1e2e6e3..299374154f 100644 --- a/packages/core/src/contracts/data-engine.ts +++ b/packages/core/src/contracts/data-engine.ts @@ -1,38 +1,50 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. -import { +import { EngineQueryOptions, - DataEngineInsertOptions, - EngineUpdateOptions, + DataEngineInsertOptions, + EngineUpdateOptions, EngineDeleteOptions, - EngineAggregateOptions, + EngineAggregateOptions, EngineCountOptions, DataEngineRequest, + DroppedFieldsEvent, } from '@objectstack/spec/data'; +/** + * In-process write-observability hooks for `insert`/`update` (#3407). + * Mirror of `WriteObservabilityOptions` in `@objectstack/spec/contracts` — + * see that definition for the full rationale (in-process only; never part of + * the serializable options schemas or the RPC boundary). + */ +export interface WriteObservabilityOptions { + /** Called once per strip pass that dropped ≥1 caller-supplied field. */ + onFieldsDropped?: (event: DroppedFieldsEvent) => void; +} + /** * IDataEngine - Standard Data Engine Interface - * + * * Abstract interface for data persistence capabilities. * Following the Dependency Inversion Principle - plugins depend on this interface, * not on concrete database implementations. - * + * * All query methods use standard QueryAST parameter names * (where/fields/orderBy/limit/offset/expand) to eliminate mechanical translation * between the Engine and Driver layers. - * + * * Aligned with 'src/data/data-engine.zod.ts' in @objectstack/spec. */ export interface IDataEngine { find(objectName: string, query?: EngineQueryOptions): Promise; findOne(objectName: string, query?: EngineQueryOptions): Promise; - insert(objectName: string, data: any | any[], options?: DataEngineInsertOptions): Promise; - update(objectName: string, data: any, options?: EngineUpdateOptions): Promise; + insert(objectName: string, data: any | any[], options?: DataEngineInsertOptions & WriteObservabilityOptions): Promise; + update(objectName: string, data: any, options?: EngineUpdateOptions & WriteObservabilityOptions): Promise; delete(objectName: string, options?: EngineDeleteOptions): Promise; count(objectName: string, query?: EngineCountOptions): Promise; aggregate(objectName: string, query: EngineAggregateOptions): Promise; - + /** * Vector Search (AI/RAG) */ @@ -48,5 +60,3 @@ export interface IDataEngine { */ execute?(command: any, options?: Record): Promise; } - - diff --git a/packages/objectql/src/engine.test.ts b/packages/objectql/src/engine.test.ts index c01297737e..62e4637f8b 100644 --- a/packages/objectql/src/engine.test.ts +++ b/packages/objectql/src/engine.test.ts @@ -1002,6 +1002,112 @@ describe('ObjectQL Engine', () => { }); }); + /** + * #3407 — the readonly/readonlyWhen strips are legal semantics, but they + * must not be SILENT to the caller: `options.onFieldsDropped` reports each + * strip pass's dropped keys + reason so callers (flow `update_record` + * steps) can surface a warning on an otherwise-successful write. + */ + describe('Dropped-field write observability (#3407)', () => { + beforeEach(async () => { + engine.registerDriver(mockDriver, true); + await engine.init(); + (mockDriver as any).updateMany = vi.fn().mockResolvedValue(1); + }); + + const docSchema = { + name: 'doc', + fields: { + title: { type: 'text' }, + created_by: { type: 'text', readonly: true }, + }, + } as any; + + it('reports caller-supplied static-readonly fields stripped on a single-id update', async () => { + vi.mocked(SchemaRegistry.getObject).mockReturnValue(docSchema); + const events: any[] = []; + await engine.update('doc', { id: '1', title: 'x', created_by: 'attacker' }, { + onFieldsDropped: (e: any) => events.push(e), + } as any); + + expect(events).toEqual([{ object: 'doc', fields: ['created_by'], reason: 'readonly' }]); + // The strip behavior itself is unchanged: the write proceeded without the field. + const [, , data] = vi.mocked(mockDriver.update).mock.calls[0]; + expect(data).not.toHaveProperty('created_by'); + expect(data).toHaveProperty('title', 'x'); + }); + + it('reports a readonlyWhen-locked field on a single-id update (reason readonly_when)', async () => { + vi.mocked(SchemaRegistry.getObject).mockReturnValue({ + name: 'invoice', + fields: { amount: { type: 'number', readonlyWhen: 'record.locked == true' } }, + } as any); + vi.mocked(mockDriver.findOne).mockResolvedValue({ id: '1', locked: true, amount: 100 } as any); + + const events: any[] = []; + await engine.update('invoice', { id: '1', amount: 999 }, { + onFieldsDropped: (e: any) => events.push(e), + } as any); + + expect(events).toEqual([{ object: 'invoice', fields: ['amount'], reason: 'readonly_when' }]); + const [, , data] = vi.mocked(mockDriver.update).mock.calls[0]; + expect(data).not.toHaveProperty('amount'); + }); + + it('reports bulk-path strips too (multi update — locked in ≥1 matched row)', async () => { + vi.mocked(SchemaRegistry.getObject).mockReturnValue({ + name: 'invoice', + fields: { amount: { type: 'number', readonlyWhen: 'record.locked == true' } }, + } as any); + vi.mocked(mockDriver.find).mockResolvedValue([ + { id: 'a', locked: false, amount: 10 }, + { id: 'b', locked: true, amount: 20 }, + ] as any); + + const events: any[] = []; + await engine.update('invoice', { amount: 999 }, { + where: { status: 'draft' }, multi: true, + onFieldsDropped: (e: any) => events.push(e), + } as any); + + expect(events).toEqual([{ object: 'invoice', fields: ['amount'], reason: 'readonly_when' }]); + const [, , data] = (mockDriver as any).updateMany.mock.calls[0]; + expect(data).not.toHaveProperty('amount'); + }); + + it('does NOT report for a system-context update (the readonly strip is skipped)', async () => { + vi.mocked(SchemaRegistry.getObject).mockReturnValue(docSchema); + const events: any[] = []; + await engine.update('doc', { id: '1', created_by: 'importer' }, { + context: { isSystem: true }, + onFieldsDropped: (e: any) => events.push(e), + } as any); + expect(events).toEqual([]); + // System writes legitimately set read-only columns — kept, not stripped. + const [, , data] = vi.mocked(mockDriver.update).mock.calls[0]; + expect(data).toHaveProperty('created_by', 'importer'); + }); + + it('does NOT report when nothing was stripped', async () => { + vi.mocked(SchemaRegistry.getObject).mockReturnValue(docSchema); + const events: any[] = []; + await engine.update('doc', { id: '1', title: 'clean' }, { + onFieldsDropped: (e: any) => events.push(e), + } as any); + expect(events).toEqual([]); + }); + + it('a throwing listener never breaks the write', async () => { + vi.mocked(SchemaRegistry.getObject).mockReturnValue(docSchema); + await expect( + engine.update('doc', { id: '1', created_by: 'x' }, { + onFieldsDropped: () => { throw new Error('listener bug'); }, + } as any), + ).resolves.not.toThrow(); + expect(vi.mocked(mockDriver.update)).toHaveBeenCalledTimes(1); + }); + }); + 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 e818e33c94..da6fa0f234 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -8,8 +8,10 @@ import { EngineUpdateOptions, EngineDeleteOptions, EngineAggregateOptions, - EngineCountOptions + EngineCountOptions, + type DroppedFieldsEvent } from '@objectstack/spec/data'; +import type { WriteObservabilityOptions } from '@objectstack/spec/contracts'; import { parseAutonumberFormat, renderAutonumber, missingFieldValues, isTenancyDisabled } from '@objectstack/spec/data'; import { ExecutionContext, ExecutionContextSchema } from '@objectstack/spec/kernel'; import { IDataDriver, IDataEngine, Logger, createLogger, withTransientRetry, type RetryOptions } from '@objectstack/core'; @@ -2345,7 +2347,13 @@ export class ObjectQL implements IDataEngine { * validation passes, so a doomed attempt no longer consumes a sequence value * (no number-range gaps from a rejected batch). */ - async insert(object: string, data: any | any[], options?: DataEngineInsertOptions): Promise { + // [#3407] `WriteObservabilityOptions.onFieldsDropped` is accepted for + // signature symmetry with `update()` but never fires here: INSERT is + // deliberately exempt from the readonly/readonlyWhen strips (a create may + // legitimately seed read-only columns), and the FLS write gate throws + // instead of stripping. If insert ever gains a silent strip, wire the + // listener at that strip site — do not let it go silent. + async insert(object: string, data: any | any[], options?: DataEngineInsertOptions & WriteObservabilityOptions): Promise { object = this.resolveObjectName(object); this.logger.debug('Insert operation starting', { object, isBatch: Array.isArray(data) }); this.assertWriteAllowed(object, 'insert'); @@ -2597,7 +2605,7 @@ export class ObjectQL implements IDataEngine { return this.insert(object, rows, { ...(options ?? {}), __partialRowErrors: true } as any); } - async update(object: string, data: any, options?: EngineUpdateOptions): Promise { + async update(object: string, data: any, options?: EngineUpdateOptions & WriteObservabilityOptions): Promise { object = this.resolveObjectName(object); this.logger.debug('Update operation starting', { object }); this.assertWriteAllowed(object, 'update'); @@ -2650,6 +2658,33 @@ export class ObjectQL implements IDataEngine { Object.keys((opCtx.data ?? {}) as Record), ); + // [#3407] Structured strip observability. The readonly/readonlyWhen strips + // below are LEGAL semantics (the write still succeeds without the locked + // fields), but until now the only trace was a server-side logger warn — a + // caller that reports success per requested field (a flow's `update_record` + // step) saw a clean success while the DB value never changed. When the + // caller registers `onFieldsDropped`, report each strip pass's dropped + // keys back with its reason. Diffing before/after key sets is exact here: + // every strip helper returns the SAME reference when nothing was dropped, + // else a shallow copy with keys removed. A listener fault must never break + // the write. + const onFieldsDropped = options?.onFieldsDropped; + const reportDroppedFields = ( + before: Record | null | undefined, + after: Record | null | undefined, + reason: DroppedFieldsEvent['reason'], + ): void => { + if (!onFieldsDropped || before === after || !before) return; + const afterObj = (after ?? {}) as Record; + const fields = Object.keys(before).filter((k) => !(k in afterObj)); + if (fields.length === 0) return; + try { + onFieldsDropped({ object, fields, reason }); + } catch (err) { + this.logger.warn('onFieldsDropped listener threw — ignored', { object, error: err }); + } + }; + await this.executeWithMiddleware(opCtx, async () => { const hookContext: HookContext = { object, @@ -2688,14 +2723,18 @@ export class ObjectQL implements IDataEngine { // B2: drop writes to fields locked by a TRUE `readonlyWhen` — the // field is read-only for this record's state, so the incoming // change is ignored (the persisted value is kept). - hookContext.input.data = stripReadonlyWhenFields(updateSchema as any, hookContext.input.data as Record, priorRecord, this.logger) as any; + const preRoWhen = hookContext.input.data as Record; + hookContext.input.data = stripReadonlyWhenFields(updateSchema as any, preRoWhen, priorRecord, this.logger) as any; + reportDroppedFields(preRoWhen, hookContext.input.data as Record, 'readonly_when'); // [#2948] Enforce STATIC `readonly` on the write path for // non-system callers (system writes legitimately set read-only // columns and are exempt). Runs AFTER hooks/middleware stamped // their columns; `suppliedKeys` ensures only caller-forged // read-only writes are dropped, never the server stamps. if (!opCtx.context?.isSystem) { - hookContext.input.data = stripReadonlyFields(updateSchema as any, hookContext.input.data as Record, suppliedKeys, this.logger) as any; + const preRo = hookContext.input.data as Record; + hookContext.input.data = stripReadonlyFields(updateSchema as any, preRo, suppliedKeys, this.logger) as any; + reportDroppedFields(preRo, hookContext.input.data as Record, 'readonly'); } evaluateValidationRules(updateSchema as any, hookContext.input.data as Record, 'update', { previous: priorRecord, logger: this.logger, currentUser: this.buildEvalUser(opCtx.context) }); result = await driver.update(object, hookContext.input.id as string, hookContext.input.data as Record, hookContext.input.options as any); @@ -2739,14 +2778,18 @@ export class ObjectQL implements IDataEngine { // `where` to reach the unlocked rows). Symmetric with the // single-id `stripReadonlyWhenFields`; INSERT stays exempt. if (payloadHasReadonlyWhen) { - hookContext.input.data = stripReadonlyWhenFieldsMulti(updateSchema as any, hookContext.input.data as Record, priorRows, this.logger) as any; + const preRoWhenMulti = hookContext.input.data as Record; + hookContext.input.data = stripReadonlyWhenFieldsMulti(updateSchema as any, preRoWhenMulti, priorRows, this.logger) as any; + reportDroppedFields(preRoWhenMulti, hookContext.input.data as Record, 'readonly_when'); } // [#2948] Same static-`readonly` write guard on the bulk path — // a forged read-only column in a multi-row update is dropped for // non-system callers (a foreign `organization_id` is additionally // rejected upstream by the tenant write wall, #2946). if (!opCtx.context?.isSystem) { - hookContext.input.data = stripReadonlyFields(updateSchema as any, hookContext.input.data as Record, suppliedKeys, this.logger) as any; + const preRoMulti = hookContext.input.data as Record; + hookContext.input.data = stripReadonlyFields(updateSchema as any, preRoMulti, suppliedKeys, this.logger) as any; + reportDroppedFields(preRoMulti, hookContext.input.data as Record, 'readonly'); } // [#3106] Same enforcement the single-id branch runs at its // `evaluateValidationRules` call, applied per matched row: any diff --git a/packages/services/service-automation/src/builtin/crud-dropped-fields.test.ts b/packages/services/service-automation/src/builtin/crud-dropped-fields.test.ts new file mode 100644 index 0000000000..4259c3a01f --- /dev/null +++ b/packages/services/service-automation/src/builtin/crud-dropped-fields.test.ts @@ -0,0 +1,200 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3407 — `update_record` used to report an unconditional `success` even when + * the data layer silently stripped the requested write fields (static + * `readonly` #2948, conditional `readonlyWhen` #3042): the strip's only trace + * was a server-side logger warn, invisible in the flow run trace — which is how + * #3356's approval stage write-backs failed end-to-end behind a clean 3ms + * `success`. + * + * The engine now reports strips via `options.onFieldsDropped` + * (`WriteObservabilityOptions`, @objectstack/spec/contracts), and the CRUD + * nodes surface them as step WARNINGS (+ a structured `droppedFields` output) + * while keeping `success: true` — stripping is legal semantics, not a failure. + * These tests pin the node-side contract with a stub engine that invokes the + * listener exactly like ObjectQL does (the engine side is pinned in + * packages/objectql/src/engine.test.ts "Dropped-field write observability"). + */ +import { describe, it, expect } from 'vitest'; +import { AutomationEngine } from '../engine.js'; +import { registerCrudNodes } from './crud-nodes.js'; + +function makeLogger(): any { + const l: any = { info() {}, warn() {}, error() {}, debug() {} }; + l.child = () => l; + return l; +} + +const ctxWith = (data: any): any => ({ + logger: makeLogger(), + getService: (n: string) => (n === 'data' ? data : undefined), +}); + +/** + * A data engine stub whose write ops report `droppedEvents` through the + * caller's `onFieldsDropped` listener — the exact seam ObjectQL fires when its + * readonly/readonlyWhen strips drop caller-supplied fields. + */ +function fakeDataWithDrops(droppedEvents: Array<{ object: string; fields: string[]; reason: string }>) { + const writes: Array<{ op: string; obj: string; fields: any }> = []; + const data: any = { + async insert(obj: string, fields: any, opts: any) { + writes.push({ op: 'insert', obj, fields }); + for (const e of droppedEvents) opts?.onFieldsDropped?.(e); + return { id: `${obj}_1`, ...fields }; + }, + async update(obj: string, fields: any, opts: any) { + writes.push({ op: 'update', obj, fields }); + for (const e of droppedEvents) opts?.onFieldsDropped?.(e); + return { ok: true }; + }, + }; + return { data, writes }; +} + +function updateFlow(name: string) { + return { + name, label: name, type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'up', type: 'update_record', label: 'Update', config: { objectName: 'case_approval', filter: { id: 'x' }, fields: { stage: 'approved', note: 'ok' } } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'up' }, + { id: 'e2', source: 'up', target: 'end' }, + ], + } as any; +} + +describe('update_record surfaces silently-stripped write fields (#3407)', () => { + it('attaches a step warning naming the dropped fields — and the step still SUCCEEDS', async () => { + const engine = new AutomationEngine(makeLogger()); + const { data } = fakeDataWithDrops([ + { object: 'case_approval', fields: ['stage'], reason: 'readonly' }, + ]); + registerCrudNodes(engine, ctxWith(data)); + engine.registerFlow('f1', updateFlow('f1')); + + const res = await engine.execute('f1', { userId: 'u1' }); + expect(res.success).toBe(true); // stripping is legal — never a failure + + const runs = await engine.listRuns('f1'); + const step = runs[0].steps.find((s) => s.nodeId === 'up')!; + expect(step.status).toBe('success'); + expect(step.warnings).toHaveLength(1); + expect(step.warnings![0]).toContain('update_record(case_approval)'); + expect(step.warnings![0]).toContain('[stage]'); + expect(step.warnings![0]).toContain('read-only'); + }); + + it('exposes the dropped field names as structured output ({up.droppedFields})', async () => { + const engine = new AutomationEngine(makeLogger()); + const { data } = fakeDataWithDrops([ + { object: 'case_approval', fields: ['stage'], reason: 'readonly' }, + { object: 'case_approval', fields: ['locked_note'], reason: 'readonly_when' }, + ]); + registerCrudNodes(engine, ctxWith(data)); + + // Probe node reads the update step's output variable after it ran. + let seen: unknown; + engine.registerNodeExecutor({ + type: 'probe', + async execute(_node, variables) { + seen = variables.get('up.droppedFields'); + return { success: true }; + }, + }); + const flow = updateFlow('f2'); + flow.nodes.splice(2, 0, { id: 'probe', type: 'probe', label: 'Probe' }); + flow.edges = [ + { id: 'e1', source: 'start', target: 'up' }, + { id: 'e2', source: 'up', target: 'probe' }, + { id: 'e3', source: 'probe', target: 'end' }, + ]; + engine.registerFlow('f2', flow); + + await engine.execute('f2', { userId: 'u1' }); + expect(seen).toEqual(['stage', 'locked_note']); + + // One warning per strip event, each with its own reason wording. + const runs = await engine.listRuns('f2'); + const step = runs[0].steps.find((s) => s.nodeId === 'up')!; + expect(step.warnings).toHaveLength(2); + expect(step.warnings![1]).toContain('readonlyWhen'); + }); + + it('emits NO warnings and NO droppedFields output when nothing was stripped', async () => { + const engine = new AutomationEngine(makeLogger()); + const { data } = fakeDataWithDrops([]); + registerCrudNodes(engine, ctxWith(data)); + engine.registerFlow('f3', updateFlow('f3')); + + const res = await engine.execute('f3', { userId: 'u1' }); + expect(res.success).toBe(true); + + const runs = await engine.listRuns('f3'); + const step = runs[0].steps.find((s) => s.nodeId === 'up')!; + expect(step.status).toBe('success'); + expect(step.warnings).toBeUndefined(); + }); +}); + +describe('create_record is wired symmetrically (#3407)', () => { + // Today ObjectQL's insert path strips nothing (INSERT is readonly-exempt, + // FLS write denial throws) — but the node listens anyway, so a future + // insert-side strip surfaces instead of going silent. + it('surfaces insert-side drop events as step warnings, keeping success', async () => { + const engine = new AutomationEngine(makeLogger()); + const { data } = fakeDataWithDrops([ + { object: 'thing', fields: ['serial'], reason: 'readonly' }, + ]); + registerCrudNodes(engine, ctxWith(data)); + engine.registerFlow('c1', { + name: 'c1', label: 'c1', type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'mk', type: 'create_record', label: 'Create', config: { objectName: 'thing', fields: { serial: 'S-1', name: 'a' } } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'mk' }, + { id: 'e2', source: 'mk', target: 'end' }, + ], + } as any); + + const res = await engine.execute('c1', { userId: 'u1' }); + expect(res.success).toBe(true); + + const runs = await engine.listRuns('c1'); + const step = runs[0].steps.find((s) => s.nodeId === 'mk')!; + expect(step.status).toBe('success'); + expect(step.warnings).toHaveLength(1); + expect(step.warnings![0]).toContain('create_record(thing)'); + expect(step.warnings![0]).toContain('[serial]'); + }); + + it('stays warning-free on a clean insert', async () => { + const engine = new AutomationEngine(makeLogger()); + const { data } = fakeDataWithDrops([]); + registerCrudNodes(engine, ctxWith(data)); + engine.registerFlow('c2', { + name: 'c2', label: 'c2', type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'mk', type: 'create_record', label: 'Create', config: { objectName: 'thing', fields: { name: 'a' } } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'mk' }, + { id: 'e2', source: 'mk', target: 'end' }, + ], + } as any); + + await engine.execute('c2', { userId: 'u1' }); + const runs = await engine.listRuns('c2'); + const step = runs[0].steps.find((s) => s.nodeId === 'mk')!; + expect(step.warnings).toBeUndefined(); + }); +}); diff --git a/packages/services/service-automation/src/builtin/crud-nodes.ts b/packages/services/service-automation/src/builtin/crud-nodes.ts index bf2f0fdcb5..19a588e771 100644 --- a/packages/services/service-automation/src/builtin/crud-nodes.ts +++ b/packages/services/service-automation/src/builtin/crud-nodes.ts @@ -3,11 +3,29 @@ import type { PluginContext } from '@objectstack/core'; import { defineActionDescriptor } from '@objectstack/spec/automation'; import type { IDataEngine } from '@objectstack/spec/contracts'; +import type { DroppedFieldsEvent } from '@objectstack/spec/data'; import type { AutomationEngine } from '../engine.js'; import { interpolate } from './template.js'; import { readAliasedConfig } from './config-aliases.js'; import { resolveRunDataContext } from '../runtime-identity.js'; +/** + * #3407 — render a data-layer strip event as a step warning. The write itself + * SUCCEEDED; the warning tells the flow author which requested fields never + * landed and why, so the run trace is not a silent 3ms `success` (#3356's + * masked approval stage write-backs). `readonlyWhen` wording covers the bulk + * "locked in ≥1 matched row" semantics too — a multi-row update drops a + * conditionally-locked field for the whole batch. + */ +const DROPPED_REASON_LABEL: Record = { + readonly: 'the field is read-only (readonly: true)', + readonly_when: 'the field is conditionally read-only (readonlyWhen; on multi-row updates: locked in ≥1 matched row)', +}; + +function droppedFieldsWarning(nodeType: string, e: DroppedFieldsEvent): string { + return `${nodeType}(${e.object}): requested field(s) [${e.fields.join(', ')}] were NOT written — ${DROPPED_REASON_LABEL[e.reason] ?? e.reason}. The write succeeded without them.`; +} + /** * CRUD built-in nodes — `get_record` / `create_record` / `update_record` / * `delete_record`, wired to the runtime data layer (ObjectQL / IDataEngine). @@ -133,7 +151,16 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext): // #1888 — honor flow.runAs (system → RLS-bypassing; user → trigger user). const dataCtx = resolveRunDataContext(context); try { - const created = await data.insert(objectName, fields, { context: dataCtx }); + // #3407 — symmetric with update_record. Today the engine's + // insert path strips nothing (INSERT is readonly-exempt and + // FLS write denial throws), so this listener never fires; + // wired anyway so a future insert-side strip surfaces here + // automatically instead of going silent. + const dropped: DroppedFieldsEvent[] = []; + const created = await data.insert(objectName, fields, { + context: dataCtx, + onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); }, + }); const createdRecord = Array.isArray(created) ? created[0] : created; const insertedId = createdRecord && typeof createdRecord === 'object' @@ -148,7 +175,19 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext): createdRecord && typeof createdRecord === 'object' ? createdRecord : { id: insertedId }, ); } - return { success: true, output: { id: insertedId, record: createdRecord, object: objectName } }; + const droppedFields = dropped.flatMap((e) => e.fields); + return { + success: true, + output: { + id: insertedId, + record: createdRecord, + object: objectName, + ...(droppedFields.length > 0 ? { droppedFields } : {}), + }, + ...(dropped.length > 0 + ? { warnings: dropped.map((e) => droppedFieldsWarning('create_record', e)) } + : {}), + }; } catch (err) { return { success: false, error: `create_record(${objectName}) failed: ${(err as Error).message}` }; } @@ -194,8 +233,30 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext): // #1888 — honor flow.runAs (system → RLS-bypassing; user → trigger user). const dataCtx = resolveRunDataContext(context); try { - const result = await data.update(objectName, fields, { where: filter, context: dataCtx }); - return { success: true, output: { result, object: objectName } }; + // #3407 — collect the data layer's silently-stripped write + // fields (readonly / readonlyWhen). The strip is LEGAL — the + // update still succeeds — but the step must say which + // requested fields never landed instead of reporting a clean + // success while the DB truth stayed unchanged. + const dropped: DroppedFieldsEvent[] = []; + const result = await data.update(objectName, fields, { + where: filter, + context: dataCtx, + onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); }, + }); + const droppedFields = dropped.flatMap((e) => e.fields); + return { + success: true, + output: { + result, + object: objectName, + // Structured list for downstream nodes ({.droppedFields}). + ...(droppedFields.length > 0 ? { droppedFields } : {}), + }, + ...(dropped.length > 0 + ? { warnings: dropped.map((e) => droppedFieldsWarning('update_record', e)) } + : {}), + }; } catch (err) { return { success: false, error: `update_record(${objectName}) failed: ${(err as Error).message}` }; } diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index a26b033894..93aa989faa 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -55,6 +55,15 @@ export interface NodeExecutionResult { success: boolean; output?: Record; error?: string; + /** + * #3407: advisory warnings surfaced on the step's log entry. The step still + * SUCCEEDS — a warning flags a legal-but-surprising outcome (e.g. an + * `update_record` whose requested fields the data layer legally stripped as + * `readonly`/`readonlyWhen`) so the run trace never shows a clean success + * for a write that partially didn't land. {@link AutomationEngine.executeNode} + * copies them onto the {@link StepLogEntry}. + */ + warnings?: string[]; /** Used by decision nodes — returns the selected branch label */ branchLabel?: string; /** @@ -368,6 +377,14 @@ export interface StepLogEntry { completedAt?: string; durationMs?: number; error?: { code: string; message: string; stack?: string }; + /** + * #3407: advisory warnings from the node executor ({@link NodeExecutionResult.warnings}). + * A `success` step may carry warnings — e.g. an `update_record` whose + * requested fields the data layer legally stripped (`readonly` / + * `readonlyWhen`) — so the Runs surface shows WHY a successful write + * partially didn't land instead of a silent success. + */ + warnings?: string[]; /** * #1479: structured-region grouping. When a step ran inside a `loop` / * `parallel` / `try_catch` body region, these tag it with its **immediate** @@ -2561,6 +2578,7 @@ export class AutomationEngine implements IAutomationService { completedAt: new Date().toISOString(), durationMs: Date.now() - stepStart, error: { code: 'NODE_FAILURE', message: errMsg }, + ...(result.warnings?.length ? { warnings: result.warnings } : {}), }); // Write error output to variable context for downstream nodes @@ -2578,7 +2596,8 @@ export class AutomationEngine implements IAutomationService { throw new Error(`Node '${node.id}' failed: ${errMsg}`); } - // Log successful step + // Log successful step (#3407: advisory executor warnings ride along + // so a legal-but-partial outcome never reads as a clean success). steps.push({ nodeId: node.id, nodeType: node.type, @@ -2586,6 +2605,7 @@ export class AutomationEngine implements IAutomationService { startedAt: stepStartedAt, completedAt: new Date().toISOString(), durationMs: Date.now() - stepStart, + ...(result.warnings?.length ? { warnings: result.warnings } : {}), }); // #1479: fold a structured-region container's body/branch/handler diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index fc62fe7695..f577b1ed15 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -276,6 +276,8 @@ "DriverOptions (type)", "DriverOptionsSchema (const)", "DriverType (const)", + "DroppedFieldsEvent (type)", + "DroppedFieldsEventSchema (const)", "ESignatureConfig (type)", "ESignatureConfigSchema (const)", "EngineAggregateOptions (type)", @@ -3714,7 +3716,8 @@ "ValidationResult (interface)", "WorkflowStatus (interface)", "WorkflowTransition (interface)", - "WorkflowTransitionResult (interface)" + "WorkflowTransitionResult (interface)", + "WriteObservabilityOptions (interface)" ], "./integration": [ "AckMode (type)", diff --git a/packages/spec/json-schema.manifest.json b/packages/spec/json-schema.manifest.json index d38c439937..a3c95aa436 100644 --- a/packages/spec/json-schema.manifest.json +++ b/packages/spec/json-schema.manifest.json @@ -734,6 +734,7 @@ "data/DriverDefinition", "data/DriverOptions", "data/DriverType", + "data/DroppedFieldsEvent", "data/ESignatureConfig", "data/EngineAggregateOptions", "data/EngineCountOptions", diff --git a/packages/spec/src/contracts/data-engine.ts b/packages/spec/src/contracts/data-engine.ts index d48cc1e22a..d52c2e6bdd 100644 --- a/packages/spec/src/contracts/data-engine.ts +++ b/packages/spec/src/contracts/data-engine.ts @@ -1,39 +1,62 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. -import { +import { EngineQueryOptions, - DataEngineInsertOptions, - EngineUpdateOptions, + DataEngineInsertOptions, + EngineUpdateOptions, EngineDeleteOptions, - EngineAggregateOptions, + EngineAggregateOptions, EngineCountOptions, DataEngineRequest, + DroppedFieldsEvent, } from '../data/index.js'; +/** + * In-process write-observability hooks for `insert`/`update` (#3407). + * + * `onFieldsDropped` is invoked by the engine when caller-supplied write fields + * are LEGALLY stripped from the payload before the driver write — static + * `readonly` (#2948) or a TRUE `readonlyWhen` predicate (#3042). The write + * still succeeds; the listener exists so callers that report per-field success + * (e.g. a flow's `update_record` step) can surface a warning instead of a + * silent success (#3356's masked stage write-backs). + * + * Lives on the TS contract — NOT in the serializable Zod options schemas + * (`EngineUpdateOptionsSchema` etc.): a function is unrepresentable in JSON + * Schema and cannot cross the RPC (Virtual Data Engine) boundary, so remote + * callers simply never receive these events. The event payload itself is + * Zod-first: `DroppedFieldsEventSchema` in `data/data-engine.zod.ts`. + * + * A listener that throws must never break the write — engines catch and log. + */ +export interface WriteObservabilityOptions { + /** Called once per strip pass that dropped ≥1 caller-supplied field. */ + onFieldsDropped?: (event: DroppedFieldsEvent) => void; +} /** * IDataEngine - Standard Data Engine Interface - * + * * Abstract interface for data persistence capabilities. * Following the Dependency Inversion Principle - plugins depend on this interface, * not on concrete database implementations. - * + * * All query methods use standard QueryAST parameter names * (where/fields/orderBy/limit/offset/expand) to eliminate mechanical translation * between the Engine and Driver layers. - * + * * Aligned with 'src/data/data-engine.zod.ts' in @objectstack/spec. */ export interface IDataEngine { find(objectName: string, query?: EngineQueryOptions): Promise; findOne(objectName: string, query?: EngineQueryOptions): Promise; - insert(objectName: string, data: any | any[], options?: DataEngineInsertOptions): Promise; - update(objectName: string, data: any, options?: EngineUpdateOptions): Promise; + insert(objectName: string, data: any | any[], options?: DataEngineInsertOptions & WriteObservabilityOptions): Promise; + update(objectName: string, data: any, options?: EngineUpdateOptions & WriteObservabilityOptions): Promise; delete(objectName: string, options?: EngineDeleteOptions): Promise; count(objectName: string, query?: EngineCountOptions): Promise; aggregate(objectName: string, query: EngineAggregateOptions): Promise; - + /** * Vector Search (AI/RAG) */ @@ -49,4 +72,3 @@ export interface IDataEngine { */ execute?(command: any, options?: Record): Promise; } - diff --git a/packages/spec/src/data/data-engine.zod.ts b/packages/spec/src/data/data-engine.zod.ts index c42eb1a934..0806255e38 100644 --- a/packages/spec/src/data/data-engine.zod.ts +++ b/packages/spec/src/data/data-engine.zod.ts @@ -172,6 +172,40 @@ export const EngineUpdateOptionsSchema = lazySchema(() => BaseEngineOptionsSchem returning: z.boolean().default(false).optional(), }).describe('QueryAST-aligned options for DataEngine.update operations')); +// -------------------------------------------------------------------------- +// Write observability: silently-dropped write fields (#3407) +// -------------------------------------------------------------------------- + +/** + * One strip event on a write path: the engine dropped caller-supplied field(s) + * from the payload for a LEGAL reason (static `readonly` (#2948) or a TRUE + * `readonlyWhen` predicate) and completed the write without them. The write + * itself still succeeds — stripping is legitimate semantics, not an error — + * but callers that report success per requested field (e.g. a flow's + * `update_record` step) need to know which fields never landed (#3407). + * + * Delivered in-process via the `onFieldsDropped` listener on the write options + * (see `WriteObservabilityOptions` in `contracts/data-engine.ts`). The + * listener itself is deliberately NOT part of this serializable options + * schema: a function is unrepresentable in JSON Schema and cannot cross the + * RPC (Virtual Data Engine) boundary. + */ +export const DroppedFieldsEventSchema = lazySchema(() => z.object({ + /** Object the write targeted (resolved object name). */ + object: z.string().describe('Object the write targeted (resolved object name)'), + /** Caller-supplied field names the engine removed from the write payload. */ + fields: z.array(z.string()).describe('Caller-supplied field names the engine removed from the write payload'), + /** + * Why the fields were dropped: + * - `readonly` — static `readonly: true` fields, caller-supplied writes are + * stripped for non-system contexts (#2948); + * - `readonly_when` — a `readonlyWhen` predicate locked the field for the + * target record's state; on a multi-row update this is "locked in ≥1 + * matched row" semantics (#3042). + */ + reason: z.enum(['readonly', 'readonly_when']).describe('Why the fields were dropped: static readonly (#2948) or a TRUE readonlyWhen predicate (#3042)'), +}).describe('A write-path strip event: caller-supplied fields legally dropped from the payload (#3407)')); + // -------------------------------------------------------------------------- // Legacy: DataEngineUpdateOptionsSchema (DEPRECATED) // -------------------------------------------------------------------------- @@ -485,6 +519,7 @@ export const DataEngineRequestSchema = lazySchema(() => z.discriminatedUnion('me // --- New: QueryAST-aligned types (preferred) --- export type EngineQueryOptions = z.infer; export type EngineUpdateOptions = z.infer; +export type DroppedFieldsEvent = z.infer; export type EngineDeleteOptions = z.infer; export type EngineAggregateOptions = z.infer; export type EngineCountOptions = z.infer;