diff --git a/.changeset/29959-flow-condition-type-mismatch-every-site.md b/.changeset/29959-flow-condition-type-mismatch-every-site.md new file mode 100644 index 00000000000..a228bec2162 --- /dev/null +++ b/.changeset/29959-flow-condition-type-mismatch-every-site.md @@ -0,0 +1,26 @@ +--- +'@objectstack/formula': minor +'@objectstack/lint': minor +'@objectstack/mcp': minor +'@objectstack/spec': minor +--- + +fix(lint)!: every flow condition's text-vs-number comparison gets a verdict that says what the run does — a record-triggered flow's `record.` read and `decision` branch are refused like its bare name, a time-relative sweep of the flow's object is refused like a record-triggered flow, a bare name a flow variable holds is no longer typed as the field, and any other flow's warning says the run fails instead of "evaluates to null" (#29959) + +Clause-②: yes (narrowing) — the author-time gate (`os build`, `os validate`, the package publish and the flow publish door) refuses, on a record-triggered flow, a `type-mismatch` in a `decision` branch and in a `record.` / `previous.` read of a start or edge condition, which it passed with no verdict before; refuses the same on a time-relative flow sweeping the object its conditions are typed against, where 18.3.0 served a warning; and a flow condition's `type-mismatch` warning is served under its own code `type-mismatch-run-may-fail` instead of `type-mismatch`. Added beside them (the widening half): the optional `severity` and `triggerKind` inputs of the `validate_expression` MCP tool, the code `type-mismatch-run-may-fail` in `@objectstack/formula`'s `ExpressionRefusalCode`, the optional `flowVariables` key of `@objectstack/formula`'s `ExprSchemaHint`, and a bare flow-condition name that a declared flow variable or node id holds is no longer typed as the field it shadows (no verdict where 18.3.0 served a `type-mismatch` warning). +Metadata-protocol: narrowed +Skills-docs: updated here — `content/docs/data-modeling/formulas.mdx` (the type-soundness row names the decision branch, the `record.` read and the warning on other flows) and the MCP skill text in `packages/mcp/src/skill-md.ts` (the two new `validate_expression` inputs); `skills/**` teaches no text-vs-number comparison in a flow condition (grep for `type mismatch`, `evaluates to null`, `validate_expression` and the decision examples), so no skill or eval moves. + + + +**BREAKING** accept-set narrowing at the author-time expression gate, served by `os build`, `os validate` and `POST /api/v1/packages/:id/publish-drafts`. + +- **Record-triggered flows — before.** The start and edge conditions refused `name > 5` on a text field (`type-mismatch-run-fails`), but `record.name > 5` in the same slot, and `name > 5` in a `decision` node's `config.conditions[].expression`, published with no verdict at all. Both fail the run the same way: the engine's `evaluateCondition` throws on the fault, the run binds the trigger record both as `record` and flattened to bare names, and a decision branch is evaluated over the same variables as an edge. +- **Now.** Both are refused as `expression-invalid` at `severity: error`, code `type-mismatch-run-fails`, with the message the bare name already gets: the run fails at the condition and the triggering write still lands. `previous.` is judged the same way. The date verdicts stay on the bare name only, as before. +- **Time-relative flows.** A sweep launches each run with the swept row as its record, so on a flow whose `timeRelative.object` is the start node's `objectName` (or is omitted) the start, edge and `decision` conditions get the record-triggered verdict above, bare name included: `type-mismatch-run-fails`, where 18.3.0 served a `type-mismatch` warning. A sweep of another object keeps the warning below. +- **Any other flow** (no `triggerType`, a `schedule` flow, an `api` flow) and an inert node `condition` keep a warning. Its text said the expression "silently evaluates to null", which is false on every flow: a condition that faults fails the run. It is now `type-mismatch-run-may-fail`, worded for the failed run, and a warning because such a flow is handed whatever its entrance carries (an API request body, an action's partial record, a parent flow's record, or nothing), so the field's type is not proven to be what the name holds. A `record.` read and a `decision` branch there get the same warning. The flow registration log line reads the same text. +- **A bare name a flow variable holds.** The run seeds a flow's declared variables (and each node's outputs, under its id) before it flattens the record, and the flattening skips a name already bound. So in a flow that declares `variables: [{ name: 'title', type: 'number' }]` over a text field `title`, a bare `title > 5` reads the number and runs; it was refused as `type-mismatch-run-fails` on the head that added that error, and warned on 18.3.0. It now gets no type verdict (the `flow-variable-shadows-field` warning still names the shadowing). `record.title > 5` reads the field and stays refused. +- **`validate_expression` (MCP).** Two optional inputs, so the tool answers what the gate will: `severity` (`error` | `warning` | `info`, site `validation`; default `error`) gives an advisory rule the gate's `type-mismatch-fail-open` instead of the fail-closed verdict, and `triggerKind` (`record_change` | `time_relative` | `schedule` | `api` | `none`, site `flow_condition`; default `record_change`) gives a `schedule`, `api` or `none` flow the gate's warning instead of the run-fails error (`time_relative` answers run-fails, its record being a row of `objectName`). Either input on a site it does not describe is refused as a tool error. A call that passes neither answers as before. +- **`@objectstack/spec`.** Upgrade guide entry `flow-record-read-and-decision-type-mismatch-refused` (semantic: no mechanical rewrite). + +**FROM → TO.** In a record-triggered flow, or a time-relative flow sweeping its own object: `record.name > 5` (or a decision branch `name > 5`) on a text field → `size(record.name) > 5` for its length, `record.score > 5` on a number field for a value, or `double(record.name) > 5` only when every value is numeric text. A boolean: `record.active > 0` → `record.active == true`. A consumer keyed to the code of a flow condition's warning: `type-mismatch` → `type-mismatch-run-may-fail` (`@objectstack/formula@18.3.0` and `@objectstack/mcp@18.3.0` serve `type-mismatch` there; the record-scoped `type-mismatch`, on a formula field or a display predicate, is unchanged). diff --git a/content/docs/data-modeling/formulas.mdx b/content/docs/data-modeling/formulas.mdx index e1a87be5270..7caa84b50a5 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 or edge condition fails the run (the write that fired it still lands); 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; 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 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 | | **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/cel-engine.ts b/packages/formula/src/cel-engine.ts index d48b48384db..780f07c832e 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -966,6 +966,14 @@ function buildTypedEnv( fieldCelTypes: Readonly>, scope: 'record' | 'flattened', temporalAsText = false, + /** + * [#29959] Flattened scope only: the field types to type `record.` / + * `previous.` with. Absent, those roots stay `map` (dyn members). It + * is a map of its own, not `fieldCelTypes`, because a bare name a flow + * variable holds is left out of the bare typing while `record.` still + * reads the field. + */ + recordRootTypes?: Readonly>, ): Environment { // `temporalAsText` declares a `date`/`datetime` field as the text the runtime // holds ({@link DATE_TEXT_CEL_TYPE}) instead of a Timestamp — see @@ -981,6 +989,18 @@ function buildTypedEnv( registerStdLib(env, () => new Date(0)); registerNullComparisons(env); if (temporalAsText) registerDateTextOverloads(env); + if (recordRootTypes) { + // [#29959] A flow run binds the record it was handed as `record` too + // (`seedRunVariables`), beside the flattened bare names, so + // `record.` is the same value as the bare `` and is typed + // the same: the `record`-scope struct, on `record` and `previous`. + const fields: Record = {}; + for (const [name, t] of Object.entries(recordRootTypes)) fields[name] = typeName(t); + try { env.registerType('OsRecordScope', { fields }); } catch { /* invalid field name — ignore */ } + for (const root of ['record', 'previous']) { + try { env.registerVariable(root, 'OsRecordScope'); } catch { /* duplicate — ignore */ } + } + } for (const root of SCOPE_ROOTS) { try { env.registerVariable(root, 'map'); } catch { /* duplicate — ignore */ } } @@ -1038,6 +1058,39 @@ function offendingField( return null; } +/** + * [#29959] `fieldCelTypes` without `names` — the bare typing of a flattened + * condition whose run binds those names as variables, which win over the + * field (see {@link firstTypeMismatch}'s `bareShadowed`). + */ +function withoutNames( + fieldCelTypes: Readonly>, + names: ReadonlySet, +): Record { + const out: Record = {}; + for (const [name, t] of Object.entries(fieldCelTypes)) if (!names.has(name)) out[name] = t; + return out; +} + +/** + * [#29959] The `record.` / `previous.` read of a field of + * `celType` in a flattened condition, as written — what + * {@link firstTypeMismatch}'s record-root pass names. `null` when no such read + * is spelled member-wise (an index read, say), so the message names no field. + */ +function offendingRecordRead( + source: string, + fieldCelTypes: Readonly>, + celType: FieldCelType, +): { field: string; root: 'record' | 'previous' } | null { + for (const [name, t] of Object.entries(fieldCelTypes)) { + if (t !== celType) continue; + const m = new RegExp(`(?` sites; `'flattened'` for bare-field flow/automation * conditions. + * + * `bareShadowed` (flattened scope only, #29959): names the run binds BEFORE it + * flattens the record's fields — a flow's declared variables and node ids. + * `seedRunVariables` seeds those first and the flattening skips a bound name, + * so a bare read of one holds the variable, not the field: it stays `dyn` + * here. `record.` still reads the field and stays typed. */ export function firstTypeMismatch( source: string, fieldCelTypes: Readonly>, scope: 'record' | 'flattened' = 'record', -): { operator: string; operands: string; celType: FieldCelType; field: string | null; category: 'date-arith' | 'type-mismatch' } | null { + bareShadowed?: ReadonlySet, +): { + operator: string; + operands: string; + celType: FieldCelType; + field: string | null; + /** [#29959] Set when a flattened condition's mismatch is a `record.` / `previous.` read. */ + root?: 'record' | 'previous'; + category: 'date-arith' | 'type-mismatch'; +} | null { if (typeof source !== 'string' || !source.trim()) return null; // An all-`dyn` record can never fault an overload — skip the parse entirely. if (!Object.values(fieldCelTypes).some((t) => t === 'string' || t === 'bool' || t === 'timestamp')) return null; - try { + /** The faulting overload `source` meets in `env`'s typing, or `null`. */ + const unsoundOverload = (env: Environment): RegExpExecArray | null => { // A null-guarded numeric branch (`cond ? n : null`) faults cel-js's ternary // unifier *before* the check reaches an inner date-arith overload; rewrite it // first (same pass the runtime uses) so the real fault surfaces (#3306). - const env = buildTypedEnv(fieldCelTypes, scope); const result = env.parse(rewriteNullableTernary(rewriteIndexedHas(source))).check?.() as | { valid?: boolean; error?: { message?: string } } | undefined; if (!result || result.valid !== false) return null; - const m = UNSOUND_OVERLOAD_RE.exec(result.error?.message ?? ''); - if (!m) return null; - const operator = m[2]; - const operands = `${m[1]} ${operator} ${m[3]}`; - // #3306 — date/duration ARITHMETIC against a number: always nulls, hard error. - if (ARITH_OPS.has(operator) - && ((DATE_CEL_TYPES.has(m[1]) && NUMERIC_CEL_TYPES.has(m[3])) - || (NUMERIC_CEL_TYPES.has(m[1]) && DATE_CEL_TYPES.has(m[3])))) { + return UNSOUND_OVERLOAD_RE.exec(result.error?.message ?? ''); + }; + /** The text/boolean operand type of a faulting overload, or `null`. */ + const textOrBool = (m: RegExpExecArray): FieldCelType | null => + m[1] === 'string' || m[1] === 'bool' ? (m[1] as FieldCelType) + : m[3] === 'string' || m[3] === 'bool' ? (m[3] as FieldCelType) + : null; + const bareTypes = scope === 'flattened' && bareShadowed && bareShadowed.size > 0 + ? withoutNames(fieldCelTypes, bareShadowed) + : fieldCelTypes; + try { + const m = unsoundOverload(buildTypedEnv(bareTypes, scope)); + if (m) { + const operator = m[2]; + const operands = `${m[1]} ${operator} ${m[3]}`; + // #3306 — date/duration ARITHMETIC against a number: always nulls, hard error. + if (ARITH_OPS.has(operator) + && ((DATE_CEL_TYPES.has(m[1]) && NUMERIC_CEL_TYPES.has(m[3])) + || (NUMERIC_CEL_TYPES.has(m[1]) && DATE_CEL_TYPES.has(m[3])))) { + return { + operator, operands, celType: 'timestamp', category: 'date-arith', + field: offendingField(source, bareTypes, 'timestamp', scope), + }; + } + // #1928 — text/boolean field in arithmetic/ordering against a number: warning. + const celType = textOrBool(m); + if (!celType) return null; return { - operator, operands, celType: 'timestamp', category: 'date-arith', - field: offendingField(source, fieldCelTypes, 'timestamp', scope), + operator, operands, celType, category: 'type-mismatch', + field: offendingField(source, bareTypes, celType, scope), }; } - // #1928 — text/boolean field in arithmetic/ordering against a number: warning. - const celType: FieldCelType | null = - m[1] === 'string' || m[1] === 'bool' ? (m[1] as FieldCelType) - : m[3] === 'string' || m[3] === 'bool' ? (m[3] as FieldCelType) - : null; - if (!celType) return null; + // [#29959] A flattened condition reads the same field as `record.` + // (and `previous.`), which the bare typing above leaves `dyn`. A + // second pass types those roots too, for the text/boolean mismatch only: + // the same verdict the bare name gets. The date verdicts stay the bare + // name's alone, as before. + if (scope !== 'flattened' || !/(?, ...Pin 'length, `double(record.name)` for its value (which itself faults on text that is not a number).', }, ], + // [#29959] A flow condition the caller declared no outcome for: still a + // warning, worded for the failed run a fault is there (never a null). The + // second case is the `record.` read, judged as its bare name is. + 'type-mismatch-run-may-fail': [ + { + produce: () => onlyWarning('predicate', 'name > 5', { scope: 'flattened', fields: ['name'], fieldTypes: { name: 'text' } }), + params: { operands: 'string > int', operator: '>', held: 'text', reference: 'name' }, + message: + 'type mismatch `string > int` — `name` holds text but is used with `>` against a number. CEL has no such operation, so ' + + 'wherever the value is not numeric text (an empty value included) the expression faults, and a flow condition that ' + + 'faults when it is evaluated fails the run: the flow stops at this condition and the work it exists to do is not done ' + + '(it does not read null). Whether `name` holds text there depends on what the run binds, which this check cannot ' + + 'prove here, so this is a warning, not a refusal. ' + + 'Compare like with like: use a number field, compare text with text, or convert explicitly — `size(name)` for its ' + + 'length, `double(name)` for its value (which itself faults on text that is not a number).', + }, + { + produce: () => + onlyWarning('predicate', 'record.active > 0', { scope: 'flattened', fields: ['active'], fieldTypes: { active: 'boolean' } }), + params: { operands: 'bool > int', operator: '>', held: 'boolean', reference: 'record.active' }, + message: + 'type mismatch `bool > int` — `record.active` holds a boolean but is used with `>` against a number. CEL has no such ' + + 'operation, so wherever the value is not numeric text (an empty value included) the expression faults, and a flow ' + + 'condition that faults when it is evaluated fails the run: the flow stops at this condition and the work it exists ' + + 'to do is not done (it does not read null). Whether `record.active` holds a boolean there depends on what the run ' + + 'binds, which this check cannot prove here, so this is a warning, not a refusal. ' + + 'Compare a boolean with `true` / `false`, or use a number field.', + }, + ], 'field-near-miss': [ { produce: () => onlyWarning('predicate', "stauts == 'x'", { fields: ['status'], objectName: 'deal' }), @@ -611,6 +640,9 @@ const SWEEP_SCHEMAS: readonly (ExprSchemaHint | undefined)[] = [ fieldTypes: { name: 'text', active: 'boolean' }, faultOutcome, })), + // [#29959] A flattened (flow) condition with no declared outcome reaches + // `type-mismatch-run-may-fail`, bare and through `record.`. + { scope: 'flattened', objectName: 'deal', fields: ['amount', 'name', 'active'], fieldTypes: { name: 'text', active: 'boolean' } }, { fields: ['status'] }, { fieldTypes: { account: 'lookup' }, traversalHydration: true }, ]; diff --git a/packages/formula/src/expression-refusal.ts b/packages/formula/src/expression-refusal.ts index 8b8b2591c50..0716b7daf45 100644 --- a/packages/formula/src/expression-refusal.ts +++ b/packages/formula/src/expression-refusal.ts @@ -182,8 +182,9 @@ export interface ExpressionRefusalParams { 'date-duration-value': { readonly reference?: string }; /** * WARNING, never an error: a text or boolean field used with an arithmetic - * or ordering `operator` against a number, at a site where a fault reads - * null (a formula field) or whose fault outcome the caller did not declare. + * or ordering `operator` against a number, at a `record`-scoped site where a + * fault reads null (a formula field) or whose fault outcome the caller did + * not declare. A flattened (flow) condition is `type-mismatch-run-may-fail`. */ 'type-mismatch': TypeMismatchParams; /** @@ -205,6 +206,14 @@ export interface ExpressionRefusalParams { * advisory never fires. */ 'type-mismatch-fail-open': TypeMismatchParams; + /** + * [#29959] WARNING, never an error: the same mismatch in a flow condition + * whose caller declared no fault outcome — a flow that is not + * record-triggered, or a node condition nothing evaluates. A flow condition + * that faults fails the run (it never reads null), but what the run binds + * under the name is not proven to be the field's value there. + */ + 'type-mismatch-run-may-fail': TypeMismatchParams; /** * WARNING, never an error: a bare identifier in a flattened condition that * is one typo away from a field. Absent `objectName` reads "the trigger object". @@ -270,6 +279,7 @@ const CODE_TABLE = { 'type-mismatch-fail-closed': true, 'type-mismatch-run-fails': true, 'type-mismatch-fail-open': true, + 'type-mismatch-run-may-fail': true, 'field-near-miss': true, 'traversal-bare-and-traversed': true, 'traversal-multi-hop': true, diff --git a/packages/formula/src/validate.test.ts b/packages/formula/src/validate.test.ts index 467eea48d11..a74d27435d6 100644 --- a/packages/formula/src/validate.test.ts +++ b/packages/formula/src/validate.test.ts @@ -806,6 +806,67 @@ describe('validateExpression (ADR-0032)', () => { } }); + // [#29959] A flow condition reads the field bare AND as `record.` + // (`seedRunVariables` binds both to the trigger record), so the record + // read gets the bare name's verdict: here the run-fails error. + it('judges a flattened condition\'s record. and previous. reads as its bare name (#29959)', () => { + const flat = { ...schema, scope: 'flattened' as const, faultOutcome: 'run-fails' as const }; + for (const [source, reference] of [ + ['record.name > 5', 'record.name'], + ['previous.name * 2 > 1', 'previous.name'], + ['record.is_active > 0', 'record.is_active'], + ] as const) { + const r = validateExpression('predicate', source, flat); + expect(r.warnings, source).toHaveLength(0); + expect(r.errors.map((e) => [e.code, (e.params as { reference?: string }).reference]), source).toEqual([['type-mismatch-run-fails', reference]]); + expect(r.errors[0].message, source).toContain(`\`${reference}\``); + } + // Controls: a well-typed record read, a number field, and the + // conversions the message names stay clean. + for (const fixed of ['size(record.name) > 5', 'double(record.name) > 5', 'record.amount > 5', "record.name == 'x'", 'record.is_active == true']) { + const ok = validateExpression('predicate', fixed, flat); + expect([ok.errors, ok.warnings], fixed).toEqual([[], []]); + } + }); + + // [#29959] A bare name the run binds as a flow variable (`flowVariables`) + // holds the variable — `seedRunVariables` seeds it before the flattening, + // which skips a bound name — so it is not typed as the field it shadows. + // `record.` still reads the field and keeps its verdict. + it('does not type a bare name a flow variable holds, and keeps its record. read typed (#29959)', () => { + const flat = { ...schema, scope: 'flattened' as const, faultOutcome: 'run-fails' as const, flowVariables: ['name', 'is_active'] }; + for (const source of ['name > 5', 'is_active > 0', 'name * 2 > 1']) { + const r = validateExpression('predicate', source, flat); + expect([r.errors, r.warnings], source).toEqual([[], []]); + } + for (const [source, reference] of [['record.name > 5', 'record.name'], ['previous.is_active > 0', 'previous.is_active']] as const) { + const r = validateExpression('predicate', source, flat); + expect(r.errors.map((e) => [e.code, (e.params as { reference?: string }).reference]), source).toEqual([['type-mismatch-run-fails', reference]]); + } + // A field the hint does not name is judged as before, in the same source. + const other = validateExpression('predicate', 'name > 5 && title > 5', { ...flat, flowVariables: ['name'] }); + expect(other.errors.map((e) => (e.params as { reference?: string }).reference)).toEqual(['title']); + // The hint is the flattened scope's: a `record`-scoped site ignores it. + expect(validateExpression('predicate', 'record.name > 5', { ...schema, flowVariables: ['name'] }).warnings.map((w) => w.code)) + .toEqual(['type-mismatch']); + }); + + // [#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. + it('warns with the run-may-fail wording on a flattened condition with no declared outcome (#29959)', () => { + const flat = { ...schema, scope: 'flattened' as const }; + for (const source of ['name > 5', 'record.name > 5']) { + const r = validateExpression('predicate', source, flat); + expect(r.errors, source).toHaveLength(0); + expect(r.warnings.map((w) => w.code), source).toEqual(['type-mismatch-run-may-fail']); + expect(r.warnings[0].message, source).toContain('fails the run'); + expect(r.warnings[0].message, source).not.toMatch(/evaluates to null|still lands|is refused/); + } + // The record scope's warning keeps its own code and its true text there. + expect(validateExpression('predicate', 'record.name > 5', schema).warnings.map((w) => w.code)).toEqual(['type-mismatch']); + }); + it('errors with the skipped-advisory wording where the caller declares fail-open (#29926)', () => { const r = validateExpression('predicate', 'record.name > 5', { ...schema, faultOutcome: 'fail-open' }); expect(r.warnings).toHaveLength(0); diff --git a/packages/formula/src/validate.ts b/packages/formula/src/validate.ts index 7c6ee47dc4f..c76a0d4cdb2 100644 --- a/packages/formula/src/validate.ts +++ b/packages/formula/src/validate.ts @@ -96,7 +96,9 @@ export interface ExprSchemaHint { * text or boolean field used with an arithmetic/ordering operator against a * number faults at runtime and the expression silently evaluates to `null`, * so it is surfaced as a NON-blocking warning. Absent ⇒ the check is skipped. - * Only consulted for `scope: 'record'` sites (where refs are `record.`). + * Consulted at both scopes: `record.` at a `'record'` site, the bare + * `` and its `record.` / `previous.` read at a + * `'flattened'` one (#29959). */ fieldTypes?: Readonly>; /** @@ -235,7 +237,7 @@ export interface ExprSchemaHint { * `readonlyWhen`, an option `visibleWhen` on the write path, a hook * `condition`. * - * `'run-fails'`: a record-triggered flow's start or edge condition — the + * `'run-fails'`: a record-triggered flow's start, edge or `decision` branch condition — the * engine's `evaluateCondition` throws on the fault (ADR-0032 §1c), so the * run fails there, while the record-change trigger isolates the write that * fired it: the write lands, the flow's work is not done. @@ -247,11 +249,24 @@ export interface ExprSchemaHint { * With any of the three, a `type-mismatch` (text or a boolean ordered / * arithmetic'd against a number) is an ERROR under that outcome's own code * and wording — the severity the nullable-comparison verdict already has at - * the same sites. Absent: a fault reads as null (a formula field), and - * `type-mismatch` stays the advisory warning whose "evaluates to null" text - * is true there. + * the same sites. Absent at a `record`-scoped site: a fault reads as null (a + * formula field), and `type-mismatch` stays the advisory warning whose + * "evaluates to null" text is true there. Absent at a `'flattened'` (flow) + * site: `type-mismatch-run-may-fail`, a warning saying the run fails where + * the condition faults (#29959) — a flow condition never reads null. */ faultOutcome?: ExprFaultOutcome; + /** + * [#29959] `'flattened'` scope only: the names the flow's run binds BEFORE it + * flattens the record's fields — its declared variables and node ids + * (`@objectstack/lint`'s `collectFlowVariableNames`), and the names the + * engine binds over the flattening. `seedRunVariables` seeds those first and + * the flattening skips a name already bound, so a bare read of one holds the + * variable, not the field: the type-soundness check does not type it as the + * field. `record.` / `previous.` still read the field and stay + * typed. Absent ⇒ every bare field name is typed, exactly as before this key. + */ + flowVariables?: readonly string[]; } /** @@ -382,7 +397,11 @@ const TYPE_MISMATCH_FAULT_ARMS: { * declares the site's fault outcome (`faultOutcome`, #29904 / #29926): * there the fault is not a null but a refused write, a failed flow run or a * rule that never applies, each under its own code - * ({@link TYPE_MISMATCH_FAULT_ARMS}). + * ({@link TYPE_MISMATCH_FAULT_ARMS}). In a flattened (flow) condition with + * no declared outcome it is `type-mismatch-run-may-fail`, still a warning, + * worded for the failed run a fault there is (#29959); a flattened + * condition's `record.` / `previous.` read is judged as its + * bare name is. * - `date-text-arithmetic` / `date-duration-value` (**error**, #28944): what * the Timestamp typing passes, re-checked with a date field declared as the * text the runtime holds — see {@link dateTextIssue}. @@ -394,12 +413,25 @@ function typeSoundnessIssue( scope: 'record' | 'flattened', role: 'predicate' | 'value', faultOutcome: ExprSchemaHint['faultOutcome'], + flowVariables?: readonly string[], ): { issue: ExprValidationError; severity: 'error' | 'warning' } | null { const celFieldTypes = toCelFieldTypes(fieldTypes); - const mismatch = firstTypeMismatch(source, celFieldTypes, scope); - if (!mismatch) return dateTextIssue(source, celFieldTypes, scope, role); + // [#29959] A bare name a flow variable holds is not the field (see + // `ExprSchemaHint.flowVariables`), for every bare-name verdict here. + const shadowed = scope === 'flattened' && flowVariables && flowVariables.length > 0 + ? new Set(flowVariables) + : undefined; + const mismatch = firstTypeMismatch(source, celFieldTypes, scope, shadowed); + if (!mismatch) { + const bareCelTypes = shadowed + ? Object.fromEntries(Object.entries(celFieldTypes).filter(([name]) => !shadowed.has(name))) + : celFieldTypes; + return dateTextIssue(source, bareCelTypes, scope, role); + } const reference = mismatch.field - ? (scope === 'record' ? `record.${mismatch.field}` : mismatch.field) + ? (mismatch.root !== undefined + ? `${mismatch.root}.${mismatch.field}` + : scope === 'record' ? `record.${mismatch.field}` : mismatch.field) : undefined; const ref = reference !== undefined ? `\`${reference}\`` : null; if (mismatch.category === 'date-arith') { @@ -417,38 +449,57 @@ function typeSoundnessIssue( ), }; } - const heldKind = mismatch.celType === 'bool' ? 'boolean' : 'text'; + const heldKind: 'boolean' | 'text' = mismatch.celType === 'bool' ? 'boolean' : 'text'; const held = heldKind === 'boolean' ? 'a boolean' : 'text'; const subject = ref ? `${ref} holds ${held}` : `${held === 'a boolean' ? 'a boolean' : 'a text'} field`; + const params = { + operands: mismatch.operands, + operator: mismatch.operator, + held: heldKind, + ...(reference !== undefined ? { reference } : {}), + }; + // [#29904] Measured on the evaluator: `record.name > 5` faults on `"abc"` + // and on null, and the ADR-0032 §1c rescue answers only numeric text + // (`"7"`). `size()` answers text; `double()` / `int()` validate clean but + // fault on text that is not a number — so the hint says so. + const operand = reference ?? (heldKind === 'boolean' ? 'the field' : 'the text'); + const convert = heldKind === 'boolean' + ? 'Compare a boolean with `true` / `false`, or use a number field.' + : `Compare like with like: use a number field, compare text with text, or convert ` + + `explicitly — \`size(${operand})\` for its length, \`double(${operand})\` for its value ` + + `(which itself faults on text that is not a number).`; + const faults = + `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) { - // [#29904] Measured on the evaluator: `record.name > 5` faults on `"abc"` - // and on null, and the ADR-0032 §1c rescue answers only numeric text - // (`"7"`). `size()` answers text; `double()` / `int()` validate clean but - // fault on text that is not a number — so the hint says so. - const operand = reference ?? (heldKind === 'boolean' ? 'the field' : 'the text'); - const convert = heldKind === 'boolean' - ? 'Compare a boolean with `true` / `false`, or use a number field.' - : `Compare like with like: use a number field, compare text with text, or convert ` + - `explicitly — \`size(${operand})\` for its length, \`double(${operand})\` for its value ` + - `(which itself faults on text that is not a number).`; // [#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]; return { severity: 'error', + issue: refusal(arm.code, params, source, `${faults}, and here ${arm.clause} ${convert}`), + }; + } + if (scope === 'flattened') { + // [#29959] A flattened condition is a flow's, and the engine's + // `evaluateCondition` throws on a fault (ADR-0032 §1c) on every flow: it + // never reads null. The caller declared no outcome because what the run + // binds under the name is not proven to be the field's value there — a + // flow whose entrance hands over an API request body, an action's partial + // record, a parent's record or nothing; a node's inert `condition` — so + // the mismatch stays a warning, worded for the run without naming the + // flow's kind (the engine's registration log serves it on every flow). + return { + severity: 'warning', issue: refusal( - arm.code, - { - operands: mismatch.operands, - operator: mismatch.operator, - held: heldKind, - ...(reference !== undefined ? { reference } : {}), - }, + 'type-mismatch-run-may-fail', + params, source, - `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, and here ${arm.clause} ` + - convert, + `${faults}, and a flow condition that faults when it is evaluated fails the run: the flow stops ` + + `at this condition and the work it exists to do is not done (it does not read null). Whether ` + + `${ref ?? 'the field'} holds ${held} there depends on what the run binds, which this check cannot ` + + `prove here, so this is a warning, not a refusal. ${convert}`, ), }; } @@ -1354,7 +1405,7 @@ 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); + const r = typeSoundnessIssue(source, schema.fieldTypes, 'flattened', role, schema.faultOutcome, schema.flowVariables); if (r) (r.severity === 'error' ? errors : warnings).push(r.issue); } } diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index 5934e9a14fe..9a3712d413f 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -18,6 +18,7 @@ import { PREDICATE_SLOT_STRING_REFUSAL, STRUCTURAL_CONDITION_SHAPE_REFUSAL, TEXT_SLOT_TEMPLATE_REFUSAL, + TimeRelativeTriggerSchema, VALUE_SLOT_TEMPLATE_REFUSAL, flowNodeConfigRefusals, predicateSlotRefusal, @@ -1004,33 +1005,188 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { expect(found[0].message).toMatch(/`size\(title\)`/); }); - it('keeps the warning on a flow that is not record-triggered: start and edge conditions (#29926)', () => { - // No `triggerType` (the shape the pre-existing flattened-flow pin uses) - // and a `schedule` start: neither is `record_change`, so the ledger - // declares no outcome there and the verdict stays the warning. - for (const [name, startConfig] of [ - ['plain_flow', {}], - ['sched_flow', { schedule: '0 * * * *' }], + it('keeps the warning on a flow that is not record-triggered, worded for the run it fails (#29926, #29959)', () => { + // No `triggerType` (the shape the pre-existing flattened-flow pin uses), + // a `schedule` start and an `api` flow: none is `record_change`, so the + // ledger declares no outcome there and the verdict stays the warning. + // [#29959] Its text is the run's: `evaluateCondition` throws on a fault + // on every flow, so "silently evaluates to null" was false here too. The + // `record.` read and the `decision` branch get the same verdict. + for (const [name, startConfig, flowType] of [ + ['plain_flow', {}, 'autolaunched'], + ['sched_flow', { schedule: '0 * * * *' }, 'autolaunched'], + ['api_flow', {}, 'api'], ] as const) { const issues = validateStackExpressions({ objects: [{ name: 'crm_opportunity', fields: { title: { type: 'text', required: true } } }], flows: [{ name, + type: flowType, nodes: [ { id: 'start', type: 'start', config: { objectName: 'crm_opportunity', condition: 'title * 2 > 10', ...startConfig } }, + { id: 'route', type: 'decision', config: { conditions: [{ label: 'Long', expression: 'title > 5' }] } }, { id: 'end', type: 'end' }, ], - edges: [{ id: 'e1', source: 'start', target: 'end', condition: 'title > 5' }], + edges: [ + { id: 'e1', source: 'start', target: 'route', condition: 'record.title > 5' }, + { id: 'e2', source: 'route', target: 'end', label: 'Long' }, + ], }], }); const found = typeMismatches(issues); expect(found.map(i => [i.where, i.severity]), name).toEqual([ [`flow '${name}' · node 'start' (start) condition`, 'warning'], - [`flow '${name}' · edge 'e1' (start→end) condition`, 'warning'], + [`flow '${name}' · node 'route' (decision) decision branch expression at config.conditions[0].expression`, 'warning'], + [`flow '${name}' · edge 'e1' (start→route) condition`, 'warning'], + ]); + for (const i of found) { + expect(i.message, `${name} ${i.where}`).toContain('a flow condition that faults when it is evaluated fails the run'); + expect(i.message, `${name} ${i.where}`).not.toMatch(/evaluates to null|still lands/); + } + expect(found[2]!.message).toContain('`record.title` holds text'); + } + }); + + // [#29959] The two sites #29926 left without the run-fails verdict on a + // record-triggered flow: a `record.` read (the same trigger record + // as the bare name) and a `decision` branch (evaluated like an edge). + it('refuses a type mismatch in a record.x read and a decision branch of a record-triggered flow (#29959)', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'crm_opportunity', fields: { title: { type: 'text', required: true }, won: { type: 'boolean' } } }], + flows: [{ + name: 'rt_flow', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_opportunity', triggerType: 'record-after-update', condition: 'record.title > 5' } }, + { id: 'route', type: 'decision', config: { conditions: [{ label: 'Long', expression: 'title * 2 > 10' }, { label: 'Won', expression: 'record.won > 0' }] } }, + { id: 'end', type: 'end' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'route', condition: 'previous.title > 5' }, + { id: 'e2', source: 'route', target: 'end', label: 'Long' }, + { id: 'e3', source: 'route', target: 'end', label: 'Won' }, + ], + }], + }); + const found = typeMismatches(issues); + expect(found.map(i => [i.where, i.severity, i.path])).toEqual([ + ["flow 'rt_flow' · node 'start' (start) condition", 'error', 'flows[0].nodes[0].config.condition'], + ["flow 'rt_flow' · node 'route' (decision) decision branch expression at config.conditions[0].expression", 'error', 'flows[0].nodes[1].config.conditions[0].expression'], + ["flow 'rt_flow' · node 'route' (decision) decision branch expression at config.conditions[1].expression", 'error', 'flows[0].nodes[1].config.conditions[1].expression'], + ["flow 'rt_flow' · edge 'e1' (start→route) condition", 'error', 'flows[0].edges[0].condition'], + ]); + for (const i of found) { + expect(i.message, i.where).toContain('fails the run: the flow stops at this condition, the write that fired it still lands'); + expect(i.message, i.where).not.toMatch(/evaluates to null|is refused/); + } + expect(found.map(i => /`([\w.]+)` holds/.exec(i.message)?.[1])).toEqual(['record.title', 'title', 'record.won', 'previous.title']); + }); + + it('leaves a well-typed record.x read and decision branch of a record-triggered flow clean (#29959)', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'crm_opportunity', fields: { title: { type: 'text', required: true }, amount: { type: 'number', required: true }, won: { type: 'boolean' } } }], + flows: [{ + name: 'rt_flow', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_opportunity', triggerType: 'record-after-update', condition: 'size(record.title) > 5' } }, + { id: 'route', type: 'decision', config: { conditions: [{ label: 'Big', expression: 'record.amount > 10' }, { label: 'Won', expression: 'won == true' }] } }, + { id: 'end', type: 'end' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'route', condition: "record.title != 'x'" }, + { id: 'e2', source: 'route', target: 'end', label: 'Big' }, + { id: 'e3', source: 'route', target: 'end', label: 'Won' }, + ], + }], + }); + expect(typeMismatches(issues)).toEqual([]); + expect(issues.filter(i => i.severity === 'error')).toEqual([]); + }); + + // [#29959] `time-relative-trigger.ts` launches each run with the swept row + // as `{ record: row }` — a stored row of the swept object, flattened like a + // record-change trigger's — so a sweep of the object the conditions are + // typed against gets the run-fails verdict, bare name included. A sweep of + // another object binds a row of THAT object: the warning stays. + it('refuses a type mismatch on a time-relative sweep of the flow object, and warns on a sweep of another (#29959)', () => { + const flowOf = (name: string, sweepObject: string | undefined) => ({ + name, + nodes: [ + { + id: 'start', type: 'start', + config: { + objectName: 'crm_opportunity', condition: 'title > 5', schedule: '0 * * * *', + timeRelative: { ...(sweepObject ? { object: sweepObject } : {}), dateField: 'close_date', offsetDays: 3 }, + }, + }, + { id: 'route', type: 'decision', config: { conditions: [{ label: 'Long', expression: 'record.title > 5' }] } }, + { id: 'end', type: 'end' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'route', condition: 'title * 2 > 10' }, + { id: 'e2', source: 'route', target: 'end', label: 'Long' }, + ], + }); + const objects = [ + { name: 'crm_opportunity', fields: { title: { type: 'text', required: true }, close_date: { type: 'date' } } }, + { name: 'crm_task', fields: { close_date: { type: 'date' } } }, + ]; + for (const [name, sweepObject, severity] of [ + ['tr_default', undefined, 'error'], + ['tr_same', 'crm_opportunity', 'error'], + ['tr_other', 'crm_task', 'warning'], + ] as const) { + const found = typeMismatches(validateStackExpressions({ objects, flows: [flowOf(name, sweepObject)] })); + expect(found.map(i => [i.where, i.severity]), name).toEqual([ + [`flow '${name}' · node 'start' (start) condition`, severity], + [`flow '${name}' · node 'route' (decision) decision branch expression at config.conditions[0].expression`, severity], + [`flow '${name}' · edge 'e1' (start→route) condition`, severity], ]); + for (const i of found) { + expect(i.message, `${name} ${i.where}`).toContain( + severity === 'error' ? 'the write that fired it still lands' : 'so this is a warning, not a refusal', + ); + } } }); + // [#29959] `seedRunVariables` seeds the flow's declared variables before it + // flattens the record, and the flattening skips a name already bound: a + // bare `title` here reads the NUMBER variable and `title > 5` runs. Only + // the shadowing warning may name it; `record.title` still reads the field. + it('does not type a bare name a declared flow variable holds as the field it shadows (#29959)', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'crm_opportunity', fields: { title: { type: 'text', required: true } } }], + flows: [{ + name: 'rt_vars', + variables: [{ name: 'title', type: 'number', defaultValue: 3 }], + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_opportunity', triggerType: 'record-after-update', condition: 'title > 5' } }, + { 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: 'route', condition: 'title > 5' }, + { id: 'e2', source: 'route', target: 'end', label: 'Big' }, + { id: 'e3', source: 'route', target: 'end', label: 'Long' }, + ], + }], + }); + const found = typeMismatches(issues); + expect(found.map(i => [i.where, i.severity])).toEqual([ + ["flow 'rt_vars' · node 'route' (decision) decision branch expression at config.conditions[1].expression", 'error'], + ]); + expect(found[0]!.message).toContain('`record.title` holds text'); + // The shadowing itself is still said, at the start and edge sites that read it. + const shadowSites = issues.filter(i => /declared flow variable/i.test(i.message) || /shadow/i.test(i.message)).map(i => i.where); + expect(shadowSites).toEqual(expect.arrayContaining([ + "flow 'rt_vars' · node 'start' (start) condition", + "flow 'rt_vars' · edge 'e1' (start→route) condition", + ])); + expect(issues.filter(i => i.severity === 'error').map(i => i.where)).toEqual([ + "flow 'rt_vars' · node 'route' (decision) decision branch expression at config.conditions[1].expression", + ]); + }); + 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 } } }], @@ -3650,6 +3806,15 @@ const READ_SURFACES: Array<{ receiver: string; expected: string[]; declaredBy: s declaredBy: 'AppLandingEntrySchema', keys: () => Object.keys(AppLandingEntrySchema.shape), }, + { + // [#29959] A start node's `config.timeRelative` sweep descriptor: its + // `object` decides whether the sweep binds a row of the object the flow's + // conditions are typed against (the fault-outcome ledger's time-relative row). + receiver: 'sweep', + expected: ['object'], + declaredBy: 'TimeRelativeTriggerSchema', + keys: () => shapeKeysOf(TimeRelativeTriggerSchema), + }, { receiver: 'obj', // `validationRules` is absent — one of the five #5017 removed. @@ -3959,9 +4124,14 @@ describe('validateStackExpressions — reads only keys the spec declares (meta-t // methods, never metadata keys. 'rootScope', 'declaredVariables', 'nullGuardBare', // [#29926] The fault-outcome ledger's own site descriptor (`kind`, - // `severity`, `recordTriggered`), built at each call site from the + // `severity`, `triggerKind`, `sweepsTypedObject`), built at each call site from the // 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 + // walk's own `objectName`, the ledger's answer and the flow's bound + // names — never read off metadata itself. + 'flowCondition', ]); 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 1ed367d38ca..8c6be82b2a3 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -125,7 +125,7 @@ import { actionCopyTemplateIssues } from './validate-action-copy-templates.js'; // authored"), the CEL rule in `validateExpression('value', …)`. Neither // refusal string is spelled here. import { FlowValueSlotSchema, VALUE_ENVELOPE_REFUSAL } from '@objectstack/spec/automation'; -import type { FlowNodeParsed, FlowEdgeParsed } from '@objectstack/spec/automation'; +import type { FlowNodeParsed, FlowEdgeParsed, FlowTriggerKind } from '@objectstack/spec/automation'; // [#17495] The blank-source half of the structural-condition refusal, imported // rather than restated: this is the EDGE door's own rule // (`FlowEdgeSchema.condition` composes it since #15807) and the very schema @@ -1300,10 +1300,11 @@ function isFlattenedNearMissFor(diagnostic: string, roots: readonly string[]): b * | field `requiredWhen`, `readonlyWhen` | `rule-validator.ts`: refuses the write (ADR-0137 D2) | `fail-closed` | * | option `visibleWhen` | `evaluateOptionVisibility`: refuses a write picking it (ADR-0137 D2, #22402) | `fail-closed` | * | hook `condition` | `hook-wrappers.ts`: aborts the operation (#4775) | `fail-closed` | - * | flow start / edge condition — record-triggered flow | `evaluateCondition` throws (ADR-0032 §1c); the trigger isolates the write: the run fails, the write lands | `run-fails` | - * | flow `decision` branch — record-triggered flow | the same; the null-guard verdict only (the branch is validated without the object's field types, so no `type-mismatch` reaches it) | `run-fails` | - * | flow condition, any other flow | no total trigger record is bound (see `validate-null-guards.ts`'s ledger) | — not declared | - * | a flow node's `condition` other than the top-level start node's | inert (`flow-inert-node-condition`) | — | + * | 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 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 | — | * | sharing rule `condition` | compiled to a query filter, not evaluated as CEL per row | — | @@ -1319,7 +1320,12 @@ function isFlattenedNearMissFor(diagnostic: string, roots: readonly string[]): b type FaultSite = | { readonly kind: 'validation rule'; readonly severity: unknown } | { readonly kind: 'field write rule' | 'option visibleWhen' | 'hook condition' } - | { readonly kind: 'flow condition'; readonly recordTriggered: boolean }; + | { + readonly kind: 'flow condition'; + readonly triggerKind: FlowTriggerKind | undefined; + /** A time-relative sweep's object is the one the conditions are typed against. */ + readonly sweepsTypedObject: boolean; + }; /** The ledger above, as the one function every call site asks. */ function faultOutcomeOf(site: FaultSite): (ExprFaultOutcome & NullGuardOutcome) | undefined { @@ -1328,7 +1334,9 @@ function faultOutcomeOf(site: FaultSite): (ExprFaultOutcome & NullGuardOutcome) // The runtime's own test: `severity !== 'error'` (default `'error'`). return site.severity === 'warning' || site.severity === 'info' ? 'fail-open' : 'fail-closed'; case 'flow condition': - return site.recordTriggered ? 'run-fails' : undefined; + return site.triggerKind === 'record_change' || (site.triggerKind === 'time_relative' && site.sweepsTypedObject) + ? 'run-fails' + : undefined; default: return 'fail-closed'; } @@ -2469,20 +2477,32 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression * Absent where the ledger declares none. */ 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. + */ + flowVariables?: readonly string[], ): void => { if (raw == null) return; // Absent unless this is a served-row site on an object that declares a // read attachment. const attachedOnRead = objectName && servedRowSite ? attachedOnReadIndex.get(objectName) : undefined; const fields = objectName ? withAttachedBlocks(fieldIndex.get(objectName), attachedOnRead) : undefined; - // Field types feed the #1928 tier-4 soundness warning; only consulted for - // `record`-scoped sites, so it is harmless to pass for flattened ones too. + // Field types feed the #1928 tier-4 soundness check, at both scopes: the + // `record.` read at a `record` site; the bare name and its + // `record.` / `previous.` read at a flattened one (#29959). const fieldTypes = objectName ? fieldTypeIndex.get(objectName) : undefined; // Spread only when given, so every other site's hint is unchanged. const roots = extraRoots ? { roots: extraRoots } : {}; const res = validateExpression('predicate', raw as string | { dialect?: string; source?: string }, objectName - ? { objectName, fields, attachedOnRead, fieldTypes, scope, traversalHydration, ...roots, ...(faultOutcome ? { faultOutcome } : {}) } + ? { + objectName, fields, attachedOnRead, fieldTypes, scope, traversalHydration, ...roots, + ...(faultOutcome ? { faultOutcome } : {}), + ...(flowVariables ? { flowVariables } : {}), + } : { scope, ...roots }); for (const e of res.errors) { if (fieldRuleVerdictIssued && isBareReferenceToAny(e.message, FIELD_RULE_NOWHERE_BOUND_ROOTS)) continue; @@ -2553,8 +2573,22 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression * A declared bare-CEL slot (#4027). No object schema is passed: these slots * bind the *screen's own* collected values, not the trigger record's fields, so * a field-existence pass would report every field name as unknown. + * + * [#29959] `flowCondition` — set for a slot the engine evaluates like an + * edge (a `decision` branch's `expression`, `builtin/logic-nodes.ts`, over + * the same run variables): there the trigger record IS flattened in, so the + * slot is judged by {@link check} exactly as an edge condition is — the + * object's fields and types, and the flow's declared fault outcome. */ - const checkDeclaredPredicate = (where: string, raw: unknown): { refused: boolean } => { + const checkDeclaredPredicate = ( + where: string, + raw: unknown, + flowCondition?: { + readonly objectName: string | undefined; + readonly faultOutcome: ExprFaultOutcome | undefined; + readonly flowVariables: readonly string[]; + }, + ): { refused: boolean } => { // [#19961] No `raw == null` early return: whether an absent value is a // finding is the resolver's call, not this pass's. It emits absent / `null` // only for a `required` ledger slot (a `decision` branch's `expression`), @@ -2578,6 +2612,13 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression issues.push({ where, message: shapeRefusal.message, source: shapeRefusal.source, severity: 'error' }); return { refused: true }; } + if (flowCondition) { + check( + where, raw, flowCondition.objectName, undefined, undefined, undefined, undefined, undefined, + flowCondition.faultOutcome, flowCondition.flowVariables, + ); + return { refused: false }; + } const res = validateExpression('predicate', raw as string | { dialect?: string; source?: string }); for (const e of res.errors) issues.push({ where, message: e.message, source: e.source, severity: 'error' }); for (const w of res.warnings) issues.push({ where, message: w.message, source: w.source, severity: 'warning' }); @@ -2759,10 +2800,23 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression * - Outcome `run-fails`: the condition throws, the run fails, and the * trigger isolates the write that fired it. */ - const nullGuardObject = resolveFlowTriggerKind(flow) === 'record_change' ? objectName : undefined; - // [#29926] The ledger's flow row: `run-fails` on a record-triggered flow, - // for both verdicts; nothing declared on any other flow. - const flowFaultOutcome = faultOutcomeOf({ kind: 'flow condition', recordTriggered: nullGuardObject !== undefined }); + const triggerKind = resolveFlowTriggerKind(flow); + const nullGuardObject = triggerKind === 'record_change' ? objectName : undefined; + // [#29926] The ledger's flow rows: `run-fails` on a record-triggered flow, + // for both verdicts, and on a time-relative sweep of the typed object + // (#29959; the null-guard twin stays on record-triggered flows, the one + // entrance that binds `previous` too); nothing declared on any other flow. + // The sweep's object is `timeRelative.object`, else the start node's. + const sweep = startCfg.timeRelative && typeof startCfg.timeRelative === 'object' ? startCfg.timeRelative as AnyRec : {}; + const flowFaultOutcome = faultOutcomeOf({ + kind: 'flow condition', + triggerKind, + sweepsTypedObject: typeof sweep.object !== 'string' || sweep.object === objectName, + }); + // [#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])]; const nullGuardBare = new Set(); if (nullGuardObject && rootScope.provable) { for (const name of nullableIndex.get(nullGuardObject) ?? []) { @@ -2899,7 +2953,10 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression // [#29926] The outcome only on the trigger gate: on any other node the // key is inert (see the null-guard call below), so nothing fails there. const isTriggerGate = node.type === 'start' && graph.path.length === 0; - check(nodeCondWhere, cfg.condition, objectName, undefined, undefined, undefined, undefined, undefined, isTriggerGate ? flowFaultOutcome : undefined); + check( + nodeCondWhere, cfg.condition, objectName, undefined, undefined, undefined, undefined, undefined, + isTriggerGate ? flowFaultOutcome : undefined, flowVariables, + ); warnShadowedFieldReads(nodeCondWhere, cfg.condition); refuseUnboundRoots(nodeCondWhere, cfg.condition, from, 'predicate'); // [#29026] The start node's is the trigger gate; on any other node @@ -2972,7 +3029,10 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression // [#15572] A slot refused on SHAPE gets no second diagnostic: the // shadowing warning is about which scope a CEL source resolves in, // and a value that is not CEL text has no source to resolve. - if (checkDeclaredPredicate(slotWhere, found.value).refused) continue; + // [#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; + 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 // belongs on every `predicate` slot the ledger declares, not just the @@ -3131,7 +3191,10 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression const first = issues.length; if (!checkStructuralCondition(edgeCondWhere, edge.condition).refused) { const from = issues.length; - check(edgeCondWhere, edge.condition, objectName, undefined, undefined, undefined, undefined, undefined, flowFaultOutcome); + check( + edgeCondWhere, edge.condition, objectName, undefined, undefined, undefined, undefined, undefined, + flowFaultOutcome, flowVariables, + ); warnShadowedFieldReads(edgeCondWhere, edge.condition); refuseUnboundRoots(edgeCondWhere, edge.condition, from, 'predicate'); checkFlowNullGuards(edgeCondWhere, edge.condition, true); diff --git a/packages/mcp/src/mcp-http-tools.ts b/packages/mcp/src/mcp-http-tools.ts index 06bc77302d6..a06be3b9361 100644 --- a/packages/mcp/src/mcp-http-tools.ts +++ b/packages/mcp/src/mcp-http-tools.ts @@ -86,6 +86,7 @@ import { strictUnknownKeyError } from '@objectstack/spec/shared'; // formula checks, so a blank source gets the gate's verdict and its published // sentence, never a second blank rule written here. import { EvaluatedExpressionInputSchema, TemplateExpressionInputSchema } from '@objectstack/spec/shared'; +import { FLOW_TRIGGER_KINDS, type FlowTriggerKind } from '@objectstack/spec/automation'; import { METADATA_UNAVAILABLE_CODE, metadataPartialListingSentence, @@ -634,21 +635,57 @@ function jsonText(value: unknown): string { */ const VALIDATE_SITE_MAP: Record< string, - { role: FieldRole; scope: 'record' | 'flattened'; slot: z.ZodType; optionLabels?: true; faultOutcome?: ExprFaultOutcome } + { + role: FieldRole; + scope: 'record' | 'flattened'; + slot: z.ZodType; + optionLabels?: true; + /** + * [#29904, #29926, #29959] What the gate's runtime does on a fault at this + * site, from the caller's optional `severity` / `triggerKind` — the two + * facts `@objectstack/lint`'s `faultOutcomeOf` ledger reads off the rule + * and the flow. Absent: the site declares none. + */ + faultOutcome?: (inputs: { severity: ValidationSeverity; triggerKind: FlowTriggerOrNone }) => ExprFaultOutcome | undefined; + } > = { formula: { role: 'value', scope: 'record', slot: EvaluatedExpressionInputSchema, optionLabels: true }, - // [#29904] A validation rule fails closed (a fault refuses the write), so the - // publish gate grades a text-vs-number comparison there an error; the tool - // answers what the gate will. - validation: { role: 'predicate', scope: 'record', slot: EvaluatedExpressionInputSchema, faultOutcome: 'fail-closed' }, - // [#29926] A condition with the object's fields bound bare is a - // record-triggered flow's (the trigger record is flattened into the run's - // variables); there a fault fails the run while the write that fired it - // lands, and the gate grades a text-vs-number comparison an error worded so. - flow_condition: { role: 'predicate', scope: 'flattened', slot: EvaluatedExpressionInputSchema, faultOutcome: 'run-fails' }, + // [#29904, #29926] A `severity: 'error'` rule (the default) fails closed: a + // fault refuses the write. An advisory rule (`warning` / `info`) fails open: + // the rule is skipped and the write proceeds. The gate grades a + // text-vs-number comparison an error worded for each; the tool answers what + // the gate will for the severity it is told. + validation: { + role: 'predicate', scope: 'record', slot: EvaluatedExpressionInputSchema, + faultOutcome: ({ severity }) => (severity === 'warning' || severity === 'info' ? 'fail-open' : 'fail-closed'), + }, + // [#29926, #29959] A record-triggered flow's start, edge or `decision` + // branch condition (the default `triggerKind`): the trigger record is + // flattened into the run's variables, a fault fails the run while the write + // that fired it lands, and the gate grades a text-vs-number comparison an + // error worded so. A time-relative sweep launches each run with the swept + // row of `objectName` as its record, so it is graded the same. Any other + // trigger kind declares no outcome: the gate's `type-mismatch-run-may-fail` + // warning. + flow_condition: { + role: 'predicate', scope: 'flattened', slot: EvaluatedExpressionInputSchema, + faultOutcome: ({ triggerKind }) => + (triggerKind === 'record_change' || triggerKind === 'time_relative' ? 'run-fails' : undefined), + }, template: { role: 'template', scope: 'record', slot: TemplateExpressionInputSchema }, }; +/** [#29959] A validation rule's `severity`, as `ValidationRuleSchema` spells it. */ +type ValidationSeverity = 'error' | 'warning' | 'info'; +const VALIDATION_SEVERITIES = ['error', 'warning', 'info'] as const satisfies readonly ValidationSeverity[]; +/** + * [#29959] A flow's trigger kind, as `resolveFlowTriggerKind` answers it, or + * `none` for a flow with no auto-launch trigger (a screen flow, an + * autolaunched flow started by hand, an action or a parent flow). + */ +type FlowTriggerOrNone = FlowTriggerKind | 'none'; +const FLOW_TRIGGER_INPUTS: [FlowTriggerOrNone, ...FlowTriggerOrNone[]] = ['none', ...FLOW_TRIGGER_KINDS]; + // ── Tool arguments: an undeclared key is REFUSED, not stripped (#16913) ────── /** @@ -1053,10 +1090,13 @@ export function registerObjectTools( description: 'Validate a CEL expression against an object\'s schema before authoring it into metadata. Returns ' + 'build-time errors (bare field refs, unknown fields, unknown functions) and advisory warnings ' + - '(text/boolean fields misused in arithmetic, an error at a `validation` site, where it refuses ' + - 'the write, and at a `flow_condition`, where it fails the run; date-equality pitfalls), plus the fields and stdlib ' + + '(text/boolean fields misused in arithmetic, an error at a `validation` site, worded for the rule\'s ' + + '`severity`, and at a `flow_condition` of a record-triggered or time-relative flow, where it fails the run; ' + + 'a warning on any other flow; date-equality pitfalls), plus the fields and stdlib ' + 'functions in scope so you can self-correct. `site` says where the expression will live: a `formula` ' + - 'field, a `validation`/predicate, or a `flow_condition` (fields are bound bare in flow conditions).', + 'field, a `validation`/predicate, or a `flow_condition` (fields are bound bare in flow conditions). ' + + 'Pass the rule\'s `severity` (validation) or the flow\'s `triggerKind` (flow_condition) so the verdict ' + + 'is the one the publish gate gives that rule or flow.', inputSchema: strictToolInput( { surface: 'this validate_expression call', @@ -1077,13 +1117,37 @@ export function registerObjectTools( .describe( 'Where the expression will live. formula/validation bind `record.`; flow_condition binds fields bare. Default: formula.', ), + severity: z + .enum(VALIDATION_SEVERITIES) + .optional() + .describe( + 'site validation only: the rule\'s `severity`. A `warning` / `info` rule is advisory, so a fault skips it ' + + 'instead of refusing the write, and the verdict says so. Default: error, the rule default.', + ), + triggerKind: z + .enum(FLOW_TRIGGER_INPUTS) + .optional() + .describe( + 'site flow_condition only: how the flow is started — record_change (start triggerType record-*), ' + + 'time_relative, schedule, api, or none (started by hand, an action or a parent flow). A type mismatch ' + + 'is an error on a record_change or time_relative flow (its record is a row of objectName) and a warning ' + + 'on any other. Default: record_change.', + ), }, ), annotations: { readOnlyHint: true, destructiveHint: false, openWorldHint: false }, }, - async ({ objectName, expression, site }) => { + async ({ objectName, expression, site, severity, triggerKind }) => { const bad = guard(objectName); if (bad) return errorResult(bad); + // [#29959] Each input describes one site; on another it would be + // silently meaningless, so it is refused instead. + if (severity !== undefined && site !== 'validation') { + return errorResult(`\`severity\` describes a validation rule; it applies only with site "validation", not "${site ?? 'formula'}".`); + } + if (triggerKind !== undefined && site !== 'flow_condition') { + return errorResult(`\`triggerKind\` describes a flow; it applies only with site "flow_condition", not "${site ?? 'formula'}".`); + } try { const def = (await bridge.describeObject(objectName)) as { fields?: unknown } | null; if (!def) return errorResult(`Object "${objectName}" not found`); @@ -1095,7 +1159,8 @@ export function registerObjectTools( fields.push(f.name); if (typeof f?.type === 'string') fieldTypes[f.name] = f.type; } - const { role, scope, slot, optionLabels, faultOutcome } = VALIDATE_SITE_MAP[site ?? 'formula']; + const { role, scope, slot, optionLabels, faultOutcome: outcomeOf } = VALIDATE_SITE_MAP[site ?? 'formula']; + const faultOutcome = outcomeOf?.({ severity: severity ?? 'error', triggerKind: triggerKind ?? 'record_change' }); // [#28715] The object's option-bearing fields, where the gate reads // them: `label(record.)` on a declared field outside this list // is refused as `label-field-no-options` by the one validator. diff --git a/packages/mcp/src/mcp-validate-expression.test.ts b/packages/mcp/src/mcp-validate-expression.test.ts index a16dfc1bd5e..587f1921724 100644 --- a/packages/mcp/src/mcp-validate-expression.test.ts +++ b/packages/mcp/src/mcp-validate-expression.test.ts @@ -7,6 +7,8 @@ import { TYPED_EXPRESSION_SOURCE_REQUIRED, } from '@objectstack/spec/shared'; +import { validateStackExpressions } from '@objectstack/lint'; + import { MCPServerRuntime } from './mcp-server-runtime.js'; import type { McpDataBridge } from './mcp-http-tools.js'; @@ -131,6 +133,93 @@ describe('validate_expression MCP tool (#1928)', () => { expect(bad.errors[0].message).toContain('fails the run'); }); + // [#29959] The two facts the gate's fault-outcome ledger reads off the rule + // and the flow, as optional inputs: for every value, the tool's verdict on + // a text-vs-number comparison is the publish gate's own (severity and + // message), read off `validateStackExpressions` on the same rule or flow. + describe('answers the publish gate for each severity and trigger kind (#29959)', () => { + const fields = { title: { type: 'text', required: true } }; + const typeVerdicts = (issues: ReturnType) => + issues.filter((i) => /^type mismatch/.test(i.message)).map((i) => [i.severity, i.message]); + const toolVerdicts = (r: { errors: { message: string }[]; warnings: { message: string }[] }) => [ + ...r.errors.map((e) => ['error', e.message]), + ...r.warnings.map((w) => ['warning', w.message]), + ]; + + for (const severity of [undefined, 'error', 'warning', 'info'] as const) { + it(`site validation, severity ${severity ?? '(omitted)'}`, async () => { + const expression = 'record.title > 5'; + const gate = typeVerdicts(validateStackExpressions({ + objects: [{ + name: 'crm_opportunity', + fields, + validations: [{ name: 'r', type: 'script', condition: expression, message: 'm', ...(severity ? { severity } : {}) }], + }], + })); + expect(gate).toHaveLength(1); + const r = await validate(runtime, bridge, { objectName: 'crm_opportunity', expression, site: 'validation', ...(severity ? { severity } : {}) }); + expect(toolVerdicts(r)).toEqual(gate); + }); + } + + // The severity column is the ledger's own answer (`faultOutcomeOf`): a + // time-relative sweep binds a row of `objectName` as its record (#29959). + for (const [triggerKind, startConfig, flowType, severity] of [ + [undefined, { triggerType: 'record-after-update' }, 'autolaunched', 'error'], + ['record_change', { triggerType: 'record-after-create' }, 'autolaunched', 'error'], + ['time_relative', { timeRelative: { object: 'crm_opportunity', field: 'close_date', offsetDays: 1 }, schedule: '0 * * * *' }, 'autolaunched', 'error'], + ['schedule', { schedule: '0 * * * *' }, 'autolaunched', 'warning'], + ['api', {}, 'api', 'warning'], + ['none', {}, 'autolaunched', 'warning'], + ] as const) { + it(`site flow_condition, triggerKind ${triggerKind ?? '(omitted)'}`, async () => { + const expression = 'title * 2 > 10'; + const gate = typeVerdicts(validateStackExpressions({ + objects: [{ name: 'crm_opportunity', fields }], + flows: [{ + name: 'f', + type: flowType, + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_opportunity', condition: expression, ...startConfig } }, + { id: 'end', type: 'end' }, + ], + edges: [{ id: 'e1', source: 'start', target: 'end' }], + }], + })); + expect(gate).toHaveLength(1); + expect(gate[0]![0]).toBe(severity); + const r = await validate(runtime, bridge, { objectName: 'crm_opportunity', expression, site: 'flow_condition', ...(triggerKind ? { triggerKind } : {}) }); + expect(toolVerdicts(r)).toEqual(gate); + }); + } + + it('refuses an input on a site it does not describe', async () => { + for (const args of [ + { site: 'flow_condition', severity: 'warning' }, + { site: 'formula', severity: 'error' }, + { severity: 'info' }, + { site: 'validation', triggerKind: 'api' }, + { triggerKind: 'record_change' }, + ]) { + const { json } = await call(runtime, toolsCall(1, 'validate_expression', { objectName: 'crm_opportunity', expression: 'record.title > 5', ...args }), bridge); + const { result } = json as { result: { isError?: boolean; content: { text: string }[] } }; + expect(result.isError, JSON.stringify(args)).toBe(true); + expect(result.content[0]!.text, JSON.stringify(args)).toMatch(/applies only with site/); + } + }); + + it('leaves a well-typed condition clean at every input', async () => { + for (const severity of ['error', 'warning', 'info'] as const) { + const r = await validate(runtime, bridge, { objectName: 'crm_opportunity', expression: 'size(record.title) > 5', site: 'validation', severity }); + expect([r.errors, r.warnings], severity).toEqual([[], []]); + } + for (const triggerKind of ['record_change', 'time_relative', 'schedule', 'api', 'none'] as const) { + const r = await validate(runtime, bridge, { objectName: 'crm_opportunity', expression: 'size(title) > 5', site: 'flow_condition', triggerKind }); + expect([r.errors, r.warnings], triggerKind).toEqual([[], []]); + } + }); + }); + it('errors clearly when the object does not exist', async () => { const { json } = await call(runtime, toolsCall(1, 'validate_expression', { objectName: 'nope', expression: 'record.x > 1' }), bridge); expect(json.result.isError).toBe(true); diff --git a/packages/mcp/src/skill-md.ts b/packages/mcp/src/skill-md.ts index ea29efe40d4..590c08b3247 100644 --- a/packages/mcp/src/skill-md.ts +++ b/packages/mcp/src/skill-md.ts @@ -264,14 +264,17 @@ create/update payload. - **list_objects()** — list available objects (system \`sys_*\` objects are hidden). - **describe_object({ objectName })** — an object's fields and features. -- **validate_expression({ objectName, expression, site? })** — check a CEL +- **validate_expression({ objectName, expression, site?, severity?, triggerKind? })** — check a CEL expression against an object's schema *before* you author it into a formula, validation, or flow condition. Returns build-time errors (bare field refs, unknown fields/functions) and advisory warnings (text/boolean fields misused - in arithmetic; an error at a \`validation\` site, where it refuses the write, - and at a \`flow_condition\`, where it fails the run), plus the fields and + in arithmetic; an error at a \`validation\` site, worded for the rule's + \`severity\`, and at a \`flow_condition\` of a record-triggered or + time-relative flow, where it fails the run; a warning on any other flow), plus the fields and stdlib functions in scope. \`site\` is \`formula\` | \`validation\` | - \`flow_condition\` | \`template\` (default \`formula\`). + \`flow_condition\` | \`template\` (default \`formula\`). Pass the rule's + \`severity\` or the flow's \`triggerKind\` (\`record_change\` by default) to + get the verdict the publish gate gives that rule or flow. - **query_records({ objectName, where?, fields?, limit?, offset?, orderBy? })** — read records. \`where\` is a field→value match, e.g. \`{ "status": "open" }\`. Results are page-capped; use \`limit\`/\`offset\` to page. 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 d823820e388..0a63c50621b 100644 --- a/packages/objectql/src/publish-type-mismatch-fail-closed.test.ts +++ b/packages/objectql/src/publish-type-mismatch-fail-closed.test.ts @@ -264,3 +264,104 @@ describe('publish refuses a type mismatch in a fail-closed role (#29904)', () => expect(issue.message).not.toMatch(/evaluates to null/); }); }); + +/** + * [#29959] Stage the lead object and one flow into PKG and publish the + * package, through the same real door as the cases above. + */ +async function publishLeadFlow( + startConfig: Record, + nodes: unknown[], + edges: unknown[], + flowType = 'autolaunched', +): Promise { + const p = await makeProtocol(); + await (p as any).saveMetaItem({ + type: 'object', + name: 'edu_lead', + item: leadObject({ name: { type: 'text', label: 'Name', required: true } }), + mode: 'draft', + packageId: PKG, + }); + await (p as any).saveMetaItem({ + type: 'flow', + name: 'edu_lead_flow', + item: { + name: 'edu_lead_flow', + label: 'Lead flow', + type: flowType, + nodes: [{ id: 'start', type: 'start', label: 'Start', config: { objectName: 'edu_lead', ...startConfig } }, ...nodes], + edges, + }, + mode: 'draft', + packageId: PKG, + }); + return p.publishPackageDrafts({ packageId: PKG }); +} + +const routeTo = (expression: string) => ({ + nodes: [ + { id: 'route', type: 'decision', label: 'Route', config: { conditions: [{ label: 'Long', expression }] } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'route' }, + { id: 'e2', source: 'route', target: 'end', label: 'Long' }, + ], +}); + +const flowTypeIssue = (raw: any): any => { + const causal = raw.failed?.find((f: any) => f.code === 'INVALID_METADATA' && f.type === 'flow'); + return causal?.issues.find((i: any) => i.rule === 'expression-invalid' && /type mismatch/.test(i.message)); +}; + +describe('publish gives every flow condition a type-mismatch verdict that says what the run does (#29959)', () => { + const RECORD_TRIGGERED = { triggerType: 'record-after-update' }; + + it('refuses a `record.name > 5` start condition of a record-triggered flow with the run-fails wording', async () => { + const raw = await publishLeadFlow( + { ...RECORD_TRIGGERED, condition: 'record.name > 5' }, + [{ id: 'end', type: 'end', label: 'End' }], + [{ id: 'e1', source: 'start', target: 'end' }], + ); + expect(raw.outcome, JSON.stringify(raw)).toBe('refused'); + const issue = flowTypeIssue(raw); + expect(issue, `failed: ${JSON.stringify(raw.failed)}`).toBeDefined(); + expect(issue.severity).toBe('error'); + expect(issue.message).toMatch(/`record\.name` holds text/); + expect(issue.message).toMatch(/fails the run: the flow stops at this condition, the write that fired it still lands/); + }); + + it('refuses a `name > 5` decision branch of a record-triggered flow with the run-fails wording', async () => { + const { nodes, edges } = routeTo('name > 5'); + const raw = await publishLeadFlow(RECORD_TRIGGERED, nodes, edges); + expect(raw.outcome, JSON.stringify(raw)).toBe('refused'); + const issue = flowTypeIssue(raw); + expect(issue, `failed: ${JSON.stringify(raw.failed)}`).toBeDefined(); + expect(issue.severity).toBe('error'); + expect(issue.path).toMatch(/nodes\[1\]\.config\.conditions\[0\]\.expression$/); + expect(issue.message).toMatch(/fails the run: the flow stops at this condition, the write that fired it still lands/); + }); + + it('publishes the same comparison in a flow that is not record-triggered, with a warning that says the run fails', async () => { + const { nodes, edges } = routeTo('name > 5'); + const raw = await publishLeadFlow({ schedule: '0 * * * *', condition: 'record.name > 5' }, 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(2); + for (const a of advisories) { + expect(a.severity).toBe('warning'); + expect(a.message).toMatch(/a flow condition that faults when it is evaluated fails the run/); + expect(a.message).not.toMatch(/evaluates to null|still lands/); + } + }); + + it('publishes a record-triggered flow whose record read and decision branch are well typed (control)', async () => { + const { nodes, edges } = routeTo('size(name) > 5'); + const raw = await publishLeadFlow({ ...RECORD_TRIGGERED, condition: 'size(record.name) > 0' }, 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([]); + }); +}); diff --git a/packages/spec/src/migrations/entries/semantic/18.flow-record-read-and-decision-type-mismatch-refused.ts b/packages/spec/src/migrations/entries/semantic/18.flow-record-read-and-decision-type-mismatch-refused.ts new file mode 100644 index 00000000000..473046098e1 --- /dev/null +++ b/packages/spec/src/migrations/entries/semantic/18.flow-record-read-and-decision-type-mismatch-refused.ts @@ -0,0 +1,37 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import type { SemanticMigration } from '../../types.js'; + +export const entry: SemanticMigration = { + id: 'flow-record-read-and-decision-type-mismatch-refused', + surface: + 'automation.Flow of a record-triggered flow (start config.triggerType record-*): a decision node\'s ' + + 'config.conditions[].expression, and a record.FIELD or previous.FIELD read in the start node ' + + 'config.condition, edges[].condition or a decision branch, where FIELD is a text or boolean field of ' + + 'the trigger object ordered or arithmetic\'d against a number (record.name > 5, title > 5 in a ' + + 'decision branch, record.active > 0); and the same conditions, bare name included, of a time-relative ' + + 'flow (start config.timeRelative) sweeping the start node\'s objectName', + replacement: + 'Compare like with like. size(record.name) > 5 for the text\'s length; a number field ' + + '(record.score > 5) for a value; double(record.name) > 5 only when every value is numeric text, ' + + 'because it faults on any other. A boolean: record.active == true', + reason: + 'A decision branch is evaluated by the engine\'s evaluateCondition over the same run variables as an ' + + 'edge, and the run binds the trigger record both as record (and previous) and flattened to bare ' + + 'names, so record.name and name are the same value. evaluateCondition throws when the expression ' + + 'faults (ADR-0032 section 1c); CEL has no ordering or arithmetic between text and a number, so the ' + + 'run fails at that condition while the record-change trigger isolates the write that fired it. The ' + + 'publish gate refused the bare name in a start or edge condition already, but passed the record.FIELD ' + + 'read and every decision branch with no verdict at all. It now refuses them with the same run-fails ' + + 'wording. A time-relative sweep launches each run with the swept row as its record, the same total ' + + 'row of the same object, so its conditions get the same refusal where 18.3.0 warned. A bare name a ' + + 'declared flow variable or node id holds is the variable, not the field (the run seeds those before ' + + 'it flattens the record), so it is not typed as the field. Any other flow keeps a warning, now worded for the failed run instead ' + + 'of a null. There is no mechanical rewrite: whether the author meant the length, a number stored as ' + + 'text, or another field is a judgement about the condition.', + acceptanceCriteria: + 'os validate and os build report no type-mismatch-run-fails error on a record-triggered or ' + + 'time-relative flow\'s condition, decision branch or record.FIELD read; POST /api/v1/packages/PKG/publish-drafts publishes the flow; a ' + + 'create on POST /api/v1/data/OBJECT that should start the flow answers 201 and the flow run completes ' + + 'past the condition instead of failing there.', +};