Skip to content

Commit 37f6bd8

Browse files
os-zhuangclaude
andauthored
feat(cli): two flow anti-pattern lints — date-equality filters (#1874) + phantom aggregation (#1870) (#1950)
* feat(cli): lint date-equality vs time values in flow query filters (#1874) Extends the flow anti-pattern lint from trigger CONDITIONS to query FILTERS. A scheduled flow whose get_record filter binds a field directly / via $eq / $in to a time-function value (daysFromNow/today/now/...) silently matches nothing, because a Field.date stores a time component and an exact match against a re-computed timestamp never holds — the bug the templates discrete-tier alerts hit. Range ops ($gte/$lt day windows) are the correct shape and are exempt. New rule flow-date-equality-filter; advisory warning, never fails the build. 10 tests incl. false-positive guards (windows, $or, plain ranges, non-time eq). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(cli): add phantom-aggregation flow lint (#1870) A node-config key naming a capability the automation engine lacks (aggregations/aggregate/groupBy/rollup/having) is silently ignored at runtime — the node runs and computes nothing (templates publication_rollup). Flag it and point the author to the data-layer equivalent: Field.summary for a cross-object rollup, Field.formula for a per-record computed value. New rule flow-phantom-aggregation; advisory warning. 3 tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent c17d2c8 commit 37f6bd8

3 files changed

Lines changed: 209 additions & 1 deletion

File tree

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
---
2+
"@objectstack/cli": minor
3+
---
4+
5+
feat(cli): two new flow authoring anti-pattern lints — date-equality filters (#1874) and phantom aggregation (#1870)
6+
7+
Extends the build-time flow anti-pattern lint (advisory warnings, never fail the build):
8+
9+
- **flow-date-equality-filter (#1874)**: a get_record/query filter that binds a
10+
field directly, or via `$eq`/`$in`, to a time-function value
11+
(`daysFromNow`/`today`/`now`/…). A `Field.date` stores a time component, so an
12+
exact match against a re-computed timestamp silently returns nothing. Range
13+
operators (`$gte`/`$lt` day windows) are the correct shape and are exempt.
14+
- **flow-phantom-aggregation (#1870)**: a node config key naming a capability the
15+
automation engine does not have (`aggregations`/`aggregate`/`groupBy`/`rollup`/
16+
`having`). There is no aggregate node, so the key is silently ignored and the
17+
node computes nothing. Points the author to `Field.summary` / `Field.formula`.

packages/cli/src/utils/lint-flow-patterns.test.ts

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,25 @@ import { describe, it, expect } from 'vitest';
44
import {
55
lintFlowPatterns,
66
FLOW_TIME_RELATIVE_ANTIPATTERN,
7+
FLOW_DATE_EQUALITY_FILTER,
8+
FLOW_PHANTOM_AGGREGATION,
79
FLOW_DOUBLE_BRACE_INTERP,
810
FLOW_BARE_DOLLAR_REF,
911
} from './lint-flow-patterns.js';
1012

13+
const CEL = (source: string) => ({ dialect: 'cel', source });
14+
/** A scheduled flow with a get_record node carrying `filter`. */
15+
const filterFlow = (filter: unknown) => ({
16+
flows: [{
17+
name: 'expiry_alert',
18+
nodes: [
19+
{ id: 'start', type: 'start', config: { triggerType: 'schedule', schedule: 'cron:0 9 * * *' } },
20+
{ id: 'query', type: 'get_record', config: { objectName: 'contract', filter } },
21+
],
22+
edges: [],
23+
}],
24+
});
25+
1126
const flow = (condition: unknown, triggerType = 'record-after-update') => ({
1227
flows: [{
1328
name: 'renewal_alert',
@@ -50,6 +65,82 @@ describe('lintFlowPatterns — time-relative anti-pattern (#1874)', () => {
5065
});
5166
});
5267

68+
describe('lintFlowPatterns — date-equality in query filter (#1874)', () => {
69+
it('flags a field bound directly to a time value (implicit equality)', () => {
70+
const fnds = lintFlowPatterns(filterFlow({ expires_at: CEL('daysFromNow(30)') }));
71+
expect(fnds).toHaveLength(1);
72+
expect(fnds[0].rule).toBe(FLOW_DATE_EQUALITY_FILTER);
73+
expect(fnds[0].hint).toMatch(/\$gte.*daysFromNow\(N\).*\$lt/);
74+
});
75+
76+
it('flags `$in` against time values (the original renewal_alert bug)', () => {
77+
const fnds = lintFlowPatterns(filterFlow({
78+
status: 'active',
79+
end_date: { $in: [CEL('daysFromNow(60)'), CEL('daysFromNow(30)'), CEL('daysFromNow(7)')] },
80+
}));
81+
expect(fnds).toHaveLength(1);
82+
expect(fnds[0].rule).toBe(FLOW_DATE_EQUALITY_FILTER);
83+
});
84+
85+
it('flags `$eq` against a time value', () => {
86+
expect(lintFlowPatterns(filterFlow({ d: { $eq: CEL('today()') } }))).toHaveLength(1);
87+
});
88+
89+
describe('does NOT flag (false-positive guards)', () => {
90+
it('a one-day window (the correct fix)', () => {
91+
expect(lintFlowPatterns(filterFlow({
92+
end_date: { $gte: CEL('daysFromNow(7)'), $lt: CEL('daysFromNow(8)') },
93+
}))).toHaveLength(0);
94+
});
95+
it('multi-tier windows wrapped in $or', () => {
96+
expect(lintFlowPatterns(filterFlow({
97+
status: 'active',
98+
$or: [
99+
{ end_date: { $gte: CEL('daysFromNow(7)'), $lt: CEL('daysFromNow(8)') } },
100+
{ end_date: { $gte: CEL('daysFromNow(30)'), $lt: CEL('daysFromNow(31)') } },
101+
],
102+
}))).toHaveLength(0);
103+
});
104+
it('a plain range like `due_date < today()` (overdue query)', () => {
105+
expect(lintFlowPatterns(filterFlow({ status: 'open', due_date: { $lt: CEL('today()') } }))).toHaveLength(0);
106+
});
107+
it('equality against a non-time value (status, interpolated id)', () => {
108+
expect(lintFlowPatterns(filterFlow({ status: 'active', id: '{record.id}' }))).toHaveLength(0);
109+
expect(lintFlowPatterns(filterFlow({ amount: { $eq: CEL('record.threshold') } }))).toHaveLength(0);
110+
});
111+
});
112+
});
113+
114+
describe('lintFlowPatterns — phantom aggregation capability (#1870)', () => {
115+
const scriptNode = (config: Record<string, unknown>) => ({
116+
flows: [{
117+
name: 'rollup',
118+
nodes: [
119+
{ id: 'start', type: 'start', config: {} },
120+
{ id: 'sum', type: 'script', config },
121+
],
122+
edges: [],
123+
}],
124+
});
125+
126+
it('flags `aggregations` on a script node (publication_rollup bug)', () => {
127+
const fnds = lintFlowPatterns(scriptNode({ aggregations: { total: { sum: 'amount' } } }));
128+
expect(fnds).toHaveLength(1);
129+
expect(fnds[0].rule).toBe(FLOW_PHANTOM_AGGREGATION);
130+
expect(fnds[0].hint).toMatch(/Field\.summary/);
131+
});
132+
133+
it('flags groupBy / rollup / aggregate / having too', () => {
134+
for (const key of ['groupBy', 'rollup', 'aggregate', 'having']) {
135+
expect(lintFlowPatterns(scriptNode({ [key]: {} })).map((f) => f.rule)).toContain(FLOW_PHANTOM_AGGREGATION);
136+
}
137+
});
138+
139+
it('does NOT flag an ordinary script/function node', () => {
140+
expect(lintFlowPatterns(scriptNode({ function: 'helpdesk.triage', inputs: { x: 1 } }))).toHaveLength(0);
141+
});
142+
});
143+
53144
/** A flow with a create_record node carrying `config`. */
54145
const nodeFlow = (config: Record<string, unknown>) => ({
55146
flows: [{

packages/cli/src/utils/lint-flow-patterns.ts

Lines changed: 101 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,16 +38,91 @@ function conditionSource(raw: unknown): string {
3838
}
3939

4040
const TIME_FNS = 'daysFromNow|daysAgo|today|now|date|datetime';
41+
const TIME_FN_RE = new RegExp(`\\b(?:${TIME_FNS})\\s*\\(`);
4142
// A time function adjacent to an equality operator, either side:
4243
// `end_date == daysFromNow(60)` / `today() != record.start`
4344
const DATE_EQ = new RegExp(
4445
`(?:(?:${TIME_FNS})\\s*\\([^)]*\\)\\s*(?:==|!=))|(?:(?:==|!=)\\s*(?:${TIME_FNS})\\s*\\()`,
4546
);
4647

4748
export const FLOW_TIME_RELATIVE_ANTIPATTERN = 'flow-time-relative-antipattern';
49+
export const FLOW_DATE_EQUALITY_FILTER = 'flow-date-equality-filter';
50+
export const FLOW_PHANTOM_AGGREGATION = 'flow-phantom-aggregation';
4851
export const FLOW_DOUBLE_BRACE_INTERP = 'flow-double-brace-interpolation';
4952
export const FLOW_BARE_DOLLAR_REF = 'flow-bare-dollar-reference';
5053

54+
/**
55+
* Node-config keys that name a capability the automation engine does NOT have.
56+
* There is no aggregate node, so a `script`/`loop`/… node carrying these keys is
57+
* silently ignored — the node runs and computes nothing (templates #1870,
58+
* `publication_rollup`). Aggregation belongs in the data layer, not a flow.
59+
*/
60+
const PHANTOM_AGG_KEYS = new Set(['aggregations', 'aggregate', 'groupBy', 'rollup', 'having']);
61+
62+
/** If `v` is a CEL expression whose source calls a time function, return that source. */
63+
function celTimeSource(v: unknown): string | null {
64+
if (v && typeof v === 'object' && (v as AnyRec).dialect === 'cel') {
65+
const src = (v as AnyRec).source;
66+
if (typeof src === 'string' && TIME_FN_RE.test(src)) return src;
67+
}
68+
return null;
69+
}
70+
71+
/** Range operators — the building block of the CORRECT time-window pattern, never flagged. */
72+
const RANGE_OPS = new Set(['$gte', '$gt', '$lte', '$lt', '$ne']);
73+
74+
/**
75+
* Walk a get_record/query `filter` for the date-EQUALITY footgun: a field bound
76+
* directly (`field: daysFromNow(N)`) or via `$eq` / `$in` to a time-function value.
77+
* A `Field.date` is stored with a time component, so two independently-computed
78+
* timestamps never compare equal — the query silently returns nothing (#1928 /
79+
* templates #1874). Range operators (`$gte`/`$lt` day windows) are the correct
80+
* shape and are never flagged.
81+
*/
82+
function scanFilterForDateEquality(
83+
filter: unknown,
84+
where: string,
85+
findings: FlowLintFinding[],
86+
): void {
87+
if (!filter || typeof filter !== 'object' || Array.isArray(filter)) return;
88+
for (const [key, val] of Object.entries(filter as AnyRec)) {
89+
if (key === '$or' || key === '$and') {
90+
if (Array.isArray(val)) for (const sub of val) scanFilterForDateEquality(sub, where, findings);
91+
continue;
92+
}
93+
// `key` is a field name; `val` is its constraint.
94+
const direct = celTimeSource(val); // `field: daysFromNow(N)` → implicit equality
95+
let hit: { op: string; src: string } | null = direct ? { op: '==', src: direct } : null;
96+
if (!hit && val && typeof val === 'object' && (val as AnyRec).dialect !== 'cel') {
97+
for (const [op, operand] of Object.entries(val as AnyRec)) {
98+
if (RANGE_OPS.has(op)) continue; // correct pattern — leave it
99+
if (op === '$eq') {
100+
const s = celTimeSource(operand);
101+
if (s) { hit = { op: '$eq', src: s }; break; }
102+
} else if (op === '$in' && Array.isArray(operand)) {
103+
for (const item of operand) {
104+
const s = celTimeSource(item);
105+
if (s) { hit = { op: '$in', src: s }; break; }
106+
}
107+
if (hit) break;
108+
}
109+
}
110+
}
111+
if (hit) {
112+
findings.push({
113+
where,
114+
message:
115+
`filter matches \`${key}\` by ${hit.op} against a time value (\`${hit.src}\`) — a date field carries a ` +
116+
`time component, so exact equality against \`${hit.src}\` (re-computed each run) silently matches nothing.`,
117+
hint:
118+
`Use a one-day window instead: \`${key}: { $gte: daysFromNow(N), $lt: daysFromNow(N+1) }\` ` +
119+
`(wrap multiple tiers in \`$or\`). The abutting windows tile the timeline so each row matches exactly once. (#1874)`,
120+
rule: FLOW_DATE_EQUALITY_FILTER,
121+
});
122+
}
123+
}
124+
}
125+
51126
// Flow node VALUES interpolate with SINGLE braces (`{var}` / `{rec.field}` /
52127
// `{$User.Id}`). Two wrong-syntax mistakes AI/human authors carry over from the
53128
// *formula* template dialect (`{{ path }}`) or other platforms:
@@ -106,9 +181,34 @@ export function lintFlowPatterns(stack: AnyRec): FlowLintFinding[] {
106181
// node values use SINGLE braces; double-brace `{{ }}` and bare `$ref.x`
107182
// are carried over from the formula template dialect / other platforms.
108183
for (const node of nodes) {
184+
const nodeWhere = `flow '${flowName}' · node '${node.id}' (${node.type})`;
185+
186+
// (a2) #1874 — date-EQUALITY (`==`/`$eq`/`$in`) against a time value in a
187+
// query filter. A scheduled flow that filters this way silently matches
188+
// nothing; the robust shape is a `$gte`/`$lt` day window.
189+
const cfg = (node.config ?? {}) as AnyRec;
190+
if (cfg.filter) scanFilterForDateEquality(cfg.filter, `${nodeWhere} filter`, findings);
191+
192+
// (a3) #1870 — a node-config key naming a non-existent capability (there is
193+
// no aggregate node) is silently ignored at runtime, so the node
194+
// computes nothing. Point the author at the data-layer equivalent.
195+
for (const key of Object.keys(cfg)) {
196+
if (PHANTOM_AGG_KEYS.has(key)) {
197+
findings.push({
198+
where: nodeWhere,
199+
message:
200+
`node config has \`${key}\` — the automation engine has no aggregate node, so \`${key}\` is ` +
201+
`silently ignored and this node computes nothing at runtime.`,
202+
hint:
203+
`Aggregation belongs in the data layer: use \`Field.summary\` for a cross-object rollup ` +
204+
`(sum/count of children), or \`Field.formula\` for a per-record computed value. (#1870)`,
205+
rule: FLOW_PHANTOM_AGGREGATION,
206+
});
207+
}
208+
}
209+
109210
const strings: string[] = [];
110211
collectTemplateStrings(node.config, undefined, strings);
111-
const nodeWhere = `flow '${flowName}' · node '${node.id}' (${node.type})`;
112212
for (const str of strings) {
113213
if (DOUBLE_BRACE.test(str)) {
114214
findings.push({

0 commit comments

Comments
 (0)