diff --git a/.changeset/30025-flow-bare-name-screen-open-binder.md b/.changeset/30025-flow-bare-name-screen-open-binder.md new file mode 100644 index 00000000000..8cff0cf1bbf --- /dev/null +++ b/.changeset/30025-flow-bare-name-screen-open-binder.md @@ -0,0 +1,13 @@ +--- +'@objectstack/formula': patch +'@objectstack/lint': patch +--- + +fix(lint): a flow condition's bare-name type verdict no longer refuses a name a `screen` field holds, and stands down to the warning after a node that can bind any name (#30025) + +Clause-②: yes (widening) — the author-time gate (`os build`, `os validate`, the package publish and the flow publish door) no longer refuses, on a record-triggered flow (or a time-relative sweep of its own object), a `type-mismatch-run-fails` on a BARE condition name that a `screen` node's declared field holds (no verdict now), nor on a bare name in a flow with a node that can bind a name no reader sees (`script`, `connector_action`, `wait`, `subflow`, `map`, a screen with no input contract), which is now the `type-mismatch-run-may-fail` warning. A `record.` / `previous.` read keeps its refusal, and nothing new is refused. The null-guard verdict's bare names also leave a screen's field names out. Added beside them: the optional `bareNamesUnproven` key of `@objectstack/formula`'s `ExprSchemaHint`, and `collectScreenFieldNames` in `@objectstack/lint`'s flow-variable-scope module. +Metadata-protocol: none +Skills-docs: updated here — `content/docs/data-modeling/formulas.mdx` (the type-soundness row names the screen field and the open binder); `skills/**` and the MCP skill text teach no bare flow-condition name a screen or an open binder holds, so no skill or eval moves. + +- **Before.** A record-triggered flow whose `screen` node collects `title` (a number) and whose edge after it reads `title > 5`, with `title` a text field on the trigger object, was refused `type-mismatch-run-fails`: the bare name was typed as the field. The run binds the collected number there (a resume lands a screen's values under their plain names), so the condition runs. The same refusal hit a bare name after a `script`, `connector_action`, `wait`, `subflow` or `map` node, or a screen with no input contract, any of which can bind the name to a value nothing can see at build. +- **Now.** A screen's declared field names join the names a bare read holds instead of the field, so they are not typed as it (the null-guard verdict leaves them out too). In a flow with a node that can bind any name, a bare-name mismatch is the `type-mismatch-run-may-fail` warning, as the null-guard verdict already stands its bare names down there; `record.` is still refused. The stand-down reads the flow's nodes only: an entrance (and the publish door's per-write snapshot, which sees none) changes which record both the bare name and `record.` read, not whether a node rebinds the bare name, so a bare-name refusal on a record-triggered flow with no such node is unchanged at every door. diff --git a/content/docs/data-modeling/formulas.mdx b/content/docs/data-modeling/formulas.mdx index 7caa84b50a5..2118d79bb00 100644 --- a/content/docs/data-modeling/formulas.mdx +++ b/content/docs/data-modeling/formulas.mdx @@ -242,7 +242,7 @@ come at two severities: **errors** fail the build; **warnings** are advisory | **Qualify field references** — a formula / validation / predicate binds only the `record` namespace, so a bare `amount` resolves to nothing and evaluates to `null`. Write `record.amount`. | `amount > 100` → `record.amount > 100` | error | | **Field must exist** — a typo'd `record.` is flagged with a did-you-mean. | `record.amont` → `record.amount` | error | | **Unknown function** — a misspelled or nonexistent stdlib call. | `isBlnk(record.x)` | error | -| **Type soundness** — a text or boolean field used with an arithmetic (`+ - * / %`) or ordering (`< > <= >=`) operator against a number faults at runtime unless the text is numeric. In a formula it evaluates to `null`. Where a fault does something else it is an error, worded for what happens: a validation rule, a field `requiredWhen` / `readonlyWhen`, an option `visibleWhen` or a hook `condition` refuses the write; a record-triggered flow's start, edge or `decision` branch condition fails the run (the write that fired it still lands), whether it reads the field bare or as `record.`, and so does a time-relative flow's that sweeps the same object; a bare name that a declared flow variable holds reads the variable, so it is not judged as the field; an advisory (`severity: 'warning'` / `'info'`) validation rule is skipped, so it never gives its advisory. Store it as a number field, compare text with text, or convert explicitly (`size(record.name)` for the length; `double(record.name)`, which itself faults on text that is not a number). | `record.title * 2`; `record.name > 5` in a validation rule → `size(record.name) > 5` | warning in a formula or a display predicate, and in a condition of any other flow, where the warning says the run fails if the condition faults (what such a flow binds under the name depends on how it is started); error at the sites named | +| **Type soundness** — a text or boolean field used with an arithmetic (`+ - * / %`) or ordering (`< > <= >=`) operator against a number faults at runtime unless the text is numeric. In a formula it evaluates to `null`. Where a fault does something else it is an error, worded for what happens: a validation rule, a field `requiredWhen` / `readonlyWhen`, an option `visibleWhen` or a hook `condition` refuses the write; a record-triggered flow's start, edge or `decision` branch condition fails the run (the write that fired it still lands), whether it reads the field bare or as `record.`, and so does a time-relative flow's that sweeps the same object; a bare name that a declared flow variable or a `screen` field holds reads that value, so it is not judged as the field, and a bare name after a node that can bind any name (`script`, `connector_action`, `wait`, `subflow`, `map`, a screen with no fields) gets the warning instead, while its `record.` read is still refused; an advisory (`severity: 'warning'` / `'info'`) validation rule is skipped, so it never gives its advisory. Store it as a number field, compare text with text, or convert explicitly (`size(record.name)` for the length; `double(record.name)`, which itself faults on text that is not a number). | `record.title * 2`; `record.name > 5` in a validation rule → `size(record.name) > 5` | warning in a formula or a display predicate, and in a condition of any other flow, where the warning says the run fails if the condition faults (what such a flow binds under the name depends on how it is started); error at the sites named | | **Date arithmetic** — a `date` / `datetime` field used with `+` / `-` against a number (a `YYYY-MM-DD` value is a string at runtime, so this always faulted to `null`). Use the date helpers instead. | `record.end_date - record.start_date + 1` → `daysBetween(record.start_date, record.end_date) + 1`; `today() + 30` → `daysFromNow(30)`; `record.date + n` → `addDays(record.date, n)` | error (#3306) | | **Date subtraction** — the runtime holds a `date` / `datetime` field as its text (`YYYY-MM-DD`, or an ISO-8601 instant), so subtracting two of them faults and the formula reads `null`; a timestamp minus one computes a duration, which a field reads as `{}`. Use `daysBetween`. A condition may still compare a duration (`today() - record.start_date > duration("72h")`). | `record.end_date - record.start_date` → `daysBetween(record.start_date, record.end_date)`; `today() - record.start_date` → `daysBetween(record.start_date, today())` | error | diff --git a/packages/formula/src/validate.test.ts b/packages/formula/src/validate.test.ts index a74d27435d6..4f662fec708 100644 --- a/packages/formula/src/validate.test.ts +++ b/packages/formula/src/validate.test.ts @@ -851,6 +851,39 @@ describe('validateExpression (ADR-0032)', () => { .toEqual(['type-mismatch']); }); + // [#30025] Where the run can bind a bare name no reader sees (an open + // binder node, a contract-less screen), a bare-name mismatch is not proven + // to be the field's: it takes no declared outcome and is the run-may-fail + // warning. A `record.` read still is the field and keeps the error, + // also when a bare mismatch comes first in the same source. + it('drops a bare-name mismatch to the run-may-fail warning where bare names are unproven, keeping record. refused (#30025)', () => { + const flat = { ...schema, scope: 'flattened' as const, faultOutcome: 'run-fails' as const, bareNamesUnproven: true }; + for (const source of ['name > 5', 'is_active > 0', 'name * 2 > 1']) { + const r = validateExpression('predicate', source, flat); + expect(r.errors, source).toEqual([]); + expect(r.warnings.map((w) => w.code), source).toEqual(['type-mismatch-run-may-fail']); + expect(r.warnings[0].message, source).toContain('which this check cannot prove here'); + } + for (const [source, reference] of [ + ['record.name > 5', 'record.name'], + ['previous.is_active > 0', 'previous.is_active'], + ['name > 5 && record.title > 5', 'record.title'], + ] as const) { + const r = validateExpression('predicate', source, flat); + expect(r.warnings, source).toEqual([]); + expect(r.errors.map((e) => [e.code, (e.params as { reference?: string }).reference]), source) + .toEqual([['type-mismatch-run-fails', reference]]); + } + // Controls: the key absent is the error as before; a bare name the run + // binds as a variable is still untyped; a `record`-scoped site ignores it. + expect(validateExpression('predicate', 'name > 5', { ...flat, bareNamesUnproven: undefined }).errors.map((e) => e.code)) + .toEqual(['type-mismatch-run-fails']); + const shadowed = validateExpression('predicate', 'name > 5', { ...flat, flowVariables: ['name'] }); + expect([shadowed.errors, shadowed.warnings]).toEqual([[], []]); + expect(validateExpression('predicate', 'record.name > 5', { ...schema, faultOutcome: 'fail-closed' as const, bareNamesUnproven: true }).errors.map((e) => e.code)) + .toEqual(['type-mismatch-fail-closed']); + }); + // [#29959] With no declared outcome a flattened condition is a flow's // that is not proven to bind the field: a warning, but one saying the run // fails on the fault, since a flow condition never reads null. diff --git a/packages/formula/src/validate.ts b/packages/formula/src/validate.ts index c76a0d4cdb2..d8b2c3b8571 100644 --- a/packages/formula/src/validate.ts +++ b/packages/formula/src/validate.ts @@ -267,6 +267,18 @@ export interface ExprSchemaHint { * typed. Absent ⇒ every bare field name is typed, exactly as before this key. */ flowVariables?: readonly string[]; + /** + * [#30025] `'flattened'` scope only: the flow's run can bind a name no static + * reader sees — a node that hands out the live variable map or folds a + * resume bag in (`script`, `connector_action`, `wait`, `subflow`, `map`, a + * screen with no input contract; `@objectstack/lint`'s `flowCelRootScope` + * over the flow's nodes, `provable: false`). A bare name + * is then not proven to hold the field, so a bare-name `type-mismatch` takes + * no declared {@link faultOutcome}: it is the `type-mismatch-run-may-fail` + * warning. A `record.` / `previous.` read still reads the field + * and keeps the outcome. Absent ⇒ every verdict is exactly as before this key. + */ + bareNamesUnproven?: boolean; } /** @@ -414,6 +426,7 @@ function typeSoundnessIssue( role: 'predicate' | 'value', faultOutcome: ExprSchemaHint['faultOutcome'], flowVariables?: readonly string[], + bareNamesUnproven?: boolean, ): { issue: ExprValidationError; severity: 'error' | 'warning' } | null { const celFieldTypes = toCelFieldTypes(fieldTypes); // [#29959] A bare name a flow variable holds is not the field (see @@ -421,7 +434,21 @@ function typeSoundnessIssue( const shadowed = scope === 'flattened' && flowVariables && flowVariables.length > 0 ? new Set(flowVariables) : undefined; - const mismatch = firstTypeMismatch(source, celFieldTypes, scope, shadowed); + let mismatch = firstTypeMismatch(source, celFieldTypes, scope, shadowed); + // [#30025] Where the run can bind a bare name no reader sees + // (`ExprSchemaHint.bareNamesUnproven`), a bare-name mismatch is not proven to + // be the field's, so it takes no declared outcome. A `record.` / + // `previous.` mismatch in the same source still does: with every bare + // name left `dyn`, only the record-root pass can answer, and its answer wins. + let outcome = faultOutcome; + if ( + mismatch && scope === 'flattened' && bareNamesUnproven && faultOutcome !== undefined + && mismatch.category === 'type-mismatch' && mismatch.root === undefined + ) { + const rootRead = firstTypeMismatch(source, celFieldTypes, scope, new Set(Object.keys(celFieldTypes))); + if (rootRead && rootRead.root !== undefined) mismatch = rootRead; + else outcome = undefined; + } if (!mismatch) { const bareCelTypes = shadowed ? Object.fromEntries(Object.entries(celFieldTypes).filter(([name]) => !shadowed.has(name))) @@ -472,10 +499,10 @@ function typeSoundnessIssue( `type mismatch \`${mismatch.operands}\` — ${subject} but is used with \`${mismatch.operator}\` ` + `against a number. CEL has no such operation, so wherever the value is not numeric text ` + `(an empty value included) the expression faults`; - if (faultOutcome !== undefined) { + if (outcome !== undefined) { // [#29926] One code per outcome: a code names ONE message template // (`expression-refusal.ts`), and the three outcomes are three sentences. - const arm = TYPE_MISMATCH_FAULT_ARMS[faultOutcome]; + const arm = TYPE_MISMATCH_FAULT_ARMS[outcome]; return { severity: 'error', issue: refusal(arm.code, params, source, `${faults}, and here ${arm.clause} ${convert}`), @@ -1405,7 +1432,8 @@ export function validateExpression( // date-field arithmetic (error). Flow variables stay `dyn` (never flagged); // equality/ordering of a date is runtime-safe (never flagged). if (schema.fieldTypes) { - const r = typeSoundnessIssue(source, schema.fieldTypes, 'flattened', role, schema.faultOutcome, schema.flowVariables); + const r = typeSoundnessIssue(source, schema.fieldTypes, 'flattened', role, schema.faultOutcome, schema.flowVariables, + schema.bareNamesUnproven); if (r) (r.severity === 'error' ? errors : warnings).push(r.issue); } } diff --git a/packages/lint/src/flow-variable-scope.ts b/packages/lint/src/flow-variable-scope.ts index 431b2463f09..e30efcccf10 100644 --- a/packages/lint/src/flow-variable-scope.ts +++ b/packages/lint/src/flow-variable-scope.ts @@ -131,6 +131,9 @@ export const VARIABLE_NAME_CONFIG_KEYS = ['iteratorVariable', 'indexVariable', ' */ const ASSIGNMENT_NODE_TYPE = 'assignment'; +/** The node type whose resume binds its declared fields' names ({@link collectScreenFieldNames}). */ +const SCREEN_NODE_TYPE = 'screen'; + /** * 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 @@ -248,6 +251,36 @@ export function collectFlowVariableNames( return names; } +/** + * [#30025] Every `screen` node's field `name`, across every graph of the flow: + * the names a resume binds. A screen's collected inputs land under their plain + * names (`applyResumeSignal`, the resume bag's `variables`), over whatever the + * run flattened there, so downstream of the screen a bare read of one holds the + * collected value, not the record's field. {@link collectFlowVariableNames} + * does not collect them (its consumers predate the screen row), so a caller + * that needs every name a bare read can hold instead of the field unions both. + * + * Only the names a screen DECLARES: a screen with no input contract folds any + * bag, which no reader can enumerate — `flowCelRootScope` reports such a flow + * as not provable, and that is the caller's other half. + */ +export function collectScreenFieldNames(graphs: readonly FlowGraphLike[]): ReadonlySet { + const names = new Set(); + for (const graph of graphs) { + for (const flowNode of recordsOf(graph.nodes)) { + if (flowNode.type !== SCREEN_NODE_TYPE) continue; + const rawConfig = flowNode.config; + if (!rawConfig || typeof rawConfig !== 'object' || Array.isArray(rawConfig)) continue; + const fields = (rawConfig as AnyRec).fields; + if (!Array.isArray(fields)) continue; + for (const field of recordsOf(fields)) { + if (typeof field.name === 'string' && field.name) names.add(field.name); + } + } + } + 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 diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index 9a3712d413f..c471e4892e0 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -1187,6 +1187,83 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { ]); }); + // [#30025] A screen's resume lands its collected values under their plain + // names (`applyResumeSignal`), over the flattened field: a bare `title` + // downstream reads the NUMBER the screen collected, and `title > 5` runs. + // Where a node can bind a name no reader sees (`script` here, a screen with + // no input contract), a bare name is not proven to be the field: the verdict + // is the run-may-fail warning. `record.title` is the field either way. + describe('a bare name a screen or an open binder can hold (#30025)', () => { + const rtFlow = (name: string, mid: Record): Record => ({ + name, + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_opportunity', triggerType: 'record-after-update' } }, + mid, + { id: 'route', type: 'decision', config: { conditions: [{ label: 'Big', expression: 'title > 5' }, { label: 'Long', expression: 'record.title > 5' }] } }, + { id: 'end', type: 'end' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'mid' }, + { id: 'e2', source: 'mid', target: 'route', condition: 'title > 5' }, + { id: 'e3', source: 'route', target: 'end', label: 'Big' }, + { id: 'e4', source: 'route', target: 'end', label: 'Long' }, + ], + }); + const objects = [{ name: 'crm_opportunity', fields: { title: { type: 'text', required: true } } }]; + const longBranch = (flow: string): string => + `flow '${flow}' · node 'route' (decision) decision branch expression at config.conditions[1].expression`; + + it('does not type a bare name a screen field holds as the field it shadows', () => { + const issues = validateStackExpressions({ + objects, + flows: [rtFlow('rt_screen', { id: 'mid', type: 'screen', config: { fields: [{ name: 'title', type: 'number', label: 'Title' }] } })], + }); + const found = typeMismatches(issues); + expect(found.map(i => [i.where, i.severity])).toEqual([[longBranch('rt_screen'), 'error']]); + expect(found[0]!.message).toContain('`record.title` holds text'); + }); + + it('warns, does not refuse, a bare-name mismatch after a node that can bind any name', () => { + for (const [flow, mid] of [ + ['rt_script', { id: 'mid', type: 'script', config: { function: 'calc' } }], + ['rt_formscreen', { id: 'mid', type: 'screen', config: { kind: 'object-form', objectName: 'crm_opportunity' } }], + ] as const) { + const issues = validateStackExpressions({ objects, flows: [rtFlow(flow, mid)] }); + const found = typeMismatches(issues); + expect(found.map(i => [i.where, i.severity]), flow).toEqual([ + [`flow '${flow}' · node 'route' (decision) decision branch expression at config.conditions[0].expression`, 'warning'], + [longBranch(flow), 'error'], + [`flow '${flow}' · edge 'e2' (mid→route) condition`, 'warning'], + ]); + for (const i of found.filter(f => f.severity === 'warning')) { + expect(i.message, `${flow} ${i.where}`).toContain('which this check cannot prove here, so this is a warning, not a refusal'); + } + } + }); + + it('leaves the null-guard twin silent on a bare name a screen field holds', () => { + const flow = (name: string, fields: unknown[]) => ({ + name, + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_opportunity', triggerType: 'record-after-update' } }, + { id: 'mid', type: 'screen', config: { fields } }, + { id: 'end', type: 'end' }, + ], + edges: [{ id: 'e1', source: 'start', target: 'mid' }, { id: 'e2', source: 'mid', target: 'end', condition: 'budget > 100' }], + }); + const nullable = (issues: ReturnType) => + issues.filter(i => /declares as nullable/.test(i.message)).map(i => [i.where, i.severity]); + const budgetObjects = [{ name: 'crm_opportunity', fields: { budget: { type: 'number' } } }]; + expect(nullable(validateStackExpressions({ + objects: budgetObjects, flows: [flow('ng_screen', [{ name: 'budget', type: 'number', label: 'Budget' }])], + }))).toEqual([]); + // Control: a screen collecting another name leaves `budget` the field, refused. + expect(nullable(validateStackExpressions({ + objects: budgetObjects, flows: [flow('ng_other', [{ name: 'note', type: 'text', label: 'Note' }])], + }))).toEqual([["flow 'ng_other' · edge 'e2' (mid→end) condition", 'error']]); + }); + }); + it('keeps the warning on an inert node condition of a record-triggered flow (#29926)', () => { const issues = validateStackExpressions({ objects: [{ name: 'crm_opportunity', fields: { title: { type: 'text', required: true } } }], @@ -4128,10 +4205,14 @@ describe('validateStackExpressions — reads only keys the spec declares (meta-t // tabled receivers (`rule.severity`) — never read off metadata itself. 'site', // [#29959] A `decision` branch's flow-condition hint (`objectName`, - // `faultOutcome`, `flowVariables`), built at the call site from the + // `faultOutcome`, `flowBare`), built at the call site from the // walk's own `objectName`, the ledger's answer and the flow's bound // names — never read off metadata itself. 'flowCondition', + // [#30025] One flow's bare-name facts (`variables`, `unproven`), built + // from the flow's collected names and the root judge's `provable`, and + // its screen-field name set (Set methods) — never read off metadata. + 'flowBare', 'screenFields', ]); 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 8c6be82b2a3..ae84329646e 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -141,7 +141,7 @@ import { isStoredMetadataBodyObject, STORED_METADATA_BODY_PRESCRIPTION } from '@ // off the schema at first use, never restated here. import { EvalUserSchema } from '@objectstack/spec/identity'; -import { collectFlowVariableNames, shadowedFieldReads, shadowedFieldMessage } from './flow-variable-scope.js'; +import { collectFlowVariableNames, collectScreenFieldNames, shadowedFieldReads, shadowedFieldMessage } from './flow-variable-scope.js'; import { flowCelEntrances, flowCelRootScope, @@ -530,6 +530,18 @@ function isAlwaysValuedSummary(def: AnyRec): boolean { */ const FLOW_ENGINE_BOUND_NAMES: ReadonlySet = new Set(['record', 'previous', 'vars', 'current_user']); +/** + * [#29959, #30025] What one flow's bare condition names can hold instead of + * the trigger record's field, read once per flow and handed to every flow + * condition site's type verdict (see `check()`'s `flowBare`). + */ +interface FlowBareNames { + /** The names the run binds over the flattening: never typed as the field. */ + readonly variables: readonly string[]; + /** The run can bind a name no reader sees: a bare-name mismatch is the warning. */ + readonly unproven: boolean; +} + function buildNullableFieldIndex(objects: AnyRec[]): Map> { const idx = new Map>(); for (const obj of objects) { @@ -1303,7 +1315,8 @@ function isFlattenedNearMissFor(diagnostic: string, roots: readonly string[]): b * | flow start / edge condition, `decision` branch — record-triggered flow | `evaluateCondition` throws (ADR-0032 §1c); the trigger isolates the write: the run fails, the write lands. A bare name and its `record.` read are the same total trigger record (#29959) | `run-fails` | * | the same, time-relative flow sweeping the flow's own object | `evaluateCondition` throws; `time-relative-trigger.ts` launches each run with the swept row of that object as `record` (`{ record: row }`, the stored row a find returns), so the field's type is what the name holds there too (#29959) | `run-fails` | * | flow condition, any other flow (API, schedule, no trigger, or a sweep of another object than the one the conditions are typed against) | `evaluateCondition` throws there too, but no row of the typed object is bound (see `validate-null-guards.ts`'s ledger): the run holds what its entrance handed it — an API request body, an action's partial record, a parent's record, or nothing — so the field's type is not proven to be what the name holds | — not declared: the `type-mismatch-run-may-fail` WARNING, worded for the run (#29959) | - * | a bare name the flow's run binds as a variable (a declared variable, a node id) | the run seeds it before the flattening, which skips a bound name: it holds the variable, not the field | — not typed as the field at all (`ExprSchemaHint.flowVariables`); `record.` still is (#29959) | + * | a bare name the flow's run binds as a variable (a declared variable, a node id, a `screen` field's `name`) | the run seeds it before the flattening, which skips a bound name, or a screen's resume lands it over the flattened field: it holds the variable, not the field | — not typed as the field at all (`ExprSchemaHint.flowVariables`); `record.` still is (#29959, #30025) | + | a bare name in a flow a node of which can bind names no reader sees (`flowCelRootScope` read with no entrance: an open binder — `script`, `connector_action`, `wait`, `subflow`, `map` —, a screen with no input contract) | the run can bind the name to a value no reader sees | — no declared outcome for a bare-name mismatch: the `type-mismatch-run-may-fail` WARNING (`ExprSchemaHint.bareNamesUnproven`); `record.` keeps the row's outcome (#30025) | * | a flow node's `condition` other than the top-level start node's | inert (`flow-inert-node-condition`) | — the same warning | * | field `expression` (formula) | reads null | — the warning's text is true | * | field `visibleWhen` / `settledWhen`, `conditionalFormatting`, field group `visibleWhen`, action `visible` / `disabled` / `pressed`, app landing `visible` | evaluated by the renderer: shows, hides or styles, never refuses a write | — | @@ -2478,12 +2491,17 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression */ faultOutcome?: ExprFaultOutcome, /** - * [#29959] A flow site's names its run binds as variables (declared - * variables, node ids, the engine's own), handed to the shared validator - * as `ExprSchemaHint.flowVariables`, so a bare read of one is not typed as - * the field it shadows. Absent, the call is byte for byte what it was. + * [#29959, #30025] What a flow site's bare names can hold instead of the + * field: `variables`, the names its run binds as variables (declared + * variables, node ids, screen fields, the engine's own), handed to the + * shared validator as `ExprSchemaHint.flowVariables`, so a bare read of one + * is not typed as the field it shadows; `unproven`, a node of the flow can + * bind a name no reader sees (`flowCelRootScope` with no entrance), handed on as + * `ExprSchemaHint.bareNamesUnproven`, so a bare-name mismatch is the + * run-may-fail warning, not the declared outcome. Absent, the call is byte + * for byte what it was. */ - flowVariables?: readonly string[], + flowBare?: FlowBareNames, ): void => { if (raw == null) return; // Absent unless this is a served-row site on an object that declares a @@ -2501,7 +2519,8 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression ? { objectName, fields, attachedOnRead, fieldTypes, scope, traversalHydration, ...roots, ...(faultOutcome ? { faultOutcome } : {}), - ...(flowVariables ? { flowVariables } : {}), + ...(flowBare ? { flowVariables: flowBare.variables } : {}), + ...(flowBare?.unproven ? { bareNamesUnproven: true } : {}), } : { scope, ...roots }); for (const e of res.errors) { @@ -2586,7 +2605,7 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression flowCondition?: { readonly objectName: string | undefined; readonly faultOutcome: ExprFaultOutcome | undefined; - readonly flowVariables: readonly string[]; + readonly flowBare: FlowBareNames; }, ): { refused: boolean } => { // [#19961] No `raw == null` early return: whether an absent value is a @@ -2615,7 +2634,7 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression if (flowCondition) { check( where, raw, flowCondition.objectName, undefined, undefined, undefined, undefined, undefined, - flowCondition.faultOutcome, flowCondition.flowVariables, + flowCondition.faultOutcome, flowCondition.flowBare, ); return { refused: false }; } @@ -2816,11 +2835,29 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression // [#29959] The names the run binds over the flattening — its declared // variables and node ids, and the engine's own — which a bare read holds // instead of the field: the type check never types them as the field. - const flowVariables = [...new Set([...declaredVariables, ...FLOW_ENGINE_BOUND_NAMES])]; + // [#30025] A screen's declared field names too: a resume lands the + // collected values under their plain names, over the flattened field. And + // where a NODE of the flow can bind a name no reader sees (an open binder + // such as `script` / `connector_action` / `wait` / `subflow` / `map`, or a + // screen with no input contract), a bare name is not proven to be the field + // at all: the type verdict stands down to the run-may-fail warning on it. + // The judge is `flowCelRootScope`'s own, read with no entrance: an entrance + // whose record is not in hand (and the runtime publish gate's per-write + // snapshot, which sees none) changes which record `record.` and the + // bare name both read, not whether a node rebinds the bare name, so it + // moves neither verdict here — while the null-guard twin keeps standing + // its bare names down on the whole `rootScope`, as before. + const screenFields = collectScreenFieldNames(graphs); + const flowBare: FlowBareNames = { + variables: [...new Set([...declaredVariables, ...screenFields, ...FLOW_ENGINE_BOUND_NAMES])], + unproven: !flowCelRootScope(flow, graphs, objectName, fieldIndex).provable, + }; const nullGuardBare = new Set(); if (nullGuardObject && rootScope.provable) { for (const name of nullableIndex.get(nullGuardObject) ?? []) { - if (!declaredVariables.has(name) && !FLOW_ENGINE_BOUND_NAMES.has(name)) nullGuardBare.add(name); + if (!declaredVariables.has(name) && !screenFields.has(name) && !FLOW_ENGINE_BOUND_NAMES.has(name)) { + nullGuardBare.add(name); + } } } const startConditionSource = nullGuardObject ? celSourceOf(startCfg.condition) : undefined; @@ -2955,7 +2992,7 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression const isTriggerGate = node.type === 'start' && graph.path.length === 0; check( nodeCondWhere, cfg.condition, objectName, undefined, undefined, undefined, undefined, undefined, - isTriggerGate ? flowFaultOutcome : undefined, flowVariables, + isTriggerGate ? flowFaultOutcome : undefined, flowBare, ); warnShadowedFieldReads(nodeCondWhere, cfg.condition); refuseUnboundRoots(nodeCondWhere, cfg.condition, from, 'predicate'); @@ -3031,7 +3068,7 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression // and a value that is not CEL text has no source to resolve. // [#29959] A `decision` branch is evaluated like an edge, so it is // judged like one: the object's field types and the flow's outcome. - const branchCondition = nodeType === 'decision' ? { objectName, faultOutcome: flowFaultOutcome, flowVariables } : undefined; + const branchCondition = nodeType === 'decision' ? { objectName, faultOutcome: flowFaultOutcome, flowBare } : undefined; if (checkDeclaredPredicate(slotWhere, found.value, branchCondition).refused) continue; // [#14288] The shadowing warning is about the SCOPE an expression is // evaluated in, not about which key it was authored under — so it @@ -3193,7 +3230,7 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression const from = issues.length; check( edgeCondWhere, edge.condition, objectName, undefined, undefined, undefined, undefined, undefined, - flowFaultOutcome, flowVariables, + flowFaultOutcome, flowBare, ); warnShadowedFieldReads(edgeCondWhere, edge.condition); refuseUnboundRoots(edgeCondWhere, edge.condition, from, 'predicate'); diff --git a/packages/lint/src/validate-null-guards.ts b/packages/lint/src/validate-null-guards.ts index 691ee209e83..2bc1ae6bf53 100644 --- a/packages/lint/src/validate-null-guards.ts +++ b/packages/lint/src/validate-null-guards.ts @@ -259,9 +259,14 @@ * included, wherever no variable holds the name. So a bare name is * judged as `record.` when the flow's bound set is provable * (`flowCelRootScope`) and the name is not one of its declared - * variables or node ids (`collectFlowVariableNames`) — those win, and - * a flow whose nodes can bind names nobody declared leaves its bare - * names unjudged. Guarding either spelling guards both. + * variables or node ids (`collectFlowVariableNames`) or a screen's + * field names (`collectScreenFieldNames`, which a resume binds) — + * those win, and a flow whose nodes can bind names nobody declared + * leaves its bare names unjudged. The type verdict reads the same names + * and the same judge, on the flow's nodes alone + * (`validate-expressions.ts`, `flowBare`): there a bare-name mismatch + * stands down to a warning instead (#30025). Guarding either spelling + * guards both. * 3. **The start condition gates every run.** `execute()` evaluates it * before any node, whatever the entrance, so what it proves non-null * is presumed at every edge and `decision` branch of the flow diff --git a/packages/objectql/src/publish-type-mismatch-fail-closed.test.ts b/packages/objectql/src/publish-type-mismatch-fail-closed.test.ts index 0a63c50621b..c6fe15b6aa8 100644 --- a/packages/objectql/src/publish-type-mismatch-fail-closed.test.ts +++ b/packages/objectql/src/publish-type-mismatch-fail-closed.test.ts @@ -365,3 +365,54 @@ describe('publish gives every flow condition a type-mismatch verdict that says w expect((el.advisories ?? []).filter((a: any) => /type mismatch/.test(a.message))).toEqual([]); }); }); + +/** + * [#30025] Through the same real door: a bare name a screen collects is the + * collected value downstream of the screen (a resume lands it under its plain + * name), so it is not typed as the field; a bare name after a node that can + * bind any name (`script`) is not proven to be the field, so its mismatch is + * the run-may-fail warning. `record.name` is the field in both, and refused. + */ +describe('publish stands a bare flow-condition name down where a screen or an open binder can hold it (#30025)', () => { + const RECORD_TRIGGERED = { triggerType: 'record-after-update' }; + const through = (mid: Record, edgeCondition: string) => ({ + nodes: [mid, { id: 'end', type: 'end', label: 'End' }], + edges: [ + { id: 'e1', source: 'start', target: 'mid' }, + { id: 'e2', source: 'mid', target: 'end', condition: edgeCondition }, + ], + }); + const SCREEN = { id: 'mid', type: 'screen', label: 'Ask', config: { fields: [{ name: 'name', type: 'number', label: 'Count' }] } }; + const SCRIPT = { id: 'mid', type: 'script', label: 'Calc', config: { function: 'calc' } }; + + it('publishes a bare `name > 5` a screen field holds, with no type verdict', async () => { + const { nodes, edges } = through(SCREEN, 'name > 5'); + const raw = await publishLeadFlow(RECORD_TRIGGERED, nodes, edges); + expect(raw.outcome, JSON.stringify(raw)).toBe('published'); + const el = raw.published.find((e: any) => e.name === 'edu_lead_flow'); + expect((el.advisories ?? []).filter((a: any) => /type mismatch/.test(a.message))).toEqual([]); + }); + + it('publishes a bare `name > 5` after a script node with the run-may-fail warning', async () => { + const { nodes, edges } = through(SCRIPT, 'name > 5'); + const raw = await publishLeadFlow(RECORD_TRIGGERED, nodes, edges); + expect(raw.outcome, JSON.stringify(raw)).toBe('published'); + const el = raw.published.find((e: any) => e.name === 'edu_lead_flow'); + const advisories = (el.advisories ?? []).filter((a: any) => a.rule === 'expression-invalid' && /type mismatch/.test(a.message)); + expect(advisories, `advisories: ${JSON.stringify(el.advisories)}`).toHaveLength(1); + expect(advisories[0].severity).toBe('warning'); + expect(advisories[0].message).toMatch(/which this check cannot prove here, so this is a warning, not a refusal/); + }); + + it('still refuses `record.name > 5` after either node', async () => { + for (const mid of [SCREEN, SCRIPT]) { + const { nodes, edges } = through(mid, 'record.name > 5'); + const raw = await publishLeadFlow(RECORD_TRIGGERED, nodes, edges); + expect(raw.outcome, `${mid.type}: ${JSON.stringify(raw)}`).toBe('refused'); + const issue = flowTypeIssue(raw); + expect(issue, `${mid.type} failed: ${JSON.stringify(raw.failed)}`).toBeDefined(); + expect(issue.severity).toBe('error'); + expect(issue.message).toMatch(/`record\.name` holds text/); + } + }); +});