From 50f343ad3073f7415737cc0bf04314279d268ac7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 18:24:58 +0000 Subject: [PATCH 1/3] fix(lint): flow template rules reach a {record.FIELD} token outside a node filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `flow-template-unknown-field` and `flow-template-lookup-traversal` each declare a per-position severity — `error` inside a filter-guarded CRUD node's `filter`, `warning` everywhere else — and the `warning` half never fired on the shape a real hand-off flow has. The cause is one key, and it is in the shared flow walk rather than in either rule. `WalkedFlowNode.localConfig` is the region-stripped view a recursive config scan must read or it reports every nested finding a second time against the container; it was built by removing every key that holds a region on ANY node type. `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 saw it — and the http executor interpolates its raw config wholesale, so a `{record.}` there renders an empty value into an outbound request on every run. `stripRegions` now takes the keys to remove; `walkFlowNodes` passes the slots the node's own type declares, the same lookup that decides where the walk descends. Nothing is double-reported and nothing that was never a region is dropped. The flat-union view stays the default argument so the helper's other caller keeps the behaviour it was written against. Tests: the four measured injections as four pins on one hand-off fixture — the two filter positions as negative controls that must stay `error`, the two payload positions as the `warning` half — plus the both-positions dedupe, a clean flow, and a payload nested in a loop body reported once on the node that carries it. Every assertion reads the findings array; `warning` does not move the exit code. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --- .../flow-template-leaves-outside-filters.md | 11 ++ packages/lint/src/flow-walk.test.ts | 56 +++++++ packages/lint/src/flow-walk.ts | 60 ++++++- .../src/validate-flow-template-paths.test.ts | 155 ++++++++++++++++++ 4 files changed, 273 insertions(+), 9 deletions(-) create mode 100644 .changeset/flow-template-leaves-outside-filters.md diff --git a/.changeset/flow-template-leaves-outside-filters.md b/.changeset/flow-template-leaves-outside-filters.md new file mode 100644 index 0000000000..e09f794613 --- /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, at authoring time and at run time alike, so a hand-off payload naming a renamed field ships an empty value and nothing anywhere says so. 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..8992cf6e57 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,32 @@ 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. * - * Copy-on-write: a config with no region key comes back by reference. + * The union stays the DEFAULT only so this function's other caller keeps the + * behaviour it was written against; it is not the correct argument for any new + * one. `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 +162,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 +194,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)', () => { From e1c87b19d9ba952f6a42d2248e8c86ef6cd49bfe Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 19:13:34 +0000 Subject: [PATCH 2/3] docs(changeset): state where the silent half was silent, at both ends MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The clause read "at authoring time and at run time alike" against "renders as an empty string", which puts the rendering at authoring time. The rendering is a run-time event; what happens at BOTH ends is that nothing reports it — no build-time finding, no run-time error — which is the property that makes the failure survive to production. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --- .changeset/flow-template-leaves-outside-filters.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/flow-template-leaves-outside-filters.md b/.changeset/flow-template-leaves-outside-filters.md index e09f794613..2c654af03e 100644 --- a/.changeset/flow-template-leaves-outside-filters.md +++ b/.changeset/flow-template-leaves-outside-filters.md @@ -4,7 +4,7 @@ `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, at authoring time and at run time alike, so a hand-off payload naming a renamed field ships an empty value and nothing anywhere says so. That half was silent. +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. From d5a30458f812ae2b851a367efda1d428d90ddaa6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 19:16:16 +0000 Subject: [PATCH 3/3] docs(lint): name the sibling call site the union default still traps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The note read as if the flat-union default were correct for the caller that still takes it. It is not: `lint-flow-patterns.ts` runs its own recursive template scan over the union view, so it is blind to an `http` node's `body` for exactly the reason this function's own doc gives one paragraph earlier — the same defect, one call site over. The default is scope control for this change, not a verdict, and the note now says which. Also states the endgame, so the next reader does not have to rediscover it: once that caller passes its own slots, `regionKeys` has no default-takers left and must become required, or the shorter call keeps handing out the trap. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --- packages/lint/src/flow-walk.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/lint/src/flow-walk.ts b/packages/lint/src/flow-walk.ts index 8992cf6e57..034991774f 100644 --- a/packages/lint/src/flow-walk.ts +++ b/packages/lint/src/flow-walk.ts @@ -151,9 +151,16 @@ export function ownRegionKeys(nodeType: unknown): readonly string[] { * 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 only so this function's other caller keeps the - * behaviour it was written against; it is not the correct argument for any new - * one. `regionKeys: []` is a real answer (strip nothing) and is distinct from + * 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. + * + * `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