diff --git a/.changeset/tidy-jars-shave.md b/.changeset/tidy-jars-shave.md new file mode 100644 index 0000000000..6efb114e5f --- /dev/null +++ b/.changeset/tidy-jars-shave.md @@ -0,0 +1,30 @@ +--- +'@objectstack/lint': minor +--- + +Warn when a bare identifier in a flow node/edge condition is shadowed by a declared flow variable + +A flow `condition` is evaluated in a flattened scope, so a bare `status` normally +resolves to the trigger record's field and is the correct, canon-taught spelling. +`objectstack validate` deliberately never judged a bare identifier there, and it +still does not — with one exception it now names. + +When the same name is BOTH a declared flow variable and a field on the bound +object, the two collide silently: a run seeds its declared variables first and +flattens the record's fields only where nothing is bound yet, so the variable +wins, the field is unreachable under its own name, and nothing anywhere reports +it. The author reads `status` and gets the variable. On this surface that is the +least visible failure there is — a flow condition that never fires produces no +record, no error and no log line. + +`validateStackExpressions` now emits a `warning` (never an error) on exactly that +case, naming the mechanism and both repairs: `record.status` for the field, or +rename the variable. A bare name that is only a field, or only a variable, stays +silent as before. + +The variable set is collected across every ADR-0031 region of the flow, since a +run holds one variable map: flow-level declarations, loop/map iterator and index +variables, the try/catch error variable, node output variables, assignment +targets in all three shapes the executor accepts (including a legacy assignment +node with no `assignments` wrapper, whose top-level config keys are the variable +names), and node ids, which are bare CEL roots at runtime. diff --git a/packages/lint/src/flow-variable-scope.test.ts b/packages/lint/src/flow-variable-scope.test.ts new file mode 100644 index 0000000000..7a027f9083 --- /dev/null +++ b/packages/lint/src/flow-variable-scope.test.ts @@ -0,0 +1,278 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { SCOPE_ROOTS } from '@objectstack/formula'; +import { + FlowSchema, + FlowVariableSchema, + FlowNodeSchema, + LoopConfigSchema, + TryCatchConfigSchema, +} from '@objectstack/spec/automation'; + +import { + collectFlowVariableNames, + shadowedFieldReads, + shadowedFieldMessage, + VARIABLE_NAME_CONFIG_KEYS, + ASSIGNMENT_ENTRY_NAME_KEYS, +} from './flow-variable-scope.js'; + +/** + * Unit coverage for the #14089 collection surface. The end-to-end behaviour + * (which conditions warn, which stay silent) lives in + * `validate-expressions.test.ts`; what is pinned HERE is the collection + * surface's completeness, row by row, because the ruling's criterion is only as + * closed as this set is. + */ +describe('collectFlowVariableNames (#14089)', () => { + const graphOf = (...nodes: Array>) => [{ nodes }]; + + it('row 1 — the flow\'s own declared variables', () => { + const names = collectFlowVariableNames( + { variables: [{ name: 'batch_size', type: 'number' }, { name: 'cursor', type: 'text' }] }, + [], + ); + expect([...names].sort()).toEqual(['batch_size', 'cursor']); + }); + + it('rows 2-6 — the four declared config keys whose VALUE is a name', () => { + const names = collectFlowVariableNames({}, graphOf( + { id: 'sweep', type: 'loop', config: { iteratorVariable: 'item_row', indexVariable: 'i' } }, + { id: 'guard', type: 'try_catch', config: { errorVariable: 'caught' } }, + { id: 'fetch', type: 'query_records', config: { outputVariable: 'rows' } }, + )); + for (const expected of ['item_row', 'i', 'caught', 'rows']) expect(names.has(expected)).toBe(true); + }); + + it('row 7 — all THREE assignment shapes, including the wrapper-less one', () => { + expect(collectFlowVariableNames({}, graphOf( + { id: 'a', type: 'assignment', config: { assignments: { total: 1 } } }, + )).has('total')).toBe(true); + + expect(collectFlowVariableNames({}, graphOf( + { id: 'a', type: 'assignment', config: { assignments: [{ variable: 'total', value: 1 }] } }, + )).has('total')).toBe(true); + + // Shape 3 — no wrapper at all. `logic-nodes.ts`'s `else` branch reads the + // config's own keys, and this is the row a hand-written collector misses. + expect(collectFlowVariableNames({}, graphOf( + { id: 'a', type: 'assignment', config: { total: 1, label: 'x' } }, + )).has('total')).toBe(true); + }); + + it('row 7 shape 2 — the `name` and `key` spellings the executor also accepts', () => { + const names = collectFlowVariableNames({}, graphOf({ + id: 'a', type: 'assignment', + config: { assignments: [{ name: 'by_name', value: 1 }, { key: 'by_key', value: 2 }] }, + })); + expect([...names].sort()).toEqual(['a', 'by_key', 'by_name']); + }); + + it('row 7 is gated on the node TYPE — a non-assignment node donates no config keys', () => { + const names = collectFlowVariableNames({}, graphOf( + { id: 'fetch', type: 'http', config: { url: 'https://example.test', method: 'GET' } }, + )); + // Only the node id (row 8). `url` / `method` are config, not variables — + // reading them would manufacture a warning on any object with a `url` field. + expect([...names]).toEqual(['fetch']); + }); + + it('row 8 — a node id is collected, because it is a bare CEL root at runtime', () => { + const names = collectFlowVariableNames({}, graphOf({ id: 'lookup_owner', type: 'query_records' })); + expect(names.has('lookup_owner')).toBe(true); + }); + + it('is FLOW-scoped: every graph contributes to one flat set', () => { + // `collectFlowGraphs` yields each ADR-0031 region as its own graph, and + // `seedRunVariables` builds ONE map per run — so a name declared inside a + // region is in scope for the whole flow, not just that region. + const names = collectFlowVariableNames({}, [ + { nodes: [{ id: 'start', type: 'start' }] }, + { nodes: [{ id: 'inner', type: 'assignment', config: { region_local: 1 } }] }, + ]); + expect(names.has('region_local')).toBe(true); + }); + + it('tolerates the shapes an unparsed source can carry', () => { + expect([...collectFlowVariableNames({}, [])]).toEqual([]); + expect([...collectFlowVariableNames({ variables: 'nonsense' }, [])]).toEqual([]); + expect([...collectFlowVariableNames({ variables: [null, 7, { type: 'text' }] }, [])]).toEqual([]); + expect([...collectFlowVariableNames({}, graphOf({ id: 'n', config: 'nonsense' }))]).toEqual(['n']); + }); + + /** + * The alias `control-flow.zod.ts` REJECTS by name. Reading it here would be + * consumer-side tolerance of a shape the schema refuses (Prime Directive #12) + * — and it cannot arrive on the parsed path this rule runs on anyway. + */ + it('does not read the rejected `itemVariable` alias', () => { + const names = collectFlowVariableNames({}, graphOf( + { id: 'sweep', type: 'loop', config: { itemVariable: 'should_not_be_collected' } }, + )); + expect(names.has('should_not_be_collected')).toBe(false); + }); +}); + +describe('shadowedFieldReads (#14089)', () => { + const vars = (...names: string[]) => new Set(names); + + it('reports a bare name that is BOTH a variable and a field', () => { + expect(shadowedFieldReads('status == "dispatched"', vars('status'), ['status', 'amount'])) + .toEqual(['status']); + }); + + it('is silent when the name is a field only', () => { + expect(shadowedFieldReads('status == "dispatched"', vars('other'), ['status'])).toEqual([]); + }); + + it('is silent when the name is a variable only', () => { + expect(shadowedFieldReads('batch_size > 0', vars('batch_size'), ['status'])).toEqual([]); + }); + + it('is silent on the dotted spelling it prescribes', () => { + expect(shadowedFieldReads('record.status == "x"', vars('status'), ['status'])).toEqual([]); + }); + + it('reports every shadowed root in one predicate, in discovery order', () => { + const found = shadowedFieldReads('status == "x" && amount > 1', vars('status', 'amount'), ['status', 'amount']); + expect(found.sort()).toEqual(['amount', 'status']); + }); + + /** + * The reason `firstUndeclaredReference` is the oracle and + * `collectCelRootIdentifiers` is not (maintainer's implementation input, item + * 6). A comprehension macro binds its own variable; an AST root scan reports + * that binder as a root, so a macro variable sharing a field's name would be + * flagged for a collision that cannot exist. The declaredness oracle acts only + * on cel-js's own `Unknown variable` fault, so it never sees the binder. + */ + it('does not flag a comprehension-macro variable that shares a field name', () => { + expect(shadowedFieldReads( + 'record.lines.exists(status, status.ok)', + vars('status'), + ['status'], + )).toEqual([]); + }); + + it('does not flag a function name that shares a field name', () => { + expect(shadowedFieldReads('size(record.lines) > 0', vars('size'), ['size'])).toEqual([]); + }); + + it('costs nothing when the two authored sets do not intersect', () => { + expect(shadowedFieldReads('anything at all', vars('a'), ['b'])).toEqual([]); + expect(shadowedFieldReads('anything at all', new Set(), ['b'])).toEqual([]); + expect(shadowedFieldReads('anything at all', vars('a'), [])).toEqual([]); + }); + + it('is empty on a source that does not parse — the syntax pass owns that defect', () => { + expect(shadowedFieldReads('status == ', vars('status'), ['status'])).toEqual([]); + }); + + /** + * ⚠️ The oracle's known, DELIBERATE blind spot, pinned so it is a recorded + * property rather than a surprise: `SCOPE_ROOTS` are declared in the strict + * environment, so a flow variable named after one of them is never reported as + * a bare root. That is an UNDER-report — the safe direction for a warning — + * and closing it means consulting the AST, which re-opens the macro-variable + * false positive above. The pin reads the real baseline rather than a copied + * word, so a future `SCOPE_ROOTS` member keeps this honest. + */ + it('under-reports a variable named after a SCOPE_ROOTS member (documented blind spot)', () => { + const root = SCOPE_ROOTS[0]; + expect(SCOPE_ROOTS.length).toBeGreaterThan(0); + expect(shadowedFieldReads(`${root} == "x"`, vars(root), [root])).toEqual([]); + }); +}); + +describe('shadowedFieldMessage (#14089)', () => { + it('names the mechanism and both repairs', () => { + const message = shadowedFieldMessage('status', 'duly_assignment'); + expect(message).toContain('`status`'); + expect(message).toContain('`duly_assignment`'); + expect(message).toContain('record.status'); + expect(message).toMatch(/rename the variable/); + }); +}); + +/** + * ── The declared-key guard for this module (#5017's pattern, #14089's surface) ── + * + * `validate-expressions.test.ts` pins that every key its rule reads off a + * metadata receiver is one `@objectstack/spec` declares. This module reads + * metadata too, so it carries the same guard rather than escaping it by living + * in a different file: the collection surface is exactly where an undeclared + * key would go unnoticed, since a key nobody declares simply collects nothing + * and the diagnostic stays silent — a green gate over a surface nothing read. + * + * It is asserted against the module's EXPORTED constants rather than a scan of + * its source text. That is the stronger of the two: a source scan pins the + * spelling someone typed, while these pin the values the collection walk + * actually indexes with — and it needs no private comment-stripper, the class + * `check:comment-mask-adoption` exists to keep out of this tree. + */ + +/** + * Declared keys of a schema, unwrapping the optional / lazy layers these + * schemas are built with. Mirrors `validate-expressions.test.ts`'s helper of the + * same name; `lazySchema` proxies a FUNCTION target, so the `typeof` guard has + * to admit both or every lazily-built schema answers "declares nothing" and the + * guard goes vacuous. + */ +function shapeKeysOf(schema: unknown, depth = 0): string[] { + const s = schema as { shape?: Record; _def?: Record; unwrap?: () => unknown }; + if (!s || (typeof s !== 'object' && typeof s !== 'function') || depth > 12) return []; + if (s.shape) return Object.keys(s.shape); + const d = (s._def ?? {}) as Record; + const getter = d.getter as (() => unknown) | undefined; + for (const next of [d.innerType, d.element, d.valueType, getter?.(), d.in, d.out]) { + const r = shapeKeysOf(next, depth + 1); + if (r.length) return r; + } + if (typeof s.unwrap === 'function') return shapeKeysOf(s.unwrap(), depth + 1); + return []; +} + +describe('flow-variable-scope reads only keys the spec declares (meta-test)', () => { + it('the flow-level keys it reads are declared by `FlowSchema` / `FlowVariableSchema`', () => { + const flowKeys = shapeKeysOf(FlowSchema); + expect(flowKeys.length, 'FlowSchema resolved to no keys — the guard would be vacuous').toBeGreaterThan(0); + expect(flowKeys).toContain('variables'); + + const variableKeys = shapeKeysOf(FlowVariableSchema); + expect(variableKeys.length, 'FlowVariableSchema resolved to no keys — the guard would be vacuous').toBeGreaterThan(0); + expect(variableKeys).toContain('name'); + }); + + it('the node keys it reads are declared by `FlowNodeSchema`', () => { + const nodeKeys = shapeKeysOf(FlowNodeSchema); + expect(nodeKeys.length, 'FlowNodeSchema resolved to no keys — the guard would be vacuous').toBeGreaterThan(0); + for (const key of ['id', 'type', 'config']) expect(nodeKeys).toContain(key); + }); + + it('the config-key list is exactly the four declared name-valued keys', () => { + expect([...VARIABLE_NAME_CONFIG_KEYS]) + .toEqual(['iteratorVariable', 'indexVariable', 'errorVariable', 'outputVariable']); + // ⛔ The alias `control-flow.zod.ts` rejects BY NAME must not be here — reading + // it would be consumer-side tolerance of a shape the schema refuses (Prime + // Directive #12), and it cannot arrive on the parsed path this rule runs on. + expect([...VARIABLE_NAME_CONFIG_KEYS]).not.toContain('itemVariable'); + // Every one of them is a key some node-config schema really declares — a + // list of keys nothing declares would collect nothing, silently. + const declaredAnywhere = new Set([ + ...shapeKeysOf(LoopConfigSchema), + ...shapeKeysOf(TryCatchConfigSchema), + ]); + expect(declaredAnywhere.size, 'the node-config schemas resolved to no keys — the guard would be vacuous').toBeGreaterThan(0); + for (const key of ['iteratorVariable', 'indexVariable']) expect(declaredAnywhere).toContain(key); + expect(declaredAnywhere).toContain('errorVariable'); + }); + + it('the assignment entry-name keys are the three the executor reads, in its order', () => { + // The node TYPE itself is module-private (see the comment on it): a + // slug-shaped `export const` in this package is read as a rule id that a + // published barrel must carry. Its gate is pinned by BEHAVIOUR above — + // `row 7 is gated on the node TYPE` — which is the property that matters. + expect([...ASSIGNMENT_ENTRY_NAME_KEYS]).toEqual(['variable', 'name', 'key']); + }); +}); diff --git a/packages/lint/src/flow-variable-scope.ts b/packages/lint/src/flow-variable-scope.ts new file mode 100644 index 0000000000..2059fdbc90 --- /dev/null +++ b/packages/lint/src/flow-variable-scope.ts @@ -0,0 +1,314 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * @module flow-variable-scope + * + * **The one sub-case of a bare identifier in a flattened flow scope that is + * genuinely ambiguous AND genuinely breaks: a name bound as BOTH a declared + * flow variable and a field on the bound object** (#14089). + * + * ## Why a bare identifier is normally correct here, and stays uncriticised + * + * A flow node/edge `condition` is evaluated in a *flattened* scope: the trigger + * record's own fields are spread to top level, so `status == "dispatched"` is + * the shape the platform's own canon teaches and the engine deliberately + * supports. `@objectstack/formula` says so in its published contract + * (`ExprSchemaHint.scope`: on flattened scope "bare `status` is correct and is + * NOT an error"; `cel-engine.ts`: the record-scope checker "must NOT be applied + * to flow / automation conditions"), and `AutomationEngine.seedRunVariables` + * says so at the other end — it flattens the record's fields "so bare + * references (`status`, `budget`) resolve in start conditions and edge + * predicates". Two shipped example apps read fields bare in exactly that way. + * + * ⛔ So this module does NOT judge a bare identifier for being bare. Rejecting + * the form would make this package refuse what the platform's own contract + * blesses — the "linter denying its own contract" shape #5378 already paid down + * inside `validate-expressions.ts` — and warning on it would fire on canon, the + * trust-killer ADR-0072 D1 names. Maintainer ruling, 2026-09-01 (director batch + * #23), verbatim and untranslated: + * + * > **C**:新诊断只命中**真正含糊且真正会坏**的子情形 —— 裸名**既**匹配已声明 + * > 流程变量**又**匹配绑定对象上的字段(遮蔽:`seedRunVariables` 先播变量、 + * > record 字段只在未绑定守卫下扁平化 ⇒ 变量赢且无处声张)。判据封闭(两边都是 + * > 被编写的元数据),零迁移,不动契约、不动 pin、不碰示例应用 + * + * ## The mechanism, measured at both ends + * + * `seedRunVariables` seeds the flow's declared variables FIRST, then flattens + * the record's fields only where nothing is bound yet: + * + * ```ts + * const variables = this.seedDeclaredVariables(flow, context); // (1) variables + * if (context?.record) { + * variables.set('record', context.record); + * for (const [k, v] of Object.entries(context.record)) { + * if (!variables.has(k)) variables.set(k, v); // (2) guarded + * } + * } + * ``` + * + * So when one name is both, **the variable wins and nothing anywhere reports + * the collision**. The author reads `status` on a flow that also declares a + * `status` variable and is reading the variable, not the field — a wrong + * predicate at the surface where a wrong predicate is least visible, because a + * flow condition that never fires produces no record, no error and no log line. + * + * The node-id row (below) shadows even harder, in `evaluateCondition`'s scope + * build: a variable keyed `"."` is expanded into a nested + * object path, and the expansion **overwrites** a non-object value already + * sitting at `` — so a flattened field named like a node id is replaced + * by an object at scope-build time. + * + * ## Severity + * + * `warning`, never `error` — whether the shadow actually bites on a given run + * depends on values the author has not written down (a declared input with no + * `defaultValue` is bound only when a param supplies one), and this diagnostic + * must not fail a build over a collision that may be intentional. + */ + +import { firstUndeclaredReference } from '@objectstack/formula'; + +type AnyRec = Record; + +/** The node shape this module reads: `collectFlowGraphs`' element type, loosened. */ +interface FlowNodeLike { + readonly id?: unknown; + readonly type?: unknown; + readonly config?: unknown; +} + +/** One graph out of `collectFlowGraphs` — every region is already its own graph. */ +export interface FlowGraphLike { + readonly nodes: readonly FlowNodeLike[]; +} + +/** The flow-level slice this module reads. */ +export interface FlowVariableHost { + readonly variables?: unknown; +} + +/** + * `config` keys whose VALUE is a variable name the runtime binds. + * + * Read off every node rather than gated on an enumerated node-type list, and + * that is the deliberate direction: the type list is what drifts silently when + * a new container arrives, while these key spellings are specific enough that a + * node carrying one and not binding a variable does not exist. Measured + * declaration → binder pairs: + * + * | key | declared by | bound at | + * |---|---|---| + * | `iteratorVariable` | `control-flow.zod.ts` (loop), `builtin-node-config.zod.ts` (map) | `loop-node.ts`, `map-node.ts` | + * | `indexVariable` | the same two | the same two | + * | `errorVariable` | `control-flow.zod.ts` (try_catch) | `try-catch-node.ts` | + * | `outputVariable` | `builtin-node-config.zod.ts`, `schemaless-node-config.zod.ts` | `crud-nodes.ts`, `screen-nodes.ts`, `map-node.ts`, `subflow-node.ts` | + * + * ⚠️ `control-flow.zod.ts` REJECTS the `itemVariable` alias by name, so on the + * parsed path only the canonical spelling can arrive — reading the alias here + * would be a consumer-side tolerance of a shape the schema refuses (Prime + * Directive #12). + */ +export const VARIABLE_NAME_CONFIG_KEYS = ['iteratorVariable', 'indexVariable', 'errorVariable', 'outputVariable'] as const; + +/** + * The node type whose `config` names variables *structurally* rather than + * through a declared key. Gated on the type on purpose: shape 3 below reads + * EVERY top-level config key as a variable name, which is correct for this node + * and catastrophic over-collection for any other (a `start` node would donate + * `objectName` and `condition`, an `http` node its `url`). + */ +/* + * ⛔ Module-private, deliberately. `rule-id-barrel-exports.test.ts` reads every + * `export const NAME = '';` in `src/` as a RULE ID that a published + * barrel must re-export — and `'assignment'` is slug-shaped. This is a node + * type, not a rule id, and putting it in the package's public surface to + * satisfy that scan would publish an internal detail nobody consumes. The + * type gate is pinned through BEHAVIOUR instead (a non-assignment node donates + * no config keys), which is the property that actually matters. + */ +const ASSIGNMENT_NODE_TYPE = 'assignment'; + +/** + * The keys an `assignments` ARRAY entry may name its target with — the three + * `logic-nodes.ts` reads, in its precedence order. Exported so the guard can + * assert the list rather than re-derive it from the source text. + */ +export const ASSIGNMENT_ENTRY_NAME_KEYS = ['variable', 'name', 'key'] as const; + +/** + * Variable names an `assignment` node binds — **three shapes**, mirroring the + * executor's own dispatch in `logic-nodes.ts` branch for branch. + * + * Shape 3 is the class a hand-written collector misses: with no `assignments` + * wrapper at all, the top-level `config` keys ARE the variable names, and the + * node config schema states that exemption deliberately (`builtin-node-config.zod.ts`). + * An implementer who looks for `assignments`, finds nothing and collects zero + * names from a legacy assignment node loses every variable that node sets — + * here that is a MISSED warning, and under any stricter rule it would have been + * a false rejection. + */ +function assignmentTargets(config: AnyRec): string[] { + const raw = config.assignments; + const out: string[] = []; + if (Array.isArray(raw)) { + // Shape 2: [{ variable | name | key, value }, …] + for (const item of raw) { + if (!item || typeof item !== 'object') continue; + const entry = item as AnyRec; + for (const nameKey of ASSIGNMENT_ENTRY_NAME_KEYS) { + const name = entry[nameKey]; + if (typeof name === 'string' && name) { out.push(name); break; } + } + } + return out; + } + if (raw && typeof raw === 'object') { + // Shape 1: { : , … } — the canonical Studio shape. + return Object.keys(raw as AnyRec); + } + // Shape 3: no wrapper — the config's own keys are the variable names. + return Object.keys(config); +} + +/** + * Every name that is bound as a flow variable at some point in this flow's run. + * + * **Flow-scoped, not graph-scoped, and that is measured**: `seedRunVariables` + * builds ONE map per run, so a name declared inside a `loop` body is in scope + * for the whole flow. The set is therefore one flat union gathered across every + * graph `collectFlowGraphs` yields — which already includes every ADR-0031 + * region slot (`loop.body`, `parallel.branches[]`, `try_catch.try/catch`). + * + * ⚠️ Consequence for the caller: this cannot be interleaved with the checking + * walk. A condition on the first node is in scope for a variable declared by + * the last one, so collection must COMPLETE before any condition is judged; + * collecting as you go would make the verdict depend on traversal order. + * + * The nine-row surface, with the two rows that are easiest to miss called out: + * + * 1. `flow.variables[].name` — `FlowVariableSchema`, seeded by `seedDeclaredVariables` + * 2-5. the four {@link VARIABLE_NAME_CONFIG_KEYS} above + * 6. node `config.outputVariable` (folded into the same list) + * 7. assignment targets — three shapes, see {@link assignmentTargets} + * 8. **node ids** — not a "variable" in any schema sense, but a bare CEL root at + * runtime: the engine writes each of a node's outputs under a variable key + * spelled "node id, dot, output key", and `evaluateCondition` expands that + * dotted key into a nested object AT the node id, overwriting whatever scalar + * was flattened there + * 9. engine-reserved handles (`record`, `previous`, the dollar-prefixed run + * handles) — deliberately NOT collected: they are `SCOPE_ROOTS` members or + * dollar-prefixed, so they can never be the bare undeclared identifier this + * module looks for, and adding them would only widen the set with names no + * object may declare as a field anyway + */ +export function collectFlowVariableNames( + flow: FlowVariableHost, + graphs: readonly FlowGraphLike[], +): ReadonlySet { + const names = new Set(); + + // Row 1 — the flow's own declarations. + const declared = flow.variables; + if (Array.isArray(declared)) { + for (const item of declared) { + if (!item || typeof item !== 'object') continue; + const flowVar = item as AnyRec; + if (typeof flowVar.name === 'string' && flowVar.name) names.add(flowVar.name); + } + } + + for (const graph of graphs) { + for (const item of graph.nodes) { + const flowNode = item as AnyRec; + // Row 8 — the node id itself. + if (typeof flowNode.id === 'string' && flowNode.id) names.add(flowNode.id); + const rawConfig = flowNode.config; + if (!rawConfig || typeof rawConfig !== 'object' || Array.isArray(rawConfig)) continue; + const config = rawConfig as AnyRec; + // Rows 2-6 — declared keys whose value is a name. + for (const key of VARIABLE_NAME_CONFIG_KEYS) { + const value = config[key]; + if (typeof value === 'string' && value) names.add(value); + } + // Row 7 — assignment targets. + if (flowNode.type === ASSIGNMENT_NODE_TYPE) { + for (const target of assignmentTargets(config)) if (target) names.add(target); + } + } + } + + return names; +} + +/** + * Upper bound on the bare-root enumeration below. A predicate reaching for more + * distinct undeclared roots than this is pathological, and the loop must + * terminate on a source the oracle keeps faulting on for a reason we did not + * anticipate. + */ +const MAX_BARE_ROOTS = 64; + +/** + * Every bare (undeclared) top-level identifier in `source`, discovered by + * re-asking the oracle with the ones already found declared. + * + * ⛔ The oracle is `firstUndeclaredReference`, **not** `collectCelRootIdentifiers`, + * and the difference is the whole false-positive budget: the former acts only on + * cel-js's own `Unknown variable` fault, so a comprehension-macro variable + * (`items.exists(x, x.n > 1)`) and a function name are never reported; the + * latter reads the AST and reports macro variables as roots, so a macro variable + * sharing a field's name would be flagged for a collision that does not exist. + * + * ⚠️ **Known, deliberate blind spot**: `SCOPE_ROOTS` are declared in the oracle's + * strict environment, so a flow variable named `result` / `data` / `item` / + * `config` (all `SCOPE_ROOTS` members) is never reported as a bare root and its + * shadow goes unwarned. That is an UNDER-report, the safe direction for a new + * warning, and it is the price of the pinned oracle — closing it would mean + * consulting the AST, which is what re-opens the macro-variable false positive. + */ +function bareRootsOf(source: string): string[] { + const found: string[] = []; + for (let i = 0; i < MAX_BARE_ROOTS; i++) { + const next = firstUndeclaredReference(source, found); + if (next === null || found.includes(next)) break; + found.push(next); + } + return found; +} + +/** + * The names in `source` that are read BARE while being bound as both a declared + * flow variable and a field on the bound object — i.e. the shadowing case, in + * discovery order. + * + * Empty (at zero CEL cost) whenever the two authored sets do not intersect, + * which is the overwhelmingly common case: a flow whose variables share no name + * with the trigger object's fields can never produce this finding, so the + * expensive half never runs. + */ +export function shadowedFieldReads( + source: string, + declaredVariables: ReadonlySet, + fieldNames: readonly string[], +): string[] { + if (declaredVariables.size === 0 || fieldNames.length === 0) return []; + const candidates = new Set(fieldNames.filter((name) => declaredVariables.has(name))); + if (candidates.size === 0) return []; + return bareRootsOf(source).filter((root) => candidates.has(root)); +} + +/** + * The diagnostic. Names the mechanism rather than only the collision, because + * the mechanism is the part the author cannot see: both spellings look + * reasonable, and nothing at author time or run time says which one won. + */ +export function shadowedFieldMessage(name: string, objectName: string): string { + return ( + `bare reference \`${name}\` is BOTH a declared flow variable and a field on \`${objectName}\` — ` + + `a flow run seeds its declared variables FIRST and flattens the record's fields only where ` + + `nothing is bound yet, so this reads the VARIABLE, the field is unreachable under its own ` + + `name, and nothing reports the collision. Write \`record.${name}\` if you meant the field, ` + + `or rename the variable if you meant the variable.` + ); +} diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index f74e4eb669..b5994c8911 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -343,6 +343,250 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { }); }); + /** + * ── #14089 — the flattened-scope SHADOWING warning ──────────────────────── + * + * Maintainer ruling 2026-09-01 (director batch #23), option C: warn ONLY when + * a bare name is BOTH a declared flow variable AND a field on the bound + * object. Both halves are authored metadata, so the criterion is closed. The + * two negative controls below are the ruling's other two branches, and they + * are load-bearing rather than decorative: a bare name that is only a field is + * the form `@objectstack/formula`'s published contract calls correct and both + * example apps ship, and a bare name that is only a variable is an ordinary + * flow-variable read. Either one warning here would be the trust-killer + * ADR-0072 D1 names — which is why options A and B were excluded. + */ + describe('flattened-scope shadowing (#14089)', () => { + const shadowFields = { status: { type: 'select' }, amount: { type: 'currency' } }; + + it('warns when a bare name is BOTH a declared flow variable and a field', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'duly_assignment', fields: shadowFields }], + flows: [{ + name: 'record_change', + variables: [{ name: 'status', type: 'text' }], + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'duly_assignment', condition: 'status == "dispatched"' } }, + ], + edges: [], + }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].severity).toBe('warning'); + expect(issues[0].where).toContain("node 'start'"); + expect(issues[0].message).toMatch(/BOTH a declared flow variable and a field on `duly_assignment`/); + // The prescription names both repairs, because either may be the intent. + expect(issues[0].message).toMatch(/Write `record\.status`/); + expect(issues[0].message).toMatch(/rename the variable/); + }); + + it('warns on the same shape on an EDGE condition', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'duly_assignment', fields: shadowFields }], + flows: [{ + name: 'record_change', + variables: [{ name: 'amount', type: 'number' }], + nodes: [{ id: 'start', type: 'start', config: { objectName: 'duly_assignment' } }], + edges: [{ id: 'e1', source: 'start', target: 'end', condition: 'amount > 100000' }], + }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].severity).toBe('warning'); + expect(issues[0].where).toContain("edge 'e1'"); + expect(issues[0].message).toMatch(/bare reference `amount`/); + }); + + // NEGATIVE CONTROL 1 — the shipped, canon-taught form. Option A would have + // turned this into a build error and option B into a warning; both excluded. + it('stays silent when the bare name is a FIELD ONLY (no such variable)', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'duly_assignment', fields: shadowFields }], + flows: [{ + name: 'record_change', + variables: [{ name: 'retry_count', type: 'number' }], + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'duly_assignment', condition: 'status == "dispatched"' } }, + ], + edges: [], + }], + }); + expect(issues).toHaveLength(0); + }); + + // NEGATIVE CONTROL 2 — an ordinary flow-variable read. + it('stays silent when the bare name is a VARIABLE ONLY (no such field)', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'duly_assignment', fields: shadowFields }], + flows: [{ + name: 'record_change', + variables: [{ name: 'batch_size', type: 'number' }], + nodes: [{ id: 'start', type: 'start', config: { objectName: 'duly_assignment' } }], + edges: [{ id: 'e1', source: 'start', target: 'end', condition: 'batch_size > 0' }], + }], + }); + expect(issues).toHaveLength(0); + }); + + // The dotted spelling is never the shadowed one — `record.status` reads the + // field whatever the variable map holds, so it must stay clean even here. + it('stays silent on the dotted spelling, which is what the warning prescribes', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'duly_assignment', fields: shadowFields }], + flows: [{ + name: 'record_change', + variables: [{ name: 'status', type: 'text' }], + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'duly_assignment', condition: 'record.status == "dispatched"' } }, + ], + edges: [], + }], + }); + expect(issues).toHaveLength(0); + }); + + /** + * ROW 7 SHAPE 3 — an `assignment` node with NO `assignments` wrapper: the + * top-level `config` keys ARE the variable names (`logic-nodes.ts`'s `else` + * branch, and the shape `engine.test.ts` pins live). A collector that only + * looks for `assignments` collects zero names from this node and the warning + * never fires. + */ + it('collects assignment targets written with NO `assignments` wrapper (row 7 shape 3)', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'duly_assignment', fields: shadowFields }], + flows: [{ + name: 'record_change', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'duly_assignment' } }, + { id: 'set', type: 'assignment', config: { status: 'approved' } }, + ], + edges: [{ id: 'e1', source: 'set', target: 'end', condition: 'status == "approved"' }], + }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].severity).toBe('warning'); + expect(issues[0].message).toMatch(/bare reference `status`/); + }); + + it('collects assignment targets from the wrapper OBJECT and ARRAY shapes too (row 7 shapes 1-2)', () => { + const objectShape = validateStackExpressions({ + objects: [{ name: 'duly_assignment', fields: shadowFields }], + flows: [{ + name: 'wrapper_object', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'duly_assignment' } }, + { id: 'set', type: 'assignment', config: { assignments: { status: 'approved' } } }, + ], + edges: [{ id: 'e1', source: 'set', target: 'end', condition: 'status == "approved"' }], + }], + }); + expect(objectShape).toHaveLength(1); + expect(objectShape[0].message).toMatch(/bare reference `status`/); + + const arrayShape = validateStackExpressions({ + objects: [{ name: 'duly_assignment', fields: shadowFields }], + flows: [{ + name: 'wrapper_array', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'duly_assignment' } }, + { id: 'set', type: 'assignment', config: { assignments: [{ variable: 'status', value: 'approved' }] } }, + ], + edges: [{ id: 'e1', source: 'set', target: 'end', condition: 'status == "approved"' }], + }], + }); + expect(arrayShape).toHaveLength(1); + expect(arrayShape[0].message).toMatch(/bare reference `status`/); + }); + + // The row-7 gate is on the node TYPE, and it has to be: shape 3 reads every + // top-level config key, so an ungated collector would donate `objectName`, + // `url`, `condition` … from every other node and manufacture warnings. + it('does NOT read a non-assignment node\'s config keys as variable names', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'duly_assignment', fields: { status: { type: 'select' }, url: { type: 'text' } } }], + flows: [{ + name: 'record_change', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'duly_assignment' } }, + { id: 'fetch', type: 'http', config: { url: 'https://example.test' } }, + ], + edges: [{ id: 'e1', source: 'fetch', target: 'end', condition: 'url != ""' }], + }], + }); + expect(issues).toHaveLength(0); + }); + + /** + * ROW 8 — a node id is a bare CEL root at runtime. The engine writes a + * node's outputs under a variable key spelled "node id, dot, output key", + * and `evaluateCondition` expands that dotted key into a nested object AT + * the node id — overwriting the scalar the record flattening put there. So a + * node id colliding with a field name shadows harder than a plain variable. + */ + it('treats a NODE ID as a bare root (row 8)', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'duly_assignment', fields: shadowFields }], + flows: [{ + name: 'record_change', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'duly_assignment' } }, + { id: 'status', type: 'query_records', config: { objectName: 'duly_assignment' } }, + ], + edges: [{ id: 'e1', source: 'status', target: 'end', condition: 'status == "dispatched"' }], + }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].severity).toBe('warning'); + expect(issues[0].message).toMatch(/bare reference `status`/); + }); + + // Variables are FLOW-scoped, not graph-scoped: `seedRunVariables` builds one + // map per run, so a name declared inside a `loop` body is in scope for a + // condition on the top-level graph. This is the case that forces collection + // to complete before any condition is judged. + it('sees a variable declared inside a region from a top-level condition', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'duly_assignment', fields: shadowFields }], + flows: [{ + name: 'record_change', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'duly_assignment' } }, + { + id: 'sweep', + type: 'loop', + config: { + collection: '{items}', + iteratorVariable: 'status', + body: { nodes: [{ id: 'inner', type: 'assignment', config: { assignments: {} } }], edges: [] }, + }, + }, + ], + edges: [{ id: 'e1', source: 'start', target: 'sweep', condition: 'status == "dispatched"' }], + }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].severity).toBe('warning'); + expect(issues[0].message).toMatch(/bare reference `status`/); + }); + + // The diagnostic never fails a build (ruling 1.5), so a stack carrying only + // this finding still lints clean at `error` severity. + it('is advisory only — never an error', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'duly_assignment', fields: shadowFields }], + flows: [{ + name: 'record_change', + variables: [{ name: 'status', type: 'text' }], + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'duly_assignment', condition: 'status == "dispatched"' } }, + ], + edges: [], + }], + }); + expect(issues.filter((i) => i.severity !== 'warning')).toEqual([]); + }); + }); + // #1928 tier 4 — a text/boolean field used with an arithmetic/ordering // operator against a number is a silent-null bug; the lint surfaces it as a // non-blocking warning, threading each object's field types into the checker. @@ -2447,6 +2691,13 @@ describe('validateStackExpressions — reads only keys the spec declares (meta-t // here would have been excused into masking a genuine // `validations[].message` read. 'verdict', 'diagnostic', + // [#14089] NOT a receiver at all — the tail of the `'./flow-variable-scope.js'` + // import specifier, which this scan cannot tell from `scope.j…`. The two + // entries above it in this set (`fields`, `guards`) are the same artefact + // of `'./system-fields.js'` / `'./validate-null-guards.js'`. Nothing in + // this file is a local named `scope`; `graph.scope` is a KEY read off the + // tabled `graph` receiver, so the metadata guard loses no coverage here. + 'scope', ]); expect(receivers.filter((r) => !tabled.has(r) && !PLUMBING.has(r))).toEqual([]); }); diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index 56f78d28bb..300918c182 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -82,6 +82,7 @@ import { validateExpression, collectCelRootIdentifiers, parseCelToAst, SCOPE_ROO import { collectFlowGraphs, resolveFlowNodeExpressions } from '@objectstack/spec/automation'; import type { FlowNodeParsed } from '@objectstack/spec/automation'; +import { collectFlowVariableNames, shadowedFieldReads, shadowedFieldMessage } from './flow-variable-scope.js'; import { injectedColumnsFor, unprovisionedInjectedColumnsFor } from './system-fields.js'; import { findUnguardedNullableOperands, nullGuardMessage } from './validate-null-guards.js'; import type { NullGuardOutcome } from './validate-null-guards.js'; @@ -1080,11 +1081,46 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { // `objectstack validate` and shipped. This is the author-time half of the // same traversal the engine's registration pass now does; `scope` names the // region so the located message still points at one edge. - for (const graph of collectFlowGraphs(flow as { nodes?: FlowNodeParsed[] })) { + const graphs = collectFlowGraphs(flow as { nodes?: FlowNodeParsed[] }); + + // [#14089] The flattened-scope shadowing pass needs the flow's COMPLETE + // variable set before any condition is judged, so it is a separate walk over + // the (already materialized) graph list rather than an interleaved one. + // That is forced, not stylistic: `seedRunVariables` builds ONE variable map + // per run, so a name declared by the LAST node is in scope for a condition + // on the first — collecting as the checking walk goes would make the verdict + // depend on traversal order. `collectFlowGraphs` is still called once. + const declaredVariables = collectFlowVariableNames(flow, graphs); + + /** + * [#14089] The one bare-identifier case a flattened flow scope may not stay + * silent about: a name bound as BOTH a declared flow variable and a field on + * the bound object. The variable wins at runtime and nothing says so, which + * is why the diagnostic exists; every other bare identifier here is the form + * the platform's own contract and canon teach, and is deliberately NOT + * judged. Severity `warning` — see `flow-variable-scope.ts` for the ruling, + * the measured mechanism, and the oracle's known blind spot. + */ + const warnShadowedFieldReads = (where: string, raw: unknown): void => { + if (!objectName) return; + const celSource = celSourceOf(raw); + if (!celSource) return; + for (const shadowed of shadowedFieldReads(celSource, declaredVariables, fieldIndex.get(objectName) ?? [])) { + issues.push({ + where, + message: shadowedFieldMessage(shadowed, objectName), + source: celSource, + severity: 'warning', + }); + } + }; + + for (const graph of graphs) { const at = graph.scope ? `flow '${flowName}' · ${graph.scope}` : `flow '${flowName}'`; for (const node of graph.nodes as unknown as AnyRec[]) { const cfg = (node.config ?? {}) as AnyRec; check(`${at} · node '${node.id}' (${node.type}) condition`, cfg.condition, objectName); + warnShadowedFieldReads(`${at} · node '${node.id}' (${node.type}) condition`, cfg.condition); // Descriptor-declared expression slots (#4027). Before this, the traversal // hardcoded `condition` and assumed every other node string was a `{var}` @@ -1162,6 +1198,7 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { } for (const edge of graph.edges as unknown as AnyRec[]) { check(`${at} · edge '${edge.id}' (${edge.source}→${edge.target}) condition`, edge.condition, objectName); + warnShadowedFieldReads(`${at} · edge '${edge.id}' (${edge.source}→${edge.target}) condition`, edge.condition); } // No `checkNullGuards` on node/edge conditions — and NOT for the reason // #4811 first recorded (#4811 re-measured it). The stated blocker was the