Skip to content

Commit 5556455

Browse files
fix(service-automation)!: a flow's record nodes treat a computed filter operand as a value, never as a query operator (#29404)
<!-- claude-projects-attribution: {"github_login":"os-steve"} --> _Requested by **Steve** · [project thread](https://claude.ai/code/project/chan_01FdL1XeUyvVNxLreh3F4Qi9?thread=cmsg_01FdL1XeUyvVNxLreh3F4Qi9K5fJRZk7J3wEEMXeLi47fH)_ Refs #29050 Clause-②: no (narrowing) Metadata-protocol: none Skills-docs: follow-up #29418 — content/docs/automation/flows.mdx, skills/objectstack-automation/SKILL.md ## 维护者速读 - 改了什么:流程的记录节点(`get_record` / `update_record` / `delete_record`)过滤条件里由表达式算出来的值,现在只当作“值”来比较:必须是字符串、数字(含 bigint)、布尔或日期,在 `$in` / `$nin` / `$between` 处可以是它们的列表(不得含空项);算出别的东西(对象、该放单个值的位置上的列表、含非值成员或空项的列表)时,节点在查询前拒绝运行(守卫失败,不可被 fault 边吞掉),提示里点名节点、表达式和它在过滤条件里的位置。作者手写的过滤结构与字面值不变。 - 为什么改:按维护者裁决「A+C 同意」的 C 半,规则是:记录节点把过滤条件操作数位置上的求值结果当作值,绝不当作查询运算符;不是标量(或运算符接受列表时不是标量列表)的值在查询运行前即被拒绝。 - 风险与回滚:收窄(BREAKING):把对象或列表算进过滤操作数的流程会在该节点失败。仓内示例、showcase、dogfood 夹具与各包测试流程普查下来,过滤操作数的表达式全是标量 id 或字段值,无此用法;全量 dogfood 结果见下方 Verification。回滚即撤销本 PR。 - 先看:`packages/services/service-automation/src/builtin/crud-nodes.ts` 的 `resolveNodeFilter` 与新函数 `operandValueRefusal`;`crud-filter-guard.test.ts` 新增的 describe「a computed filter operand is a value, never filter structure」。 - 两分钟验证:`pnpm --filter @objectstack/service-automation exec vitest run src/builtin/crud-filter-guard.test.ts`,预期 1 个测试文件、66 个用例全部通过。 - 需要你做的:无,席位落地。 ## What changes The rule: **a flow's record nodes treat an evaluated value in a filter's operand position as a value, never as a query operator: a value that is not a scalar (or a list of scalars where the operator takes a list) is refused before the query runs.** - **`resolveNodeFilter`** (`packages/services/service-automation/src/builtin/crud-nodes.ts`), the one place `get_record`, `update_record` and `delete_record` resolve their filter, now judges each operand an envelope evaluates to, at every position `flowFilterOperands` locates (a field's value under any key, an operator's comparand, a list element, a nested-relation value, a combinator member, `$not`), before writing it into the copied filter. - **A value** is a string, number, bigint, boolean or date: the platform's accepted comparand set (`isAcceptedFilterComparand`, `@objectstack/spec/data`) less `null`, which the nothing guard owns. At the comparand of an operator whose comparand is a list (`$in`, `$nin`, `$between`; the operand's path ends in that operator), a list whose every member is such a value is one too; a list with an entry that is nothing is refused, as an operand that is nothing is. - **Anything else is refused** with a guard (`refuseNode`), as the existing nothing guard in the same function is: the message names the node type, each refused operand's expression and its place in the filter (`filter.owner.$ne`), and what kind of thing it evaluated to (`an object`, `a list`, `a list whose entry [1] is an object`, `a list whose entry [1] is nothing`). The value itself is never echoed. The data engine is not called. - **Unchanged:** the structure the author wrote (fields, operators, `$and` / `$or` / `$not`, nested relations), every literal operand, query placeholders, and the nothing guard (an operand that evaluates to `null` keeps its message and wins when both apply). - **Docblock:** `storedMetadataFilterRefusal`'s note on reading the resolved filter is reworded, since an operand can no longer evaluate to a field reference. - **Changeset:** `.changeset/29050-record-node-operand-value.md`, patch for `@objectstack/service-automation`, BREAKING (narrowing), ADR-0087 `not-required (no-migration-prescription)`. The list-operator set is local to the executor (`LIST_COMPARAND_OPERATORS`): the spec's own split is deliberately kept out of its published barrel, and no `packages/spec` edit is in scope here. A test reconciles it against `FieldOperatorsSchema` behaviourally, so a list operator added to the grammar reds this package's suite. Reading the set from spec instead is #29419. ## Tests `packages/services/service-automation/src/builtin/crud-filter-guard.test.ts`, new describe "a computed filter operand is a value, never filter structure" (29 cases), on the file's existing data double: - Each record node: an evaluated non-scalar operand is refused before the query, naming the node and the operand; the engine is never called. - `delete_record`, at twelve positions (any key, an operator's comparand, a list element, a nested relation, a combinator member's value, a combinator member, a combinator's list, `$not`, a list where one value belongs, a list at a list element, a list holding a non-value, a list holding an empty entry): refused, located. - Several refused operands are all named in the one refusal. - Each record node: an evaluated list at `$in` / `$nin` / `$between` passes; an evaluated string, number, boolean and date pass; the structure the author wrote reaches the engine unchanged. - Reconciliation: every operator `FieldOperatorsSchema` declares takes an evaluated list exactly when its enforced slot takes a list. - Executor level, each record node: the refusal is a guard and nothing is queried. **Forced test edits:** none. Each pin goes red with its check removed (four ablations, each restored to the HEAD blob). ## Acceptance notes - **Guard, not a routable failure.** The refusal is classed like the nothing guard beside it: the filter the author wrote is not the filter that would run, so a `fault` edge does not route it; a `try_catch` region still catches it. - **Docs and skill follow-up: #29418.** `content/docs/automation/flows.mdx` (the CRUD filter paragraph and the guard list) and `skills/objectstack-automation/SKILL.md` (Filter operands, the does-not-route table) describe operands as value slots but do not yet say a non-value is refused. Both are outside this card's file surface; `skills/**` is a governed surface. - `get-record-stored-metadata-filter-refusal.integration.test.ts`: one of its cases is now refused by this rule before the stored-metadata check is reached; the case still holds (refused, the engine never asked) and is unchanged. ## Verification Every reading below was taken on head `cf1d52e8` (round 2 on top of `37f60fd6`, which merged origin/main `eef116e4`), except the first three ablations, taken on `04dd52eb` (the fix commit, before round 2). <details><summary>Tests, typecheck and lint</summary> - Under the verify lock: `turbo run build --filter='@objectstack/service-automation^...'` 31/31 tasks, exit 0; `vitest run` in `packages/services/service-automation`: 260 files / 3369 tests passed; `pnpm --filter @objectstack/service-automation typecheck` exit 0. - `eslint --no-inline-config` over `packages/services/service-automation`: exit 0. The repo-wide run is CI's. - `test:repo` is not run locally (no file of that project changed); it is CI's `Test Core`. </details> <details><summary>Ablations</summary> Each through `scripts/ablation-replace.mjs` (anchor hit once, blob changed, restored to the HEAD blob with `git diff HEAD` empty), running `crud-filter-guard.test.ts`; the test imports the executor from source, so no `dist` leg applies. - The value check short-circuited: 19 failed / 46 passed of 65 (every refusal pin red; the passing pins stay green). - The list-position check removed: 3 failed / 62 passed of 65. - The list-member check removed: 2 failed / 63 passed of 65. - The empty-entry check removed (on `cf1d52e8`): 1 failed / 65 passed of 66, the new pin. </details> <details><summary>Full dogfood battery</summary> - `cf1d52e8` · `os-verify-lock: VERDICT command-exit 0` (closure build `turbo run build --filter='@objectstack/dogfood^...'` exit 0, then `pnpm --filter @objectstack/dogfood test`) · Test Files 393 passed, 3 skipped (396) · Tests 3243 passed, 28 skipped (3271) · no failures. </details> <details><summary>Dispatch gates</summary> - `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derived the same 60 commands (3 paths, 306 changed lines, under the 3000-line threshold). All 60 run with stdin from /dev/null and the exit captured before any pipe: 60 exit 0. `check:type-check-debt` first read exit 3 (its closure build queued behind the dogfood run on the lock and timed out, nothing measured); re-run under the lock it read exit 0 (`126/129 workspace packages type-checked … 0 in the DEBT ledger`). - `--ran`: 60 derived, 60 run, 0 NOT-MEASURED, 0 UNRUN (a derived zero). - CI check-runs on `cf1d52e8`: not read at the time of writing. </details> ## Human Minimum - [x] 1. Schemas start as Zod; `packages/spec` holds no runtime logic. (No schema touched.) - [x] 2. Every name I added uses the casing for its kind. - [x] 3. No formatter ran; `pnpm lint` passes. (eslint over `packages/services/service-automation`; the repo-wide run is CI's.) - [x] 4. One issue, one branch off an up-to-date `main`, one pull request. - [x] 5. A change that publishes carries a changeset, and both declaration lines are filled in here and in the changeset. - [x] 6. `packages/spec` touched: its generated artifacts are regenerated and committed. (Not touched.) - [x] 7. No hand edits to `content/docs/references/`, `packages/*/CHANGELOG.md` or `content/docs/releases/`. - [x] 8. Title, body and comments are in English. (The 维护者速读 section is Chinese by the repo's maintainer rule.) - [x] The *Before Submitting* list in CONTRIBUTING.md is done. --- _Generated by [Claude Code](https://claude.ai/code/session_01ADHkmR4vTcUse1dqBKZNXq)_ <!-- signed: domain:server#5 · os-steve · session_01ADHkmR4vTcUse1dqBKZNXq --> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent e8b10a6 commit 5556455

3 files changed

Lines changed: 300 additions & 6 deletions

File tree

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
---
2+
'@objectstack/service-automation': patch
3+
---
4+
5+
fix(service-automation)!: a flow's record nodes treat a computed filter operand as a value, never as a query operator
6+
7+
Clause-②: no (narrowing)
8+
Metadata-protocol: none
9+
10+
<!-- adr-0087: not-required (no-migration-prescription) No key of any schema is added, removed, renamed or re-shaped, and no export or type changes. A flow definition the save doors accept is accepted as before; what narrows is the run-time value a `get_record`, `update_record` or `delete_record` node takes from an evaluated filter operand. There is no stored row or authored key for `objectstack migrate meta` to rewrite and no tombstone. -->
11+
12+
**The rule.** A flow's record nodes (`get_record`, `update_record`, `delete_record`) treat an evaluated value in a filter's operand position as a value, never as a query operator: a value that is not a scalar (or a list of scalars where the operator takes a list) is refused before the query runs.
13+
14+
- **A value.** A string, a number, a bigint, a boolean or a date. At the comparand of `$in`, `$nin` or `$between`, a list of them with no empty entry.
15+
- **Refused.** Anything else an operand expression evaluates to (an object, a list where one value belongs, a list holding anything but values, a list with an entry that is nothing), at every operand position and under every key, whatever the value came from. The node fails as a guard before the data engine is asked, and the message names the node, the expression and the operand's place in the filter.
16+
- **Unchanged.** The filter the author wrote: its fields, operators and `$and` / `$or` / `$not` conditions, and every literal operand. An operand that evaluates to nothing is refused as before.
17+
18+
**BREAKING** for a flow that hands a computed object or list into a filter operand where the rule above refuses it: it now fails at that node. Make the expression evaluate to the value the condition compares against (for a record, read its `id`), and write any condition the flow needs in the filter itself.

‎packages/services/service-automation/src/builtin/crud-filter-guard.test.ts‎

Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
44
import { VALUE_SLOT_TEMPLATE_REFUSAL } from '@objectstack/spec/automation';
5+
import { FieldOperatorsSchema } from '@objectstack/spec/data';
56
import { AutomationEngine } from '../engine.js';
67
import { registerCrudNodes } from './crud-nodes.js';
78
import { assertEngineFindOnePredicate } from '@objectstack/metadata-core';
@@ -286,6 +287,193 @@ describe('crud nodes — refuse a filter whose operand evaluated to nothing (#38
286287
});
287288
});
288289

