From 61a4cdbf1f30155ffa53e25239722c5791af0ad0 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 16:03:44 +0000 Subject: [PATCH 1/2] fix(lint)!: a type mismatch in a record-triggered flow condition or an advisory validation rule is served a verdict that says what really happens - Flow start / edge conditions of a record-triggered flow: refused with the run-fails wording (the run fails, the triggering write lands), the outcome and severity the null-guard twin already has there. - Advisory (`warning` / `info`) validation rules: still refused, as the twin refuses them, but both verdicts now say the rule is skipped and the write proceeds instead of "the write is refused". - One refusal code per outcome: type-mismatch (warning), type-mismatch-fail-closed, type-mismatch-run-fails, type-mismatch-fail-open; the role -> outcome decision lives in one ledger, `faultOutcomeOf`, for both verdicts. - D3 entry flow-condition-type-mismatch-refused; the validation-type-mismatch-refused entry's create answers 201. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01WGufzfu5kYQW45XjsF79Rp --- .../29926-type-mismatch-outcome-per-role.md | 26 ++++ content/docs/data-modeling/formulas.mdx | 2 +- .../src/expression-refusal-codes.test.ts | 65 ++++++++++ packages/formula/src/expression-refusal.ts | 49 +++++-- packages/formula/src/index.ts | 2 +- packages/formula/src/validate.test.ts | 38 +++++- packages/formula/src/validate.ts | 94 +++++++++++--- .../lint/src/validate-expressions.test.ts | 116 ++++++++++++++++- packages/lint/src/validate-expressions.ts | 121 +++++++++++------- packages/lint/src/validate-null-guards.ts | 20 +-- packages/mcp/src/mcp-http-tools.ts | 11 +- .../mcp/src/mcp-validate-expression.test.ts | 13 +- packages/mcp/src/skill-md.ts | 7 +- .../publish-type-mismatch-fail-closed.test.ts | 58 +++++++++ ...18.flow-condition-type-mismatch-refused.ts | 32 +++++ .../18.validation-type-mismatch-refused.ts | 2 +- 16 files changed, 554 insertions(+), 102 deletions(-) create mode 100644 .changeset/29926-type-mismatch-outcome-per-role.md create mode 100644 packages/spec/src/migrations/entries/semantic/18.flow-condition-type-mismatch-refused.ts diff --git a/.changeset/29926-type-mismatch-outcome-per-role.md b/.changeset/29926-type-mismatch-outcome-per-role.md new file mode 100644 index 00000000000..2b936cf4a44 --- /dev/null +++ b/.changeset/29926-type-mismatch-outcome-per-role.md @@ -0,0 +1,26 @@ +--- +'@objectstack/formula': minor +'@objectstack/lint': minor +'@objectstack/mcp': minor +'@objectstack/spec': minor +--- + +fix(lint)!: a text-vs-number comparison in a record-triggered flow condition is refused with what really happens (the run fails, the write lands), and in an advisory validation rule the refusal no longer says the write is refused (#29926) + +Clause-②: no (narrowing) — the author-time gate (`os build`, `os validate`, the package publish and the flow publish door) refuses a `type-mismatch` in a record-triggered flow's start or edge condition that it used to pass with a warning; and the fail-closed arm of the verdict is served under its own code `type-mismatch-fail-closed` instead of `type-mismatch` with `params.outcome`. Added beside them: the codes `type-mismatch-run-fails` and `type-mismatch-fail-open`, and the exported type `ExprFaultOutcome`. +Metadata-protocol: narrowed +Skills-docs: updated here — `content/docs/data-modeling/formulas.mdx` (the type-soundness row names each outcome) and the MCP skill text in `packages/mcp/src/skill-md.ts` (`flow_condition` now errors too); `skills/**` teaches no text-vs-number comparison in a flow condition or an advisory rule, 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 flow conditions — before.** `name > 5` on a text field, as the start node's `condition` or an edge `condition` of a flow whose start `triggerType` is `record-*`, published with an `expression-invalid` warning saying the expression "silently evaluates to null". It does not: the engine's `evaluateCondition` throws on the fault, so the run fails at that condition, and the record-change trigger isolates the write that fired it, so the write lands and the flow's work is not done. +- **Now.** The same condition is refused as `expression-invalid` at `severity: error`, code `type-mismatch-run-fails`, with a message saying the run fails at the condition and the triggering write still lands. This is the severity and outcome the nullable-comparison verdict already has on those conditions (#29026). A flow that is not record-triggered, and a node `condition` other than the start node's (inert), keep the warning. The `validate_expression` MCP tool answers the same for `site: 'flow_condition'`. +- **Advisory validation rules** (`severity: 'warning'` / `'info'`). Still refused at publish, as the nullable twin refuses them, but under `type-mismatch-fail-open` with a message that says what happens: the rule is skipped, the write proceeds, and the rule never gives its advisory. The nullable twin's message on the same rule now says the same instead of "the write is rejected fail-closed". A `severity: 'error'` rule keeps both refusals and their wording. +- **Refusal codes (`@objectstack/formula`).** A code names one message template, so each outcome has its own: `type-mismatch` (warning, reads null), `type-mismatch-fail-closed`, `type-mismatch-run-fails`, `type-mismatch-fail-open` (all errors). `ExprSchemaHint.faultOutcome` takes `'fail-closed' | 'run-fails' | 'fail-open'` (exported as `ExprFaultOutcome`); the `outcome` param on `type-mismatch` is gone. +- **`@objectstack/spec`.** Upgrade guide entry `flow-condition-type-mismatch-refused` (semantic: no mechanical rewrite). The `validation-type-mismatch-refused` entry's acceptance criteria now say a create answers 201, the data door's status. + +**FROM → TO.** Flow condition `name > 5` on a text field → `size(name) > 5` for its length, `score > 5` on a number field for a value, or `double(name) > 5` only when every value is numeric text. A boolean: `active > 0` → `active == true`. A consumer keyed to the code: `type-mismatch` with `params.outcome === 'fail-closed'` → `type-mismatch-fail-closed`. + +**Supersedes, in the same release.** The #29904 changeset (`29904-type-mismatch-fail-closed.md`) says the refusal's `params` carry `outcome: 'fail-closed'`, and that flow conditions keep the warning. Both are replaced here: the fail-closed refusal is served as `type-mismatch-fail-closed` with no `outcome` param, and a record-triggered flow's start and edge conditions are refused as above. diff --git a/content/docs/data-modeling/formulas.mdx b/content/docs/data-modeling/formulas.mdx index cbb55b9b273..e1a87be5270 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 refuses the write instead — a validation rule, a field `requiredWhen` / `readonlyWhen`, an option `visibleWhen`, a hook `condition` — it is an error. 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; error where a fault refuses the write | +| **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 | | **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/expression-refusal-codes.test.ts b/packages/formula/src/expression-refusal-codes.test.ts index f9ff4df6c64..117d53225c8 100644 --- a/packages/formula/src/expression-refusal-codes.test.ts +++ b/packages/formula/src/expression-refusal-codes.test.ts @@ -371,6 +371,62 @@ const PINS: { readonly [C in ExpressionRefusalCode]: readonly [Pin, ...Pin 'field, or drop the arithmetic/comparison.', }, ], + // [#29926] One code per fault outcome the caller declares (`faultOutcome`): + // the three sentences differ in what the runtime does, so they are three + // codes. `type-mismatch-fail-closed` is the sentence #29904 served as + // `type-mismatch` with `outcome: 'fail-closed'`, byte for byte. + 'type-mismatch-fail-closed': [ + { + produce: () => + onlyError('predicate', 'record.name > 5', { scope: 'record', fieldTypes: { name: 'text' }, faultOutcome: 'fail-closed' }), + params: { operands: 'string > int', operator: '>', held: 'text', reference: 'record.name' }, + message: + 'type mismatch `string > int` — `record.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 here ' + + 'an expression that cannot be evaluated fails closed: the write (or the operation a hook guards) is refused, not ' + + 'skipped. ' + + 'Compare like with like: use a number field, compare text with text, or convert explicitly — `size(record.name)` for its ' + + 'length, `double(record.name)` for its value (which itself faults on text that is not a number).', + }, + { + produce: () => + onlyError('predicate', 'record.active > 0', { scope: 'record', fieldTypes: { active: 'boolean' }, faultOutcome: 'fail-closed' }), + 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 here ' + + 'an expression that cannot be evaluated fails closed: the write (or the operation a hook guards) is refused, not ' + + 'skipped. Compare a boolean with `true` / `false`, or use a number field.', + }, + ], + 'type-mismatch-run-fails': [ + { + produce: () => + onlyError('predicate', 'name > 5', { scope: 'flattened', fields: ['name'], fieldTypes: { name: 'text' }, faultOutcome: 'run-fails' }), + 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 here ' + + 'a flow condition that cannot be evaluated fails the run: the flow stops at this condition, the write that fired ' + + 'it still lands, and the work the flow exists to do is not done. ' + + '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).', + }, + ], + 'type-mismatch-fail-open': [ + { + produce: () => + onlyError('predicate', 'record.name > 5', { scope: 'record', fieldTypes: { name: 'text' }, faultOutcome: 'fail-open' }), + params: { operands: 'string > int', operator: '>', held: 'text', reference: 'record.name' }, + message: + 'type mismatch `string > int` — `record.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 here ' + + 'the rule is advisory, so a rule that cannot be evaluated is skipped, not refused: the write proceeds, the rule is ' + + 'never applied (its advisory never fires), and the only trace is a log line saying it could not be evaluated. ' + + 'Compare like with like: use a number field, compare text with text, or convert explicitly — `size(record.name)` for its ' + + 'length, `double(record.name)` for its value (which itself faults on text that is not a number).', + }, + ], 'field-near-miss': [ { produce: () => onlyWarning('predicate', "stauts == 'x'", { fields: ['status'], objectName: 'deal' }), @@ -546,6 +602,15 @@ const SWEEP_SCHEMAS: readonly (ExprSchemaHint | undefined)[] = [ optionFields: ['status'], }, { fields: ['status', 'close_date'], objectName: 'deal', fieldTypes: { close_date: 'date' } }, + // [#29926] The deal schema once per declared fault outcome, so the sweep + // reaches each `type-mismatch-*` code and holds its params to its message. + ...(['fail-closed', 'run-fails', 'fail-open'] as const).map((faultOutcome): ExprSchemaHint => ({ + scope: 'record', + objectName: 'deal', + fields: ['amount', 'name', 'active'], + fieldTypes: { name: 'text', active: 'boolean' }, + faultOutcome, + })), { fields: ['status'] }, { fieldTypes: { account: 'lookup' }, traversalHydration: true }, ]; diff --git a/packages/formula/src/expression-refusal.ts b/packages/formula/src/expression-refusal.ts index 61a282637ef..8b8b2591c50 100644 --- a/packages/formula/src/expression-refusal.ts +++ b/packages/formula/src/expression-refusal.ts @@ -57,6 +57,18 @@ export type CelFieldRole = Exclude; */ export type ExpressionSourceKind = 'array' | 'object' | 'number' | 'boolean' | 'bigint' | 'symbol' | 'function'; +/** + * The params every `type-mismatch*` code interpolates: the CEL operand types, + * the `operator`, what the field holds, and the field as written when known. + * The four codes differ in the outcome their sentence states, never in params. + */ +interface TypeMismatchParams { + readonly operands: string; + readonly operator: string; + readonly held: 'text' | 'boolean'; + readonly reference?: string; +} + /** Code → the params its message interpolates. The keys ARE the closed set. */ export interface ExpressionRefusalParams { /** An envelope whose `source` is present but is not text. */ @@ -169,18 +181,30 @@ export interface ExpressionRefusalParams { */ 'date-duration-value': { readonly reference?: string }; /** - * A text or boolean field used with an arithmetic or ordering `operator` - * against a number. A WARNING (the expression reads null) unless `outcome` - * is `'fail-closed'`: the site refuses the write it guards when the - * expression faults, and the verdict is an ERROR (#29904). + * 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. */ - 'type-mismatch': { - readonly operands: string; - readonly operator: string; - readonly held: 'text' | 'boolean'; - readonly reference?: string; - readonly outcome?: 'fail-closed'; - }; + 'type-mismatch': TypeMismatchParams; + /** + * [#29926] ERROR: the same mismatch at a site that FAILS CLOSED — the + * expression faulting refuses the write (or aborts the operation a hook + * guards). Served as `type-mismatch` with `outcome: 'fail-closed'` by #29904 + * until this code gave the sentence its own name. + */ + 'type-mismatch-fail-closed': TypeMismatchParams; + /** + * [#29926] ERROR: the same mismatch in a record-triggered flow's start or + * edge condition — the run fails at the condition, the write that fired it + * lands. + */ + 'type-mismatch-run-fails': TypeMismatchParams; + /** + * [#29926] ERROR: the same mismatch in an advisory (`warning` / `info`) + * validation rule — the rule is skipped, the write proceeds, and the + * advisory never fires. + */ + 'type-mismatch-fail-open': 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". @@ -243,6 +267,9 @@ const CODE_TABLE = { 'date-text-arithmetic': true, 'date-duration-value': true, 'type-mismatch': true, + 'type-mismatch-fail-closed': true, + 'type-mismatch-run-fails': true, + 'type-mismatch-fail-open': true, 'field-near-miss': true, 'traversal-bare-and-traversed': true, 'traversal-multi-hop': true, diff --git a/packages/formula/src/index.ts b/packages/formula/src/index.ts index 7043686f0b5..6a29696bf30 100644 --- a/packages/formula/src/index.ts +++ b/packages/formula/src/index.ts @@ -165,7 +165,7 @@ export type { UnknownFunctionCall } from './unknown-function'; // ADR-0032 — shared validator + introspection (one validator for build, // registration, and the agent-callable validate_expression tool). export { validateExpression, introspectScope, expectedDialect, inferExpressionType, nearestName, CEL_STDLIB_FUNCTIONS } from './validate'; -export type { FieldRole, ExprInput, ExprSchemaHint, ExprValidationError, ExprValidationResult, InferredValueType } from './validate'; +export type { FieldRole, ExprInput, ExprSchemaHint, ExprFaultOutcome, ExprValidationError, ExprValidationResult, InferredValueType } from './validate'; // #22727 — the declaration authoring stamps onto a formula field: its // `returnType`, and for a provably-money result its currency, copied from the // source field's own `currencyConfig`. The producer behind `returnType: 'currency'`. diff --git a/packages/formula/src/validate.test.ts b/packages/formula/src/validate.test.ts index b96452bacc9..467eea48d11 100644 --- a/packages/formula/src/validate.test.ts +++ b/packages/formula/src/validate.test.ts @@ -771,13 +771,16 @@ describe('validateExpression (ADR-0032)', () => { expect(r.ok).toBe(false); expect(r.warnings).toHaveLength(0); expect(r.errors).toHaveLength(1); - expect(r.errors[0]).toMatchObject({ - code: 'type-mismatch', - params: { operands: 'string > int', operator: '>', held: 'text', reference: 'record.name', outcome: 'fail-closed' }, + // [#29926] Its own code: the fail-closed sentence is not the warning's. + expect(r.errors[0]).toEqual({ + source: 'record.name > 5', + message: expect.any(String), + code: 'type-mismatch-fail-closed', + params: { operands: 'string > int', operator: '>', held: 'text', reference: 'record.name' }, }); expect(r.errors[0].message).not.toMatch(/evaluates to null/); const bool = validateExpression('predicate', 'record.is_active > 0', { ...schema, faultOutcome: 'fail-closed' }); - expect(bool.errors.map((e) => e.code)).toEqual(['type-mismatch']); + expect(bool.errors.map((e) => e.code)).toEqual(['type-mismatch-fail-closed']); // The conversions the message names validate clean in this dialect. for (const fixed of ['size(record.name) > 5', 'double(record.name) > 5']) { const ok = validateExpression('predicate', fixed, { ...schema, faultOutcome: 'fail-closed' }); @@ -785,6 +788,33 @@ describe('validateExpression (ADR-0032)', () => { } }); + // [#29926] The two other outcomes a caller can declare, each an error + // under its own code, worded for what that runtime does. + it('errors with the run-fails wording where the caller declares a flow condition (#29926)', () => { + const flat = { ...schema, scope: 'flattened' as const, faultOutcome: 'run-fails' as const }; + const r = validateExpression('predicate', 'name > 5', flat); + expect(r.warnings).toHaveLength(0); + expect(r.errors.map((e) => [e.code, e.params])).toEqual([ + ['type-mismatch-run-fails', { operands: 'string > int', operator: '>', held: 'text', reference: 'name' }], + ]); + expect(r.errors[0].message).toContain('fails the run'); + expect(r.errors[0].message).toContain('the write that fired it still lands'); + expect(r.errors[0].message).not.toMatch(/evaluates to null|is refused/); + for (const fixed of ['size(name) > 5', 'double(name) > 5']) { + const ok = validateExpression('predicate', fixed, flat); + expect([ok.errors, ok.warnings], fixed).toEqual([[], []]); + } + }); + + 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); + expect(r.errors.map((e) => e.code)).toEqual(['type-mismatch-fail-open']); + expect(r.errors[0].message).toContain('the write proceeds'); + expect(r.errors[0].message).toContain('its advisory never fires'); + expect(r.errors[0].message).not.toMatch(/evaluates to null|is refused/); + }); + it('warns on a text field ordered against a number', () => { const r = validateExpression('predicate', 'record.title >= 5', schema); expect(r.ok).toBe(true); diff --git a/packages/formula/src/validate.ts b/packages/formula/src/validate.ts index 6fcce9072d7..7c6ee47dc4f 100644 --- a/packages/formula/src/validate.ts +++ b/packages/formula/src/validate.ts @@ -223,24 +223,43 @@ export interface ExprSchemaHint { */ optionFields?: readonly string[]; /** - * [#29904] What the site's runtime does when this expression FAULTS, as - * declared by the caller that knows the site — the validator cannot tell a - * validation rule's `predicate` from a flow condition's. + * [#29904, #29926] What the site's runtime does when this expression FAULTS, + * as declared by the caller that knows the site — the validator cannot tell a + * validation rule's `predicate` from a flow condition's. The tokens are the + * null-guard verdict's `NullGuardOutcome` (`@objectstack/lint`), so one site + * names one outcome to both verdicts. * * `'fail-closed'`: an expression that cannot be evaluated refuses the write - * (or aborts the operation) it guards instead of being skipped — an object - * validation rule, a field `requiredWhen` / `readonlyWhen`, an option - * `visibleWhen` on the write path, a hook `condition`. There a - * `type-mismatch` (text or a boolean ordered / arithmetic'd against a - * number) is an ERROR worded for that outcome, the severity the - * nullable-comparison verdict already has at the same sites. + * (or aborts the operation) it guards instead of being skipped — a + * `severity: 'error'` object validation rule, a field `requiredWhen` / + * `readonlyWhen`, an option `visibleWhen` on the write path, a hook + * `condition`. * - * Absent: a fault reads as null (a formula field), and `type-mismatch` stays - * the advisory warning whose "evaluates to null" text is true there. + * `'run-fails'`: a record-triggered flow's start or edge 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. + * + * `'fail-open'`: an advisory (`severity: 'warning' | 'info'`) validation + * rule — a rule that cannot be evaluated is skipped and logged, the write + * proceeds, and the rule never gives its advisory (#24782). + * + * 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. */ - faultOutcome?: 'fail-closed'; + faultOutcome?: ExprFaultOutcome; } +/** + * [#29926] What a site's runtime does once an expression there faults — see + * {@link ExprSchemaHint.faultOutcome}. + */ +export type ExprFaultOutcome = 'fail-closed' | 'run-fails' | 'fail-open'; + /** * One refusal (in `errors[]`) or advisory (in `warnings[]`). * @@ -317,6 +336,40 @@ function toCelFieldTypes(fieldTypes: Readonly>): Record>): Record 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 @@ -376,22 +431,23 @@ function typeSoundnessIssue( : `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( - 'type-mismatch', + arm.code, { operands: mismatch.operands, operator: mismatch.operator, held: heldKind, ...(reference !== undefined ? { reference } : {}), - outcome: 'fail-closed', }, 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 an expression that cannot be ` + - `evaluated fails closed: the write (or the operation a hook guards) is refused, not skipped. ` + + `(an empty value included) the expression faults, and here ${arm.clause} ` + convert, ), }; diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index cb9734690f0..e1f3594d061 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -976,6 +976,113 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { expect(typeMismatches(issues)).toEqual([]); }); + // [#29926] A record-triggered flow's start and edge conditions: the engine + // throws on the fault, so the RUN fails there while the write that fired + // it lands. The null-guard twin refuses there with that outcome + // (`run-fails`, #29026); the type mismatch now does the same, worded so. + it('refuses a type mismatch in a record-triggered flow condition with the run-fails wording (#29926)', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'crm_opportunity', fields: { title: { type: 'text', required: true } } }], + flows: [{ + name: 'opp_flow', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_opportunity', triggerType: 'record-after-update', condition: 'title * 2 > 10' } }, + { id: 'end', type: 'end' }, + ], + edges: [{ id: 'e1', source: 'start', target: 'end', condition: 'title > 5' }], + }], + }); + const found = typeMismatches(issues); + expect(found.map(i => [i.where, i.severity, i.path])).toEqual([ + ["flow 'opp_flow' · node 'start' (start) condition", 'error', 'flows[0].nodes[0].config.condition'], + ["flow 'opp_flow' · edge 'e1' (start→end) condition", 'error', 'flows[0].edges[0].condition'], + ]); + for (const i of found) { + expect(i.message).toContain('fails the run: the flow stops at this condition, the write that fired it still lands'); + expect(i.message).not.toMatch(/evaluates to null|is refused/); + } + expect(found[0].message).toMatch(/`size\(title\)`/); + }); + + it('keeps the warning on a flow that is not record-triggered, and on an inert node condition (#29926)', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'crm_opportunity', fields: { title: { type: 'text', required: true } } }], + flows: [{ + name: 'rt_flow', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_opportunity', triggerType: 'record-after-update' } }, + { id: 'mid', type: 'assignment', config: { condition: 'title > 5' } }, + { id: 'end', type: 'end' }, + ], + edges: [{ id: 'e1', source: 'start', target: 'mid' }, { id: 'e2', source: 'mid', target: 'end' }], + }], + }); + const found = typeMismatches(issues); + expect(found.map(i => [i.where, i.severity])).toEqual([ + ["flow 'rt_flow' · node 'mid' (assignment) condition", 'warning'], + ]); + }); + + // [#29926] An advisory rule (`severity: 'warning' | 'info'`) that cannot be + // evaluated is logged and skipped — the write proceeds (#24782). The + // nullable twin refuses it at publish; so does the type mismatch, and + // both now say what really happens instead of "the write is refused". + it('refuses a type mismatch in an advisory validation rule without claiming the write is refused (#29926)', () => { + for (const severity of ['warning', 'info'] as const) { + const issues = validateStackExpressions({ + objects: [{ + name: 'crm_lead', + fields: { title: { type: 'text', required: true } }, + validations: [ + { name: 'adv', severity, condition: 'record.title > 5' }, + { + name: 'c', + severity, + type: 'conditional', + when: 'record.title == "y"', + then: { name: 'c_then', type: 'script', condition: 'record.title * 2 > 1', message: 'm' }, + message: 'm', + }, + ], + }], + }); + const found = typeMismatches(issues); + expect(found.map(i => [i.where, i.severity]), severity).toEqual([ + ["object 'crm_lead' · validation 'adv'", 'error'], + ["object 'crm_lead' · validation rule 'c' then → 'c_then'", 'error'], + ]); + for (const i of found) { + expect(i.message).toContain('the rule is advisory, so a rule that cannot be evaluated is skipped, not refused'); + expect(i.message).not.toMatch(/is refused|evaluates to null/); + } + } + }); + + it('gives the nullable twin the same outcome on one advisory rule, and keeps fail-closed on an error rule (#29926)', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'crm_lead', + fields: { qty: { type: 'number' }, title: { type: 'text', required: true } }, + validations: [ + { name: 'adv', severity: 'warning', condition: 'record.qty > 1000 && record.title > 5' }, + { name: 'hard', condition: 'record.qty > 1000 && record.title > 5' }, + ], + }], + }); + const adv = issues.filter(i => i.path === 'objects[0].validations[0].condition'); + const hard = issues.filter(i => i.path === 'objects[0].validations[1].condition'); + expect(adv.map(i => i.severity)).toEqual(['error', 'error']); + expect(hard.map(i => i.severity)).toEqual(['error', 'error']); + // Neither verdict on the advisory rule claims a refused write … + for (const i of adv) { + expect(i.message).toMatch(/advisory/); + expect(i.message).not.toMatch(/rejected|is refused/); + } + // … and on the `error` rule both do. + expect(hard.find(i => /declares as nullable/.test(i.message))!.message).toMatch(/write is rejected fail-closed/); + expect(typeMismatches(hard)[0].message).toMatch(/the write \(or the operation a hook guards\) is refused/); + }); + it('warns on a flattened flow condition doing arithmetic on a bare text field', () => { const issues = validateStackExpressions({ objects: [{ name: 'crm_opportunity', fields: { title: { type: 'text' }, amount: { type: 'currency' } } }], @@ -3557,8 +3664,9 @@ const READ_SURFACES: Array<{ receiver: string; expected: string[]; declaredBy: s { receiver: 'rule', // `expression` / `predicate` / `formula` are absent — the chain that put - // the canonical `condition` in third place (#5017). - expected: ['condition', 'name', 'when'], + // the canonical `condition` in third place (#5017). [#29926] `severity` + // picks the rule's fault outcome (an advisory rule is skipped, not refused). + expected: ['condition', 'name', 'severity', 'when'], declaredBy: 'the ObjectSchema.validations[] union', keys: () => shapeKeysOf(ObjectSchema.shape.validations), }, @@ -3823,6 +3931,10 @@ describe('validateStackExpressions — reads only keys the spec declares (meta-t // bare-name set. Their keys are that module's result key and Set // 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 + // tabled receivers (`rule.severity`) — never read off metadata itself. + 'site', ]); 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 6d224dd6e4f..1ed367d38ca 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -96,7 +96,7 @@ import { buildScope, declaresOptionsOnceServed, } from '@objectstack/formula'; -import type { RelationshipTraversalAnalysis } from '@objectstack/formula'; +import type { ExprFaultOutcome, RelationshipTraversalAnalysis } from '@objectstack/formula'; import { collectFlowGraphs, flowNodeConfigRefusals, @@ -1283,36 +1283,62 @@ function isFlattenedNearMissFor(diagnostic: string, roots: readonly string[]): b } /** - * [#20078] The field-level predicate slots a read THROUGH a reference field is - * refused on: the two field-rule slots the server evaluates on a write, and a - * select option's own `visibleWhen`. - */ -/** - * [#29904] The predicate sites whose runtime FAILS CLOSED — an expression that - * cannot be evaluated refuses the write (or aborts the operation) it guards, - * never skipped — handed to the shared validator as - * `ExprSchemaHint.faultOutcome`. There a `type-mismatch` (a text or boolean - * field ordered or arithmetic'd against a number) is an ERROR, as the - * nullable-comparison verdict (`checkNullGuards`, `'fail-closed'`) already is - * at the same sites; everywhere else it stays the advisory warning. - * - * The one decision of which site fails closed, read off each evaluator: - * - * | site | runtime on a fault | passes | - * |:---------------------------------------|:-----------------------------------------------------------|:-------| - * | validation rule `condition` / `when`, nested branches | `rule-validator.ts` `checkPredicate` / `checkConditional`: "write rejected" (#4649) | yes | - * | field `requiredWhen`, `readonlyWhen` | `rule-validator.ts`: refuses the write (ADR-0137 D2) | yes | - * | option `visibleWhen` | `evaluateOptionVisibility`: refuses a write picking it (ADR-0137 D2, #22402) | yes | - * | hook `condition` | `hook-wrappers.ts`: aborts the operation (#4775) | yes | - * | field `expression` (formula) | reads null | no — the warning's text is true | - * | flow start / edge / `decision` condition | the run fails; the write that fired it lands (`'run-fails'`) | no — not a write gate | - * | 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 | no | - * | sharing rule `condition` | compiled to a query filter, not evaluated as CEL per row | no | + * [#29904, #29926] The FAULT-OUTCOME ledger: what each predicate site's runtime + * does once an expression there faults, decided here and nowhere else. The + * answer is handed to the shared validator as `ExprSchemaHint.faultOutcome` + * (the `type-mismatch` verdict: a text or boolean field ordered or + * arithmetic'd against a number) and to {@link checkNullGuards} as its + * `NullGuardOutcome` (the nullable-comparison verdict) — the same token, so + * the two verdicts never tell one rule two different stories. + * + * Read off each evaluator: + * + * | site | runtime on a fault | outcome | + * |:---------------------------------------|:-----------------------------------------------------------|:--------| + * | validation rule `condition` / `when`, nested branches — `severity: 'error'` (the default) | `rule-validator.ts` `checkPredicate` / `checkConditional`: "write rejected" (#4649) | `fail-closed` | + * | the same, `severity: 'warning'` / `'info'` (the TOP-LEVEL rule's; a nested branch inherits it) | `evaluateValidationRules`: logged, "write proceeded: the rule is advisory" (#24782) | `fail-open` | + * | 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`) | — | + * | 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 | — | * * RLS / permission-set row predicates never reach this validator: they are * judged by their own enforceability gates and compiled to a filter too. + * Where a site declares an outcome, a `type-mismatch` is an ERROR under that + * outcome's own code, the severity the null-guard verdict has there; where it + * declares none, it stays the advisory warning. An advisory rule is graded + * `error` too: the twin refuses it, and a rule that can never be evaluated + * never gives its advisory — the message says so instead of a refused write. + */ +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 }; + +/** The ledger above, as the one function every call site asks. */ +function faultOutcomeOf(site: FaultSite): (ExprFaultOutcome & NullGuardOutcome) | undefined { + switch (site.kind) { + case 'validation rule': + // 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; + default: + return 'fail-closed'; + } +} + +/** + * [#20078] The field-level predicate slots a read THROUGH a reference field is + * refused on: the two field-rule slots the server evaluates on a write, and a + * select option's own `visibleWhen`. */ -const FAIL_CLOSED = 'fail-closed' as const; type FieldTraversalSlot = 'requiredWhen' | 'readonlyWhen' | 'option visibleWhen'; @@ -2438,11 +2464,11 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression */ extraRoots?: readonly string[], /** - * [#29904] {@link FAIL_CLOSED} where this site's runtime refuses the write - * (or aborts the operation) when the predicate faults. Absent everywhere - * else; see the constant for the ledger of which sites pass it. + * [#29904, #29926] What this site's runtime does when the predicate + * faults, from {@link faultOutcomeOf} — never spelled at a call site. + * Absent where the ledger declares none. */ - faultOutcome?: typeof FAIL_CLOSED, + faultOutcome?: ExprFaultOutcome, ): void => { if (raw == null) return; // Absent unless this is a served-row site on an object that declares a @@ -2734,6 +2760,9 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression * 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 nullGuardBare = new Set(); if (nullGuardObject && rootScope.provable) { for (const name of nullableIndex.get(nullGuardObject) ?? []) { @@ -2746,7 +2775,7 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression : new Set(); const checkFlowNullGuards = (where: string, raw: unknown, afterStart: boolean): void => { if (!nullGuardObject) return; - checkNullGuards(where, where, raw, nullGuardObject, 'run-fails', { + checkNullGuards(where, where, raw, nullGuardObject, flowFaultOutcome, { bareFields: nullGuardBare, ...(afterStart ? { presumed: nullGuardPresumed } : {}), }); @@ -2867,13 +2896,16 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression nodeSlotKeys.set(nodeCondWhere, 'config.condition'); if (!checkStructuralCondition(nodeCondWhere, cfg.condition).refused) { const from = issues.length; - check(nodeCondWhere, cfg.condition, objectName); + // [#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); warnShadowedFieldReads(nodeCondWhere, cfg.condition); refuseUnboundRoots(nodeCondWhere, cfg.condition, from, 'predicate'); // [#29026] The start node's is the trigger gate; on any other node // the key is inert (`flow-inert-node-condition` says so) and judging // it would only add a second finding about a predicate nothing runs. - if (node.type === 'start' && graph.path.length === 0) checkFlowNullGuards(nodeCondWhere, cfg.condition, false); + if (isTriggerGate) checkFlowNullGuards(nodeCondWhere, cfg.condition, false); } // Descriptor-declared expression slots (#4027). Before this, the traversal @@ -3099,7 +3131,7 @@ 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); + check(edgeCondWhere, edge.condition, objectName, undefined, undefined, undefined, undefined, undefined, flowFaultOutcome); warnShadowedFieldReads(edgeCondWhere, edge.condition); refuseUnboundRoots(edgeCondWhere, edge.condition, from, 'predicate'); checkFlowNullGuards(edgeCondWhere, edge.condition, true); @@ -3134,6 +3166,9 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression // position, so the door's findings and their order are the build's. for (const rule of recordsOf(validations)) { const where = `object '${objectName}' · validation '${(rule.name as string) ?? '?'}'`; + // [#29926] One outcome for every predicate the rule carries: a nested + // branch is governed by the top-level rule's severity at runtime. + const ruleOutcome = faultOutcomeOf({ kind: 'validation rule', severity: rule.severity }); // [#27643] Every finding below also carries the judged slot's key path // (`objects[0].validations[1].condition`), indexed over the lists the // author wrote, in the spelling a flow edge's findings use (#26858), so @@ -3157,10 +3192,10 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression // (#22042), and on a formula field's `expression` (`judgeFieldFormula`, // #24197), and NOWHERE else. let first = issues.length; - check(where, rule.condition, objectName, 'record', undefined, true, undefined, undefined, FAIL_CLOSED); + check(where, rule.condition, objectName, 'record', undefined, true, undefined, undefined, ruleOutcome); atSlot('condition', first); first = issues.length; - check(`${where} when`, (rule as AnyRec).when, objectName, 'record', undefined, true, undefined, undefined, FAIL_CLOSED); + check(`${where} when`, (rule as AnyRec).when, objectName, 'record', undefined, true, undefined, undefined, ruleOutcome); atSlot('when', first); const predicates = rulePredicates(rule, ''); // [#22042] The same verdict one level down, and every level below it: a @@ -3177,7 +3212,7 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression for (const p of predicates) { if (p.depth === 0) continue; first = issues.length; - check(`object '${objectName}' · ${p.label}`, p.raw, objectName, 'record', undefined, true, undefined, undefined, FAIL_CLOSED); + check(`object '${objectName}' · ${p.label}`, p.raw, objectName, 'record', undefined, true, undefined, undefined, ruleOutcome); atSlot(p.key, first); } // [#24720] The rule's root verdict, over every predicate it carries, @@ -3201,7 +3236,7 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression // `then`/`otherwise` branches included. for (const p of predicates) { first = issues.length; - checkNullGuards(`object '${objectName}' · ${p.label}`, p.label, p.raw, objectName); + checkNullGuards(`object '${objectName}' · ${p.label}`, p.label, p.raw, objectName, ruleOutcome); atSlot(p.key, first); } } @@ -3403,7 +3438,7 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression const verdict = fieldRuleRootVerdict(key, raw); // [#29904] Only the two slots the server judges a write by fail closed // (ADR-0137 D2); `visibleWhen` / `settledWhen` are display only. - const writeGate = key === 'requiredWhen' || key === 'readonlyWhen' ? FAIL_CLOSED : undefined; + const writeGate = key === 'requiredWhen' || key === 'readonlyWhen' ? faultOutcomeOf({ kind: 'field write rule' }) : undefined; check(where, raw, objectName, 'record', verdict !== null, undefined, undefined, undefined, writeGate); if (verdict) { issues.push({ where, message: verdict.message, source: verdict.source, severity: 'error' }); @@ -3480,7 +3515,7 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression const optionWhere = `object '${objectName}' · field '${fname}' option ${label} visibleWhen`; // [#29904] Fail-closed on the write path (ADR-0137 D2, #22402): a fault // refuses every write that picks this option. - check(optionWhere, opt.visibleWhen, objectName, 'record', undefined, undefined, undefined, undefined, FAIL_CLOSED); + check(optionWhere, opt.visibleWhen, objectName, 'record', undefined, undefined, undefined, undefined, faultOutcomeOf({ kind: 'option visibleWhen' })); // [#22157] After `check`, before the traversal refusal: every root this // verdict judges is declared in the strict env, so `check` never // reports a bare reference to it and the two stay disjoint. @@ -3735,7 +3770,7 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression for (const hook of objectWrite ? [] : recordsOf(stack.hooks)) { const hookName = (hook.name as string) ?? '?'; if (typeof hook.object === 'string') { - check(`hook '${hookName}' (${hook.object}) condition`, hook.condition, hook.object, 'record', undefined, undefined, undefined, undefined, FAIL_CLOSED); + check(`hook '${hookName}' (${hook.object}) condition`, hook.condition, hook.object, 'record', undefined, undefined, undefined, undefined, faultOutcomeOf({ kind: 'hook condition' })); // #4763 — the third instance the issue found lived on exactly this path. checkNullGuards( `hook '${hookName}' (${hook.object}) condition`, @@ -3768,7 +3803,7 @@ export function runStackExpressionPasses(stack: AnyRec, options: StackExpression const kept: ExprIssue[] = []; for (const target of targets) { const mark = issues.length; - check(`hook '${hookName}' (${target}) condition`, hook.condition, target, 'record', undefined, undefined, undefined, undefined, FAIL_CLOSED); + check(`hook '${hookName}' (${target}) condition`, hook.condition, target, 'record', undefined, undefined, undefined, undefined, faultOutcomeOf({ kind: 'hook condition' })); checkNullGuards( `hook '${hookName}' (${target}) condition`, `hook '${hookName}' condition`, diff --git a/packages/lint/src/validate-null-guards.ts b/packages/lint/src/validate-null-guards.ts index a4f2bae1c0d..691ee209e83 100644 --- a/packages/lint/src/validate-null-guards.ts +++ b/packages/lint/src/validate-null-guards.ts @@ -85,7 +85,7 @@ * * | surface | binding | evidence | verdict | * |:-------------------------------|:--------|:------------------------------------------------------------|:--------| - * | object validation rules | TOTAL | `rule-validator.ts` `materializeDeclaredFields(merged, …)` | covered | + * | object validation rules | TOTAL | `rule-validator.ts` `materializeDeclaredFields(merged, …)` | covered; outcome `fail-closed`, or `fail-open` for an advisory rule (#29926) | * | lifecycle hook `condition` | TOTAL | `hook-wrappers.ts` `materializeDeclaredFields(…)` | covered | * | field `requiredWhen` | TOTAL | same `merged` in `evaluateValidationRules` — fail-OPEN, so an unguarded predicate enforces NOTHING in silence | covered (#4811) | * | field `readonlyWhen` | TOTAL | `rule-validator.ts` `readonlyWhenBindings` materialises BOTH roots (#4953 clause 1, landed in #6454) | excluded — NOT on totality; see below | @@ -739,15 +739,16 @@ export function findUnguardedNullableOperands( * surface's runtime and name what it really implements. */ export type NullGuardOutcome = - /** Validation rules + hook conditions: the fault propagates, the write is refused (#4761). */ + /** `error` validation rules + hook conditions: the fault propagates, the write is refused (#4761). */ | 'fail-closed' /** * A surface whose runtime logs and SKIPS the aborted predicate, so nothing is - * enforced. Written for the field `requiredWhen` (#4811), which no longer - * behaves this way: since ADR-0137 D2 a faulting `requiredWhen` refuses the - * write and takes `'fail-closed'`. No surface passes this outcome today, and - * its clause below still describes the `requiredWhen` runtime it was written - * for — re-word it before a surface that genuinely skips adopts it. + * enforced and the write proceeds: an ADVISORY (`severity: 'warning'` / + * `'info'`) validation rule, whose unevaluable predicate is logged as "write + * proceeded: the rule is advisory" and never gives its advisory (#24782, + * adopted by #29926). Written for the field `requiredWhen` (#4811), which no + * longer behaves this way: since ADR-0137 D2 a faulting `requiredWhen` + * refuses the write and takes `'fail-closed'`. */ | 'fail-open' /** @@ -767,9 +768,8 @@ const OUTCOME_CLAUSE: Record = { 'so the rule enforces nothing and the write is rejected fail-closed (a predicate that cannot ' + 'evaluate refuses the write rather than being skipped)', 'fail-open': - 'so the predicate is SKIPPED fail-open — the field is never actually required, the write ' + - 'proceeds unchecked, and the only trace is a `requiredWhen … failed to evaluate — skipped` ' + - 'log line', + 'so the rule is SKIPPED fail-open: it is advisory, so the write proceeds, the rule is never applied ' + + '(its advisory never fires), and the only trace is a log line saying it could not be evaluated', }; /** diff --git a/packages/mcp/src/mcp-http-tools.ts b/packages/mcp/src/mcp-http-tools.ts index f19f1a8cbde..06bc77302d6 100644 --- a/packages/mcp/src/mcp-http-tools.ts +++ b/packages/mcp/src/mcp-http-tools.ts @@ -71,6 +71,7 @@ import { validateExpression, introspectScope, inferExpressionType, + type ExprFaultOutcome, type FieldRole, } from '@objectstack/formula'; // [#16913] The unknown-key error map every `strictToolInput` shape below is @@ -633,14 +634,18 @@ function jsonText(value: unknown): string { */ const VALIDATE_SITE_MAP: Record< string, - { role: FieldRole; scope: 'record' | 'flattened'; slot: z.ZodType; optionLabels?: true; faultOutcome?: 'fail-closed' } + { role: FieldRole; scope: 'record' | 'flattened'; slot: z.ZodType; optionLabels?: true; faultOutcome?: ExprFaultOutcome } > = { 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' }, - flow_condition: { role: 'predicate', scope: 'flattened', slot: EvaluatedExpressionInputSchema }, + // [#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' }, template: { role: 'template', scope: 'record', slot: TemplateExpressionInputSchema }, }; @@ -1049,7 +1054,7 @@ export function registerObjectTools( '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; date-equality pitfalls), plus the fields and stdlib ' + + 'the write, and at a `flow_condition`, where it fails the run; 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).', inputSchema: strictToolInput( diff --git a/packages/mcp/src/mcp-validate-expression.test.ts b/packages/mcp/src/mcp-validate-expression.test.ts index 8386145e56d..a16dfc1bd5e 100644 --- a/packages/mcp/src/mcp-validate-expression.test.ts +++ b/packages/mcp/src/mcp-validate-expression.test.ts @@ -113,8 +113,7 @@ describe('validate_expression MCP tool (#1928)', () => { it('errors on a text field ordered against a number at a validation site (#29904)', async () => { const r = await validate(runtime, bridge, { objectName: 'crm_opportunity', expression: 'record.title > 5', site: 'validation' }); expect(r.ok).toBe(false); - expect(r.errors.map((e: any) => e.code)).toEqual(['type-mismatch']); - expect(r.errors[0].params.outcome).toBe('fail-closed'); + expect(r.errors.map((e: any) => e.code)).toEqual(['type-mismatch-fail-closed']); expect(r.warnings).toHaveLength(0); }); @@ -122,8 +121,14 @@ describe('validate_expression MCP tool (#1928)', () => { const ok = await validate(runtime, bridge, { objectName: 'crm_opportunity', expression: 'stage == "won" && amount > 1000', site: 'flow_condition' }); expect(ok.ok).toBe(true); expect(ok.warnings).toHaveLength(0); - const warn = await validate(runtime, bridge, { objectName: 'crm_opportunity', expression: 'title * 2 > 10', site: 'flow_condition' }); - expect(warn.warnings.some((w: any) => /type mismatch/i.test(w.message))).toBe(true); + // [#29926] A flow condition fails the run on the fault, and the publish + // gate refuses the comparison on a record-triggered flow; the tool's + // `flow_condition` site answers what the gate will. + const bad = await validate(runtime, bridge, { objectName: 'crm_opportunity', expression: 'title * 2 > 10', site: 'flow_condition' }); + expect(bad.ok).toBe(false); + expect(bad.warnings).toHaveLength(0); + expect(bad.errors.map((e: any) => e.code)).toEqual(['type-mismatch-run-fails']); + expect(bad.errors[0].message).toContain('fails the run'); }); it('errors clearly when the object does not exist', async () => { diff --git a/packages/mcp/src/skill-md.ts b/packages/mcp/src/skill-md.ts index 380daaa07c6..ea29efe40d4 100644 --- a/packages/mcp/src/skill-md.ts +++ b/packages/mcp/src/skill-md.ts @@ -268,9 +268,10 @@ create/update payload. 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), - plus the fields and stdlib functions in scope. \`site\` is - \`formula\` | \`validation\` | \`flow_condition\` | \`template\` (default \`formula\`). + 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 + stdlib functions in scope. \`site\` is \`formula\` | \`validation\` | + \`flow_condition\` | \`template\` (default \`formula\`). - **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 2e24c661f68..d823820e388 100644 --- a/packages/objectql/src/publish-type-mismatch-fail-closed.test.ts +++ b/packages/objectql/src/publish-type-mismatch-fail-closed.test.ts @@ -205,4 +205,62 @@ describe('publish refuses a type mismatch in a fail-closed role (#29904)', () => expect(issues[0].severity).toBe('error'); expect(issues[0].message).toMatch(/declares as nullable/); }); + + // [#29926] An advisory rule is skipped on a fault, not refused: the publish + // still refuses it (the nullable twin does too), but the served message + // says the write proceeds and the rule never applies. + it('refuses the comparison in an advisory rule with a message that does not claim a refused write (#29926)', async () => { + const raw = await publishObject(leadObject( + { name: { type: 'text', label: 'Name', required: true } }, + { validations: [{ name: 'name_gt', type: 'script', condition: 'record.name > 5', message: 'bad', severity: 'warning' }] }, + )); + + expect(raw.outcome).toBe('refused'); + const causal = raw.failed.find((f: any) => f.code === 'INVALID_METADATA'); + const issue = causal.issues.find((i: any) => i.rule === 'expression-invalid'); + expect(issue, `issues: ${JSON.stringify(causal.issues)}`).toBeDefined(); + expect(issue.severity).toBe('error'); + expect(issue.path).toMatch(/validations\[0\]\.condition$/); + expect(issue.message).toMatch(/the rule is advisory, so a rule that cannot be evaluated is skipped, not refused: the write proceeds/); + expect(issue.message).not.toMatch(/is refused, not skipped/); + }); + + // [#29926] A record-triggered flow's condition fails the RUN on a fault; + // the publish refuses it with that wording, as the nullable twin does. + it('refuses the comparison in a record-triggered flow condition with the run-fails wording (#29926)', async () => { + 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: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start', config: { objectName: 'edu_lead', triggerType: 'record-after-update', condition: 'name > 5' } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [{ id: 'e1', source: 'start', target: 'end' }], + }, + mode: 'draft', + packageId: PKG, + }); + const raw: any = await p.publishPackageDrafts({ packageId: PKG }); + + expect(raw.outcome, JSON.stringify(raw)).toBe('refused'); + const causal = raw.failed.find((f: any) => f.code === 'INVALID_METADATA' && f.type === 'flow'); + expect(causal, `failed: ${JSON.stringify(raw.failed)}`).toBeDefined(); + const issue = causal.issues.find((i: any) => i.rule === 'expression-invalid' && /type mismatch/.test(i.message)); + expect(issue, `issues: ${JSON.stringify(causal.issues)}`).toBeDefined(); + expect(issue.severity).toBe('error'); + expect(issue.message).toMatch(/fails the run: the flow stops at this condition, the write that fired it still lands/); + expect(issue.message).not.toMatch(/evaluates to null/); + }); }); diff --git a/packages/spec/src/migrations/entries/semantic/18.flow-condition-type-mismatch-refused.ts b/packages/spec/src/migrations/entries/semantic/18.flow-condition-type-mismatch-refused.ts new file mode 100644 index 00000000000..07bc9ecbae5 --- /dev/null +++ b/packages/spec/src/migrations/entries/semantic/18.flow-condition-type-mismatch-refused.ts @@ -0,0 +1,32 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import type { SemanticMigration } from '../../types.js'; + +export const entry: SemanticMigration = { + id: 'flow-condition-type-mismatch-refused', + surface: + 'automation.Flow start node config.condition and edges[].condition of a record-triggered flow ' + + '(start config.triggerType record-*): a text or boolean field of the trigger object ordered or ' + + 'arithmetic\'d against a number (name > 5 on a text field, active > 0 on a boolean)', + replacement: + 'Compare like with like. size(name) > 5 for the text\'s length; a number field (score > 5) for a ' + + 'value; double(name) > 5 only when every value is numeric text, because it faults on any other. ' + + 'A boolean: active == true', + reason: + 'A record-triggered flow\'s start and edge conditions are evaluated by the engine\'s ' + + 'evaluateCondition, which throws when the expression faults (ADR-0032 section 1c) instead of ' + + 'reading false or null. CEL has no ordering or arithmetic between text and a number, so name > 5 ' + + 'faults on every value that is not numeric text: the run fails at that condition, while the ' + + 'record-change trigger isolates the write that fired it, so the write lands and the work the flow ' + + 'exists to do is not done. The publish gate passed it with a warning saying the expression ' + + 'silently evaluates to null, which is true only of a formula field. The gate now refuses it there ' + + 'with the run-fails wording, the severity the nullable-comparison verdict already has on the same ' + + 'conditions. Flows that are not record-triggered keep the warning. 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 flow\'s ' + + 'start or edge condition; 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.', +}; diff --git a/packages/spec/src/migrations/entries/semantic/18.validation-type-mismatch-refused.ts b/packages/spec/src/migrations/entries/semantic/18.validation-type-mismatch-refused.ts index 4b7b67cd5ae..002a9e573ae 100644 --- a/packages/spec/src/migrations/entries/semantic/18.validation-type-mismatch-refused.ts +++ b/packages/spec/src/migrations/entries/semantic/18.validation-type-mismatch-refused.ts @@ -27,6 +27,6 @@ export const entry: SemanticMigration = { acceptanceCriteria: 'os validate and os build report no type mismatch error on a validation rule, field requiredWhen / ' + 'readonlyWhen, option visibleWhen or hook condition; POST /api/v1/packages/PKG/publish-drafts ' - + 'publishes the object; a create on POST /api/v1/data/OBJECT that the rule should admit answers 200 ' + + 'publishes the object; a create on POST /api/v1/data/OBJECT that the rule should admit answers 201 ' + 'where it answered 400 VALIDATION_FAILED "could not be evaluated".', }; From 06abf5acf76683083d0ef57c5a0d7af70053f4fa Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 17:30:26 +0000 Subject: [PATCH 2/2] =?UTF-8?q?fix(lint):=20rework=20r1=20=E2=80=94=20the?= =?UTF-8?q?=20changeset=20states=20the=20renamed=20code=20shipped=20in=201?= =?UTF-8?q?8.3.0;=20the=20non-record=20flow=20warning=20is=20pinned=20on?= =?UTF-8?q?=20a=20start=20and=20an=20edge?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Changeset: replace the "Supersedes, in the same release" paragraph. `@objectstack/formula@18.3.0` / `@objectstack/mcp@18.3.0` already serve `type-mismatch` + `params.outcome: 'fail-closed'` (shipped without a changelog line); this release renames that arm and removes `outcome`. - Lint test: the non-record-triggered case now covers a flow with no `triggerType` and a `schedule` flow, start and edge condition each; the inert-node case is its own test. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01WGufzfu5kYQW45XjsF79Rp --- .../29926-type-mismatch-outcome-per-role.md | 2 +- .../lint/src/validate-expressions.test.ts | 29 ++++++++++++++++++- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/.changeset/29926-type-mismatch-outcome-per-role.md b/.changeset/29926-type-mismatch-outcome-per-role.md index 2b936cf4a44..fcdd474ae02 100644 --- a/.changeset/29926-type-mismatch-outcome-per-role.md +++ b/.changeset/29926-type-mismatch-outcome-per-role.md @@ -23,4 +23,4 @@ Skills-docs: updated here — `content/docs/data-modeling/formulas.mdx` (the typ **FROM → TO.** Flow condition `name > 5` on a text field → `size(name) > 5` for its length, `score > 5` on a number field for a value, or `double(name) > 5` only when every value is numeric text. A boolean: `active > 0` → `active == true`. A consumer keyed to the code: `type-mismatch` with `params.outcome === 'fail-closed'` → `type-mismatch-fail-closed`. -**Supersedes, in the same release.** The #29904 changeset (`29904-type-mismatch-fail-closed.md`) says the refusal's `params` carry `outcome: 'fail-closed'`, and that flow conditions keep the warning. Both are replaced here: the fail-closed refusal is served as `type-mismatch-fail-closed` with no `outcome` param, and a record-triggered flow's start and edge conditions are refused as above. +**The renamed code shipped in 18.3.0.** `@objectstack/formula@18.3.0` and `@objectstack/mcp@18.3.0` already serve #29904's fail-closed refusal as `type-mismatch` with `params.outcome: 'fail-closed'` (the `outcome` param is in the 18.3.0 `ExpressionRefusalParams` type), although 18.3.0's changelog has no line for it: its changeset (`29904-type-mismatch-fail-closed.md`) was not consumed by that release. This release renames that arm `type-mismatch-fail-closed` and removes the `outcome` param, so a consumer keyed to 18.3.0's `type-mismatch` + `outcome` moves to the new code (FROM → TO above). The same changeset also says flow conditions keep the warning; a record-triggered flow's start and edge conditions are now refused, as above. diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index e1f3594d061..5934e9a14fe 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -1004,7 +1004,34 @@ 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, and on an inert node condition (#29926)', () => { + 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 * * * *' }], + ] as const) { + const issues = validateStackExpressions({ + objects: [{ name: 'crm_opportunity', fields: { title: { type: 'text', required: true } } }], + flows: [{ + name, + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_opportunity', condition: 'title * 2 > 10', ...startConfig } }, + { id: 'end', type: 'end' }, + ], + edges: [{ id: 'e1', source: 'start', target: 'end', condition: 'title > 5' }], + }], + }); + 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'], + ]); + } + }); + + 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 } } }], flows: [{