diff --git a/.changeset/flow-nested-region-walk.md b/.changeset/flow-nested-region-walk.md new file mode 100644 index 0000000000..6e1464fecb --- /dev/null +++ b/.changeset/flow-nested-region-walk.md @@ -0,0 +1,77 @@ +--- +"@objectstack/lint": patch +--- + +fix(lint): flow rules see into try_catch / loop / parallel regions (#4380) + +Every lint rule that inspects flow nodes had hand-written the same one-liner — + +```ts +const nodes = Array.isArray(flow.nodes) ? (flow.nodes as AnyRec[]) : []; +``` + +— and every one of them was therefore blind to the same thing. +`FlowRegionSchema` holds a full `nodes: z.array(FlowNodeSchema)`, and four +config slots carry one: `try_catch.config.try` / `.catch`, `loop.config.body`, +and `parallel.config.branches[].nodes`. Regions nest arbitrarily. Move a node +into any of them and the checking stayed behind. + +Measured before the fix, the same bad nodes at the top level vs inside a +`try_catch`: + +| rule | severity | flat | nested | +| :--- | :--- | :--- | :--- | +| `flow-node-write-unknown-field` | error | 1 | **0** | +| `flow-update-readonly-field` | error | 1 | **0** | +| `approval-approver-*` | error/warning | 1 | **0** | +| `flow-template-unknown-field` (filter position) | error | 1 | **1, as a warning** | + +**The last row is the one a reader would not predict.** +`validate-flow-template-paths` scans a node's whole `config` for string leaves, +so it still *saw* tokens inside a region — but its `filter`-position split only +looks at the top level of the node it was handed. A nested filter token lost its +position, so the #3810 finding ("this node cannot run — an erased condition +WIDENS the query") silently degraded to an advisory warning, reported against +the wrapping `try_catch` instead of the `get_record` that is broken: + +``` +FLAT error flow "f" node "get_record" flows[0].nodes[1] +NESTED warning flow "f" node "try_catch" flows[0].nodes[1] +``` + +Being visible is not the same as being judged correctly. That is worse than a +clean miss: a yellow line reads as "checked and merely advisory". + +**One shared walk, not five.** `flow-walk.ts` — the flow-side counterpart of the +existing `page-walk.ts`, and here for the same stated reason: getting the +traversal right is subtle enough that duplicating it has already produced dead +rules. `walkFlowNodes(flow, flowPath)` yields every node with its real config +path (`flows[0].nodes[1].config.catch.nodes[0]`), a region breadcrumb for +diagnostics (`try_catch "Guard" › catch`), and depth. Four rules now route +through it: the two flow write rules, the template-path rule, and the approval +rule. + +Findings now land on the node that is actually wrong, which is the point — a +path pointing at the container is not actionable in a flow with several regions. + +**The double-count trap is handled, not left to each caller.** A container node +is walked too (it has its own config worth checking — a `loop`'s `collection`, a +`try_catch`'s `retry`), but its `config` physically contains every descendant, +so a rule that scans config recursively would report each nested finding twice. +`WalkedFlowNode.localConfig` is the container's config with region slots +removed; the recursive scanner uses it, and a test pins that a nested token is +reported once while the container's own `collection` token still is. + +`REGION_SLOTS` is declared as data and pinned against the spec's own +region-bearing config schemas — derived behaviourally (a slot is one that +accepts `{nodes: […]}`), not restated — so a fifth construct fails that test +instead of becoming a fifth silent blind spot. A `MAX_REGION_DEPTH` cap keeps a +hand-authored (pre-parse) stack from hanging a lint. + +Verified end to end: nested now matches flat on every rule, including the +restored `error` severity. app-showcase ships an `update_record` inside a +`catch` branch (`showcase_resilient_sync`) that had never been checked by +anything — it is correct, so validation stays clean, and breaking its field name +on purpose now fails `os validate` with +`flows[24].nodes[1].config.catch.nodes[0].config.fields.sync_statuss` and the +region trail `try_catch "Push with retry" › catch › node "Flag Sync Failure"`. diff --git a/packages/lint/src/flow-walk.test.ts b/packages/lint/src/flow-walk.test.ts new file mode 100644 index 0000000000..033ab8435c --- /dev/null +++ b/packages/lint/src/flow-walk.test.ts @@ -0,0 +1,206 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { + LoopConfigSchema, + ParallelConfigSchema, + TryCatchConfigSchema, +} from '@objectstack/spec/automation'; + +import { + walkFlowNodes, + flowNodeLabel, + REGION_SLOTS, + REGION_CONFIG_KEYS, + MAX_REGION_DEPTH, +} from './flow-walk.js'; + +const node = (id: string, extra: Record = {}) => ({ id, type: 'script', ...extra }); + +describe('REGION_SLOTS — pinned against the spec, not restated', () => { + // The ledger's job is to make a NEW region-bearing construct fail here rather + // than become a fifth silent blind spot. Derived behaviourally from the + // spec's own config schemas: a region slot is one that accepts `{nodes: […]}`. + const REGION_BEARING_CONFIGS = { + try_catch: TryCatchConfigSchema, + loop: LoopConfigSchema, + parallel: ParallelConfigSchema, + } as const; + + /** Keys of `schema` that accept a region (or an array of them). */ + const regionKeysOf = (schema: { safeParse: (v: unknown) => { success: boolean; data?: unknown } }): string[] => { + // `label` is required by FlowNodeSchema — a probe node without it fails the + // parse and would make every slot look non-region. + const region = { nodes: [{ id: 'probe', type: 'script', label: 'Probe' }], edges: [] }; + const probes: Record = { + // Required siblings so the parse reaches the region keys at all. + collection: '{items}', + try: region, + catch: region, + body: region, + branches: [{ nodes: region.nodes, edges: [] }, { nodes: region.nodes, edges: [] }], + }; + const parsed = schema.safeParse(probes); + if (!parsed.success) return []; + const data = parsed.data as Record; + return Object.keys(data).filter((k) => { + const v = data[k]; + if (Array.isArray(v)) return v.every((e) => !!e && typeof e === 'object' && Array.isArray((e as never)['nodes'])); + return !!v && typeof v === 'object' && Array.isArray((v as Record).nodes); + }); + }; + + it('declares exactly the region slots each construct actually accepts', () => { + for (const [type, schema] of Object.entries(REGION_BEARING_CONFIGS)) { + expect([...(REGION_SLOTS.get(type) ?? [])].sort(), `region slots for '${type}'`).toEqual( + regionKeysOf(schema).sort(), + ); + } + }); + + it('derives REGION_CONFIG_KEYS from the per-type slots', () => { + expect([...REGION_CONFIG_KEYS].sort()).toEqual([...new Set([...REGION_SLOTS.values()].flat())].sort()); + }); +}); + +describe('walkFlowNodes', () => { + it('yields top-level nodes with a flat path and an empty trail', () => { + const walked = walkFlowNodes({ nodes: [node('a'), node('b')] }, 'flows[0]'); + expect(walked.map((w) => w.path)).toEqual(['flows[0].nodes[0]', 'flows[0].nodes[1]']); + expect(walked.every((w) => w.regionTrail === '' && w.depth === 0)).toBe(true); + }); + + it('reaches try_catch try + catch regions', () => { + const flow = { + nodes: [ + { + id: 'guard', + type: 'try_catch', + label: 'Guard', + config: { + try: { nodes: [node('push')], edges: [] }, + catch: { nodes: [node('flag')], edges: [] }, + }, + }, + ], + }; + const walked = walkFlowNodes(flow, 'flows[0]'); + expect(walked.map((w) => w.path)).toEqual([ + 'flows[0].nodes[0]', + 'flows[0].nodes[0].config.try.nodes[0]', + 'flows[0].nodes[0].config.catch.nodes[0]', + ]); + expect(walked[2].regionTrail).toBe('try_catch "Guard" › catch'); + expect(walked[2].depth).toBe(1); + }); + + it('reaches a loop body', () => { + const flow = { + nodes: [{ id: 'each', type: 'loop', config: { collection: '{items}', body: { nodes: [node('inner')], edges: [] } } }], + }; + const walked = walkFlowNodes(flow, 'flows[0]'); + expect(walked.map((w) => w.path)).toEqual(['flows[0].nodes[0]', 'flows[0].nodes[0].config.body.nodes[0]']); + expect(walked[1].regionTrail).toBe('loop "each" › body'); + }); + + it('reaches every parallel branch, named or indexed', () => { + const flow = { + nodes: [ + { + id: 'fan', + type: 'parallel', + config: { + branches: [ + { name: 'left', nodes: [node('l')], edges: [] }, + { nodes: [node('r')], edges: [] }, + ], + }, + }, + ], + }; + const walked = walkFlowNodes(flow, 'flows[0]'); + expect(walked.map((w) => w.path)).toEqual([ + 'flows[0].nodes[0]', + 'flows[0].nodes[0].config.branches[0].nodes[0]', + 'flows[0].nodes[0].config.branches[1].nodes[0]', + ]); + expect(walked[1].regionTrail).toBe('parallel "fan" › branch left'); + expect(walked[2].regionTrail).toBe('parallel "fan" › branch #1'); + }); + + it('recurses through nested regions and accumulates the trail', () => { + const flow = { + nodes: [ + { + id: 'outer', + type: 'try_catch', + config: { + try: { + nodes: [ + { + id: 'inner', + type: 'loop', + config: { collection: '{x}', body: { nodes: [node('deep')], edges: [] } }, + }, + ], + edges: [], + }, + }, + }, + ], + }; + const walked = walkFlowNodes(flow, 'flows[0]'); + const deep = walked.find((w) => w.node.id === 'deep'); + expect(deep?.path).toBe('flows[0].nodes[0].config.try.nodes[0].config.body.nodes[0]'); + expect(deep?.regionTrail).toBe('try_catch "outer" › try › loop "inner" › body'); + expect(deep?.depth).toBe(2); + }); + + // The trap that makes a recursive config scan double-report. + it('localConfig strips region slots but keeps the container’s own config', () => { + const flow = { + nodes: [ + { + id: 'each', + type: 'loop', + config: { collection: '{items}', maxIterations: 10, body: { nodes: [node('inner')], edges: [] } }, + }, + ], + }; + const [container, inner] = walkFlowNodes(flow, 'flows[0]'); + expect(Object.keys(container.localConfig ?? {}).sort()).toEqual(['collection', 'maxIterations']); + // The raw node is untouched — stripping is a view, not a mutation. + expect((container.node.config as Record).body).toBeDefined(); + // A non-container node's localConfig is its config, not a copy-with-holes. + expect(inner.localConfig).toEqual(inner.node.config ?? undefined); + }); + + it('leaves localConfig undefined for a node with no config', () => { + const [only] = walkFlowNodes({ nodes: [{ id: 'x', type: 'end' }] }, 'flows[0]'); + expect(only.localConfig).toBeUndefined(); + }); + + it('labels a node by label, then id, then index', () => { + expect(flowNodeLabel({ label: 'L', id: 'i' }, 0)).toBe('L'); + expect(flowNodeLabel({ id: 'i' }, 0)).toBe('i'); + expect(flowNodeLabel({}, 3)).toBe('#3'); + }); + + it('tolerates missing/!array nodes and non-record entries', () => { + expect(walkFlowNodes({}, 'flows[0]')).toEqual([]); + expect(walkFlowNodes({ nodes: 'nope' }, 'flows[0]')).toEqual([]); + expect(walkFlowNodes({ nodes: [null, 'x', node('ok')] }, 'flows[0]').map((w) => w.node.id)).toEqual(['ok']); + expect(walkFlowNodes({ nodes: [{ id: 'c', type: 'try_catch', config: { try: 'nope' } }] }, 'flows[0]')).toHaveLength(1); + }); + + it('stops at the depth cap rather than recursing without bound', () => { + // Build MAX_REGION_DEPTH + 3 levels of try nesting. + let deepest: Record = { id: 'leaf', type: 'script' }; + for (let i = 0; i < MAX_REGION_DEPTH + 3; i++) { + deepest = { id: `t${i}`, type: 'try_catch', config: { try: { nodes: [deepest], edges: [] } } }; + } + const walked = walkFlowNodes({ nodes: [deepest] }, 'flows[0]'); + expect(walked.length).toBeGreaterThan(0); + expect(Math.max(...walked.map((w) => w.depth))).toBeLessThanOrEqual(MAX_REGION_DEPTH); + }); +}); diff --git a/packages/lint/src/flow-walk.ts b/packages/lint/src/flow-walk.ts new file mode 100644 index 0000000000..455df54ab0 --- /dev/null +++ b/packages/lint/src/flow-walk.ts @@ -0,0 +1,191 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Shared flow-node traversal for the rules that inspect `FlowNode.config` + * (issue #4380) — the flow-side counterpart of `page-walk.ts`, and here for the + * same reason: every rule had hand-written the same one-liner, + * + * ```ts + * const nodes = Array.isArray(flow.nodes) ? (flow.nodes as AnyRec[]) : []; + * ``` + * + * …and every one of them was therefore blind to the same thing. + * + * ## What the one-liner misses + * + * `FlowRegionSchema` (`@objectstack/spec/automation`) holds a FULL + * `nodes: z.array(FlowNodeSchema)`, and four config slots carry one: + * + * | node type | slot(s) | + * |-------------|----------------------------------| + * | `try_catch` | `config.try`, `config.catch` | + * | `loop` | `config.body` | + * | `parallel` | `config.branches[].nodes` | + * + * Regions nest arbitrarily (a region node may itself be a `try_catch`). Before + * this walk, a node moved into any of them left the checking behind: + * `flow-node-write-unknown-field` and `flow-update-readonly-field` — both + * GATING errors — reported nothing, and `approval-approver-*` went quiet too. + * + * `validate-flow-template-paths` failed a third way, worth naming because it is + * the one a reader would not predict: it scans a node's whole `config` for + * string leaves, so it still SAW tokens inside a region — but its `filter` + * position split only looks at the top level of the node it was handed, so a + * nested filter token lost its position and the #3810 finding silently + * downgraded from `error` to `warning`, reported against the wrapping + * `try_catch` instead of the `get_record` that cannot run. Being visible is not + * the same as being judged correctly, which is why {@link WalkedFlowNode} + * carries a real per-node `path` rather than only the node object. + * + * ## The double-count trap + * + * A container node is yielded too — it has its own config worth checking (a + * `loop`'s `collection`, a `try_catch`'s `retry`). But its `config` physically + * CONTAINS every descendant, so any rule that walks config recursively would + * report each nested finding twice: once at the inner node, once at the + * container. {@link WalkedFlowNode.localConfig} is the container's config with + * the region slots removed — the view a recursive scan must use. Rules that + * read named keys (`config.fields`, `config.objectName`) can use either. + */ + +export type AnyRec = Record; + +function isRec(v: unknown): v is AnyRec { + return !!v && typeof v === 'object' && !Array.isArray(v); +} + +function strName(v: unknown): string | undefined { + return typeof v === 'string' && v.length > 0 ? v : undefined; +} + +/** + * Config keys that hold a nested region, by owning node type. Declared as data + * so a new construct is one entry here rather than a fifth silent blind spot, + * and pinned against the spec's own region-bearing schemas by + * `flow-walk.test.ts`. + */ +export const REGION_SLOTS: ReadonlyMap = new Map([ + ['try_catch', ['try', 'catch']], + ['loop', ['body']], + // `parallel` is the odd one: `config.branches[]` is an ARRAY of regions, not + // a region. Handled separately in the walk; named here so the key set stays + // one list. + ['parallel', ['branches']], +]); + +/** Every config key that may hold region nodes, across all node types. */ +export const REGION_CONFIG_KEYS: ReadonlySet = new Set( + [...REGION_SLOTS.values()].flat(), +); + +/** + * Depth cap. Regions are a tree in parsed metadata, so this is not a cycle + * guard — it is a cheap promise that a hand-authored (pre-parse) stack cannot + * make a lint hang. Well past anything reviewable: five levels of nested + * try/loop/parallel is already an unreadable flow. + */ +export const MAX_REGION_DEPTH = 16; + +/** A visited flow node plus everything needed to locate and describe it. */ +export interface WalkedFlowNode { + /** The node record itself. */ + node: AnyRec; + /** Config path, e.g. `flows[0].nodes[1].config.catch.nodes[0]`. */ + path: string; + /** + * The node's config with region slots stripped — what a rule that scans + * config RECURSIVELY must read, or it reports every descendant's finding a + * second time against this node. `undefined` when the node has no config. + */ + localConfig?: AnyRec; + /** + * Region breadcrumb from the flow root, e.g. `try_catch "Guard" › catch`. + * Empty string for a top-level node, so a caller can append it unconditionally. + */ + regionTrail: string; + /** 0 for a top-level node; 1 inside one region; and so on. */ + depth: number; +} + +/** A node's label for diagnostics: `label` → `id` → `#index`. */ +export function flowNodeLabel(node: AnyRec, index: number): string { + return strName(node.label) ?? strName(node.id) ?? `#${index}`; +} + +/** `config` minus the region slots, or `undefined` when there is no config. */ +function stripRegions(config: unknown): AnyRec | undefined { + if (!isRec(config)) return undefined; + let out: AnyRec | undefined; + for (const key of Object.keys(config)) { + if (!REGION_CONFIG_KEYS.has(key)) continue; + out ??= { ...config }; + delete out[key]; + } + return out ?? config; +} + +/** + * Walk every node of a flow, depth-first, including those nested in + * `try_catch` / `loop` / `parallel` regions. Yields each with its own config + * path, so a finding lands on the node that is actually wrong. + * + * `flowPath` is the caller's path prefix for the flow (e.g. `flows[3]`). + */ +export function walkFlowNodes(flow: AnyRec, flowPath: string): WalkedFlowNode[] { + const out: WalkedFlowNode[] = []; + if (!isRec(flow)) return out; + + const visitList = (nodes: unknown, basePath: string, trail: string, depth: number): void => { + if (!Array.isArray(nodes) || depth > MAX_REGION_DEPTH) return; + nodes.forEach((raw, index) => { + if (!isRec(raw)) return; + const path = `${basePath}[${index}]`; + out.push({ + node: raw, + path, + localConfig: stripRegions(raw.config), + regionTrail: trail, + depth, + }); + + const type = strName(raw.type); + const slots = type ? REGION_SLOTS.get(type) : undefined; + if (!slots || !isRec(raw.config)) return; + const config = raw.config; + const here = `${type} "${flowNodeLabel(raw, index)}"`; + + for (const slot of slots) { + const value = config[slot]; + if (slot === 'branches') { + // parallel: an array of regions, each with its own nodes. + if (!Array.isArray(value)) continue; + value.forEach((branch, b) => { + if (!isRec(branch)) return; + const branchName = strName(branch.name) ?? `#${b}`; + visitList( + branch.nodes, + `${path}.config.branches[${b}].nodes`, + joinTrail(trail, `${here} › branch ${branchName}`), + depth + 1, + ); + }); + continue; + } + if (!isRec(value)) continue; + visitList( + value.nodes, + `${path}.config.${slot}.nodes`, + joinTrail(trail, `${here} › ${slot}`), + depth + 1, + ); + } + }); + }; + + visitList(flow.nodes, `${flowPath}.nodes`, '', 0); + return out; +} + +function joinTrail(trail: string, segment: string): string { + return trail ? `${trail} › ${segment}` : segment; +} diff --git a/packages/lint/src/validate-approval-approvers.test.ts b/packages/lint/src/validate-approval-approvers.test.ts index deb74d516c..b603426c19 100644 --- a/packages/lint/src/validate-approval-approvers.test.ts +++ b/packages/lint/src/validate-approval-approvers.test.ts @@ -371,3 +371,44 @@ describe('cross-organization targeting (ADR-0105 D9)', () => { expect(findings.filter(f => f.rule === APPROVAL_APPROVER_CROSS_ORG_UNSUPPORTED)).toEqual([]); }); }); + +// #4380 — an approval inside a loop body or a try/catch branch is still an +// approval. Before the shared flow walk, every rule here stopped at the top +// level and a nested node was checked by nothing. +describe('nested regions', () => { + const nestedApproval = (containerType: string, config: Record) => ({ + flows: [ + { + name: 'expense_approval', + nodes: [ + { id: 'start', type: 'start', config: {} }, + { id: 'guard', type: containerType, label: 'Guard', config }, + ], + edges: [], + }, + ], + }); + const badApproval = { + id: 'step1', + type: 'approval', + label: 'Approve', + config: { approvers: [{ type: 'bogus_type', value: 'x' }] }, + }; + + it('checks an approval nested in a loop body', () => { + const findings = validateApprovalApprovers( + nestedApproval('loop', { collection: '{items}', body: { nodes: [badApproval], edges: [] } }), + ); + expect(findings.some((f) => f.rule === APPROVAL_APPROVER_TYPE_UNKNOWN)).toBe(true); + expect(findings.find((f) => f.rule === APPROVAL_APPROVER_TYPE_UNKNOWN)?.path).toBe( + 'flows[0].nodes[1].config.body.nodes[0].config.approvers[0].type', + ); + }); + + it('checks an approval nested in a try_catch branch', () => { + const findings = validateApprovalApprovers( + nestedApproval('try_catch', { try: { nodes: [badApproval], edges: [] } }), + ); + expect(findings.some((f) => f.rule === APPROVAL_APPROVER_TYPE_UNKNOWN)).toBe(true); + }); +}); diff --git a/packages/lint/src/validate-approval-approvers.ts b/packages/lint/src/validate-approval-approvers.ts index e0cd38ef16..9360fcde12 100644 --- a/packages/lint/src/validate-approval-approvers.ts +++ b/packages/lint/src/validate-approval-approvers.ts @@ -49,6 +49,7 @@ import { } from '@objectstack/spec/automation'; import { BUILTIN_MEMBERSHIP_ROLES } from '@objectstack/spec'; import { collectCelRootIdentifiers } from '@objectstack/formula'; +import { walkFlowNodes } from './flow-walk.js'; export const APPROVAL_APPROVER_NOT_MEMBERSHIP_TIER = 'approval-approver-not-membership-tier'; export const APPROVAL_APPROVER_TYPE_DEPRECATED = 'approval-approver-type-deprecated'; @@ -151,10 +152,12 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin const flow = flows[fi]; if (!flow || typeof flow !== 'object') continue; const flowName = typeof flow.name === 'string' ? flow.name : `(flow ${fi})`; - const nodes = Array.isArray(flow.nodes) ? (flow.nodes as AnyRec[]) : []; + // Every node, INCLUDING those nested in try_catch / loop / parallel regions + // — an approval inside a loop body is still an approval (#4380). + const walked = walkFlowNodes(flow, `flows[${fi}]`); - for (let ni = 0; ni < nodes.length; ni++) { - const node = nodes[ni]; + for (let ni = 0; ni < walked.length; ni++) { + const { node, path: nodePath } = walked[ni]; if (!node || node.type !== APPROVAL_NODE_TYPE) continue; const nodeId = typeof node.id === 'string' ? node.id : `(node ${ni})`; const cfg = (node.config ?? {}) as AnyRec; @@ -166,7 +169,7 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin if (!a || typeof a !== 'object') continue; const type = typeof a.type === 'string' ? a.type : ''; const value = typeof a.value === 'string' ? a.value : ''; - const path = `flows[${fi}].nodes[${ni}].config.approvers[${ai}]`; + const path = `${nodePath}.config.approvers[${ai}]`; if (type && !validTypes.has(type)) { const fix = TYPE_FIX[type]; @@ -353,7 +356,7 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin severity: 'info', rule: APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY, where, - path: `flows[${fi}].nodes[${ni}].config.approvers`, + path: `${nodePath}.config.approvers`, message: `every approver on this node routes to a group (position/team/department) whose ` + `members are runtime data — if none is staffed, the request resolves to an empty ` + @@ -379,7 +382,7 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin severity: 'info', rule: APPROVAL_EXPRESSION_NO_EMPTY_POLICY, where, - path: `flows[${fi}].nodes[${ni}].config`, + path: `${nodePath}.config`, message: `this node resolves approvers from an expression but declares no onEmptyApprovers — ` + `an empty result falls back to the default ('admin_rescue': request opens, only a ` + @@ -403,7 +406,7 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin severity: 'error', rule: APPROVAL_DECISION_OUTPUTS_RESERVED, where, - path: `flows[${fi}].nodes[${ni}].config.decisionOutputs`, + path: `${nodePath}.config.decisionOutputs`, message: `decisionOutputs declares reserved key(s) \`${reserved.join('`, `')}\` — the resume ` + `envelope owns them, so every decide carrying them is rejected.`, @@ -422,7 +425,7 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin severity: 'warning', rule: APPROVAL_ESCALATION_REASSIGN_NO_TARGET, where, - path: `flows[${fi}].nodes[${ni}].config.escalation.escalateTo`, + path: `${nodePath}.config.escalation.escalateTo`, message: `escalation.action is 'reassign' but escalateTo is empty — at runtime the ` + `escalation degrades to a notify and the request stays with the original approvers.`, diff --git a/packages/lint/src/validate-flow-node-writes.test.ts b/packages/lint/src/validate-flow-node-writes.test.ts index d6d89ea4dd..afa425d0fe 100644 --- a/packages/lint/src/validate-flow-node-writes.test.ts +++ b/packages/lint/src/validate-flow-node-writes.test.ts @@ -385,6 +385,90 @@ describe('validateFlowNodeWrites', () => { expect(findings).toEqual([]); }); + // ── nested regions (#4380) ─────────────────────────────────────────── + // + // A gating rule that stops at the top level stops gating the moment an author + // wraps the write in error handling — which is exactly what a `catch` branch + // holding an `update_record` is for. app-showcase ships one. + describe('nested regions', () => { + const nested = (containerType: string, config: Record) => ({ + objects: [dealObject], + flows: [ + { + name: 'sync', + nodes: [ + { id: 'start', type: 'start', config: {} }, + { id: 'guard', type: containerType, label: 'Guard', config }, + ], + }, + ], + }); + const badWrite = { + id: 'flag', + type: 'update_record', + label: 'Flag', + config: { objectName: 'deal', fields: { stagee: 'failed' } }, + }; + + it('reaches a try_catch catch branch', () => { + const findings = validateFlowNodeWrites( + nested('try_catch', { try: { nodes: [], edges: [] }, catch: { nodes: [badWrite], edges: [] } }), + ); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('error'); + expect(findings[0].path).toBe('flows[0].nodes[1].config.catch.nodes[0].config.fields.stagee'); + expect(findings[0].where).toBe('flow "sync" › try_catch "Guard" › catch › node "Flag"'); + }); + + it('reaches a loop body', () => { + const findings = validateFlowNodeWrites( + nested('loop', { collection: '{items}', body: { nodes: [badWrite], edges: [] } }), + ); + expect(findings).toHaveLength(1); + expect(findings[0].path).toBe('flows[0].nodes[1].config.body.nodes[0].config.fields.stagee'); + }); + + it('reaches every parallel branch', () => { + const findings = validateFlowNodeWrites( + nested('parallel', { + branches: [ + { name: 'a', nodes: [badWrite], edges: [] }, + { name: 'b', nodes: [{ ...badWrite, id: 'flag2' }], edges: [] }, + ], + }), + ); + expect(findings).toHaveLength(2); + expect(findings.map((f) => f.path)).toEqual([ + 'flows[0].nodes[1].config.branches[0].nodes[0].config.fields.stagee', + 'flows[0].nodes[1].config.branches[1].nodes[0].config.fields.stagee', + ]); + }); + + it('reaches a region nested inside a region', () => { + const findings = validateFlowNodeWrites( + nested('try_catch', { + try: { + nodes: [ + { id: 'each', type: 'loop', label: 'Each', config: { collection: '{x}', body: { nodes: [badWrite], edges: [] } } }, + ], + edges: [], + }, + }), + ); + expect(findings).toHaveLength(1); + expect(findings[0].path).toBe( + 'flows[0].nodes[1].config.try.nodes[0].config.body.nodes[0].config.fields.stagee', + ); + }); + + it('reports a nested finding exactly once, not also against its container', () => { + const findings = validateFlowNodeWrites( + nested('try_catch', { try: { nodes: [badWrite], edges: [] } }), + ); + expect(findings).toHaveLength(1); + }); + }); + // ── the family boundary ────────────────────────────────────────────── it('does not duplicate the readonly rule: a declared readonly field is that rule’s business, not this one', () => { const withReadonly = { diff --git a/packages/lint/src/validate-flow-node-writes.ts b/packages/lint/src/validate-flow-node-writes.ts index 90d4d4fac1..ac531043e5 100644 --- a/packages/lint/src/validate-flow-node-writes.ts +++ b/packages/lint/src/validate-flow-node-writes.ts @@ -85,6 +85,7 @@ import { findClosestMatches, formatSuggestion } from '@objectstack/spec/shared'; import { indexObjectFields, judgeableFieldsOf, IMPLICIT_FIELDS } from './validate-hook-body-writes.js'; +import { walkFlowNodes, flowNodeLabel } from './flow-walk.js'; export type FlowNodeWriteSeverity = 'error'; @@ -190,10 +191,12 @@ export function validateFlowNodeWrites(stack: AnyRec): FlowNodeWriteFinding[] { flows.forEach((flow, flowIndex) => { const flowName = typeof flow.name === 'string' && flow.name ? flow.name : `#${flowIndex}`; - const nodes = Array.isArray(flow.nodes) ? (flow.nodes as AnyRec[]) : []; + // Every node, INCLUDING those nested in try_catch / loop / parallel regions + // — a gating rule that stops at the top level simply stops gating the + // moment an author wraps the write in error handling (#4380). + const walked = walkFlowNodes(flow, `flows[${flowIndex}]`); - nodes.forEach((node, nodeIndex) => { - if (!isRec(node)) return; + walked.forEach(({ node, path: nodePath, regionTrail }, walkIndex) => { if (typeof node.type !== 'string' || !COVERED_TYPES.has(node.type)) return; const config = isRec(node.config) ? node.config : undefined; @@ -217,12 +220,10 @@ export function validateFlowNodeWrites(stack: AnyRec): FlowNodeWriteFinding[] { const known = judgeableFieldsOf(objectFields, objectName); if (!known) return; - const nodeName = - typeof node.label === 'string' && node.label - ? node.label - : typeof node.id === 'string' && node.id - ? node.id - : `#${nodeIndex}`; + const nodeName = flowNodeLabel(node, walkIndex); + // A nested node names the region that holds it, or "node X" is ambiguous + // in a flow where the same label appears in a try and a catch branch. + const nodeWhere = regionTrail ? `${regionTrail} › node "${nodeName}"` : `node "${nodeName}"`; for (const fieldName of written) { if (known.has(fieldName) || IMPLICIT_FIELDS.has(fieldName)) continue; @@ -233,8 +234,8 @@ export function validateFlowNodeWrites(stack: AnyRec): FlowNodeWriteFinding[] { findings.push({ severity: 'error', rule: FLOW_NODE_WRITE_UNKNOWN_FIELD, - where: `flow "${flowName}" › node "${nodeName}"`, - path: `flows[${flowIndex}].nodes[${nodeIndex}].config.fields.${fieldName}`, + where: `flow "${flowName}" › ${nodeWhere}`, + path: `${nodePath}.config.fields.${fieldName}`, message: `${node.type} writes '${fieldName}', but object '${objectName}' declares no such field. Nothing ` + `between the node and storage removes the key: on a SQL datasource the driver rejects the whole ` + diff --git a/packages/lint/src/validate-flow-template-paths.test.ts b/packages/lint/src/validate-flow-template-paths.test.ts index 7fd673d56c..92e062207b 100644 --- a/packages/lint/src/validate-flow-template-paths.test.ts +++ b/packages/lint/src/validate-flow-template-paths.test.ts @@ -344,4 +344,78 @@ describe('validateFlowTemplatePaths', () => { expect(findings).toHaveLength(0); }); }); + // ── nested regions (#4380) ───────────────────────────────────────────── + // + // This rule was not merely blind to nested nodes — it was WORSE than blind. + // The recursive string-leaf scan already saw a nested node's tokens through + // its container's `config`, but the `filter` split only looked at the top + // level of the node it was handed, so a nested filter token lost its position + // and the gating #3810 finding silently degraded to a warning reported + // against the wrapping `try_catch`. + describe('nested regions', () => { + const nestedFilterFlow = (container: Record) => ({ + objects: [LEAD_OBJECT], + flows: [ + { + name: 'guarded', + type: 'record_change', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_lead', triggerType: 'record-created' } }, + { id: 'guard', type: 'try_catch', label: 'Guard', config: container }, + ], + }, + ], + }); + const badFilterNode = { + id: 'fetch', + type: 'get_record', + label: 'Fetch', + config: { objectName: 'crm_lead', filter: { company: '{record.budget}' } }, + }; + + it('keeps the gating filter-position severity inside a region', () => { + const findings = validateFlowTemplatePaths( + nestedFilterFlow({ try: { nodes: [badFilterNode], edges: [] } }), + ); + expect(findings).toHaveLength(1); + // The whole point: `error`, not the `warning` it used to degrade to. + expect(findings[0].severity).toBe('error'); + expect(findings[0].path).toBe('flows[0].nodes[1].config.try.nodes[0]'); + expect(findings[0].where).toBe('flow "guarded" try_catch "Guard" › try node "get_record"'); + }); + + it('reports a nested token once, not also against the container', () => { + const findings = validateFlowTemplatePaths( + nestedFilterFlow({ catch: { nodes: [badFilterNode], edges: [] } }), + ); + expect(findings).toHaveLength(1); + }); + + it('still checks the container node\'s own config', () => { + const findings = validateFlowTemplatePaths({ + objects: [LEAD_OBJECT], + flows: [ + { + name: 'looped', + type: 'record_change', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_lead', triggerType: 'record-created' } }, + { + id: 'each', + type: 'loop', + label: 'Each', + // The loop's OWN collection token is a non-filter position on + // the container itself — warning, and not swallowed by the + // region-stripping that prevents double reporting. + config: { collection: '{record.budget}', body: { nodes: [], edges: [] } }, + }, + ], + }, + ], + }); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('warning'); + expect(findings[0].path).toBe('flows[0].nodes[1]'); + }); + }); }); diff --git a/packages/lint/src/validate-flow-template-paths.ts b/packages/lint/src/validate-flow-template-paths.ts index 6384732e5b..0f32c45bb0 100644 --- a/packages/lint/src/validate-flow-template-paths.ts +++ b/packages/lint/src/validate-flow-template-paths.ts @@ -56,6 +56,7 @@ // carry legitimate sub-paths — their `.` access is left alone. import { SYSTEM_FIELDS } from './system-fields.js'; +import { walkFlowNodes } from './flow-walk.js'; export type FlowTemplatePathSeverity = 'error' | 'warning'; @@ -302,16 +303,33 @@ export function validateFlowTemplatePaths(stack: AnyRec): FlowTemplatePathFindin const fieldTypes = fieldTypesOf(obj); const expandSet = declaredExpandOf(flow); - nodes.forEach((node, nodeIndex) => { - if (typeof node !== 'object' || !node) return; + // Every node, INCLUDING those nested in try_catch / loop / parallel regions + // (#4380). This rule was not merely blind to them — it was WORSE than + // blind: the recursive string-leaf scan already saw a nested node's tokens + // through its container's `config`, but `collectNodeLeaves` splits `filter` + // only at the top level of the node it is handed, so a nested filter token + // lost its position and the gating #3810 finding silently degraded to a + // warning reported against the wrapping `try_catch`. Walking to the real + // node restores both the severity and the location. + walkFlowNodes(flow, `flows[${flowIndex}]`).forEach(({ node, path: nodePath, regionTrail, localConfig }, walkIndex) => { const nodeLabel = - typeof node.type === 'string' ? node.type : typeof node.id === 'string' ? node.id : `#${nodeIndex}`; + typeof node.type === 'string' ? node.type : typeof node.id === 'string' ? node.id : `#${walkIndex}`; + const where = regionTrail + ? `flow "${flowName}" ${regionTrail} node "${nodeLabel}"` + : `flow "${flowName}" node "${nodeLabel}"`; // Collect templated string leaves from the config-bearing blocks only, // tagging filter positions when this node type guards its filter (#3810). const nodeType = typeof node.type === 'string' ? node.type : ''; const guarded = FILTER_GUARDED_NODE_TYPES.has(nodeType); - const leaves = collectNodeLeaves(node as AnyRec, guarded); + // Scan the container's config WITHOUT its region slots: their nodes are + // walked in their own right, and leaving them in would report every + // nested finding a second time against the container. + const scanNode = + localConfig !== undefined && localConfig !== node.config + ? ({ ...node, config: localConfig } as AnyRec) + : (node as AnyRec); + const leaves = collectNodeLeaves(scanNode, guarded); if (leaves.length === 0) return; // Dedupe references so one repeated typo yields one finding per node. @@ -334,8 +352,8 @@ export function validateFlowTemplatePaths(stack: AnyRec): FlowTemplatePathFindin findings.push({ severity: inFilter ? 'error' : 'warning', rule: FLOW_TEMPLATE_UNKNOWN_FIELD, - where: `flow "${flowName}" node "${nodeLabel}"`, - path: `flows[${flowIndex}].nodes[${nodeIndex}]`, + where, + path: nodePath, message: inFilter ? `${nodeType} filter references '{record.${rest.join('.')}}', but '${head}' is not a field on ` + `object '${objectName}' — the token resolves to nothing, which DROPS the condition from the ` + @@ -362,8 +380,8 @@ export function validateFlowTemplatePaths(stack: AnyRec): FlowTemplatePathFindin findings.push({ severity: inFilter ? 'error' : 'warning', rule: FLOW_TEMPLATE_LOOKUP_TRAVERSAL, - where: `flow "${flowName}" node "${nodeLabel}"`, - path: `flows[${flowIndex}].nodes[${nodeIndex}]`, + where, + path: nodePath, message: inFilter ? `${nodeType} filter references '{record.${key}}', a cross-object hop through the ` + `${headType} field '${head}' — the flow record carries '${head}' as a scalar id, not an ` + diff --git a/packages/lint/src/validate-readonly-flow-writes.test.ts b/packages/lint/src/validate-readonly-flow-writes.test.ts index 5b87218538..01e0948ed0 100644 --- a/packages/lint/src/validate-readonly-flow-writes.test.ts +++ b/packages/lint/src/validate-readonly-flow-writes.test.ts @@ -243,4 +243,70 @@ describe('validateReadonlyFlowWrites', () => { expect(findings[0].where).toBe('flow "f" › node "my_node"'); expect(findings[0].path).toBe('flows[0].nodes[0].config.fields.approval_status'); }); + + // ── nested regions (#4380) ─────────────────────────────────────────── + // A readonly write inside a `catch` branch is the same certain no-op as one + // at the top level, and this rule gates on it. + it('reaches an update_record nested in a try_catch catch branch', () => { + const flow = { + name: 'sync', + runAs: 'user', + nodes: [ + { id: 'start', type: 'start', config: {} }, + { + id: 'guard', + type: 'try_catch', + label: 'Guard', + config: { + try: { nodes: [], edges: [] }, + catch: { + nodes: [ + { + id: 'flag', + type: 'update_record', + label: 'Flag', + config: { objectName: 'crm_opportunity', fields: { approval_status: 'x' } }, + }, + ], + edges: [], + }, + }, + }, + ], + edges: [], + }; + const findings = validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [flow] }); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('error'); + expect(findings[0].path).toBe('flows[0].nodes[1].config.catch.nodes[0].config.fields.approval_status'); + expect(findings[0].where).toBe('flow "sync" › try_catch "Guard" › catch › node "Flag"'); + }); + + it('reaches an update_record nested in a loop body', () => { + const flow = { + name: 'sweep', + runAs: 'user', + nodes: [ + { + id: 'each', + type: 'loop', + label: 'Each', + config: { + collection: '{items}', + body: { + nodes: [ + { id: 'u', type: 'update_record', label: 'U', config: { objectName: 'crm_opportunity', fields: { amount: 1 } } }, + ], + edges: [], + }, + }, + }, + ], + edges: [], + }; + const findings = validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [flow] }); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('warning'); // readonlyWhen field + expect(findings[0].path).toBe('flows[0].nodes[0].config.body.nodes[0].config.fields.amount'); + }); }); diff --git a/packages/lint/src/validate-readonly-flow-writes.ts b/packages/lint/src/validate-readonly-flow-writes.ts index d36d6b1447..ef7334cfa1 100644 --- a/packages/lint/src/validate-readonly-flow-writes.ts +++ b/packages/lint/src/validate-readonly-flow-writes.ts @@ -32,6 +32,8 @@ // the CLI and any other consumer (AI authoring), so hand-authored and generated // flows are held to the same bar. +import { walkFlowNodes, flowNodeLabel } from './flow-walk.js'; + export type ReadonlyFlowWriteSeverity = 'error' | 'warning'; export interface ReadonlyFlowWriteFinding { @@ -134,9 +136,12 @@ export function validateReadonlyFlowWrites(stack: AnyRec): ReadonlyFlowWriteFind const runAs = flow.runAs === 'user' || flow.runAs === 'system' ? flow.runAs : 'user'; const flowName = typeof flow.name === 'string' ? flow.name : `#${flowIndex}`; - const nodes = Array.isArray(flow.nodes) ? (flow.nodes as AnyRec[]) : []; + // Every node, INCLUDING those nested in try_catch / loop / parallel regions. + // A readonly write inside a `catch` branch is the same certain no-op as one + // at the top level, and this rule gates on it (#4380). + const walked = walkFlowNodes(flow, `flows[${flowIndex}]`); - nodes.forEach((node, nodeIndex) => { + walked.forEach(({ node, path: nodePath, regionTrail }, walkIndex) => { if (node?.type !== 'update_record') return; const config = (node.config ?? {}) as AnyRec; @@ -150,12 +155,10 @@ export function validateReadonlyFlowWrites(stack: AnyRec): ReadonlyFlowWriteFind // statically knowable — skip rather than guess. if (!fields || typeof fields !== 'object' || Array.isArray(fields)) return; - const nodeName = - typeof node.label === 'string' && node.label - ? node.label - : typeof node.id === 'string' && node.id - ? node.id - : `#${nodeIndex}`; + const nodeName = flowNodeLabel(node, walkIndex); + const where = regionTrail + ? `flow "${flowName}" › ${regionTrail} › node "${nodeName}"` + : `flow "${flowName}" › node "${nodeName}"`; for (const fieldName of Object.keys(fields as AnyRec)) { const meta = fieldMap.get(fieldName); @@ -170,8 +173,8 @@ export function validateReadonlyFlowWrites(stack: AnyRec): ReadonlyFlowWriteFind findings.push({ severity: 'error', rule: FLOW_UPDATE_READONLY_FIELD, - where: `flow "${flowName}" › node "${nodeName}"`, - path: `flows[${flowIndex}].nodes[${nodeIndex}].config.fields.${fieldName}`, + where, + path: `${nodePath}.config.fields.${fieldName}`, message: `writes field '${fieldName}', which object '${objectName}' declares readonly:true. Under ` + `runAs:'${runAs}' the engine silently strips readonly fields from the UPDATE payload (#2948), ` + @@ -185,8 +188,8 @@ export function validateReadonlyFlowWrites(stack: AnyRec): ReadonlyFlowWriteFind findings.push({ severity: 'warning', rule: FLOW_UPDATE_READONLY_WHEN_FIELD, - where: `flow "${flowName}" › node "${nodeName}"`, - path: `flows[${flowIndex}].nodes[${nodeIndex}].config.fields.${fieldName}`, + where, + path: `${nodePath}.config.fields.${fieldName}`, message: `writes field '${fieldName}', which object '${objectName}' declares readonlyWhen. On records ` + `where that predicate is TRUE, a runAs:'${runAs}' UPDATE strips the field (#3042), so this ` +