290+
/**
291+
* A computed filter operand is a VALUE, never filter structure.
292+
*
293+
* The filter's structure (its fields, operators and conditions) is what the
294+
* author wrote; an operand envelope supplies only what a condition compares
295+
* against. So an envelope must evaluate to a value: a string, number, boolean
296+
* or date, and at the comparand of a list operator (`$in`, `$nin`,
297+
* `$between`) a list of them. Anything else refuses the node, as a guard,
298+
* before the data engine is asked, at every operand position and whatever the
299+
* key. An operand the author wrote as a literal is not judged here.
300+
*
301+
* Directions decided before running: every refused case leaves the engine
302+
* uncalled (`seen[verb]` undefined); every passing case hands the engine the
303+
* filter with only the operand's value substituted.
304+
*/
305+
describe('a computed filter operand is a value, never filter structure', () => {
306+
let engine: AutomationEngine;
307+
let data: ReturnType<typeof fakeData>;
308+
309+
/** A record whose fields are values of each kind an expression may hand a filter. */
310+
const RECORD = {
311+
id: 'r1',
312+
owner: 'usr_7',
313+
amount: 50,
314+
active: true,
315+
due: new Date('2026-03-14T00:00:00.000Z'),
316+
ref: { id: 'r1' },
317+
ids: ['r1', 'r2'],
318+
stages: ['lost', 'won'],
319+
range: [10, 90],
320+
mixed: ['r1', { id: 'r2' }],
321+
holes: ['r1', null],
322+
conds: [{ status: 'open' }],
323+
};
324+
325+
/** The bulk declaration a predicate write needs, so each fixture is one the real engine accepts. */
326+
const bulk = (nodeType: string) => (nodeType === 'get_record' ? {} : { multi: true });
327+
328+
beforeEach(() => {
329+
data = fakeData();
330+
engine = new AutomationEngine(createTestLogger());
331+
registerCrudNodes(engine, ctxWith(data.service));
332+
});
333+
334+
async function run(nodeType: string, config: Record<string, unknown>) {
335+
engine.registerFlow('crud_flow', crudFlow(nodeType, config));
336+
const result = await engine.execute('crud_flow', { record: RECORD } as any);
337+
const runs = await engine.listRuns('crud_flow');
338+
const step = runs[0]?.steps.find((s) => s.nodeId === 'op');
339+
return { result, step };
340+
}
341+
342+
it.each(NODES)('%s: an evaluated non-scalar operand is refused before the query, naming the node and the operand', async (nodeType, verb, extra) => {
343+
const { result, step } = await run(nodeType, { objectName: 'deal', filter: { id: CEL('record.ref') }, ...extra });
344+
345+
expect(result.success).toBe(false);
346+
expect(step?.error?.code).toBe('NODE_FAILURE');
347+
expect(result.error).toContain(`${nodeType}: refusing to run — 1 filter operand(s) evaluated to something other than a value`);
348+
expect(result.error).toContain('`record.ref` (at filter.id) evaluated to an object');
349+
expect(data.seen[verb], 'the data engine was asked').toBeUndefined();
350+
});
351+
352+
it.each([
353+
['a field\'s value under any key', { owner: CEL('record.ref') }, 'filter.owner', 'an object'],
354+
['an operator\'s comparand', { owner: { $ne: CEL('record.ref') } }, 'filter.owner.$ne', 'an object'],
355+
['a list operator\'s single element', { id: { $in: ['r0', CEL('record.ref')] } }, 'filter.id.$in[1]', 'an object'],
356+
['a nested relation\'s value', { account: { region: CEL('record.ref') } }, 'filter.account.region', 'an object'],
357+
['a combinator member\'s value', { $or: [{ status: 'open' }, { owner: CEL('record.ref') }] }, 'filter.$or[1].owner', 'an object'],
358+
['a combinator member', { $or: [{ status: 'open' }, CEL('record.ref')] }, 'filter.$or[1]', 'an object'],
359+
['a combinator\'s list', { $and: CEL('record.conds') }, 'filter.$and', 'a list'],
360+
['a negated condition', { $not: CEL('record.ref') }, 'filter.$not', 'an object'],
361+
['a list where one value belongs', { id: CEL('record.ids') }, 'filter.id', 'a list'],
362+
['a list at a list operator\'s element', { id: { $in: [CEL('record.ids')] } }, 'filter.id.$in[0]', 'a list'],
363+
['a list operator\'s list holding a non-value', { id: { $in: CEL('record.mixed') } }, 'filter.id.$in', 'a list whose entry [1] is an object'],
364+
['a list operator\'s list holding an empty entry', { id: { $nin: CEL('record.holes') } }, 'filter.id.$nin', 'a list whose entry [1] is nothing'],
365+
])('delete_record: an evaluated non-scalar at %s is refused, located at the operand', async (_label, filter, at, kind) => {
366+
const { result } = await run('delete_record', { objectName: 'deal', filter, multi: true });
367+
368+
expect(result.success).toBe(false);
369+
expect(result.error).toContain(`(at ${at}) evaluated to ${kind}`);
370+
expect(data.seen.delete, 'the data engine was asked').toBeUndefined();
371+
});
372+
373+
it('every refused operand is named in the one refusal', async () => {
374+
const { result } = await run('get_record', {
375+
objectName: 'deal',
376+
filter: { owner: CEL('record.ref'), status: 'open', id: { $in: CEL('record.mixed') } },
377+
});
378+
379+
expect(result.success).toBe(false);
380+
expect(result.error).toContain('2 filter operand(s) evaluated to something other than a value');
381+
expect(result.error).toContain('`record.ref` (at filter.owner) evaluated to an object');
382+
expect(result.error).toContain('`record.mixed` (at filter.id.$in) evaluated to a list whose entry [1] is an object');
383+
expect(data.seen.find).toBeUndefined();
384+
});
385+
386+
it.each(NODES)('%s: an evaluated list at a list operator\'s comparand passes, each member a value', async (nodeType, verb, extra) => {
387+
const { result } = await run(nodeType, {
388+
objectName: 'deal',
389+
filter: { id: { $in: CEL('record.ids') }, stage: { $nin: CEL('record.stages') }, amount: { $between: CEL('record.range') } },
390+
...extra,
391+
...bulk(nodeType),
392+
});
393+
394+
expect(result.success, `run failed: ${JSON.stringify(result.error)}`).toBe(true);
395+
expect(data.seen[verb]).toEqual({ id: { $in: ['r1', 'r2'] }, stage: { $nin: ['lost', 'won'] }, amount: { $between: [10, 90] } });
396+
});
397+
398+
it('every declared operator whose comparand is a list takes an evaluated list, and no other does', async () => {
399+
const operators = Object.keys((FieldOperatorsSchema as unknown as { shape: Record<string, unknown> }).shape);
400+
expect(operators.length, 'control: the grammar declares its operators').toBeGreaterThan(10);
401+
const pair = ['2026-01-01', '2026-12-31'];
402+
for (const op of operators) {
403+
const takesList = FieldOperatorsSchema.safeParse({ [op]: pair }).success;
404+
data = fakeData();
405+
engine = new AutomationEngine(createTestLogger());
406+
registerCrudNodes(engine, ctxWith(data.service));
407+
engine.registerFlow('crud_flow', crudFlow('get_record', { objectName: 'deal', filter: { due: { [op]: CEL('record.pair') } } }));
408+
const result = await engine.execute('crud_flow', { record: { pair } } as any);
409+
expect(result.success, `${op}: the grammar ${takesList ? 'takes' : 'refuses'} a list there`).toBe(takesList);
410+
expect(data.seen.find, `${op}: the data engine was asked`).toEqual(takesList ? { due: { [op]: pair } } : undefined);
411+
}
412+
});
413+
414+
it.each(NODES)('%s: an evaluated scalar passes — a string, a number, a boolean and a date', async (nodeType, verb, extra) => {
415+
const { result } = await run(nodeType, {
416+
objectName: 'deal',
417+
filter: {
418+
owner: CEL('record.owner'),
419+
amount: { $gt: CEL('record.amount') },
420+
active: CEL('record.active'),
421+
due: { $lt: CEL('record.due') },
422+
},
423+
...extra,
424+
...bulk(nodeType),
425+
});
426+
427+
expect(result.success, `run failed: ${JSON.stringify(result.error)}`).toBe(true);
428+
expect(data.seen[verb]).toEqual({ owner: 'usr_7', amount: { $gt: 50 }, active: true, due: { $lt: RECORD.due } });
429+
});
430+
431+
it.each(NODES)('%s: the structure the author wrote is unchanged — only the evaluated operand is judged', async (nodeType, verb, extra) => {
432+
const { result } = await run(nodeType, {
433+
objectName: 'deal',
434+
filter: {
435+
owner: { $ne: CEL('record.owner') },
436+
status: { $in: ['open', 'won'] },
437+
closed_at: { $null: true },
438+
account: { region: 'emea' },
439+
$or: [{ stage: 'legal' }, { stage: { $nin: ['lost'] } }],
440+
},
441+
...extra,
442+
...bulk(nodeType),
443+
});
444+
445+
expect(result.success, `run failed: ${JSON.stringify(result.error)}`).toBe(true);
446+
expect(data.seen[verb]).toEqual({
447+
owner: { $ne: 'usr_7' },
448+
status: { $in: ['open', 'won'] },
449+
closed_at: { $null: true },
450+
account: { region: 'emea' },
451+
$or: [{ stage: 'legal' }, { stage: { $nin: ['lost'] } }],
452+
});
453+
});
454+
455+
it.each(NODES)('%s: the executor\'s refusal of a non-scalar operand is a guard, and nothing is queried', async (nodeType, verb, extra) => {
456+
const executors = new Map<string, { execute: (...args: any[]) => Promise<any> }>();
457+
const recorder = new Proxy(new AutomationEngine(createTestLogger()), {
458+
get(target, key, receiver) {
459+
if (key === 'registerNodeExecutor') return (e: any) => { executors.set(e.type, e); };
460+
return Reflect.get(target, key, receiver);
461+
},
462+
});
463+
registerCrudNodes(recorder as AutomationEngine, ctxWith(data.service));
464+
465+
const result = await executors.get(nodeType)!.execute(
466+
{ id: 'op', type: nodeType, label: 'Op', config: { objectName: 'deal', filter: { id: CEL('record.ref') }, ...extra } },
467+
new Map<string, unknown>([['record', RECORD]]),
468+
{},
469+
);
470+
expect(result.success).toBe(false);
471+
expect(result.errorClass).toBe('guard');
472+
expect(result.error).toContain('evaluated to something other than a value');
473+
expect(data.seen[verb]).toBeUndefined();
474+
});
475+
});
476+
289477
describe('a flow `{…}` token at a filter operand is refused at every door (#19939)', () => {
290478
/** The builtin CRUD executor itself — the run-time door, reached without `registerFlow`. */
291479
function executorOf(nodeType: string, data: ReturnType<typeof fakeData>) {

0 commit comments

Comments
 (0)