diff --git a/.changeset/flow-template-leaves-outside-filters.md b/.changeset/flow-template-leaves-outside-filters.md new file mode 100644 index 0000000000..2c654af03e --- /dev/null +++ b/.changeset/flow-template-leaves-outside-filters.md @@ -0,0 +1,11 @@ +--- +"@objectstack/lint": patch +--- + +`flow-template-unknown-field` and `flow-template-lookup-traversal` now reach a `{record.}` template that sits outside a node filter — the `warning` half both rules already declared, and never emitted. + +A `{record.}` token in a filter has always been reported as an `error`: an unresolved token there erases the condition and the CRUD node refuses to run. A token anywhere else — a message body, an http request payload, a created row's field values — is the quiet failure the rules were written for: it renders as an empty string on every run, and nothing reports it at either end — no build-time finding, no run-time error — so a hand-off payload naming a renamed field ships an empty value and the run is recorded as a success. That half was silent. + +The cause was one key, in the shared flow walk rather than in either rule. A rule that scans a node's config recursively has to read a view of it with the nested regions removed, or it reports every finding inside a `loop` / `try_catch` / `parallel` a second time against the container. That view was built by removing every key that holds a region on *any* node type — and `body` is `loop`'s region slot **and** the canonical request-payload key on an `http` node. So `config.body` was deleted from every node's view before any rule read it, and the whole of an http payload was invisible. The view now removes only the slots the node's own type declares, which is exactly the set the walk descended into: nothing is double-reported, and nothing that was never a region is dropped. + +Expect new `warning` findings on flows that publish clean today. Each one names a token that renders empty at run time; `warning` does not change `os validate`'s exit code, so a build that passed still passes. diff --git a/packages/lint/src/flow-walk.test.ts b/packages/lint/src/flow-walk.test.ts index 98c881519a..13e9e6c8c9 100644 --- a/packages/lint/src/flow-walk.test.ts +++ b/packages/lint/src/flow-walk.test.ts @@ -9,6 +9,8 @@ import { import { walkFlowNodes, flowNodeLabel, + stripRegions, + ownRegionKeys, REGION_SLOTS, REGION_CONFIG_KEYS, MAX_REGION_DEPTH, @@ -151,6 +153,60 @@ describe('walkFlowNodes', () => { expect(only.localConfig).toBeUndefined(); }); + // The other half of the same contract, and the one that was wrong: the view + // must remove what the walk descended into and NOTHING else. `body` is a + // region slot on `loop` and the canonical request-payload key on `http`, so + // stripping the flat union deleted an `http` node's whole payload from every + // recursive scan — silently, which is strictly worse than the double-count + // this view exists to prevent, because a double-count is visible. + it('keeps a config key that is a region slot on some OTHER node type', () => { + const flow = { + nodes: [ + { id: 'post', type: 'http', config: { url: 'https://x.example', body: { amount: '{record.amount}' } } }, + ], + }; + const [walked] = walkFlowNodes(flow, 'flows[0]'); + expect(Object.keys(walked.localConfig ?? {}).sort()).toEqual(['body', 'url']); + // Copy-on-write: nothing was removed, so the view is the config itself. + expect(walked.localConfig).toBe(walked.node.config); + }); + + it('strips a container slot only from the container type that declares it', () => { + const flow = { + nodes: [ + { id: 'guard', type: 'try_catch', config: { try: { nodes: [], edges: [] }, body: 'kept' } }, + ], + }; + const [walked] = walkFlowNodes(flow, 'flows[0]'); + // `try_catch` owns `try` / `catch`; `body` belongs to `loop` and stays. + expect(Object.keys(walked.localConfig ?? {}).sort()).toEqual(['body']); + }); + + describe('ownRegionKeys / stripRegions', () => { + it('answers a container its own slots and every other type none', () => { + expect([...ownRegionKeys('loop')].sort()).toEqual(['body']); + expect([...ownRegionKeys('try_catch')].sort()).toEqual(['catch', 'try']); + expect([...ownRegionKeys('parallel')].sort()).toEqual(['branches']); + expect(ownRegionKeys('http')).toEqual([]); + expect(ownRegionKeys(undefined)).toEqual([]); + // `node.type` is an open, author-controlled namespace — a prototype key + // must resolve to no slots rather than to something off Object. + expect(ownRegionKeys('constructor')).toEqual([]); + }); + + it('treats an empty key list as a real answer, distinct from omitting it', () => { + const config = { body: 'payload', try: 'kept' }; + expect(stripRegions(config, [])).toBe(config); + // Omitted: the flat-union view its remaining caller was written against. + expect(Object.keys(stripRegions(config) ?? {})).toEqual([]); + }); + + it('returns undefined for a non-record config, whatever the key list', () => { + expect(stripRegions(undefined, ownRegionKeys('loop'))).toBeUndefined(); + expect(stripRegions('nope', [])).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'); diff --git a/packages/lint/src/flow-walk.ts b/packages/lint/src/flow-walk.ts index b8b8be6ae8..034991774f 100644 --- a/packages/lint/src/flow-walk.ts +++ b/packages/lint/src/flow-walk.ts @@ -99,9 +99,20 @@ export interface WalkedFlowNode { /** 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. + * The node's config with the region slots THIS node type declares 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. + * + * "This node type declares" is the load-bearing half, and it is exactly the + * set the walk below descends into: remove fewer and a nested finding is + * reported twice, remove more and a key that was never a region is deleted + * from the view unread. That second failure is not hypothetical — `body` is a + * region slot on `loop` AND the canonical request-payload key on an `http` + * node, so stripping the flat union blinded every recursive scan to the whole + * of `http.config.body`: a `{record.}` token there interpolates at run + * time (`http-nodes.ts` interpolates the raw config wholesale) and rendered a + * silent empty value into an outbound request with nothing to see it. */ localConfig?: AnyRec; /** @@ -118,10 +129,39 @@ export function flowNodeLabel(node: AnyRec, index: number): string { return strName(node.label) ?? strName(node.id) ?? `#${index}`; } +const EMPTY_SLOTS: readonly string[] = []; + +/** The region slots a node type owns — empty for every non-container type. */ +export function ownRegionKeys(nodeType: unknown): readonly string[] { + const type = strName(nodeType); + return (type ? REGION_SLOTS.get(type) : undefined) ?? EMPTY_SLOTS; +} + /** - * `config` minus the region slots, or `undefined` when there is no config. + * `config` minus `regionKeys`, or `undefined` when there is no config. + * + * Copy-on-write: a config carrying none of those keys comes back by reference. + * + * **Pass the OWNING node's slots** ({@link ownRegionKeys}), not the flat union. + * The union is a different question — "every key that holds a region on SOME + * node type" — and answering it here deletes keys that are ordinary config on + * the node in hand: `body` is `loop`'s region slot and `http`'s request payload, + * `branches` and `try` are as available to any other node type. A recursive scan + * reading the union view therefore cannot see those keys at all, silently, which + * is the reverse of the double-count this view exists to prevent and strictly + * worse: a double-count is visible in the output. + * + * The union stays the DEFAULT to bound this change to the two rules #16111 + * names — ⛔ NOT because it is the right argument for the caller still taking + * it. `lint-flow-patterns.ts` reads the union view for its own recursive + * template scan, so it is blind to an `http` node's `body` for exactly the + * reason above: the same defect, one call site over, tracked on #16405. Once + * that caller passes its own slots this default has no callers left and + * `regionKeys` must become REQUIRED, so no later caller inherits the trap by + * writing the shorter call. * - * Copy-on-write: a config with no region key comes back by reference. + * `regionKeys: []` is a real answer (strip nothing) and is distinct from + * omitting the parameter. * * Exported since #5383 because {@link WalkedFlowNode.localConfig} is not the only * consumer that needs this view. `lint-flow-patterns.ts` walks graphs rather than @@ -129,11 +169,16 @@ export function flowNodeLabel(node: AnyRec, index: number): string { * its recursive config scans hit the identical double-count trap described above — * so it reads the same region-stripped view, from this one definition. */ -export function stripRegions(config: unknown): AnyRec | undefined { +export function stripRegions( + config: unknown, + regionKeys: Iterable = REGION_CONFIG_KEYS, +): AnyRec | undefined { if (!isRec(config)) return undefined; + const strip = regionKeys instanceof Set ? regionKeys : new Set(regionKeys); + if (strip.size === 0) return config; let out: AnyRec | undefined; for (const key of Object.keys(config)) { - if (!REGION_CONFIG_KEYS.has(key)) continue; + if (!strip.has(key)) continue; out ??= { ...config }; delete out[key]; } @@ -156,16 +201,20 @@ export function walkFlowNodes(flow: AnyRec, flowPath: string): WalkedFlowNode[] nodes.forEach((raw, index) => { if (!isRec(raw)) return; const path = `${basePath}[${index}]`; + // The slots this node OWNS — the same lookup that decides where the walk + // descends below, so `localConfig` removes exactly what was walked + // separately and nothing else. + const ownSlots = ownRegionKeys(raw.type); out.push({ node: raw, path, - localConfig: stripRegions(raw.config), + localConfig: stripRegions(raw.config, ownSlots), regionTrail: trail, depth, }); const type = strName(raw.type); - const slots = type ? REGION_SLOTS.get(type) : undefined; + const slots = ownSlots.length > 0 ? ownSlots : undefined; if (!slots || !isRec(raw.config)) return; const config = raw.config; const here = `${type} "${flowNodeLabel(raw, index)}"`; diff --git a/packages/lint/src/validate-flow-template-paths.test.ts b/packages/lint/src/validate-flow-template-paths.test.ts index aeaa97f407..37adafd316 100644 --- a/packages/lint/src/validate-flow-template-paths.test.ts +++ b/packages/lint/src/validate-flow-template-paths.test.ts @@ -419,6 +419,161 @@ describe('validateFlowTemplatePaths', () => { expect(findings[0].path).toBe('flows[0].nodes[1]'); }); }); + + // ── a template outside a node filter ─────────────────────────────────── + // + // Both rule ids declare a per-position severity (`inFilter ? 'error' : + // 'warning'`) and a second message for the non-filter half, and that half was + // unreachable on the shape a real hand-off flow has: `body` is a region slot + // on `loop`, so the region-stripped view a recursive scan must read deleted + // `config.body` from EVERY node — including the `http` node whose request + // payload it is. The `http` executor interpolates its raw config wholesale, + // so a `{record.}` there renders an empty value into an outbound + // request on every run, silently, at authoring time and at run time alike. + // + // Four pins, one fixture, matching the four injections the report measured. + // The two filter pins are NEGATIVE CONTROLS: they pass before this fix and + // must keep passing, or a later refactor could break the gating half while + // the warning half stays green. `warning` does not move the exit code, so + // every assertion below reads the findings array, never a pass/fail verdict. + describe('outside a node filter', () => { + const CLEAN_FILTER = '{record.crm_account}'; + const CLEAN_PAYLOAD = '{record.company}'; + + /** A billing hand-off: a filter-guarded read, then an http POST payload. */ + const handoff = (filterId: string, payloadAmount: string) => ({ + objects: [LEAD_OBJECT], + flows: [ + { + name: 'billing_handoff', + type: 'record_change', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_lead', triggerType: 'record-after-update' } }, + { + id: 'fetch', + type: 'get_record', + label: 'Fetch account', + config: { objectName: 'crm_lead', filter: { id: filterId } }, + }, + { + id: 'post', + type: 'http', + label: 'Hand off', + config: { + url: 'https://billing.example/handoff', + method: 'POST', + body: { amount: payloadAmount }, + }, + }, + ], + }, + ], + }); + + it('gates an unknown field in a filter (negative control)', () => { + const findings = validateFlowTemplatePaths(handoff('{record.crm_account_nope}', CLEAN_PAYLOAD)); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(FLOW_TEMPLATE_UNKNOWN_FIELD); + expect(findings[0].severity).toBe('error'); + expect(findings[0].where).toBe('flow "billing_handoff" node "get_record"'); + }); + + it('gates a lookup traversal in a filter (negative control)', () => { + const findings = validateFlowTemplatePaths(handoff('{record.crm_account.owner_id}', CLEAN_PAYLOAD)); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(FLOW_TEMPLATE_LOOKUP_TRAVERSAL); + expect(findings[0].severity).toBe('error'); + expect(findings[0].where).toBe('flow "billing_handoff" node "get_record"'); + }); + + it('warns on an unknown field in a payload outside any filter', () => { + const findings = validateFlowTemplatePaths(handoff(CLEAN_FILTER, '{record.amount_nope}')); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(FLOW_TEMPLATE_UNKNOWN_FIELD); + expect(findings[0].severity).toBe('warning'); + expect(findings[0].where).toBe('flow "billing_handoff" node "http"'); + expect(findings[0].path).toBe('flows[0].nodes[2]'); + expect(findings[0].message).toContain('empty string'); + }); + + it('warns on a lookup traversal in a payload outside any filter', () => { + const findings = validateFlowTemplatePaths(handoff(CLEAN_FILTER, '{record.crm_account.name}')); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(FLOW_TEMPLATE_LOOKUP_TRAVERSAL); + expect(findings[0].severity).toBe('warning'); + expect(findings[0].where).toBe('flow "billing_handoff" node "http"'); + expect(findings[0].message).toContain('empty string'); + }); + + it('is silent on the same fixture with both positions authored correctly', () => { + expect(validateFlowTemplatePaths(handoff(CLEAN_FILTER, CLEAN_PAYLOAD))).toEqual([]); + }); + + it('resolves one leaf used in both positions of a node at the gating severity', () => { + const findings = validateFlowTemplatePaths({ + objects: [LEAD_OBJECT], + flows: [ + { + name: 'billing_handoff', + type: 'record_change', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_lead', triggerType: 'record-after-update' } }, + { + id: 'touch', + type: 'update_record', + label: 'Touch', + config: { + objectName: 'crm_lead', + filter: { company: '{record.full_naem}' }, + fields: { company: 'echo {record.full_naem}' }, + }, + }, + ], + }, + ], + }); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('error'); + }); + + it('reports a payload nested in a loop body once, on the node that carries it', () => { + const findings = validateFlowTemplatePaths({ + objects: [LEAD_OBJECT], + flows: [ + { + name: 'billing_handoff', + type: 'record_change', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_lead', triggerType: 'record-after-update' } }, + { + id: 'each', + type: 'loop', + label: 'Each', + config: { + collection: '{items}', + body: { + nodes: [ + { + id: 'post', + type: 'http', + label: 'Hand off', + config: { url: 'https://billing.example/handoff', body: { amount: '{record.amount_nope}' } }, + }, + ], + edges: [], + }, + }, + }, + ], + }, + ], + }); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('warning'); + expect(findings[0].path).toBe('flows[0].nodes[1].config.body.nodes[0]'); + expect(findings[0].where).toBe('flow "billing_handoff" loop "Each" › body node "http"'); + }); + }); }); describe('validateFlowTemplatePaths — unprovisioned injected anchors (#8340)', () => {