diff --git a/.changeset/flow-crud-bulk-intent.md b/.changeset/flow-crud-bulk-intent.md new file mode 100644 index 0000000000..e4ba1dcf30 --- /dev/null +++ b/.changeset/flow-crud-bulk-intent.md @@ -0,0 +1,56 @@ +--- +"@objectstack/spec": minor +"@objectstack/service-automation": minor +--- + +feat(spec,automation): `update_record` / `delete_record` can declare bulk intent with `multi` (#5393) + +A flow could not express "write every row this filter matches" — at all, from +any app. `UpdateRecordConfigSchema` / `DeleteRecordConfigSchema` are +`strictObject`s and neither declared any spelling of bulk intent (`multi`, +`bulk`, `all` and `options.multi` were each rejected as an unrecognized key), +and the CRUD executors never passed `options.multi` to the data engine. The +engine accepts a write only when `where.id` is a **scalar** or `options.multi` +is truthy, and throws otherwise — so a predicate `update_record` / +`delete_record` was unreachable, while the node descriptors advertised +`Delete Records` / "Delete records matching a filter." Declared ≠ enforced +(Prime Directive #10); the symptom was #5225's showcase sweep flow, which had +never deleted a record. + +**New authorable key — `multi` (boolean, default `false`), on `update_record` +and `delete_record`.** One name for one concept (PD #12): `multi` is what the +data engine has always called it (`EngineUpdateOptions.multi` / +`EngineDeleteOptions.multi`), so the word is the same from node config to +driver call and greps end to end. + +```ts +// before — refused by the engine at run time, with no authoring-time signal +{ type: 'delete_record', config: { objectName: 'lead', filter: { stage: 'stale' } } } + +// after — the declaration makes the intent explicit and the write reachable +{ type: 'delete_record', config: { objectName: 'lead', filter: { stage: 'stale' }, multi: true } } +``` + +- **Absent or `false`** — unchanged behaviour. The executor forwards + `multi: false`, so the write must name one row by scalar `id`; anything else + (a predicate, or `id: { $in: [...] }`) is refused by the engine with + `Delete requires an ID or options.multi=true`. **That refusal is the + contract**, not a defect to route around: it is what keeps an undeclared + unbounded write from happening by accident. +- **`true`** — the executor forwards `options.multi: true`, the write lands on + `driver.updateMany` / `deleteMany`, and the step's `acted` metric reports the + affected row count. + +Additive and backward compatible: no existing flow changes behaviour, and every +by-id write keeps working untouched. + +Two guards are unchanged and worth stating explicitly. The #3810 +erased-condition guard still refuses a node whose authored filter condition +interpolated to nothing, `multi` or not — bulk intent says "many rows are +fine", never "a condition may vanish". And `multi: true` with **no** `filter` +is the whole object, by declaration: write the constraint you mean. + +Wrong spellings are answered by name rather than by edit distance (which +reaches `multi` from none of them): `bulk` / `all` / `multiple` get the +prescription, and `options: { multi: true }` is called out as the engine's +options bag written at the node's altitude. diff --git a/content/docs/references/automation/builtin-node-config.mdx b/content/docs/references/automation/builtin-node-config.mdx index e9614d9bfa..1792b6e071 100644 --- a/content/docs/references/automation/builtin-node-config.mdx +++ b/content/docs/references/automation/builtin-node-config.mdx @@ -158,6 +158,7 @@ const result = CreateRecordConfigSchema.parse(data); | :--- | :--- | :--- | :--- | | **objectName** | `string` | ✅ | Object to delete from | | **filter** | `Record` | optional | Field/value pairs identifying the record(s) to delete | +| **multi** | `boolean` | optional | Declare bulk intent: delete every row the filter matches (default false — a predicate delete without it is refused by the engine) | --- @@ -240,6 +241,7 @@ const result = CreateRecordConfigSchema.parse(data); | **objectName** | `string` | ✅ | Object to update | | **filter** | `Record` | optional | Field/value pairs identifying the record(s) to update | | **fields** | `Record` | optional | Field values to write | +| **multi** | `boolean` | optional | Declare bulk intent: update every row the filter matches (default false — a predicate update without it is refused by the engine) | --- diff --git a/packages/services/service-automation/package.json b/packages/services/service-automation/package.json index a357cc258e..e1e6964ff0 100644 --- a/packages/services/service-automation/package.json +++ b/packages/services/service-automation/package.json @@ -23,6 +23,7 @@ "@objectstack/spec": "workspace:*" }, "devDependencies": { + "@objectstack/objectql": "workspace:*", "@types/node": "^26.1.2", "typescript": "^6.0.3", "vitest": "^4.1.10" diff --git a/packages/services/service-automation/src/builtin/crud-bulk-intent.test.ts b/packages/services/service-automation/src/builtin/crud-bulk-intent.test.ts new file mode 100644 index 0000000000..09501c6ae8 --- /dev/null +++ b/packages/services/service-automation/src/builtin/crud-bulk-intent.test.ts @@ -0,0 +1,332 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * **Bulk intent on `update_record` / `delete_record` (#5393)** — the flow + * language's declaration that a write may touch every row its filter matches, + * and the executor wiring that turns the declaration into `options.multi` on + * the data-engine call. + * + * ## What was actually broken + * + * `DeleteRecordConfigSchema` / `UpdateRecordConfigSchema` are `strictObject`s + * and neither declared ANY spelling of bulk intent, while the CRUD executors + * never passed `options.multi`. The engine's contract + * (`resolveEngineDeleteDispatch`, and `engine.ts`'s twin for update) accepts a + * write only when `where.id` is a scalar OR `options.multi` is truthy, and + * throws otherwise. So a predicate `delete_record` was unreachable from any + * flow in any app — while the node descriptor said `Delete Records` / + * `Delete records matching a filter.` That is declared ≠ enforced (PD #10), + * and #5225's showcase sweep flow was the site it surfaced at. + * + * ## Why this file's delete double is PINNED and the update one is not + * + * The reason the break survived a fully green suite is the #5197 detector + * blind spot: `run-summary.test.ts`'s inline `async delete() { return false; }` + * accepts a predicate delete the real engine refuses, and — taking zero + * parameters — is not even discoverable by + * `scripts/check-engine-double-contract.mjs`, whose `isEngineDeleteShape` + * requires the engine's `(object, options)` arity. A test written the same way + * would re-close the same eye, so this file's `delete` opens with + * `assertEngineDeleteDispatch` — the producer's OWN predicate, imported from + * `@objectstack/objectql` — and therefore cannot be looser than the engine on + * any input, including the `id: { $in: [...] }` case a hand-mirrored `if` + * always drops. + * + * `update` has no such shared predicate: objectql keeps its update dispatch + * inline at `engine.ts` (`throw new Error('Update requires an ID or + * options.multi=true')`), and `check-engine-double-contract.mjs` names "update's + * twin dispatch" as deliberately out of its slice pending that extraction. So + * the update cases below assert exactly what the EXECUTOR owes — the options + * bag it hands the engine — and deliberately state no second opinion about + * what the engine would then accept. Writing a mirrored update guard here + * would be the second copy of the contract that gate exists to remove. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { assertEngineDeleteDispatch } from '@objectstack/objectql'; +import { AutomationEngine } from '../engine.js'; +import { registerCrudNodes } from './crud-nodes.js'; + +function createTestLogger(): any { + return { info: () => {}, warn: () => {}, error: () => {}, debug: () => {}, child: () => createTestLogger() }; +} + +/** Rows the fake store holds; every case starts from this shape. */ +function seedRows() { + return [ + { id: 'd1', stage: 'stale', owner: 'usr_7' }, + { id: 'd2', stage: 'stale', owner: 'usr_8' }, + { id: 'd3', stage: 'open', owner: 'usr_7' }, + ]; +} + +/** Scalar-equality match — enough for these fixtures, no operator dialect. */ +function matches(row: Record, where: Record | undefined): boolean { + for (const [k, v] of Object.entries(where ?? {})) { + if (row[k] !== v) return false; + } + return true; +} + +/** + * A data engine double whose `delete` is bound to the real engine's dispatch + * decision, so a call this fixture accepts is a call `ObjectQL.delete` accepts. + * + * `update` records the options bag verbatim and returns the `driver.updateMany` + * contract (`Promise`) for a declared bulk write, the updated row for a + * by-id write — the shape difference `writtenRowCount` reads. + */ +function pinnedFakeData() { + let rows = seedRows(); + const calls: { update?: any; delete?: any } = {}; + const service = { + find: async (_object: string, options: any) => rows.filter((r) => matches(r, options?.where)), + findOne: async (_object: string, options: any) => rows.find((r) => matches(r, options?.where)) ?? null, + insert: async (_object: string, data: any) => ({ id: 'new', ...data }), + update: async (_object: string, data: any, options: any) => { + calls.update = options; + const matched = rows.filter((r) => matches(r, options?.where)); + for (const r of matched) Object.assign(r, data); + // `driver.updateMany` resolves a COUNT; a by-id update resolves the row. + return options?.multi ? matched.length : matched[0]; + }, + delete: async (_object: string, options: any) => { + calls.delete = options; + // The producer's own decision — this line is what makes the double + // incapable of accepting a call the engine refuses (#4550/#5197). + const dispatch = assertEngineDeleteDispatch(options); + if (dispatch.kind === 'by-id') { + const before = rows.length; + rows = rows.filter((r) => r.id !== dispatch.id); + return rows.length < before; + } + const matched = rows.filter((r) => matches(r, options?.where)); + rows = rows.filter((r) => !matched.includes(r)); + // `driver.deleteMany` resolves a COUNT. + return matched.length; + }, + getObject: () => ({ name: 'deal', fields: {} }), + }; + return { service, calls, remaining: () => rows }; +} + +function ctxWith(data: any): any { + return { + logger: createTestLogger(), + getService(name: string) { + return name === 'data' ? data : undefined; + }, + }; +} + +function crudFlow(nodeType: string, config: Record) { + return { + name: 'bulk_flow', + label: 'Bulk Flow', + type: 'autolaunched' as const, + // Explicit so the ADR-0049 runAs gate (#3760) does not refuse the data + // op first — a refusal for the wrong reason passes the negative cases + // vacuously. + runAs: 'system' as const, + nodes: [ + { id: 'start', type: 'start' as const, label: 'Start' }, + { id: 'op', type: nodeType as any, label: 'Op', config }, + { id: 'end', type: 'end' as const, label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'op' }, + { id: 'e2', source: 'op', target: 'end' }, + ], + }; +} + +describe('delete_record — bulk intent reaches deleteMany, and only when declared (#5393)', () => { + let engine: AutomationEngine; + let data: ReturnType; + + beforeEach(() => { + data = pinnedFakeData(); + engine = new AutomationEngine(createTestLogger()); + registerCrudNodes(engine, ctxWith(data.service)); + }); + + it('a predicate delete WITHOUT `multi` is refused by the engine — the contract, unchanged', async () => { + engine.registerFlow('bulk_flow', crudFlow('delete_record', { + objectName: 'deal', + filter: { stage: 'stale' }, + }) as never); + + const result = await engine.execute('bulk_flow', { event: 'schedule' } as never); + + expect(result.success).toBe(false); + // The producer's own message, reached through the producer's own predicate. + expect(result.error).toContain('Delete requires an ID or options.multi=true'); + expect(data.calls.delete).toMatchObject({ multi: false }); + expect(data.remaining()).toHaveLength(3); + }); + + it('the same node WITH `multi: true` reaches deleteMany and reports the row count', async () => { + engine.registerFlow('bulk_flow', crudFlow('delete_record', { + objectName: 'deal', + filter: { stage: 'stale' }, + multi: true, + }) as never); + + const result = await engine.execute('bulk_flow', { event: 'schedule' } as never); + + expect(result.success).toBe(true); + expect(data.calls.delete).toMatchObject({ where: { stage: 'stale' }, multi: true }); + expect(data.remaining().map((r) => r.id)).toEqual(['d3']); + // #4354 — `acted` is the deleted-row count, which is what made a + // sweep flow's "did it actually delete anything" answerable at all. + expect(result.summary!.acted).toBe(2); + }); + + it('a scalar-id delete still needs no declaration — the by-id route is untouched', async () => { + engine.registerFlow('bulk_flow', crudFlow('delete_record', { + objectName: 'deal', + filter: { id: '{record.id}' }, + }) as never); + + const result = await engine.execute('bulk_flow', { record: { id: 'd2' } } as never); + + expect(result.success).toBe(true); + expect(data.calls.delete).toMatchObject({ where: { id: 'd2' }, multi: false }); + expect(data.remaining().map((r) => r.id)).toEqual(['d1', 'd3']); + }); + + it('an `$in` id set is a PREDICATE, not an id — the half a hand-mirrored guard drops', async () => { + // `where: { id: { $in: [...] } }` looks like an id and is a multi-row + // predicate. Pinning to the producer's predicate is what makes this + // case answer correctly here without anyone having remembered it. + const withoutMulti = crudFlow('delete_record', { + objectName: 'deal', + filter: { id: { $in: ['d1', 'd2'] } }, + }); + engine.registerFlow('bulk_flow', withoutMulti as never); + + const refused = await engine.execute('bulk_flow', { event: 'schedule' } as never); + expect(refused.success).toBe(false); + expect(refused.error).toContain('Delete requires an ID or options.multi=true'); + + engine.registerFlow('bulk_flow', crudFlow('delete_record', { + objectName: 'deal', + filter: { id: { $in: ['d1', 'd2'] } }, + multi: true, + }) as never); + const accepted = await engine.execute('bulk_flow', { event: 'schedule' } as never); + expect(accepted.success).toBe(true); + expect(data.calls.delete).toMatchObject({ multi: true }); + }); + + it('`multi: false` written out is the same refusal as leaving it off', async () => { + engine.registerFlow('bulk_flow', crudFlow('delete_record', { + objectName: 'deal', + filter: { stage: 'stale' }, + multi: false, + }) as never); + + const result = await engine.execute('bulk_flow', { event: 'schedule' } as never); + + expect(result.success).toBe(false); + expect(result.error).toContain('Delete requires an ID or options.multi=true'); + // The options bag is asserted, not just the refusal: the refusal alone + // holds whether the executor forwards `multi` or has never heard of it, + // so a case that stopped at `result.error` would stay green through a + // revert of the very wiring this file exists to pin. + expect(data.calls.delete).toMatchObject({ where: { stage: 'stale' }, multi: false }); + expect(data.remaining()).toHaveLength(3); + }); + + it('declaring `multi` does NOT disarm the #3810 erased-condition guard', async () => { + // The two guards are independent: bulk intent says "many rows are + // fine", it never says "a condition the author wrote may vanish". + engine.registerFlow('bulk_flow', crudFlow('delete_record', { + objectName: 'deal', + filter: { owner: '{record.ownr}' }, + multi: true, + }) as never); + + const result = await engine.execute('bulk_flow', { record: { id: 'd1', owner: 'usr_7' } } as never); + + expect(result.success).toBe(false); + expect(result.error).toContain('{record.ownr}'); + expect(data.calls.delete).toBeUndefined(); + expect(data.remaining()).toHaveLength(3); + }); +}); + +describe('update_record — the executor forwards the declared bulk intent (#5393)', () => { + let engine: AutomationEngine; + let data: ReturnType; + + beforeEach(() => { + data = pinnedFakeData(); + engine = new AutomationEngine(createTestLogger()); + registerCrudNodes(engine, ctxWith(data.service)); + }); + + it('`multi: true` is handed to the engine as `options.multi`, and `acted` is the row count', async () => { + engine.registerFlow('bulk_flow', crudFlow('update_record', { + objectName: 'deal', + filter: { stage: 'stale' }, + fields: { stage: 'archived' }, + multi: true, + }) as never); + + const result = await engine.execute('bulk_flow', { event: 'schedule' } as never); + + expect(result.success).toBe(true); + expect(data.calls.update).toMatchObject({ where: { stage: 'stale' }, multi: true }); + expect(result.summary!.acted).toBe(2); + expect(data.remaining().filter((r) => r.stage === 'archived')).toHaveLength(2); + }); + + it('an undeclared predicate update forwards `multi: false` — which is what the engine refuses on', async () => { + // The executor's whole obligation, stated positively: it reports the + // author's declaration and never invents one. What the engine then does + // with `multi: false` is the engine's contract (`engine.ts`: `Update + // requires an ID or options.multi=true`), asserted where it lives. + engine.registerFlow('bulk_flow', crudFlow('update_record', { + objectName: 'deal', + filter: { stage: 'stale' }, + fields: { stage: 'archived' }, + }) as never); + + await engine.execute('bulk_flow', { event: 'schedule' } as never); + + expect(data.calls.update).toMatchObject({ where: { stage: 'stale' }, multi: false }); + }); + + it('a by-id update is unchanged — `multi: false`, one row, `acted: 1`', async () => { + engine.registerFlow('bulk_flow', crudFlow('update_record', { + objectName: 'deal', + filter: { id: '{record.id}' }, + fields: { stage: 'won' }, + }) as never); + + const result = await engine.execute('bulk_flow', { record: { id: 'd3' } } as never); + + expect(result.success).toBe(true); + expect(data.calls.update).toMatchObject({ where: { id: 'd3' }, multi: false }); + expect(result.summary!.acted).toBe(1); + }); +}); + +describe('both nodes declare bulk intent under ONE name (#5393, PD #12)', () => { + it('the descriptor form offers `multi` on update_record and delete_record, and nowhere else in the quartet', () => { + const engine = new AutomationEngine(createTestLogger()); + registerCrudNodes(engine, ctxWith(undefined)); + + const propsOf = (t: string) => + Object.keys((engine.getActionDescriptor(t)?.configSchema as any)?.properties ?? {}); + + expect(propsOf('update_record')).toContain('multi'); + expect(propsOf('delete_record')).toContain('multi'); + // `get_record` bounds rows with `limit`; `create_record` writes one row. + // Neither dispatches on bulk intent, so offering the key there would be + // the #3528 shape — a form field nothing reads. + expect(propsOf('get_record')).not.toContain('multi'); + expect(propsOf('create_record')).not.toContain('multi'); + }); +}); diff --git a/packages/services/service-automation/src/builtin/crud-nodes.ts b/packages/services/service-automation/src/builtin/crud-nodes.ts index 3083eab935..e416dcafe5 100644 --- a/packages/services/service-automation/src/builtin/crud-nodes.ts +++ b/packages/services/service-automation/src/builtin/crud-nodes.ts @@ -361,6 +361,14 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext): objectName: { type: 'string', title: 'Object', xRef: { kind: 'object' } }, filter: { type: 'object', additionalProperties: true, title: 'Filter', description: 'Field/value pairs identifying the record(s) to update (e.g. id → {recordId}).' }, fields: { type: 'object', additionalProperties: true, title: 'Field values', description: 'Field values to write.' }, + // #5393 — the author's bulk DECLARATION. Off (default) + // the engine accepts only a write that names one row by + // scalar id; a predicate update is refused rather than + // silently narrowed or silently widened. + multi: { + type: 'boolean', title: 'Update every matching record', + description: 'Declare bulk intent: update EVERY record the filter matches. Off (default) means the filter must name one record by id — a predicate update without this is refused by the data engine.', + }, }, required: ['objectName'], }, @@ -400,6 +408,14 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext): const dropped: DroppedFieldsEvent[] = []; const result = await data.update(objectName, fields, { where: filter, + // #5393 — the author's declared bulk intent, forwarded + // to the engine's own word for it. Stated on EVERY call + // rather than spread in when true: `multi: false` is the + // half of the contract that makes the engine refuse a + // predicate update, and a reader of this call should see + // which half was asked for without inferring it from an + // absent key. + multi: cfg.multi === true, context: dataCtx, onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); }, }); @@ -437,6 +453,14 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext): properties: { objectName: { type: 'string', title: 'Object', xRef: { kind: 'object' } }, filter: { type: 'object', additionalProperties: true, title: 'Filter', description: 'Field/value pairs identifying the record(s) to delete.' }, + // #5393 — see update_record. Highest-stakes declaration + // the flow language has: without it a predicate delete + // is refused by the engine, with it every matched row + // goes, and `multi` + no filter is the whole object. + multi: { + type: 'boolean', title: 'Delete every matching record', + description: 'Declare bulk intent: delete EVERY record the filter matches. Off (default) means the filter must name one record by id — a predicate delete without this is refused by the data engine.', + }, }, required: ['objectName'], }, @@ -464,7 +488,9 @@ 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.delete(objectName, { where: filter, context: dataCtx }); + // #5393 — `multi` is the author's declaration, forwarded to + // the engine's own word for it (see update_record above). + const result = await data.delete(objectName, { where: filter, multi: cfg.multi === true, context: dataCtx }); return { success: true, output: { result, object: objectName }, diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index a7223b33b2..039bcf1179 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -128,6 +128,32 @@ interface ConfigSchemaNode { * did-you-mean and the declared set; an entry here adds the *mechanism* the * author was reaching for. */ +/** + * The bulk-intent spellings, shared by `update_record` and `delete_record` + * (#5393) — the same curation `builtin-node-config.zod.ts` carries at the + * execute-time parse, copied here because this is the FIRST door an author + * hits (registration, at boot) and #4001's finding is that detection + * generalizes for free while prose does not. + * + * All four were measured against a real `safeParse` while #5225 was diagnosed, + * back when no spelling of bulk intent — `multi` included — existed on either + * node. Edit distance reaches `multi` from none of them. + */ +const BULK_INTENT_GUIDANCE: Record = { + bulk: 'Bulk intent is `multi: true` — the data engine\'s own word for it (`options.multi`), so the concept ' + + 'keeps one name from node config to driver call (#5393). Without it the write must name one row by ' + + 'scalar `id`; a predicate write is refused by the engine rather than silently widened.', + all: 'Bulk intent is `multi: true` — the data engine\'s own word for it (`options.multi`), so the concept ' + + 'keeps one name from node config to driver call (#5393). Without it the write must name one row by ' + + 'scalar `id`; a predicate write is refused by the engine rather than silently widened.', + multiple: 'Bulk intent is `multi: true` — the data engine\'s own word for it (`options.multi`), so the ' + + 'concept keeps one name from node config to driver call (#5393). Without it the write must name one row ' + + 'by scalar `id`; a predicate write is refused by the engine rather than silently widened.', + options: 'This is the NODE config, not the data engine\'s options bag — declare `multi: true` at the top ' + + 'level of `config`, never `options: { multi: true }`. Translating that declaration into `options.multi` ' + + 'on the engine call is the executor\'s job (#5393).', +}; + const FLOW_NODE_UNKNOWN_KEY_GUIDANCE: Record> = { create_record: { fieldValues: @@ -139,7 +165,9 @@ const FLOW_NODE_UNKNOWN_KEY_GUIDANCE: Record> = { fieldValues: 'The write map is `fields` — `fieldValues` was an AI-authoring dialect that never had a ' + 'runtime reader (#2419, rejected by design).', + ...BULK_INTENT_GUIDANCE, }, + delete_record: BULK_INTENT_GUIDANCE, screen: { visibleIf: 'The visibility predicate is `visibleWhen` (bare CEL, re-evaluated client-side as the ' + diff --git a/packages/spec/authorable-surface.json b/packages/spec/authorable-surface.json index b54e5f381c..1bdedd7da2 100644 --- a/packages/spec/authorable-surface.json +++ b/packages/spec/authorable-surface.json @@ -2131,6 +2131,7 @@ "automation/DecisionOutputDef:required", "automation/DecisionOutputDef:type", "automation/DeleteRecordConfig:filter", + "automation/DeleteRecordConfig:multi", "automation/DeleteRecordConfig:objectName", "automation/ETLDestination:config", "automation/ETLDestination:connector", @@ -2417,6 +2418,7 @@ "automation/TryCatchConfig:try", "automation/UpdateRecordConfig:fields", "automation/UpdateRecordConfig:filter", + "automation/UpdateRecordConfig:multi", "automation/UpdateRecordConfig:objectName", "automation/WaitExecutorConfig:conditionMaxPolls", "automation/WaitExecutorConfig:conditionPollIntervalMs", diff --git a/packages/spec/src/automation/builtin-node-config.test.ts b/packages/spec/src/automation/builtin-node-config.test.ts index a07973b908..dbe9eeff4b 100644 --- a/packages/spec/src/automation/builtin-node-config.test.ts +++ b/packages/spec/src/automation/builtin-node-config.test.ts @@ -128,6 +128,61 @@ describe('CRUD config contracts — strict as of #4001 批 9', () => { expect(GetRecordConfigSchema.safeParse({ objectName: 'lead', outputVariable: 'lead' }).success).toBe(true); expect(CreateRecordConfigSchema.safeParse({ objectName: 'task', outputVariable: 'task' }).success).toBe(true); }); + + // ── bulk intent (#5393) ──────────────────────────────────────────── + + it.each([ + ['update_record', UpdateRecordConfigSchema, { objectName: 'lead', filter: { stage: 'stale' }, fields: { stage: 'archived' } }], + ['delete_record', DeleteRecordConfigSchema, { objectName: 'lead', filter: { stage: 'stale' } }], + ] as ReadonlyArray<[string, Parseable, Record]>)( + '%s: `multi` is authorable — the declaration a predicate write needs to be reachable at all', + (_nodeType, schema, base) => { + expect(schema.safeParse({ ...base, multi: true }).success).toBe(true); + expect(schema.safeParse({ ...base, multi: false }).success).toBe(true); + // Absent is the default and still valid: the engine then refuses a + // predicate write, which is the contract this key makes declarable. + expect(schema.safeParse(base).success).toBe(true); + // Typed, so `multi: 'yes'` is a guard, not a truthy surprise. + expect(schema.safeParse({ ...base, multi: 'yes' }).success).toBe(false); + }, + ); + + it.each([ + ['update_record', UpdateRecordConfigSchema, { objectName: 'lead' }], + ['delete_record', DeleteRecordConfigSchema, { objectName: 'lead' }], + ] as ReadonlyArray<[string, Parseable, Record]>)( + '%s: the measured wrong spellings of bulk intent get named, not a bare "unknown key"', + (_nodeType, schema, base) => { + for (const key of ['bulk', 'all', 'multiple']) { + const message = unknownKeyMessage(schema, { ...base, [key]: true })!; + expect(message, key).toContain('`multi: true`'); + expect(message, key).toContain('#5393'); + // The distance claim, pinned: none of these reaches `multi`, so + // without the curated entry the rejection would offer nothing. + expect(message, key).not.toContain(`\`${key}\` → `); + } + }, + ); + + it.each([ + ['update_record', UpdateRecordConfigSchema, { objectName: 'lead' }], + ['delete_record', DeleteRecordConfigSchema, { objectName: 'lead' }], + ] as ReadonlyArray<[string, Parseable, Record]>)( + '%s: `options: { multi: true }` is answered as a wrong LAYER, not a typo', + (_nodeType, schema, base) => { + const message = unknownKeyMessage(schema, { ...base, options: { multi: true } })!; + expect(message).toContain('NODE config'); + expect(message).toContain('executor'); + }, + ); + + it('the read-only and single-row siblings do NOT declare `multi`', () => { + // `get_record` bounds rows with `limit`; `create_record` writes one row. + // Neither dispatches on bulk intent, so the key would be inert there — + // the #3528 shape (a declared key nothing reads). + expect(GetRecordConfigSchema.safeParse({ objectName: 'lead', multi: true }).success).toBe(false); + expect(CreateRecordConfigSchema.safeParse({ objectName: 'task', multi: true }).success).toBe(false); + }); }); describe('ScreenConfigSchema / ScreenFieldConfigSchema — strict as of #4001 批 9', () => { diff --git a/packages/spec/src/automation/builtin-node-config.zod.ts b/packages/spec/src/automation/builtin-node-config.zod.ts index 645b5992c0..b3bd7c284d 100644 --- a/packages/spec/src/automation/builtin-node-config.zod.ts +++ b/packages/spec/src/automation/builtin-node-config.zod.ts @@ -156,6 +156,47 @@ const NO_OUTPUT_VARIABLE_GUIDANCE = + 'one, which is why the key looks universal and is not. To use what was written, follow this node with a ' + '`get_record` that reads the row back.'; +/** + * Bulk intent has exactly one spelling — `multi` — and the ENGINE named it + * first (#5393). + * + * `EngineUpdateOptions.multi` / `EngineDeleteOptions.multi` have carried this + * concept since the data layer was written, and `resolveEngineDeleteDispatch` + * makes it the only thing standing between a predicate write and + * `driver.deleteMany`. So the flow-authoring surface reuses the word rather + * than inventing a second one for the same concept (PD #12): one concept, one + * name, greppable end to end from the node config to the driver call. + * + * These four spellings were measured, not guessed: they are what a real + * `safeParse` was fed while #5225 was being diagnosed, back when NONE of them + * (including `multi` itself) was declared. Edit distance reaches `multi` from + * none of them, so without these entries the rejection would name the key and + * offer nothing — on the single most destructive surface the flow language has. + * + * `options` is the wrong-LAYER member of the set: `options: { multi: true }` + * is the shape of the data-engine call, not of node config. The executor is + * what performs that translation, and an author who writes the engine's bag + * into the node has the right concept at the wrong altitude. + */ +const BULK_INTENT_PRESCRIPTION = + 'Bulk intent is declared with `multi: true` — the same word the data engine has always used for it ' + + '(`options.multi`), so the concept keeps one name from node config to driver call. Until #5393 NO spelling of ' + + 'it existed on this node, which is why a predicate write was refused by the engine ' + + '(`… requires an ID or options.multi=true`) and no flow could reach `updateMany`/`deleteMany` at all. Leaving ' + + 'it off is still a valid, deliberate choice: without it the write must name one row by scalar `id`.'; + +const BULK_INTENT_OPTIONS_PRESCRIPTION = + 'This is the NODE config, not the data engine\'s options bag — declare `multi: true` at the top level of ' + + '`config`, never `options: { multi: true }`. Translating the declared intent into `options.multi` on the ' + + 'engine call is the executor\'s job, and it is the only thing that should be doing it (#5393).'; + +const CRUD_BULK_INTENT_GUIDANCE = { + bulk: BULK_INTENT_PRESCRIPTION, + all: BULK_INTENT_PRESCRIPTION, + multiple: BULK_INTENT_PRESCRIPTION, + options: BULK_INTENT_OPTIONS_PRESCRIPTION, +} as const; + // ─── CRUD quartet ──────────────────────────────────────────────────── /** @@ -221,6 +262,7 @@ export const UpdateRecordConfigSchema = lazySchema(() => strictObject({ history: BUILTIN_NODE_CONFIG_HISTORY, guidance: { ...CRUD_ALIAS_GUIDANCE, + ...CRUD_BULK_INTENT_GUIDANCE, recordId: CRUD_RECORD_ID_GUIDANCE, fieldValues: FIELD_VALUES_GUIDANCE, outputVariable: NO_OUTPUT_VARIABLE_GUIDANCE, @@ -233,6 +275,28 @@ export const UpdateRecordConfigSchema = lazySchema(() => strictObject({ .describe('Field/value pairs identifying the record(s) to update'), /** Field values to write; values interpolate `{token}` templates. */ fields: z.record(z.string(), z.unknown()).optional().describe('Field values to write'), + /** + * Declare BULK intent — this node may update EVERY row `filter` matches. + * + * Absent or `false` (the default): the executor passes no bulk intent, and + * the data engine accepts the write only when `filter` names one row by a + * **scalar** `id`. Anything else — a predicate, or `id: { $in: [...] }` — + * is refused with `Update requires an ID or options.multi=true`. That + * refusal is the contract, not a defect to route around: an unbounded write + * nobody declared is the #3810 hazard with the safety off. + * + * `true`: the executor passes `options.multi: true`, so the write lands on + * `driver.updateMany` and the step's `acted` metric reports the matched row + * count. Same word the engine has always used for the concept + * (`EngineUpdateOptions.multi`) — one concept, one name (PD #12). + * + * It does not weaken the #3810 erased-condition guard: a node whose authored + * filter condition interpolated to nothing is still refused before any write, + * `multi` or not. What it DOES make reachable is the deliberate whole-object + * write — `multi: true` with no `filter` at all is every row, by declaration. + */ + multi: z.boolean().optional() + .describe('Declare bulk intent: update every row the filter matches (default false — a predicate update without it is refused by the engine)'), })); export type UpdateRecordConfig = z.input; @@ -244,6 +308,7 @@ export const DeleteRecordConfigSchema = lazySchema(() => strictObject({ history: BUILTIN_NODE_CONFIG_HISTORY, guidance: { ...CRUD_ALIAS_GUIDANCE, + ...CRUD_BULK_INTENT_GUIDANCE, recordId: CRUD_RECORD_ID_GUIDANCE, outputVariable: NO_OUTPUT_VARIABLE_GUIDANCE, }, @@ -253,6 +318,28 @@ export const DeleteRecordConfigSchema = lazySchema(() => strictObject({ /** Field/value pairs identifying the record(s) to delete. */ filter: z.record(z.string(), z.unknown()).optional() .describe('Field/value pairs identifying the record(s) to delete'), + /** + * Declare BULK intent — this node may delete EVERY row `filter` matches. + * + * Absent or `false` (the default): the executor passes no bulk intent, and + * the data engine accepts the delete only when `filter` names one row by a + * **scalar** `id`. Anything else — a predicate, or `id: { $in: [...] }` — + * is refused with `Delete requires an ID or options.multi=true`. That + * refusal is the contract, not a defect to route around. + * + * `true`: the executor passes `options.multi: true`, so the delete lands on + * `driver.deleteMany` and the step's `acted` metric reports the deleted row + * count. Same word the engine has always used for the concept + * (`EngineDeleteOptions.multi`) — one concept, one name (PD #12). + * + * The highest-stakes key on this shape, which is why it is a DECLARATION and + * not an inference from the filter's shape: the #3810 erased-condition guard + * still refuses a node whose authored condition interpolated to nothing, but + * `multi: true` with no `filter` at all is the whole object, by declaration. + * Write the constraint you mean. + */ + multi: z.boolean().optional() + .describe('Declare bulk intent: delete every row the filter matches (default false — a predicate delete without it is refused by the engine)'), })); export type DeleteRecordConfig = z.input; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 7bf02840b0..c7cfebb386 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1975,6 +1975,9 @@ importers: specifier: workspace:* version: link:../../spec devDependencies: + '@objectstack/objectql': + specifier: workspace:* + version: link:../../objectql '@types/node': specifier: ^26.1.2 version: 26.1.2 diff --git a/scripts/engine-double-contract.baseline.json b/scripts/engine-double-contract.baseline.json index 7923b8372d..539da11399 100644 --- a/scripts/engine-double-contract.baseline.json +++ b/scripts/engine-double-contract.baseline.json @@ -175,57 +175,57 @@ "file": "packages/services/service-automation/src/builtin/crud-config-aliases.test.ts", "unguarded": 1, "kind": "DEBT", - "why": "@objectstack/service-automation does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", - "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + "why": "MEASURED (#5393): the devDependency this entry used to cite as the blocker now EXISTS — @objectstack/objectql was added to @objectstack/service-automation's devDependencies when the sibling `builtin/crud-bulk-intent.test.ts` was pinned. It is not cyclic: objectql's transitive dependency closure (12 packages) does not contain service-automation, and `turbo run build --filter=@objectstack/service-automation --dry` (turbo 2.10.7) resolved the graph without complaint. So what is left here is a one-line pin, deferred only because #5393's PR is a spec/executor change and flipping an unmeasured suite red belongs in its own PR.", + "closes": "open the fake's delete with assertEngineDeleteDispatch(opts) and run the package's suite — the devDependency is already declared" }, { "file": "packages/services/service-automation/src/builtin/crud-filter-guard.test.ts", "unguarded": 1, "kind": "DEBT", - "why": "@objectstack/service-automation does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", - "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + "why": "MEASURED (#5393): the devDependency this entry used to cite as the blocker now EXISTS — @objectstack/objectql was added to @objectstack/service-automation's devDependencies when the sibling `builtin/crud-bulk-intent.test.ts` was pinned. It is not cyclic: objectql's transitive dependency closure (12 packages) does not contain service-automation, and `turbo run build --filter=@objectstack/service-automation --dry` (turbo 2.10.7) resolved the graph without complaint. So what is left here is a one-line pin, deferred only because #5393's PR is a spec/executor change and flipping an unmeasured suite red belongs in its own PR. One extra note for whoever pins it: several of its #3810 fixtures assert success on a PREDICATE delete, which the real engine refuses — since #5393 that is expressible, so those fixtures gain `multi: true` rather than being deleted.", + "closes": "open the fake's delete with assertEngineDeleteDispatch(opts) and run the package's suite — the devDependency is already declared" }, { "file": "packages/services/service-automation/src/builtin/crud-runas.test.ts", "unguarded": 1, "kind": "DEBT", - "why": "@objectstack/service-automation does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", - "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + "why": "MEASURED (#5393): the devDependency this entry used to cite as the blocker now EXISTS — @objectstack/objectql was added to @objectstack/service-automation's devDependencies when the sibling `builtin/crud-bulk-intent.test.ts` was pinned. It is not cyclic: objectql's transitive dependency closure (12 packages) does not contain service-automation, and `turbo run build --filter=@objectstack/service-automation --dry` (turbo 2.10.7) resolved the graph without complaint. So what is left here is a one-line pin, deferred only because #5393's PR is a spec/executor change and flipping an unmeasured suite red belongs in its own PR.", + "closes": "open the fake's delete with assertEngineDeleteDispatch(opts) and run the package's suite — the devDependency is already declared" }, { "file": "packages/services/service-automation/src/fault-edge-guard-containment.test.ts", "unguarded": 1, "kind": "DEBT", - "why": "@objectstack/service-automation does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", - "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + "why": "MEASURED (#5393): the devDependency this entry used to cite as the blocker now EXISTS — @objectstack/objectql was added to @objectstack/service-automation's devDependencies when the sibling `builtin/crud-bulk-intent.test.ts` was pinned. It is not cyclic: objectql's transitive dependency closure (12 packages) does not contain service-automation, and `turbo run build --filter=@objectstack/service-automation --dry` (turbo 2.10.7) resolved the graph without complaint. So what is left here is a one-line pin, deferred only because #5393's PR is a spec/executor change and flipping an unmeasured suite red belongs in its own PR.", + "closes": "open the fake's delete with assertEngineDeleteDispatch(opts) and run the package's suite — the devDependency is already declared" }, { "file": "packages/services/service-automation/src/plugin-suspended-run-wiring.test.ts", "unguarded": 1, "kind": "DEBT", - "why": "@objectstack/service-automation does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", - "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + "why": "MEASURED (#5393): the devDependency this entry used to cite as the blocker now EXISTS — @objectstack/objectql was added to @objectstack/service-automation's devDependencies when the sibling `builtin/crud-bulk-intent.test.ts` was pinned. It is not cyclic: objectql's transitive dependency closure (12 packages) does not contain service-automation, and `turbo run build --filter=@objectstack/service-automation --dry` (turbo 2.10.7) resolved the graph without complaint. So what is left here is a one-line pin, deferred only because #5393's PR is a spec/executor change and flipping an unmeasured suite red belongs in its own PR.", + "closes": "open the fake's delete with assertEngineDeleteDispatch(opts) and run the package's suite — the devDependency is already declared" }, { "file": "packages/services/service-automation/src/record-lookup-expand.integration.test.ts", "unguarded": 1, "kind": "DEBT", - "why": "@objectstack/service-automation does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", - "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + "why": "MEASURED (#5393): the devDependency this entry used to cite as the blocker now EXISTS — @objectstack/objectql was added to @objectstack/service-automation's devDependencies when the sibling `builtin/crud-bulk-intent.test.ts` was pinned. It is not cyclic: objectql's transitive dependency closure (12 packages) does not contain service-automation, and `turbo run build --filter=@objectstack/service-automation --dry` (turbo 2.10.7) resolved the graph without complaint. So what is left here is a one-line pin, deferred only because #5393's PR is a spec/executor change and flipping an unmeasured suite red belongs in its own PR.", + "closes": "open the fake's delete with assertEngineDeleteDispatch(opts) and run the package's suite — the devDependency is already declared" }, { "file": "packages/services/service-automation/src/runas-grant-resolution.integration.test.ts", "unguarded": 1, "kind": "DEBT", - "why": "@objectstack/service-automation does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", - "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" + "why": "MEASURED (#5393): the devDependency this entry used to cite as the blocker now EXISTS — @objectstack/objectql was added to @objectstack/service-automation's devDependencies when the sibling `builtin/crud-bulk-intent.test.ts` was pinned. It is not cyclic: objectql's transitive dependency closure (12 packages) does not contain service-automation, and `turbo run build --filter=@objectstack/service-automation --dry` (turbo 2.10.7) resolved the graph without complaint. So what is left here is a one-line pin, deferred only because #5393's PR is a spec/executor change and flipping an unmeasured suite red belongs in its own PR.", + "closes": "open the fake's delete with assertEngineDeleteDispatch(opts) and run the package's suite — the devDependency is already declared" }, { "file": "packages/services/service-automation/src/suspended-run-store.test.ts", "unguarded": 1, "kind": "DEBT", - "why": "HAND-MIRRORS the guard already, which is the second copy of the contract this gate exists to remove — but @objectstack/service-automation does not depend on @objectstack/objectql, so replacing the copy with the producer's predicate needs a devDependency change.", - "closes": "add @objectstack/objectql to devDependencies, then replace the mirrored `if` with assertEngineDeleteDispatch(options)" + "why": "HAND-MIRRORS the guard already, which is the second copy of the contract this gate exists to remove. MEASURED (#5393): the devDependency that used to block replacing the copy now EXISTS — @objectstack/objectql was added to @objectstack/service-automation's devDependencies when `builtin/crud-bulk-intent.test.ts` was pinned, and the graph is acyclic (see the sibling entries). What is left is replacing the mirrored `if` with the producer's predicate.", + "closes": "replace the mirrored `if` with assertEngineDeleteDispatch(options) — the devDependency is already declared" }, { "file": "packages/services/service-datasource/src/__tests__/datasource-admin-plugin.test.ts",