Skip to content

Commit af5b96b

Browse files
os-zhuangclaude
andauthored
fix(lint): flow rules see into try_catch / loop / parallel regions (#4380) (#4388)
Every lint rule that inspects flow nodes had hand-written the same one-liner — `const nodes = Array.isArray(flow.nodes) ? flow.nodes : []` — and every one 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, same bad nodes flat vs inside a try_catch: flow-node-write-unknown-field (error) 1 → 0 flow-update-readonly-field (error) 1 → 0 approval-approver-* 1 → 0 flow-template-unknown-field (error) 1 → 1, but as a WARNING The last one is the one a reader would not predict. This rule 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") degraded to an advisory warning, reported against the wrapping try_catch instead of the get_record that is broken. Being visible is not the same as being judged correctly, and that is worse than a clean miss: a yellow line reads as "checked, merely advisory". One shared walk, not five. `flow-walk.ts` is the flow-side counterpart of the existing `page-walk.ts`, here for the same stated reason — getting the traversal right is subtle enough that duplicating it has already produced dead rules. `walkFlowNodes` 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 route through it. Findings now land on the node that is actually wrong — a path pointing at the container is not actionable in a flow with several regions. The double-count trap is handled once, not left to each caller. A container is walked too (its own config is worth checking — a loop's `collection`, a try_catch's `retry`), but its config physically contains every descendant, so a recursive scanner would report each nested finding twice. `localConfig` is the container's config with region slots removed; a test pins that a nested token is reported once while the container's own token still is. `REGION_SLOTS` is data, pinned against the spec's region-bearing config schemas by behavioural derivation rather than restatement, so a fifth construct fails that test instead of becoming a fifth blind spot. `MAX_REGION_DEPTH` 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 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` at `flows[24].nodes[1].config.catch.nodes[0].config.fields.sync_statuss`. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent c39d713 commit af5b96b

11 files changed

Lines changed: 803 additions & 39 deletions
Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
---
2+
"@objectstack/lint": patch
3+
---
4+
5+
fix(lint): flow rules see into try_catch / loop / parallel regions (#4380)
6+
7+
Every lint rule that inspects flow nodes had hand-written the same one-liner —
8+
9+
```ts
10+
const nodes = Array.isArray(flow.nodes) ? (flow.nodes as AnyRec[]) : [];
11+
```
12+
13+
— and every one of them was therefore blind to the same thing.
14+
`FlowRegionSchema` holds a full `nodes: z.array(FlowNodeSchema)`, and four
15+
config slots carry one: `try_catch.config.try` / `.catch`, `loop.config.body`,
16+
and `parallel.config.branches[].nodes`. Regions nest arbitrarily. Move a node
17+
into any of them and the checking stayed behind.
18+
19+
Measured before the fix, the same bad nodes at the top level vs inside a
20+
`try_catch`:
21+
22+
| rule | severity | flat | nested |
23+
| :--- | :--- | :--- | :--- |
24+
| `flow-node-write-unknown-field` | error | 1 | **0** |
25+
| `flow-update-readonly-field` | error | 1 | **0** |
26+
| `approval-approver-*` | error/warning | 1 | **0** |
27+
| `flow-template-unknown-field` (filter position) | error | 1 | **1, as a warning** |
28+
29+
**The last row is the one a reader would not predict.**
30+
`validate-flow-template-paths` scans a node's whole `config` for string leaves,
31+
so it still *saw* tokens inside a region — but its `filter`-position split only
32+
looks at the top level of the node it was handed. A nested filter token lost its
33+
position, so the #3810 finding ("this node cannot run — an erased condition
34+
WIDENS the query") silently degraded to an advisory warning, reported against
35+
the wrapping `try_catch` instead of the `get_record` that is broken:
36+
37+
```
38+
FLAT error flow "f" node "get_record" flows[0].nodes[1]
39+
NESTED warning flow "f" node "try_catch" flows[0].nodes[1]
40+
```
41+
42+
Being visible is not the same as being judged correctly. That is worse than a
43+
clean miss: a yellow line reads as "checked and merely advisory".
44+
45+
**One shared walk, not five.** `flow-walk.ts` — the flow-side counterpart of the
46+
existing `page-walk.ts`, and here for the same stated reason: getting the
47+
traversal right is subtle enough that duplicating it has already produced dead
48+
rules. `walkFlowNodes(flow, flowPath)` yields every node with its real config
49+
path (`flows[0].nodes[1].config.catch.nodes[0]`), a region breadcrumb for
50+
diagnostics (`try_catch "Guard" › catch`), and depth. Four rules now route
51+
through it: the two flow write rules, the template-path rule, and the approval
52+
rule.
53+
54+
Findings now land on the node that is actually wrong, which is the point — a
55+
path pointing at the container is not actionable in a flow with several regions.
56+
57+
**The double-count trap is handled, not left to each caller.** A container node
58+
is walked too (it has its own config worth checking — a `loop`'s `collection`, a
59+
`try_catch`'s `retry`), but its `config` physically contains every descendant,
60+
so a rule that scans config recursively would report each nested finding twice.
61+
`WalkedFlowNode.localConfig` is the container's config with region slots
62+
removed; the recursive scanner uses it, and a test pins that a nested token is
63+
reported once while the container's own `collection` token still is.
64+
65+
`REGION_SLOTS` is declared as data and pinned against the spec's own
66+
region-bearing config schemas — derived behaviourally (a slot is one that
67+
accepts `{nodes: […]}`), not restated — so a fifth construct fails that test
68+
instead of becoming a fifth silent blind spot. A `MAX_REGION_DEPTH` cap keeps a
69+
hand-authored (pre-parse) stack from hanging a lint.
70+
71+
Verified end to end: nested now matches flat on every rule, including the
72+
restored `error` severity. app-showcase ships an `update_record` inside a
73+
`catch` branch (`showcase_resilient_sync`) that had never been checked by
74+
anything — it is correct, so validation stays clean, and breaking its field name
75+
on purpose now fails `os validate` with
76+
`flows[24].nodes[1].config.catch.nodes[0].config.fields.sync_statuss` and the
77+
region trail `try_catch "Push with retry" › catch › node "Flag Sync Failure"`.
Lines changed: 206 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,206 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { describe, it, expect } from 'vitest';
4+
import {
5+
LoopConfigSchema,
6+
ParallelConfigSchema,
7+
TryCatchConfigSchema,
8+
} from '@objectstack/spec/automation';
9+
10+
import {
11+
walkFlowNodes,
12+
flowNodeLabel,
13+
REGION_SLOTS,
14+
REGION_CONFIG_KEYS,
15+
MAX_REGION_DEPTH,
16+
} from './flow-walk.js';
17+
18+
const node = (id: string, extra: Record<string, unknown> = {}) => ({ id, type: 'script', ...extra });
19+
20+
describe('REGION_SLOTS — pinned against the spec, not restated', () => {
21+
// The ledger's job is to make a NEW region-bearing construct fail here rather
22+
// than become a fifth silent blind spot. Derived behaviourally from the
23+
// spec's own config schemas: a region slot is one that accepts `{nodes: […]}`.
24+
const REGION_BEARING_CONFIGS = {
25+
try_catch: TryCatchConfigSchema,
26+
loop: LoopConfigSchema,
27+
parallel: ParallelConfigSchema,
28+
} as const;
29+
30+
/** Keys of `schema` that accept a region (or an array of them). */
31+
const regionKeysOf = (schema: { safeParse: (v: unknown) => { success: boolean; data?: unknown } }): string[] => {
32+
// `label` is required by FlowNodeSchema — a probe node without it fails the
33+
// parse and would make every slot look non-region.
34+
const region = { nodes: [{ id: 'probe', type: 'script', label: 'Probe' }], edges: [] };
35+
const probes: Record<string, unknown> = {
36+
// Required siblings so the parse reaches the region keys at all.
37+
collection: '{items}',
38+
try: region,
39+
catch: region,
40+
body: region,
41+
branches: [{ nodes: region.nodes, edges: [] }, { nodes: region.nodes, edges: [] }],
42+
};
43+
const parsed = schema.safeParse(probes);
44+
if (!parsed.success) return [];
45+
const data = parsed.data as Record<string, unknown>;
46+
return Object.keys(data).filter((k) => {
47+
const v = data[k];
48+
if (Array.isArray(v)) return v.every((e) => !!e && typeof e === 'object' && Array.isArray((e as never)['nodes']));
49+
return !!v && typeof v === 'object' && Array.isArray((v as Record<string, unknown>).nodes);
50+
});
51+
};
52+
53+
it('declares exactly the region slots each construct actually accepts', () => {
54+
for (const [type, schema] of Object.entries(REGION_BEARING_CONFIGS)) {
55+
expect([...(REGION_SLOTS.get(type) ?? [])].sort(), `region slots for '${type}'`).toEqual(
56+
regionKeysOf(schema).sort(),
57+
);
58+
}
59+
});
60+
61+
it('derives REGION_CONFIG_KEYS from the per-type slots', () => {
62+
expect([...REGION_CONFIG_KEYS].sort()).toEqual([...new Set([...REGION_SLOTS.values()].flat())].sort());
63+
});
64+
});
65+
66+
describe('walkFlowNodes', () => {
67+
it('yields top-level nodes with a flat path and an empty trail', () => {
68+
const walked = walkFlowNodes({ nodes: [node('a'), node('b')] }, 'flows[0]');
69+
expect(walked.map((w) => w.path)).toEqual(['flows[0].nodes[0]', 'flows[0].nodes[1]']);
70+
expect(walked.every((w) => w.regionTrail === '' && w.depth === 0)).toBe(true);
71+
});
72+
73+
it('reaches try_catch try + catch regions', () => {
74+
const flow = {
75+
nodes: [
76+
{
77+
id: 'guard',
78+
type: 'try_catch',
79+
label: 'Guard',
80+
config: {
81+
try: { nodes: [node('push')], edges: [] },
82+
catch: { nodes: [node('flag')], edges: [] },
83+
},
84+
},
85+
],
86+
};
87+
const walked = walkFlowNodes(flow, 'flows[0]');
88+
expect(walked.map((w) => w.path)).toEqual([
89+
'flows[0].nodes[0]',
90+
'flows[0].nodes[0].config.try.nodes[0]',
91+
'flows[0].nodes[0].config.catch.nodes[0]',
92+
]);
93+
expect(walked[2].regionTrail).toBe('try_catch "Guard" › catch');
94+
expect(walked[2].depth).toBe(1);
95+
});
96+
97+
it('reaches a loop body', () => {
98+
const flow = {
99+
nodes: [{ id: 'each', type: 'loop', config: { collection: '{items}', body: { nodes: [node('inner')], edges: [] } } }],
100+
};
101+
const walked = walkFlowNodes(flow, 'flows[0]');
102+
expect(walked.map((w) => w.path)).toEqual(['flows[0].nodes[0]', 'flows[0].nodes[0].config.body.nodes[0]']);
103+
expect(walked[1].regionTrail).toBe('loop "each" › body');
104+
});
105+
106+
it('reaches every parallel branch, named or indexed', () => {
107+
const flow = {
108+
nodes: [
109+
{
110+
id: 'fan',
111+
type: 'parallel',
112+
config: {
113+
branches: [
114+
{ name: 'left', nodes: [node('l')], edges: [] },
115+
{ nodes: [node('r')], edges: [] },
116+
],
117+
},
118+
},
119+
],
120+
};
121+
const walked = walkFlowNodes(flow, 'flows[0]');
122+
expect(walked.map((w) => w.path)).toEqual([
123+
'flows[0].nodes[0]',
124+
'flows[0].nodes[0].config.branches[0].nodes[0]',
125+
'flows[0].nodes[0].config.branches[1].nodes[0]',
126+
]);
127+
expect(walked[1].regionTrail).toBe('parallel "fan" › branch left');
128+
expect(walked[2].regionTrail).toBe('parallel "fan" › branch #1');
129+
});
130+
131+
it('recurses through nested regions and accumulates the trail', () => {
132+
const flow = {
133+
nodes: [
134+
{
135+
id: 'outer',
136+
type: 'try_catch',
137+
config: {
138+
try: {
139+
nodes: [
140+
{
141+
id: 'inner',
142+
type: 'loop',
143+
config: { collection: '{x}', body: { nodes: [node('deep')], edges: [] } },
144+
},
145+
],
146+
edges: [],
147+
},
148+
},
149+
},
150+
],
151+
};
152+
const walked = walkFlowNodes(flow, 'flows[0]');
153+
const deep = walked.find((w) => w.node.id === 'deep');
154+
expect(deep?.path).toBe('flows[0].nodes[0].config.try.nodes[0].config.body.nodes[0]');
155+
expect(deep?.regionTrail).toBe('try_catch "outer" › try › loop "inner" › body');
156+
expect(deep?.depth).toBe(2);
157+
});
158+
159+
// The trap that makes a recursive config scan double-report.
160+
it('localConfig strips region slots but keeps the container’s own config', () => {
161+
const flow = {
162+
nodes: [
163+
{
164+
id: 'each',
165+
type: 'loop',
166+
config: { collection: '{items}', maxIterations: 10, body: { nodes: [node('inner')], edges: [] } },
167+
},
168+
],
169+
};
170+
const [container, inner] = walkFlowNodes(flow, 'flows[0]');
171+
expect(Object.keys(container.localConfig ?? {}).sort()).toEqual(['collection', 'maxIterations']);
172+
// The raw node is untouched — stripping is a view, not a mutation.
173+
expect((container.node.config as Record<string, unknown>).body).toBeDefined();
174+
// A non-container node's localConfig is its config, not a copy-with-holes.
175+
expect(inner.localConfig).toEqual(inner.node.config ?? undefined);
176+
});
177+
178+
it('leaves localConfig undefined for a node with no config', () => {
179+
const [only] = walkFlowNodes({ nodes: [{ id: 'x', type: 'end' }] }, 'flows[0]');
180+
expect(only.localConfig).toBeUndefined();
181+
});
182+
183+
it('labels a node by label, then id, then index', () => {
184+
expect(flowNodeLabel({ label: 'L', id: 'i' }, 0)).toBe('L');
185+
expect(flowNodeLabel({ id: 'i' }, 0)).toBe('i');
186+
expect(flowNodeLabel({}, 3)).toBe('#3');
187+
});
188+
189+
it('tolerates missing/!array nodes and non-record entries', () => {
190+
expect(walkFlowNodes({}, 'flows[0]')).toEqual([]);
191+
expect(walkFlowNodes({ nodes: 'nope' }, 'flows[0]')).toEqual([]);
192+
expect(walkFlowNodes({ nodes: [null, 'x', node('ok')] }, 'flows[0]').map((w) => w.node.id)).toEqual(['ok']);
193+
expect(walkFlowNodes({ nodes: [{ id: 'c', type: 'try_catch', config: { try: 'nope' } }] }, 'flows[0]')).toHaveLength(1);
194+
});
195+
196+
it('stops at the depth cap rather than recursing without bound', () => {
197+
// Build MAX_REGION_DEPTH + 3 levels of try nesting.
198+
let deepest: Record<string, unknown> = { id: 'leaf', type: 'script' };
199+
for (let i = 0; i < MAX_REGION_DEPTH + 3; i++) {
200+
deepest = { id: `t${i}`, type: 'try_catch', config: { try: { nodes: [deepest], edges: [] } } };
201+
}
202+
const walked = walkFlowNodes({ nodes: [deepest] }, 'flows[0]');
203+
expect(walked.length).toBeGreaterThan(0);
204+
expect(Math.max(...walked.map((w) => w.depth))).toBeLessThanOrEqual(MAX_REGION_DEPTH);
205+
});
206+
});

0 commit comments

Comments
 (0)