From 19b5c0b5dfedf72d330636e11d00115a4169183f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 11:21:42 +0000 Subject: [PATCH] fix(lint): extend the flattened-scope shadowing warning to the descriptor-declared predicate slots (#14288) The #14089 shadowing warning ran on the node `condition` and the edge `condition` only. The #4027 descriptor-declared predicate slots were checked for dialect by the same traversal but never passed through the shadowing pass, so a bare name that is BOTH a declared flow variable AND a field on the bound object stayed silent there for exactly the reason it was silent on `condition` before #14089. Measured on the engine before extending the lint, because "same scope" is the whole premise: `seedRunVariables` builds ONE map per run and it threads unchanged into every node executor. `decision.conditions[].expression` evaluates against that same Map object; `screen.fields[].visibleWhen` evaluates against the persisted snapshot of it with the submitted bag overlaid (a superset, and the overlay can never restore the displaced field). One more `warnShadowedFieldReads` call site reusing the `declaredVariables` set already collected once per flow. Warning-only: no new rule id, no severity above `warning`, no accept set moved, no bare identifier judged for being bare. Co-Authored-By: Claude Code Claude-Session: https://claude.ai/code/session_01WLJQhde67SeTccsmnBVarV --- ...adow-warning-descriptor-predicate-slots.md | 35 ++++ .../lint/src/validate-expressions.test.ts | 151 ++++++++++++++++++ packages/lint/src/validate-expressions.ts | 34 +++- 3 files changed, 216 insertions(+), 4 deletions(-) create mode 100644 .changeset/lint-shadow-warning-descriptor-predicate-slots.md diff --git a/.changeset/lint-shadow-warning-descriptor-predicate-slots.md b/.changeset/lint-shadow-warning-descriptor-predicate-slots.md new file mode 100644 index 0000000000..f219519758 --- /dev/null +++ b/.changeset/lint-shadow-warning-descriptor-predicate-slots.md @@ -0,0 +1,35 @@ +--- +"@objectstack/lint": patch +--- + +fix(lint): run the flattened-scope shadowing warning on the descriptor-declared predicate slots too (#14288) + +The #14089 shadowing warning — a bare name that is BOTH a declared flow +variable AND a field on the bound object, where the variable silently wins at +runtime — reached exactly two expression positions: the node `condition` and +the edge `condition`. The `#4027` descriptor-declared predicate slots were +validated for dialect by the same traversal but were never passed through the +shadowing pass, so the identical mistake stayed silent on them. + +The warning is about the **scope** an expression is evaluated in, not the key +it was authored under, and the engine measurement says both `predicate` slots +on the ledger share the run's one flattened variable map: + +- `decision.conditions[].expression` — the decision executor evaluates against + the very `variables` parameter the engine hands every node executor, which is + the same `Map` object `seedRunVariables` built and a node `condition` is + judged against. Nothing on the path clones or narrows it. +- `screen.fields[].visibleWhen` — `refuseInvalidScreenInput` evaluates against + `run.variables` (the persisted snapshot of that same seeded map) with the + submitted bag overlaid. A superset, so the shadow still reaches it: the + overlay carries the screen's own collected values, never the bound record's + field, so it can never hand back a field the variable displaced. + +`loop.collection` and `map.collection` are `flow-template`, not `predicate`, +and the slot loop already skips them. + +Warning-only and within the 2026-09-01 option-C ruling's letter: one more call +site reusing the `declaredVariables` set already collected once per flow, no +new rule id, no severity above `warning`, no accept set moved, and no bare +identifier judged for being bare. Nothing that linted clean before can newly +fail a build. diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index b5994c8911..7220bb515a 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -585,6 +585,157 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { }); expect(issues.filter((i) => i.severity !== 'warning')).toEqual([]); }); + + /** + * ── #14288 — the same warning on the #4027 descriptor-declared slots ───── + * + * The warning is about the SCOPE, not the key it was authored under, and + * the engine measurement says both `predicate` slots on the ledger share + * the run's ONE flattened variable map: + * + * • `decision.conditions[].expression` — the decision executor + * (`builtin/logic-nodes.ts`) evaluates against the very `variables` + * parameter the engine hands every node executor, which is the same Map + * object `seedRunVariables` built and a node `condition` is judged + * against. Nothing on the path clones or narrows it. + * • `screen.fields[].visibleWhen` — `refuseInvalidScreenInput` evaluates + * against `run.variables` (the persisted snapshot of that same seeded + * map) with the submitted bag overlaid. A SUPERSET, so the shadow still + * reaches it; the overlay is the screen's own collected values and can + * never hand back the bound-object field the variable displaced. + * + * `loop.collection` / `map.collection` are `flow-template`, not + * `predicate`, and the slot loop skips them before this pass — so they are + * deliberately absent here. + */ + describe('reaches the descriptor-declared predicate slots (#14288)', () => { + // ── slot kind 1: screen field visibleWhen ── + it('warns on a screen field `visibleWhen` reading a shadowed bare name', () => { + 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' } }, + { + id: 'ask', + type: 'screen', + config: { fields: [{ name: 'note', type: 'text', visibleWhen: 'status == "dispatched"' }] }, + }, + ], + edges: [], + }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].severity).toBe('warning'); + expect(issues[0].where).toContain("node 'ask'"); + // The located slot, indexed into the repeater — same label the #4027 + // dialect check reports, so both findings point at one place. + expect(issues[0].where).toContain('screen field visibleWhen'); + expect(issues[0].where).toContain('config.fields[0].visibleWhen'); + expect(issues[0].message).toMatch(/BOTH a declared flow variable and a field on `duly_assignment`/); + }); + + // NEGATIVE CONTROL — a screen predicate reading a name that is only a + // FIELD. This is the canon-taught form; warning here would be exactly the + // over-reach options A and B were excluded for. + it('stays silent on a screen `visibleWhen` whose bare name is a FIELD ONLY', () => { + 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' } }, + { + id: 'ask', + type: 'screen', + config: { fields: [{ name: 'note', type: 'text', visibleWhen: 'status == "dispatched"' }] }, + }, + ], + edges: [], + }], + }); + expect(issues).toHaveLength(0); + }); + + // ── slot kind 2: decision branch expression ── + it('warns on a decision branch expression reading a shadowed bare name', () => { + 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' } }, + { + id: 'check', + type: 'decision', + config: { conditions: [{ label: 'Large', expression: 'amount > 100000' }] }, + }, + ], + edges: [], + }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].severity).toBe('warning'); + expect(issues[0].where).toContain("node 'check'"); + expect(issues[0].where).toContain('decision branch expression'); + expect(issues[0].where).toContain('config.conditions[0].expression'); + expect(issues[0].message).toMatch(/bare reference `amount`/); + }); + + // NEGATIVE CONTROL — an ordinary flow-variable read on the same slot. + it('stays silent on a decision expression whose bare name is a VARIABLE ONLY', () => { + 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' } }, + { + id: 'check', + type: 'decision', + config: { conditions: [{ label: 'Large', expression: 'batch_size > 0' }] }, + }, + ], + edges: [], + }], + }); + expect(issues).toHaveLength(0); + }); + + // Option C's severity floor holds on the new call site too: the slots + // gain a warning and nothing else. A `toHaveLength(1)` above would still + // pass if that one issue were an error, so this asserts it separately. + it('is advisory only on the descriptor slots — never an error', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'duly_assignment', fields: shadowFields }], + flows: [{ + name: 'record_change', + variables: [{ name: 'status', type: 'text' }, { name: 'amount', type: 'number' }], + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'duly_assignment' } }, + { + id: 'ask', + type: 'screen', + config: { fields: [{ name: 'note', type: 'text', visibleWhen: 'status == "dispatched"' }] }, + }, + { + id: 'check', + type: 'decision', + config: { conditions: [{ label: 'Large', expression: 'amount > 100000' }] }, + }, + ], + edges: [], + }], + }); + expect(issues).toHaveLength(2); + expect(issues.filter((i) => i.severity !== 'warning')).toEqual([]); + }); + }); }); // #1928 tier 4 — a text/boolean field used with an arithmetic/ordering diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index 300918c182..47a90b2603 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -1136,10 +1136,36 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { const nodeType = typeof node.type === 'string' ? node.type : ''; for (const found of resolveFlowNodeExpressions(nodeType, cfg)) { if (found.entry.role !== 'predicate') continue; - checkDeclaredPredicate( - `${at} · node '${node.id}' (${nodeType}) ${found.entry.label} at config.${found.path}`, - found.value, - ); + const slotWhere = `${at} · node '${node.id}' (${nodeType}) ${found.entry.label} at config.${found.path}`; + checkDeclaredPredicate(slotWhere, found.value); + // [#14288] The shadowing warning is about the SCOPE an expression is + // evaluated in, not about which key it was authored under — so it + // belongs on every `predicate` slot the ledger declares, not just the + // two hardcoded `condition` keys #14089 reached. Measured on the + // engine before it was extended here, because "same scope" is the + // whole premise and a narrower per-node scope would have made this + // call site a false positive: + // + // • `seedRunVariables` builds ONE map per run (`engine.ts`, declared + // variables first, then the record's fields only where nothing is + // bound yet) and it threads UNCHANGED into every node executor — + // `execute()` seeds it, `executeNode` passes it down, and + // `executor.execute(node, variables, context)` hands that same Map + // object over. No clone, no narrowing, anywhere on the path. + // • `decision.conditions[].expression` (`builtin/logic-nodes.ts`) + // evaluates against that very parameter — literally the same Map + // a node `condition` is judged against. + // • `screen.fields[].visibleWhen` (`refuseInvalidScreenInput`) + // evaluates against `run.variables` — the persisted snapshot of + // the same seeded map — with the SUBMITTED bag overlaid. A + // superset, so the shadow still reaches it: the overlay carries + // the screen's own collected values, never the bound record's + // field, so it can never hand back a field the variable displaced. + // + // Same `declaredVariables` set, same severity, no new rule id: this + // moves no accept set and judges no bare identifier for being bare + // (the 2026-09-01 option-C ruling's letter). + warnShadowedFieldReads(slotWhere, found.value); } // #1870 — a `script` node must name a callable, and since #4343 that is // the whole of what the node does: `config.function`. A node without one