Skip to content

Commit b0e5a37

Browse files
authored
fix(lint,cli): a filter reference that cannot resolve fails the build, not the run (#3426, #3810) (#3861)
`validateFlowTemplatePaths` called every `{record.<path>}` miss advisory, on the reasoning that an unresolved token renders a blank and the run completes. Since #3810 that stopped being true in one position: inside a CRUD node's `filter` an unresolved token does not blank a value, it DELETES the condition — and a removed condition matches MORE rows. Those nodes now refuse to execute. So the rule was warning about metadata whose runtime was already decided: `os validate` printed a yellow line, exited 0, and shipped a flow that cannot run. Severity now follows the runtime consequence, by position. - `filter` of get_record / update_record / delete_record -> error. The three nodes whose filter `resolveNodeFilter` guards. `create_record` is excluded: it writes a payload, no filter to widen. - Every other position -> warning, unchanged. The token still renders a blank, the run still completes, and the head object may come from another package. A reference used in both positions reports once, at error severity. `os validate` now enforces it. The step filtered this rule for `severity === 'warning'` and dropped everything else, so an error would have been invisible. It gates on errors first — rule id, config path, and `errors` in `--json` — mirroring the `validateReadonlyFlowWrites` step below it. Verified against shipped metadata: 33 flows across app-todo, app-crm and app-showcase produce no new errors; the four pre-existing traversal warnings sit in script/notify/subflow/parallel positions and keep advisory severity.
1 parent d77d1b7 commit b0e5a37

5 files changed

Lines changed: 312 additions & 35 deletions

File tree

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
---
2+
"@objectstack/lint": minor
3+
"@objectstack/cli": patch
4+
---
5+
6+
fix(lint,cli): a filter reference that cannot resolve fails the build, not the run (#3426, #3810)
7+
8+
`validateFlowTemplatePaths` reported every `{record.<path>}` miss as **advisory**,
9+
on the reasoning that an unresolved token renders a blank and the run still
10+
completes. Since #3810 that reasoning no longer holds in one position: inside a
11+
CRUD node's `filter`, an unresolved token does not blank a value, it **deletes
12+
the condition** — and a removed condition matches MORE rows, not fewer. Those
13+
nodes now refuse to execute rather than run a widened query.
14+
15+
So the rule was warning about metadata whose runtime is already decided: `os
16+
validate` printed a yellow line, exited 0, and shipped a flow that cannot run.
17+
Severity now follows the runtime consequence, by position:
18+
19+
- **`filter` of `get_record` / `update_record` / `delete_record``error`.**
20+
These are the three nodes whose filter `resolveNodeFilter` guards. The finding
21+
says what the runtime will do ("the node refuses to run at execution time")
22+
and why the build gates rather than warns (an absent condition *widens* the
23+
query). `os validate` exits 1.
24+
- **Every other position → `warning`, unchanged.** A message body, an `http`
25+
url, an `update_record` write payload: the token still renders a blank, the
26+
run still completes, and the head object may legitimately come from another
27+
installed package. `create_record` is deliberately excluded from the gating
28+
set — it writes a payload and has no filter to widen.
29+
30+
Both rules split this way (`flow-template-unknown-field` and
31+
`flow-template-lookup-traversal`), so a typo and a lookup hop are gated wherever
32+
the runtime refuses them. A reference used in both positions on one node is
33+
reported **once, at error severity**.
34+
35+
**`os validate` now enforces it.** The command filtered this rule's findings for
36+
`severity === 'warning'` and dropped everything else on the floor, so an error
37+
from it would have been invisible. It now gates on errors first — printing rule
38+
id and config path, and emitting them under `errors` in `--json` — mirroring the
39+
`validateReadonlyFlowWrites` step directly below, which makes the same
40+
shift-left split (a certain runtime failure gates; a state-dependent one
41+
advises).
42+
43+
Verified against the shipped examples: 33 flows across app-todo, app-crm and
44+
app-showcase produce **no new errors**; the four pre-existing lookup-traversal
45+
warnings sit in `script` / `notify` / `subflow` / `parallel` positions and keep
46+
their advisory severity.
47+
48+
No authoring change is required for a correct filter. A filter that this rule
49+
now fails is one the runtime would have refused anyway — the difference is that
50+
you find out at `os validate` instead of at 3am.

packages/cli/src/commands/validate.ts

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -518,12 +518,39 @@ export default class Validate extends Command {
518518
// 3g-bis. Flow template path references (#3426): a `{record.<path>}` token
519519
// in a node template that names an unknown field, or hops through a
520520
// lookup relation the seeded record carries only as a scalar id, both
521-
// render a SILENT empty string at runtime. Advisory: the head object
522-
// may come from another package (skipped there), and the runtime still
523-
// produces output (a blank), so nothing is fully broken.
521+
// render a SILENT empty string at runtime. Advisory in most positions:
522+
// the head object may come from another package (skipped there), and
523+
// the runtime still produces output (a blank), so nothing is fully
524+
// broken. GATING inside a filter-guarded CRUD node's `filter`: since
525+
// #3810 an unresolved token there does not blank a value, it deletes
526+
// the condition — which WIDENS the query — so the node refuses to
527+
// execute. The build must not ship a flow whose runtime is already
528+
// decided.
524529
if (!flags.json) printStep('Checking flow template references...');
525530
const flowTemplateFindings = validateFlowTemplatePaths(normalized as Record<string, unknown>);
531+
const flowTemplateErrors = flowTemplateFindings.filter((f) => f.severity === 'error');
526532
const flowTemplateWarnings = flowTemplateFindings.filter((f) => f.severity === 'warning');
533+
if (flowTemplateErrors.length > 0) {
534+
if (flags.json) {
535+
await emitJson({
536+
valid: false,
537+
errors: flowTemplateErrors,
538+
warnings: flowTemplateWarnings,
539+
duration: timer.elapsed(),
540+
});
541+
this.exit(1);
542+
}
543+
console.log('');
544+
printError(
545+
`Flow template reference check failed (${flowTemplateErrors.length} issue${flowTemplateErrors.length > 1 ? 's' : ''})`,
546+
);
547+
for (const f of flowTemplateErrors.slice(0, 50)) {
548+
console.log(` • ${f.where}: ${f.message}`);
549+
console.log(chalk.dim(` ${f.hint}`));
550+
console.log(chalk.dim(` rule: ${f.rule} at ${f.path}`));
551+
}
552+
this.exit(1);
553+
}
527554
if (!flags.json) {
528555
for (const w of flowTemplateWarnings.slice(0, 50)) {
529556
console.log(chalk.yellow(` ⚠ ${w.where}: ${w.message}`));

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

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,4 +232,116 @@ describe('validateFlowTemplatePaths', () => {
232232
it('returns empty when there are no flows', () => {
233233
expect(validateFlowTemplatePaths({ objects: [LEAD_OBJECT] })).toHaveLength(0);
234234
});
235+
236+
// ── severity follows position (#3810) ───────────────────────────────────
237+
// Outside a filter an unresolved token blanks a value (advisory); inside a
238+
// filter-guarded CRUD node's filter it DELETES the condition, so the node
239+
// refuses to run — the build must not ship it.
240+
describe('filter-position severity (#3810)', () => {
241+
/** A record-change flow with one CRUD node carrying the given config. */
242+
function crudFlow(type: string, config: AnyRec): AnyRec {
243+
return {
244+
objects: [LEAD_OBJECT],
245+
flows: [
246+
{
247+
name: 'crud_flow',
248+
type: 'record_change',
249+
nodes: [
250+
{ id: 'start', type: 'start', config: { objectName: 'crm_lead', triggerType: 'record-created' } },
251+
{ id: 'c1', type, config },
252+
],
253+
},
254+
],
255+
};
256+
}
257+
258+
it.each(['get_record', 'update_record', 'delete_record'])(
259+
'raises an unknown field inside a %s filter to error',
260+
(type) => {
261+
const findings = validateFlowTemplatePaths(
262+
crudFlow(type, { objectName: 'crm_lead', filter: { company: '{record.compnay}' } }),
263+
);
264+
expect(findings).toHaveLength(1);
265+
expect(findings[0].rule).toBe(FLOW_TEMPLATE_UNKNOWN_FIELD);
266+
expect(findings[0].severity).toBe('error');
267+
expect(findings[0].message).toContain('refuses to run');
268+
},
269+
);
270+
271+
it('raises a lookup traversal inside a filter to error', () => {
272+
const findings = validateFlowTemplatePaths(
273+
crudFlow('delete_record', { objectName: 'crm_lead', filter: { company: '{record.crm_account.name}' } }),
274+
);
275+
expect(findings).toHaveLength(1);
276+
expect(findings[0].rule).toBe(FLOW_TEMPLATE_LOOKUP_TRAVERSAL);
277+
expect(findings[0].severity).toBe('error');
278+
});
279+
280+
it('keeps a bad reference OUTSIDE the filter advisory on the same node', () => {
281+
const findings = validateFlowTemplatePaths(
282+
crudFlow('update_record', {
283+
objectName: 'crm_lead',
284+
filter: { company: '{record.company}' },
285+
fields: { last_name: '{record.compnay}' },
286+
}),
287+
);
288+
expect(findings).toHaveLength(1);
289+
expect(findings[0].severity).toBe('warning');
290+
});
291+
292+
it('reports a reference used in BOTH positions once, at error severity', () => {
293+
const findings = validateFlowTemplatePaths(
294+
crudFlow('update_record', {
295+
objectName: 'crm_lead',
296+
filter: { company: '{record.compnay}' },
297+
fields: { last_name: 'echo {record.compnay}' },
298+
}),
299+
);
300+
expect(findings).toHaveLength(1);
301+
expect(findings[0].severity).toBe('error');
302+
});
303+
304+
it('stays advisory for create_record, which has no filter to widen', () => {
305+
const findings = validateFlowTemplatePaths(
306+
crudFlow('create_record', { objectName: 'crm_lead', filter: { company: '{record.compnay}' } }),
307+
);
308+
expect(findings).toHaveLength(1);
309+
expect(findings[0].severity).toBe('warning');
310+
});
311+
312+
it('does NOT flag a valid filter reference', () => {
313+
const findings = validateFlowTemplatePaths(
314+
crudFlow('delete_record', {
315+
objectName: 'crm_lead',
316+
filter: { company: '{record.company}', owner: '{record.owner}' },
317+
}),
318+
);
319+
expect(findings).toHaveLength(0);
320+
});
321+
322+
it('respects declared expand for a filter traversal (#3475)', () => {
323+
const findings = validateFlowTemplatePaths({
324+
objects: [LEAD_OBJECT],
325+
flows: [
326+
{
327+
name: 'expanded_filter',
328+
type: 'record_change',
329+
nodes: [
330+
{
331+
id: 'start',
332+
type: 'start',
333+
config: { objectName: 'crm_lead', triggerType: 'record-created', expand: ['crm_account'] },
334+
},
335+
{
336+
id: 'c1',
337+
type: 'get_record',
338+
config: { objectName: 'crm_lead', filter: { company: '{record.crm_account.name}' } },
339+
},
340+
],
341+
},
342+
],
343+
});
344+
expect(findings).toHaveLength(0);
345+
});
346+
});
235347
});

0 commit comments

Comments
 (0)