From ab33804e325513196635c74f602b3ecfe27aa18b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 12:03:08 +0000 Subject: [PATCH 1/2] fix(lint,cli): a filter reference that cannot resolve fails the build, not the run (#3426, #3810) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `validateFlowTemplatePaths` called every `{record.}` 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. The message states what the runtime does and why this gates rather than warns (an absent condition WIDENS the query). `create_record` is excluded: it writes a payload, no filter. - Every other position -> warning, unchanged. The token still renders a blank, the run still completes, and the head object may come from another package. Both rules split this way, so a typo and a lookup hop are gated wherever the runtime refuses them. A reference used in both positions reports once, at error. `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, which makes the same shift-left split. 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. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01EdCvGtER9SzJopS4Yh3Pa1 --- .../flow-template-filter-position-severity.md | 50 ++++++ packages/cli/src/commands/validate.ts | 33 +++- .../src/validate-flow-template-paths.test.ts | 112 ++++++++++++++ .../lint/src/validate-flow-template-paths.ts | 144 ++++++++++++++---- 4 files changed, 304 insertions(+), 35 deletions(-) create mode 100644 .changeset/flow-template-filter-position-severity.md diff --git a/.changeset/flow-template-filter-position-severity.md b/.changeset/flow-template-filter-position-severity.md new file mode 100644 index 0000000000..3e79ff1277 --- /dev/null +++ b/.changeset/flow-template-filter-position-severity.md @@ -0,0 +1,50 @@ +--- +"@objectstack/lint": minor +"@objectstack/cli": patch +--- + +fix(lint,cli): a filter reference that cannot resolve fails the build, not the run (#3426, #3810) + +`validateFlowTemplatePaths` reported every `{record.}` miss as **advisory**, +on the reasoning that an unresolved token renders a blank and the run still +completes. Since #3810 that reasoning no longer holds 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, not fewer. Those +nodes now refuse to execute rather than run a widened query. + +So the rule was warning about metadata whose runtime is 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`.** + These are the three nodes whose filter `resolveNodeFilter` guards. The finding + says what the runtime will do ("the node refuses to run at execution time") + and why the build gates rather than warns (an absent condition *widens* the + query). `os validate` exits 1. +- **Every other position → `warning`, unchanged.** A message body, an `http` + url, an `update_record` write payload: the token still renders a blank, the + run still completes, and the head object may legitimately come from another + installed package. `create_record` is deliberately excluded from the gating + set — it writes a payload and has no filter to widen. + +Both rules split this way (`flow-template-unknown-field` and +`flow-template-lookup-traversal`), so a typo and a lookup hop are gated wherever +the runtime refuses them. A reference used in both positions on one node is +reported **once, at error severity**. + +**`os validate` now enforces it.** The command filtered this rule's findings for +`severity === 'warning'` and dropped everything else on the floor, so an error +from it would have been invisible. It now gates on errors first — printing rule +id and config path, and emitting them under `errors` in `--json` — mirroring the +`validateReadonlyFlowWrites` step directly below, which makes the same +shift-left split (a certain runtime failure gates; a state-dependent one +advises). + +Verified against the shipped examples: 33 flows across app-todo, app-crm and +app-showcase produce **no new errors**; the four pre-existing lookup-traversal +warnings sit in `script` / `notify` / `subflow` / `parallel` positions and keep +their advisory severity. + +No authoring change is required for a correct filter. A filter that this rule +now fails is one the runtime would have refused anyway — the difference is that +you find out at `os validate` instead of at 3am. diff --git a/packages/cli/src/commands/validate.ts b/packages/cli/src/commands/validate.ts index 44c582de30..720217874c 100644 --- a/packages/cli/src/commands/validate.ts +++ b/packages/cli/src/commands/validate.ts @@ -518,12 +518,39 @@ export default class Validate extends Command { // 3g-bis. Flow template path references (#3426): a `{record.}` token // in a node template that names an unknown field, or hops through a // lookup relation the seeded record carries only as a scalar id, both - // render a SILENT empty string at runtime. Advisory: the head object - // may come from another package (skipped there), and the runtime still - // produces output (a blank), so nothing is fully broken. + // render a SILENT empty string at runtime. Advisory in most positions: + // the head object may come from another package (skipped there), and + // the runtime still produces output (a blank), so nothing is fully + // broken. GATING inside a filter-guarded CRUD node's `filter`: since + // #3810 an unresolved token there does not blank a value, it deletes + // the condition — which WIDENS the query — so the node refuses to + // execute. The build must not ship a flow whose runtime is already + // decided. if (!flags.json) printStep('Checking flow template references...'); const flowTemplateFindings = validateFlowTemplatePaths(normalized as Record); + const flowTemplateErrors = flowTemplateFindings.filter((f) => f.severity === 'error'); const flowTemplateWarnings = flowTemplateFindings.filter((f) => f.severity === 'warning'); + if (flowTemplateErrors.length > 0) { + if (flags.json) { + await emitJson({ + valid: false, + errors: flowTemplateErrors, + warnings: flowTemplateWarnings, + duration: timer.elapsed(), + }); + this.exit(1); + } + console.log(''); + printError( + `Flow template reference check failed (${flowTemplateErrors.length} issue${flowTemplateErrors.length > 1 ? 's' : ''})`, + ); + for (const f of flowTemplateErrors.slice(0, 50)) { + console.log(` • ${f.where}: ${f.message}`); + console.log(chalk.dim(` ${f.hint}`)); + console.log(chalk.dim(` rule: ${f.rule} at ${f.path}`)); + } + this.exit(1); + } if (!flags.json) { for (const w of flowTemplateWarnings.slice(0, 50)) { console.log(chalk.yellow(` ⚠ ${w.where}: ${w.message}`)); diff --git a/packages/lint/src/validate-flow-template-paths.test.ts b/packages/lint/src/validate-flow-template-paths.test.ts index b133b9416c..7fd673d56c 100644 --- a/packages/lint/src/validate-flow-template-paths.test.ts +++ b/packages/lint/src/validate-flow-template-paths.test.ts @@ -232,4 +232,116 @@ describe('validateFlowTemplatePaths', () => { it('returns empty when there are no flows', () => { expect(validateFlowTemplatePaths({ objects: [LEAD_OBJECT] })).toHaveLength(0); }); + + // ── severity follows position (#3810) ─────────────────────────────────── + // Outside a filter an unresolved token blanks a value (advisory); inside a + // filter-guarded CRUD node's filter it DELETES the condition, so the node + // refuses to run — the build must not ship it. + describe('filter-position severity (#3810)', () => { + /** A record-change flow with one CRUD node carrying the given config. */ + function crudFlow(type: string, config: AnyRec): AnyRec { + return { + objects: [LEAD_OBJECT], + flows: [ + { + name: 'crud_flow', + type: 'record_change', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_lead', triggerType: 'record-created' } }, + { id: 'c1', type, config }, + ], + }, + ], + }; + } + + it.each(['get_record', 'update_record', 'delete_record'])( + 'raises an unknown field inside a %s filter to error', + (type) => { + const findings = validateFlowTemplatePaths( + crudFlow(type, { objectName: 'crm_lead', filter: { company: '{record.compnay}' } }), + ); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(FLOW_TEMPLATE_UNKNOWN_FIELD); + expect(findings[0].severity).toBe('error'); + expect(findings[0].message).toContain('refuses to run'); + }, + ); + + it('raises a lookup traversal inside a filter to error', () => { + const findings = validateFlowTemplatePaths( + crudFlow('delete_record', { objectName: 'crm_lead', filter: { company: '{record.crm_account.name}' } }), + ); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(FLOW_TEMPLATE_LOOKUP_TRAVERSAL); + expect(findings[0].severity).toBe('error'); + }); + + it('keeps a bad reference OUTSIDE the filter advisory on the same node', () => { + const findings = validateFlowTemplatePaths( + crudFlow('update_record', { + objectName: 'crm_lead', + filter: { company: '{record.company}' }, + fields: { last_name: '{record.compnay}' }, + }), + ); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('warning'); + }); + + it('reports a reference used in BOTH positions once, at error severity', () => { + const findings = validateFlowTemplatePaths( + crudFlow('update_record', { + objectName: 'crm_lead', + filter: { company: '{record.compnay}' }, + fields: { last_name: 'echo {record.compnay}' }, + }), + ); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('error'); + }); + + it('stays advisory for create_record, which has no filter to widen', () => { + const findings = validateFlowTemplatePaths( + crudFlow('create_record', { objectName: 'crm_lead', filter: { company: '{record.compnay}' } }), + ); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('warning'); + }); + + it('does NOT flag a valid filter reference', () => { + const findings = validateFlowTemplatePaths( + crudFlow('delete_record', { + objectName: 'crm_lead', + filter: { company: '{record.company}', owner: '{record.owner}' }, + }), + ); + expect(findings).toHaveLength(0); + }); + + it('respects declared expand for a filter traversal (#3475)', () => { + const findings = validateFlowTemplatePaths({ + objects: [LEAD_OBJECT], + flows: [ + { + name: 'expanded_filter', + type: 'record_change', + nodes: [ + { + id: 'start', + type: 'start', + config: { objectName: 'crm_lead', triggerType: 'record-created', expand: ['crm_account'] }, + }, + { + id: 'c1', + type: 'get_record', + config: { objectName: 'crm_lead', filter: { company: '{record.crm_account.name}' } }, + }, + ], + }, + ], + }); + expect(findings).toHaveLength(0); + }); + }); }); diff --git a/packages/lint/src/validate-flow-template-paths.ts b/packages/lint/src/validate-flow-template-paths.ts index 52e841c42c..a83525df94 100644 --- a/packages/lint/src/validate-flow-template-paths.ts +++ b/packages/lint/src/validate-flow-template-paths.ts @@ -20,17 +20,27 @@ // and yields '' silently. Not resolved today; tracked on #3426. // // A pure `(stack) => Finding[]` rule (ADR-0019), run from `os validate` and -// reusable by AI authoring. Both findings are warnings: the runtime still -// produces output (a blank), nothing is fully broken, and the head object may -// legitimately come from another installed package (skipped — see below). +// reusable by AI authoring. // -// One position is no longer merely blank at run time: inside a CRUD node's -// `config.filter`, an unresolved token used to DELETE the condition from the -// query, which widens it — `delete_record` with its only condition gone matched -// every row. Since framework#3810 those nodes refuse to execute instead. This -// rule still earns its place there: catching the typo at build time beats a -// failed run, and it is the only signal for the other config blocks, where the -// blank-output behaviour is unchanged. +// SEVERITY FOLLOWS THE RUNTIME CONSEQUENCE, which differs by POSITION: +// +// - Everywhere else (a message body, an http url, a write payload) an +// unresolved token renders a blank. The output is wrong but the run +// completes, and the head object may legitimately come from another +// installed package (skipped — see below). Advisory: WARNING. +// +// - Inside a filter-guarded CRUD node's `filter`, an unresolved token used to +// DELETE the condition from the query, and a removed condition matches MORE +// rows — `delete_record` with its only condition gone matched every row. +// Since framework#3810 those nodes REFUSE TO EXECUTE. So a finding here is +// not "the output will be blank", it is "this node cannot run": the build +// is shipping a flow whose runtime is already decided. Gating: ERROR. +// +// The split is the same shift-left `validateReadonlyFlowWrites` makes — a +// certain runtime failure gates the build, a state-dependent one advises — and +// it keeps this rule honest about what it found. Catching the typo at build +// time beats a failed run at 3am; catching it and calling it advisory, when the +// runtime has already committed to refusing, understates it. // // Deliberately conservative to keep false positives near zero: // - Only `record.`-prefixed tokens are checked. Other `{var}` tokens address @@ -105,6 +115,17 @@ const RELATION_TYPES: ReadonlySet = new Set([ 'tree', ]); +// The CRUD nodes whose `filter` the runtime guards (framework#3810): each calls +// `resolveNodeFilter`, which refuses the node when interpolation erased any +// authored condition. `create_record` is deliberately absent — it writes a +// payload and has no filter, so an unresolved token there is a blank value on +// the new row, not a widened query. +const FILTER_GUARDED_NODE_TYPES: ReadonlySet = new Set([ + 'get_record', + 'update_record', + 'delete_record', +]); + /** Build a `fieldName -> type` map for an object (declared fields only). */ function fieldTypesOf(obj: AnyRec): Map { const types = new Map(); @@ -178,6 +199,49 @@ const NODE_CONFIG_KEYS = [ 'start', ]; +/** A templated string leaf plus the one thing severity depends on: where it sits. */ +interface TemplateLeaf { + text: string; + /** Inside a filter-guarded CRUD node's `filter` — an unresolved token there is refused at runtime. */ + inFilter: boolean; +} + +/** + * Collect a node's templated string leaves, tagging those that sit under a + * `filter` key when the node type is one the runtime guards. + * + * `guarded` leaves are returned FIRST so the per-node dedupe below resolves a + * reference that appears in both positions at its higher severity: one typo + * used in a filter and echoed in a message is an error, not a warning. + */ +function collectNodeLeaves(node: AnyRec, guarded: boolean): TemplateLeaf[] { + const filterLeaves: TemplateLeaf[] = []; + const otherLeaves: TemplateLeaf[] = []; + + for (const key of NODE_CONFIG_KEYS) { + if (!(key in node)) continue; + const block = node[key]; + const splitFilter = guarded && !!block && typeof block === 'object' && !Array.isArray(block); + + if (splitFilter) { + const { filter, ...rest } = block as AnyRec; + const inFilter: string[] = []; + stringLeaves(filter, inFilter); + for (const text of inFilter) filterLeaves.push({ text, inFilter: true }); + const outside: string[] = []; + stringLeaves(rest, outside); + for (const text of outside) otherLeaves.push({ text, inFilter: false }); + continue; + } + + const plain: string[] = []; + stringLeaves(block, plain); + for (const text of plain) otherLeaves.push({ text, inFilter: false }); + } + + return [...filterLeaves, ...otherLeaves]; +} + /** True when the flow is armed by a record lifecycle event. */ function isRecordTriggered(flow: AnyRec, startConfig: AnyRec): boolean { if (flow.type === 'record_change') return true; @@ -249,11 +313,11 @@ export function validateFlowTemplatePaths(stack: AnyRec): FlowTemplatePathFindin const nodeLabel = typeof node.type === 'string' ? node.type : typeof node.id === 'string' ? node.id : `#${nodeIndex}`; - // Collect templated string leaves from the config-bearing blocks only. - const leaves: string[] = []; - for (const key of NODE_CONFIG_KEYS) { - if (key in node) stringLeaves((node as AnyRec)[key], leaves); - } + // Collect templated string leaves from the config-bearing blocks only, + // tagging filter positions when this node type guards its filter (#3810). + const nodeType = typeof node.type === 'string' ? node.type : ''; + const guarded = FILTER_GUARDED_NODE_TYPES.has(nodeType); + const leaves = collectNodeLeaves(node as AnyRec, guarded); if (leaves.length === 0) return; // Dedupe references so one repeated typo yields one finding per node. @@ -261,7 +325,8 @@ export function validateFlowTemplatePaths(stack: AnyRec): FlowTemplatePathFindin const seenTraversal = new Set(); for (const leaf of leaves) { - for (const rest of recordRefsIn(leaf)) { + const inFilter = leaf.inFilter; + for (const rest of recordRefsIn(leaf.text)) { const head = rest[0]; const hasSubPath = rest.length > 1; // A trailing numeric segment is an array index (#1872), not a hop. @@ -273,16 +338,23 @@ export function validateFlowTemplatePaths(stack: AnyRec): FlowTemplatePathFindin if (seenUnknown.has(head)) continue; seenUnknown.add(head); findings.push({ - severity: 'warning', + severity: inFilter ? 'error' : 'warning', rule: FLOW_TEMPLATE_UNKNOWN_FIELD, where: `flow "${flowName}" node "${nodeLabel}"`, path: `flows[${flowIndex}].nodes[${nodeIndex}]`, - message: - `template references '{record.${rest.join('.')}}', but '${head}' is not a field on ` + - `object '${objectName}' — it resolves to an empty string at runtime (silently).`, - hint: - `Check the field name against the object's field definitions (e.g. '{record.full_name}', ` + - `not '{record.full_naem}'). System columns like id/created_at/owner are also addressable.`, + message: inFilter + ? `${nodeType} filter references '{record.${rest.join('.')}}', but '${head}' is not a field on ` + + `object '${objectName}' — the token resolves to nothing, which DROPS the condition from the ` + + `query instead of narrowing it. The node refuses to run at execution time (#3810).` + : `template references '{record.${rest.join('.')}}', but '${head}' is not a field on ` + + `object '${objectName}' — it resolves to an empty string at runtime (silently).`, + hint: inFilter + ? `Check the field name against the object's field definitions (e.g. '{record.full_name}', ` + + `not '{record.full_naem}'); system columns like id/created_at/owner are also addressable. ` + + `This gates the build rather than warning: an absent condition WIDENS the query, so the ` + + `runtime has already decided to refuse this node.` + : `Check the field name against the object's field definitions (e.g. '{record.full_name}', ` + + `not '{record.full_naem}'). System columns like id/created_at/owner are also addressable.`, }); continue; } @@ -294,18 +366,26 @@ export function validateFlowTemplatePaths(stack: AnyRec): FlowTemplatePathFindin if (seenTraversal.has(key)) continue; seenTraversal.add(key); findings.push({ - severity: 'warning', + severity: inFilter ? 'error' : 'warning', rule: FLOW_TEMPLATE_LOOKUP_TRAVERSAL, where: `flow "${flowName}" node "${nodeLabel}"`, path: `flows[${flowIndex}].nodes[${nodeIndex}]`, - message: - `template references '{record.${key}}', a cross-object hop through the ${headType} field ` + - `'${head}' — the flow record carries '${head}' as a scalar id, not an expanded object, so ` + - `this resolves to an empty string at runtime (silently).`, - hint: - `Opt in to resolve it: add '${head}' to the start node's config.expand (#3475) and the ` + - `engine re-reads it as the run's identity. Otherwise reference the foreign-key id directly ` + - `('{record.${head}}'), or project the value via a formula field on '${objectName}'.`, + message: inFilter + ? `${nodeType} filter references '{record.${key}}', a cross-object hop through the ` + + `${headType} field '${head}' — the flow record carries '${head}' as a scalar id, not an ` + + `expanded object, so the token resolves to nothing and the condition is DROPPED from the ` + + `query instead of narrowing it. The node refuses to run at execution time (#3810).` + : `template references '{record.${key}}', a cross-object hop through the ${headType} field ` + + `'${head}' — the flow record carries '${head}' as a scalar id, not an expanded object, so ` + + `this resolves to an empty string at runtime (silently).`, + hint: inFilter + ? `Opt in to resolve it: add '${head}' to the start node's config.expand (#3475) and the ` + + `engine re-reads it as the run's identity. Otherwise filter on the foreign-key id directly ` + + `('{record.${head}}'), or project the value via a formula field on '${objectName}'. This ` + + `gates the build rather than warning: an absent condition WIDENS the query.` + : `Opt in to resolve it: add '${head}' to the start node's config.expand (#3475) and the ` + + `engine re-reads it as the run's identity. Otherwise reference the foreign-key id directly ` + + `('{record.${head}}'), or project the value via a formula field on '${objectName}'.`, }); } // STRUCTURED_TYPES + any other scalar `.sub` access is left alone: From a09042101ad48198ab570037dfbe6ef1ca72df0a Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 12:07:00 +0000 Subject: [PATCH 2/2] docs(skills): the filter-token guard now has a build-time half MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The query skill's filters rule described the #3810 runtime refusal as the only signal. Two of its three causes — a mistyped field and an un-expanded lookup hop — are statically decidable, and `objectstack validate` now fails on them in a filter position rather than warning. Records which half catches what, so an author reading the rule knows a filter typo is a build failure, the same reference outside a filter is still only a warning, and an unresolved flow variable remains run-time-only (it is not statically checkable). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01EdCvGtER9SzJopS4Yh3Pa1 --- skills/objectstack-query/rules/filters.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/skills/objectstack-query/rules/filters.md b/skills/objectstack-query/rules/filters.md index 1c59687a78..08c6b428a5 100644 --- a/skills/objectstack-query/rules/filters.md +++ b/skills/objectstack-query/rules/filters.md @@ -273,6 +273,14 @@ query. That covers a mistyped field (`{record.ownr}`), an input the run never received, and a lookup hop (`{record.account.name}` — the trigger record carries a scalar id; add the relation to the start node's `config.expand`). +Two of those three are caught earlier: `objectstack validate` **fails** on a +`{record.}` filter token naming an unknown field, or hopping through a +relation the start node does not `expand`, because the runtime has already +committed to refusing that node. The same reference outside a filter — in a +message body, an `http` url, a write payload — stays a warning, since there it +renders a blank rather than widening a query. An unresolved *flow variable* +(`{someInput}`) is not statically checkable and still surfaces at run time. + **Unknown tokens are rejected, not ignored.** `{current_user}` (the RLS expression root) and `{this_quarter_start}` are near-misses, not tokens: `objectstack build` fails on them and the runtime resolver throws. A filter