Skip to content

Commit dff0bdd

Browse files
claude[bot]claude
andauthored
fix(lint): flow template rules reach a {record.FIELD} token outside a node filter (#16407)
* fix(lint): flow template rules reach a {record.FIELD} token outside a node filter `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.<typo>}` 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 * docs(changeset): state where the silent half was silent, at both ends 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 * docs(lint): name the sibling call site the union default still traps 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 9b8bc82 commit dff0bdd

4 files changed

Lines changed: 280 additions & 9 deletions

File tree

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
"@objectstack/lint": patch
3+
---
4+
5+
`flow-template-unknown-field` and `flow-template-lookup-traversal` now reach a `{record.<field>}` template that sits outside a node filter — the `warning` half both rules already declared, and never emitted.
6+
7+
A `{record.<field>}` 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.
8+
9+
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.
10+
11+
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.

packages/lint/src/flow-walk.test.ts

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ import {
99
import {
1010
walkFlowNodes,
1111
flowNodeLabel,
12+
stripRegions,
13+
ownRegionKeys,
1214
REGION_SLOTS,
1315
REGION_CONFIG_KEYS,
1416
MAX_REGION_DEPTH,
@@ -151,6 +153,60 @@ describe('walkFlowNodes', () => {
151153
expect(only.localConfig).toBeUndefined();
152154
});
153155

156+
// The other half of the same contract, and the one that was wrong: the view
157+
// must remove what the walk descended into and NOTHING else. `body` is a
158+
// region slot on `loop` and the canonical request-payload key on `http`, so
159+
// stripping the flat union deleted an `http` node's whole payload from every
160+
// recursive scan — silently, which is strictly worse than the double-count
161+
// this view exists to prevent, because a double-count is visible.
162+
it('keeps a config key that is a region slot on some OTHER node type', () => {
163+
const flow = {
164+
nodes: [
165+
{ id: 'post', type: 'http', config: { url: 'https://x.example', body: { amount: '{record.amount}' } } },
166+
],
167+
};
168+
const [walked] = walkFlowNodes(flow, 'flows[0]');
169+
expect(Object.keys(walked.localConfig ?? {}).sort()).toEqual(['body', 'url']);
170+
// Copy-on-write: nothing was removed, so the view is the config itself.
171+
expect(walked.localConfig).toBe(walked.node.config);
172+
});
173+
174+
it('strips a container slot only from the container type that declares it', () => {
175+
const flow = {
176+
nodes: [
177+
{ id: 'guard', type: 'try_catch', config: { try: { nodes: [], edges: [] }, body: 'kept' } },
178+
],
179+
};
180+
const [walked] = walkFlowNodes(flow, 'flows[0]');
181+
// `try_catch` owns `try` / `catch`; `body` belongs to `loop` and stays.
182+
expect(Object.keys(walked.localConfig ?? {}).sort()).toEqual(['body']);
183+
});
184+
185+
describe('ownRegionKeys / stripRegions', () => {
186+
it('answers a container its own slots and every other type none', () => {
187+
expect([...ownRegionKeys('loop')].sort()).toEqual(['body']);
188+
expect([...ownRegionKeys('try_catch')].sort()).toEqual(['catch', 'try']);
189+
expect([...ownRegionKeys('parallel')].sort()).toEqual(['branches']);
190+
expect(ownRegionKeys('http')).toEqual([]);
191+
expect(ownRegionKeys(undefined)).toEqual([]);
192+
// `node.type` is an open, author-controlled namespace — a prototype key
193+
// must resolve to no slots rather than to something off Object.
194+
expect(ownRegionKeys('constructor')).toEqual([]);
195+
});
196+
197+
it('treats an empty key list as a real answer, distinct from omitting it', () => {
198+
const config = { body: 'payload', try: 'kept' };
199+
expect(stripRegions(config, [])).toBe(config);
200+
// Omitted: the flat-union view its remaining caller was written against.
201+
expect(Object.keys(stripRegions(config) ?? {})).toEqual([]);
202+
});
203+
204+
it('returns undefined for a non-record config, whatever the key list', () => {
205+
expect(stripRegions(undefined, ownRegionKeys('loop'))).toBeUndefined();
206+
expect(stripRegions('nope', [])).toBeUndefined();
207+
});
208+
});
209+
154210
it('labels a node by label, then id, then index', () => {
155211
expect(flowNodeLabel({ label: 'L', id: 'i' }, 0)).toBe('L');
156212
expect(flowNodeLabel({ id: 'i' }, 0)).toBe('i');

packages/lint/src/flow-walk.ts

Lines changed: 58 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -99,9 +99,20 @@ export interface WalkedFlowNode {
9999
/** Config path, e.g. `flows[0].nodes[1].config.catch.nodes[0]`. */
100100
path: string;
101101
/**
102-
* The node's config with region slots stripped — what a rule that scans
103-
* config RECURSIVELY must read, or it reports every descendant's finding a
104-
* second time against this node. `undefined` when the node has no config.
102+
* The node's config with the region slots THIS node type declares stripped —
103+
* what a rule that scans config RECURSIVELY must read, or it reports every
104+
* descendant's finding a second time against this node. `undefined` when the
105+
* node has no config.
106+
*
107+
* "This node type declares" is the load-bearing half, and it is exactly the
108+
* set the walk below descends into: remove fewer and a nested finding is
109+
* reported twice, remove more and a key that was never a region is deleted
110+
* from the view unread. That second failure is not hypothetical — `body` is a
111+
* region slot on `loop` AND the canonical request-payload key on an `http`
112+
* node, so stripping the flat union blinded every recursive scan to the whole
113+
* of `http.config.body`: a `{record.<field>}` token there interpolates at run
114+
* time (`http-nodes.ts` interpolates the raw config wholesale) and rendered a
115+
* silent empty value into an outbound request with nothing to see it.
105116
*/
106117
localConfig?: AnyRec;
107118
/**
@@ -118,22 +129,56 @@ export function flowNodeLabel(node: AnyRec, index: number): string {
118129
return strName(node.label) ?? strName(node.id) ?? `#${index}`;
119130
}
120131

132+
const EMPTY_SLOTS: readonly string[] = [];
133+
134+
/** The region slots a node type owns — empty for every non-container type. */
135+
export function ownRegionKeys(nodeType: unknown): readonly string[] {
136+
const type = strName(nodeType);
137+
return (type ? REGION_SLOTS.get(type) : undefined) ?? EMPTY_SLOTS;
138+
}
139+
121140
/**
122-
* `config` minus the region slots, or `undefined` when there is no config.
141+
* `config` minus `regionKeys`, or `undefined` when there is no config.
142+
*
143+
* Copy-on-write: a config carrying none of those keys comes back by reference.
144+
*
145+
* **Pass the OWNING node's slots** ({@link ownRegionKeys}), not the flat union.
146+
* The union is a different question — "every key that holds a region on SOME
147+
* node type" — and answering it here deletes keys that are ordinary config on
148+
* the node in hand: `body` is `loop`'s region slot and `http`'s request payload,
149+
* `branches` and `try` are as available to any other node type. A recursive scan
150+
* reading the union view therefore cannot see those keys at all, silently, which
151+
* is the reverse of the double-count this view exists to prevent and strictly
152+
* worse: a double-count is visible in the output.
153+
*
154+
* The union stays the DEFAULT to bound this change to the two rules #16111
155+
* names — ⛔ NOT because it is the right argument for the caller still taking
156+
* it. `lint-flow-patterns.ts` reads the union view for its own recursive
157+
* template scan, so it is blind to an `http` node's `body` for exactly the
158+
* reason above: the same defect, one call site over, tracked on #16405. Once
159+
* that caller passes its own slots this default has no callers left and
160+
* `regionKeys` must become REQUIRED, so no later caller inherits the trap by
161+
* writing the shorter call.
123162
*
124-
* Copy-on-write: a config with no region key comes back by reference.
163+
* `regionKeys: []` is a real answer (strip nothing) and is distinct from
164+
* omitting the parameter.
125165
*
126166
* Exported since #5383 because {@link WalkedFlowNode.localConfig} is not the only
127167
* consumer that needs this view. `lint-flow-patterns.ts` walks graphs rather than
128168
* nodes (it needs each region's `edges` too, which this walk does not carry), but
129169
* its recursive config scans hit the identical double-count trap described above —
130170
* so it reads the same region-stripped view, from this one definition.
131171
*/
132-
export function stripRegions(config: unknown): AnyRec | undefined {
172+
export function stripRegions(
173+
config: unknown,
174+
regionKeys: Iterable<string> = REGION_CONFIG_KEYS,
175+
): AnyRec | undefined {
133176
if (!isRec(config)) return undefined;
177+
const strip = regionKeys instanceof Set ? regionKeys : new Set(regionKeys);
178+
if (strip.size === 0) return config;
134179
let out: AnyRec | undefined;
135180
for (const key of Object.keys(config)) {
136-
if (!REGION_CONFIG_KEYS.has(key)) continue;
181+
if (!strip.has(key)) continue;
137182
out ??= { ...config };
138183
delete out[key];
139184
}
@@ -156,16 +201,20 @@ export function walkFlowNodes(flow: AnyRec, flowPath: string): WalkedFlowNode[]
156201
nodes.forEach((raw, index) => {
157202
if (!isRec(raw)) return;
158203
const path = `${basePath}[${index}]`;
204+
// The slots this node OWNS — the same lookup that decides where the walk
205+
// descends below, so `localConfig` removes exactly what was walked
206+
// separately and nothing else.
207+
const ownSlots = ownRegionKeys(raw.type);
159208
out.push({
160209
node: raw,
161210
path,
162-
localConfig: stripRegions(raw.config),
211+
localConfig: stripRegions(raw.config, ownSlots),
163212
regionTrail: trail,
164213
depth,
165214
});
166215

167216
const type = strName(raw.type);
168-
const slots = type ? REGION_SLOTS.get(type) : undefined;
217+
const slots = ownSlots.length > 0 ? ownSlots : undefined;
169218
if (!slots || !isRec(raw.config)) return;
170219
const config = raw.config;
171220
const here = `${type} "${flowNodeLabel(raw, index)}"`;

packages/lint/src/validate-flow-template-paths.test.ts

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,161 @@ describe('validateFlowTemplatePaths', () => {
419419
expect(findings[0].path).toBe('flows[0].nodes[1]');
420420
});
421421
});
422+
423+
// ── a template outside a node filter ───────────────────────────────────
424+
//
425+
// Both rule ids declare a per-position severity (`inFilter ? 'error' :
426+
// 'warning'`) and a second message for the non-filter half, and that half was
427+
// unreachable on the shape a real hand-off flow has: `body` is a region slot
428+
// on `loop`, so the region-stripped view a recursive scan must read deleted
429+
// `config.body` from EVERY node — including the `http` node whose request
430+
// payload it is. The `http` executor interpolates its raw config wholesale,
431+
// so a `{record.<typo>}` there renders an empty value into an outbound
432+
// request on every run, silently, at authoring time and at run time alike.
433+
//
434+
// Four pins, one fixture, matching the four injections the report measured.
435+
// The two filter pins are NEGATIVE CONTROLS: they pass before this fix and
436+
// must keep passing, or a later refactor could break the gating half while
437+
// the warning half stays green. `warning` does not move the exit code, so
438+
// every assertion below reads the findings array, never a pass/fail verdict.
439+
describe('outside a node filter', () => {
440+
const CLEAN_FILTER = '{record.crm_account}';
441+
const CLEAN_PAYLOAD = '{record.company}';
442+
443+
/** A billing hand-off: a filter-guarded read, then an http POST payload. */
444+
const handoff = (filterId: string, payloadAmount: string) => ({
445+
objects: [LEAD_OBJECT],
446+
flows: [
447+
{
448+
name: 'billing_handoff',
449+
type: 'record_change',
450+
nodes: [
451+
{ id: 'start', type: 'start', config: { objectName: 'crm_lead', triggerType: 'record-after-update' } },
452+
{
453+
id: 'fetch',
454+
type: 'get_record',
455+
label: 'Fetch account',
456+
config: { objectName: 'crm_lead', filter: { id: filterId } },
457+
},
458+
{
459+
id: 'post',
460+
type: 'http',
461+
label: 'Hand off',
462+
config: {
463+
url: 'https://billing.example/handoff',
464+
method: 'POST',
465+
body: { amount: payloadAmount },
466+
},
467+
},
468+
],
469+
},
470+
],
471+
});
472+
473+
it('gates an unknown field in a filter (negative control)', () => {
474+
const findings = validateFlowTemplatePaths(handoff('{record.crm_account_nope}', CLEAN_PAYLOAD));
475+
expect(findings).toHaveLength(1);
476+
expect(findings[0].rule).toBe(FLOW_TEMPLATE_UNKNOWN_FIELD);
477+
expect(findings[0].severity).toBe('error');
478+
expect(findings[0].where).toBe('flow "billing_handoff" node "get_record"');
479+
});
480+
481+
it('gates a lookup traversal in a filter (negative control)', () => {
482+
const findings = validateFlowTemplatePaths(handoff('{record.crm_account.owner_id}', CLEAN_PAYLOAD));
483+
expect(findings).toHaveLength(1);
484+
expect(findings[0].rule).toBe(FLOW_TEMPLATE_LOOKUP_TRAVERSAL);
485+
expect(findings[0].severity).toBe('error');
486+
expect(findings[0].where).toBe('flow "billing_handoff" node "get_record"');
487+
});
488+
489+
it('warns on an unknown field in a payload outside any filter', () => {
490+
const findings = validateFlowTemplatePaths(handoff(CLEAN_FILTER, '{record.amount_nope}'));
491+
expect(findings).toHaveLength(1);
492+
expect(findings[0].rule).toBe(FLOW_TEMPLATE_UNKNOWN_FIELD);
493+
expect(findings[0].severity).toBe('warning');
494+
expect(findings[0].where).toBe('flow "billing_handoff" node "http"');
495+
expect(findings[0].path).toBe('flows[0].nodes[2]');
496+
expect(findings[0].message).toContain('empty string');
497+
});
498+
499+
it('warns on a lookup traversal in a payload outside any filter', () => {
500+
const findings = validateFlowTemplatePaths(handoff(CLEAN_FILTER, '{record.crm_account.name}'));
501+
expect(findings).toHaveLength(1);
502+
expect(findings[0].rule).toBe(FLOW_TEMPLATE_LOOKUP_TRAVERSAL);
503+
expect(findings[0].severity).toBe('warning');
504+
expect(findings[0].where).toBe('flow "billing_handoff" node "http"');
505+
expect(findings[0].message).toContain('empty string');
506+
});
507+
508+
it('is silent on the same fixture with both positions authored correctly', () => {
509+
expect(validateFlowTemplatePaths(handoff(CLEAN_FILTER, CLEAN_PAYLOAD))).toEqual([]);
510+
});
511+
512+
it('resolves one leaf used in both positions of a node at the gating severity', () => {
513+
const findings = validateFlowTemplatePaths({
514+
objects: [LEAD_OBJECT],
515+
flows: [
516+
{
517+
name: 'billing_handoff',
518+
type: 'record_change',
519+
nodes: [
520+
{ id: 'start', type: 'start', config: { objectName: 'crm_lead', triggerType: 'record-after-update' } },
521+
{
522+
id: 'touch',
523+
type: 'update_record',
524+
label: 'Touch',
525+
config: {
526+
objectName: 'crm_lead',
527+
filter: { company: '{record.full_naem}' },
528+
fields: { company: 'echo {record.full_naem}' },
529+
},
530+
},
531+
],
532+
},
533+
],
534+
});
535+
expect(findings).toHaveLength(1);
536+
expect(findings[0].severity).toBe('error');
537+
});
538+
539+
it('reports a payload nested in a loop body once, on the node that carries it', () => {
540+
const findings = validateFlowTemplatePaths({
541+
objects: [LEAD_OBJECT],
542+
flows: [
543+
{
544+
name: 'billing_handoff',
545+
type: 'record_change',
546+
nodes: [
547+
{ id: 'start', type: 'start', config: { objectName: 'crm_lead', triggerType: 'record-after-update' } },
548+
{
549+
id: 'each',
550+
type: 'loop',
551+
label: 'Each',
552+
config: {
553+
collection: '{items}',
554+
body: {
555+
nodes: [
556+
{
557+
id: 'post',
558+
type: 'http',
559+
label: 'Hand off',
560+
config: { url: 'https://billing.example/handoff', body: { amount: '{record.amount_nope}' } },
561+
},
562+
],
563+
edges: [],
564+
},
565+
},
566+
},
567+
],
568+
},
569+
],
570+
});
571+
expect(findings).toHaveLength(1);
572+
expect(findings[0].severity).toBe('warning');
573+
expect(findings[0].path).toBe('flows[0].nodes[1].config.body.nodes[0]');
574+
expect(findings[0].where).toBe('flow "billing_handoff" loop "Each" › body node "http"');
575+
});
576+
});
422577
});
423578

424579
describe('validateFlowTemplatePaths — unprovisioned injected anchors (#8340)', () => {

0 commit comments

Comments
 (0)