Skip to content

fix(spec,service-automation,lint): flow metadata is canonicalized inside structured regions (#4347) #4381

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
os-zhuang merged 1 commit into from
Jul 31, 2026
Merged

fix(spec,service-automation,lint): flow metadata is canonicalized inside structured regions (#4347) #4381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions .changeset/region-metadata-parity.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
---
'@objectstack/spec': patch
'@objectstack/service-automation': patch
'@objectstack/lint': patch
---

Flow metadata is canonicalized inside structured regions, not just at the top level (#4347).

`registerFlow` canonicalizes a stored flow through three passes — the ADR-0087 conversion
table, `FlowSchema.parse`, and the ADR-0032 predicate validation — and every one of them
walked `flow.nodes` / `flow.edges` only. An ADR-0031 container keeps a whole sub-graph in
its open `config` (`loop.config.body`, `parallel.config.branches[]`,
`try_catch.config.try`/`.catch`), so all three stopped at the container and metadata came
out **position-dependent**: the same node converted at the top level and did not one level
in, and the same predicate was stored as a `{ dialect: 'cel', source }` envelope on a
top-level edge and left a bare string on a loop-body edge.

The reporting app shipped three sweeps whose gates never opened. Each run reported
`success: true`, queried correctly, selected exactly the right records, and then did
nothing — which is indistinguishable from "this sweep had no work to do" unless you assert
on records written.

- **`mapFlowNodes` recurses into regions**, to any depth. Every conversion in the table now
reaches a nested node, which matters most for the two that change behaviour rather than
spelling: a `webhook` / `http_request` callout inside a loop body kept a type no executor
owns (the run failed), and a `delete_record` kept `config.filters`, leaving the canonical
`filter` the executor reads absent — the erased-condition hazard
`flow-node-crud-filter-alias` exists to prevent. Notice paths carry the region
(`flows[0].nodes[3].config.body.nodes[1].config.filter`), so the warning points at the
node to edit.
- **New `normalizeControlFlowRegions`**, called at the load seam after
`validateControlFlow`: each region is parsed through its own schema (recursively — regions
nest), so nested edges and nodes carry the same canonical shapes as top-level ones. A
region that does not parse is left untouched; rejecting one stays `validateControlFlow`'s
job, so which flows register is unchanged.
- **New `collectFlowGraphs`** yields a flow's own graph plus every nested region, each with
a scope label. Both predicate validators iterate it instead of `flow.nodes` — the engine's
`validateFlowExpressions` and `@objectstack/lint`'s author-time
`validateStackExpressions` — so the `{record.x}` brace-trap they exist to catch is now
caught inside a loop body too, naming the region (`loop 'sweep' body · edge 'b1' …`). It
used to pass `objectstack validate`, pass registration, and fail at run time with the
diagnostic suppressed.

The container executors already parse their own config at run time (`parseNodeConfig`,
#4277), so a nested predicate did evaluate correctly on current `main` — what was still
wrong is everything that reads a region *without* re-parsing it (the Studio designer,
`getFlow`, the version history), and every conversion, none of which the executors replay.

Also hardened, per the issue's secondary finding: `evaluateCondition`'s legacy `{var}`
template path **refuses an unresolved dotted reference** instead of comparing it as a
string. `'oppRecord.amount > 500000'` was compared `'oppRecord.amount' > '500000'` — `'o'`
against `'5'` — so it was constantly true regardless of the amount: silently wrong in the
*true* direction, a gate that reports success while never gating. It now throws with the
source and the fix (a CEL envelope, or brace the reference if the `{var}` dialect was
meant), the same "never swallow a broken predicate" rule ADR-0032 §1c set for the CEL path.
The `try { … } catch { return false }` around that block went with it: nothing in it throws,
so it guarded nothing and would have swallowed the new refusal straight back into the silent
wrong answer. Bare-word comparisons (`'{status} == active'`) and `{var}` templates are
unchanged — only dotted references, which substitution can never leave behind, are refused.
76 changes: 76 additions & 0 deletions packages/lint/src/validate-expressions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,82 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => {
expect(issues).toHaveLength(0);
});

/**
* #4347 — the walk used to stop at a container, so a predicate written in
* the wrong dialect inside a `loop` body passed `objectstack validate` and
* shipped, while the identical predicate one level out was a build error.
*/
describe('structured regions', () => {
const flowWith = (container: Record<string, unknown>) => ({
objects,
flows: [{
name: 'sweep',
nodes: [{ id: 'start', type: 'start', config: { objectName: 'crm_lead' } }, container],
edges: [],
}],
});
const badRegion = () => ({
nodes: [
{ id: 'gate', type: 'decision', config: { condition: '{record.rating} >= 4' } },
{ id: 'act', type: 'update_record' },
],
edges: [{ id: 'b1', source: 'gate', target: 'act', condition: '{record.status} == "open"' }],
});

it('flags a brace-in-CEL predicate inside a loop body, naming the region', () => {
const issues = validateStackExpressions(flowWith({
id: 'loop_1', type: 'loop', config: { collection: '{rows}', body: badRegion() },
}));
// Both the body node's `config.condition` and the body edge's condition.
expect(issues).toHaveLength(2);
for (const issue of issues) expect(issue.where).toContain("loop 'loop_1' body");
expect(issues.map(i => i.source)).toEqual(['{record.rating} >= 4', '{record.status} == "open"']);
});

it('flags them in parallel branches and try_catch regions too', () => {
expect(validateStackExpressions(flowWith({
id: 'par', type: 'parallel', config: { branches: [badRegion(), badRegion()] },
}))).toHaveLength(4);

const tc = validateStackExpressions(flowWith({
id: 'tc', type: 'try_catch', config: { try: badRegion(), catch: badRegion() },
}));
expect(tc).toHaveLength(4);
expect(tc.filter(i => i.where.includes("try_catch 'tc' catch"))).toHaveLength(2);
});

it('reaches a container nested inside another region', () => {
const issues = validateStackExpressions(flowWith({
id: 'outer', type: 'loop',
config: {
collection: '{rows}',
body: {
nodes: [{ id: 'inner', type: 'loop', config: { collection: '{cols}', body: badRegion() } }],
edges: [],
},
},
}));
expect(issues).toHaveLength(2);
for (const issue of issues) expect(issue.where).toContain("loop 'outer' body → loop 'inner' body");
});

it('leaves a correct region alone', () => {
expect(validateStackExpressions(flowWith({
id: 'loop_1', type: 'loop',
config: {
collection: '{rows}',
body: {
nodes: [
{ id: 'gate', type: 'decision', config: { condition: 'record.rating >= 4' } },
{ id: 'act', type: 'update_record' },
],
edges: [{ id: 'b1', source: 'gate', target: 'act', condition: 'record.status == "open"' }],
},
},
}))).toHaveLength(0);
});
});

it('tolerates a screen with no fields, a non-array fields, and no config', () => {
expect(validateStackExpressions(screenFlow([]))).toHaveLength(0);
expect(validateStackExpressions({
Expand Down
136 changes: 73 additions & 63 deletions packages/lint/src/validate-expressions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
*/

import { validateExpression } from '@objectstack/formula';
import { resolveFlowNodeExpressions } from '@objectstack/spec/automation';
import { collectFlowGraphs, resolveFlowNodeExpressions } from '@objectstack/spec/automation';
import type { FlowNodeParsed } from '@objectstack/spec/automation';

export interface ExprIssue {
where: string;
Expand Down Expand Up @@ -130,76 +131,85 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] {
for (const flow of asArray(stack.flows)) {
const flowName = typeof flow.name === 'string' ? flow.name : '(unnamed flow)';
const nodes = Array.isArray(flow.nodes) ? (flow.nodes as AnyRec[]) : [];
const edges = Array.isArray(flow.edges) ? (flow.edges as AnyRec[]) : [];
// The record-change target object — `record.*` refs resolve against it.
const startNode = nodes.find(n => n.type === 'start');
const startCfg = (startNode?.config ?? {}) as AnyRec;
const objectName = typeof startCfg.objectName === 'string' ? startCfg.objectName : undefined;

for (const node of nodes) {
const cfg = (node.config ?? {}) as AnyRec;
check(`flow '${flowName}' · node '${node.id}' (${node.type}) condition`, cfg.condition, objectName);
// #4347 — every graph in the flow, not just `flow.nodes`/`flow.edges`. An
// ADR-0031 container keeps a whole sub-graph in its `config`, so the
// top-level walk validated PART of the flow while reporting on all of it: a
// predicate written in the wrong dialect inside a `loop` body passed
// `objectstack validate` and shipped. This is the author-time half of the
// same traversal the engine's registration pass now does; `scope` names the
// region so the located message still points at one edge.
for (const graph of collectFlowGraphs(flow as { nodes?: FlowNodeParsed[] })) {
const at = graph.scope ? `flow '${flowName}' · ${graph.scope}` : `flow '${flowName}'`;
for (const node of graph.nodes as unknown as AnyRec[]) {
const cfg = (node.config ?? {}) as AnyRec;
check(`${at} · node '${node.id}' (${node.type}) condition`, cfg.condition, objectName);

// Descriptor-declared expression slots (#4027). Before this, the traversal
// hardcoded `condition` and assumed every other node string was a `{var}`
// template — so `screen.fields[].visibleWhen`, declared bare CEL since
// #3304, was validated by nobody and #3528 shipped a template-dialect
// predicate through compile, validate and run time in silence.
// Only `predicate` slots are checkable: `flow-template` slots take the
// single-brace `{var}` dialect `interpolate()` implements, which no
// validator covers (the `template` role enforces ADR-0032 §3's
// double-brace text template and would reject every correct
// `loop.collection`). The ledger records them regardless, so the
// reconciliation ratchet still sees the marker.
const nodeType = typeof node.type === 'string' ? node.type : '';
for (const found of resolveFlowNodeExpressions(nodeType, cfg)) {
if (found.entry.role !== 'predicate') continue;
checkDeclaredPredicate(
`flow '${flowName}' · node '${node.id}' (${nodeType}) ${found.entry.label} at config.${found.path}`,
found.value,
);
}
// #1870 — a `script` node must declare a callable target (`actionType` or
// `function`). A node with neither is a silent no-op that otherwise passes
// build. (Function *existence* isn't checkable here — functions are code,
// not serialized into the artifact — so this is a structural check; the
// runtime verifies the named function is actually registered.)
if (node.type === 'script') {
// `function` is canonical; a pre-parse source may still carry the
// `functionName` alias during the protocol-17 window, until the
// 'flow-node-script-config-aliases' conversion (#3796) canonicalizes it.
const fn =
(typeof cfg.function === 'string' ? cfg.function.trim() : '') ||
(typeof cfg.functionName === 'string' ? cfg.functionName.trim() : '');
const action = typeof cfg.actionType === 'string' ? cfg.actionType.trim() : '';
// Inline `config.script` (a JS body) is also a declared form — the
// built-in runtime doesn't execute it (warned at run time), but the node
// is not the empty no-op this check targets, so don't flag it.
const inline = typeof cfg.script === 'string' ? cfg.script.trim() : '';
if (!fn && !action && !inline) {
issues.push({
where: `flow '${flowName}' · node '${node.id}' (script) callable`,
message:
`script node declares neither \`actionType\` nor \`function\` — it would do nothing at runtime. ` +
`Name a built-in action (e.g. \`actionType: 'email'\`) or a registered function ` +
`(\`function: 'my_fn'\`, registered via \`defineStack({ functions })\`).`,
source: JSON.stringify({ id: node.id, type: node.type, config: cfg }),
});
} else if (action === 'invoke_function' && !fn) {
// `actionType: 'invoke_function'` is a marker that names no callable on
// its own — the function name must be in `function`/`functionName`.
issues.push({
where: `flow '${flowName}' · node '${node.id}' (script) callable`,
message:
`script node uses \`actionType: 'invoke_function'\` but no \`function\` (or \`functionName\`) — ` +
`it names no callable. Set \`function: 'my_fn'\` and register it via \`defineStack({ functions })\`.`,
source: JSON.stringify({ id: node.id, type: node.type, config: cfg }),
});
// Descriptor-declared expression slots (#4027). Before this, the traversal
// hardcoded `condition` and assumed every other node string was a `{var}`
// template — so `screen.fields[].visibleWhen`, declared bare CEL since
// #3304, was validated by nobody and #3528 shipped a template-dialect
// predicate through compile, validate and run time in silence.
// Only `predicate` slots are checkable: `flow-template` slots take the
// single-brace `{var}` dialect `interpolate()` implements, which no
// validator covers (the `template` role enforces ADR-0032 §3's
// double-brace text template and would reject every correct
// `loop.collection`). The ledger records them regardless, so the
// reconciliation ratchet still sees the marker.
const nodeType = typeof node.type === 'string' ? node.type : '';
for (const found of resolveFlowNodeExpressions(nodeType, cfg)) {
if (found.entry.role !== 'predicate') continue;
checkDeclaredPredicate(
`${at} · node '${node.id}' (${nodeType}) ${found.entry.label} at config.${found.path}`,
found.value,
);
}
// #1870 — a `script` node must declare a callable target (`actionType` or
// `function`). A node with neither is a silent no-op that otherwise passes
// build. (Function *existence* isn't checkable here — functions are code,
// not serialized into the artifact — so this is a structural check; the
// runtime verifies the named function is actually registered.)
if (node.type === 'script') {
// `function` is canonical; a pre-parse source may still carry the
// `functionName` alias during the protocol-17 window, until the
// 'flow-node-script-config-aliases' conversion (#3796) canonicalizes it.
const fn =
(typeof cfg.function === 'string' ? cfg.function.trim() : '') ||
(typeof cfg.functionName === 'string' ? cfg.functionName.trim() : '');
const action = typeof cfg.actionType === 'string' ? cfg.actionType.trim() : '';
// Inline `config.script` (a JS body) is also a declared form — the
// built-in runtime doesn't execute it (warned at run time), but the node
// is not the empty no-op this check targets, so don't flag it.
const inline = typeof cfg.script === 'string' ? cfg.script.trim() : '';
if (!fn && !action && !inline) {
issues.push({
where: `${at} · node '${node.id}' (script) callable`,
message:
`script node declares neither \`actionType\` nor \`function\` — it would do nothing at runtime. ` +
`Name a built-in action (e.g. \`actionType: 'email'\`) or a registered function ` +
`(\`function: 'my_fn'\`, registered via \`defineStack({ functions })\`).`,
source: JSON.stringify({ id: node.id, type: node.type, config: cfg }),
});
} else if (action === 'invoke_function' && !fn) {
// `actionType: 'invoke_function'` is a marker that names no callable on
// its own — the function name must be in `function`/`functionName`.
issues.push({
where: `${at} · node '${node.id}' (script) callable`,
message:
`script node uses \`actionType: 'invoke_function'\` but no \`function\` (or \`functionName\`) — ` +
`it names no callable. Set \`function: 'my_fn'\` and register it via \`defineStack({ functions })\`.`,
source: JSON.stringify({ id: node.id, type: node.type, config: cfg }),
});
}
}
}
}
for (const edge of edges) {
check(`flow '${flowName}' · edge '${edge.id}' (${edge.source}→${edge.target}) condition`, edge.condition, objectName);
for (const edge of graph.edges as unknown as AnyRec[]) {
check(`${at} · edge '${edge.id}' (${edge.source}→${edge.target}) condition`, edge.condition, objectName);
}
}
}

Expand Down
Loading
Loading