diff --git a/.changeset/decision-predicate-envelope-refused.md b/.changeset/decision-predicate-envelope-refused.md new file mode 100644 index 0000000000..2d29f4e39b --- /dev/null +++ b/.changeset/decision-predicate-envelope-refused.md @@ -0,0 +1,16 @@ +--- +"@objectstack/spec": minor +"@objectstack/service-automation": minor +"@objectstack/lint": minor +--- + +A flow predicate authored as a CEL envelope is now refused at build time, instead of running unread by either validator. + +A `predicate`-role expression slot holds **bare CEL text** — `DecisionConditionSchema.expression` is declared `z.string()`, and so is a screen field's `visibleWhen`. An author who instead wrote the `{ dialect, source }` expression *envelope* there reached a shape nothing could see: a flow node's `config` is an open `z.record(z.unknown())` that no Zod schema is parsed against, the unknown-key walk exempts the schemaless node types on purpose (`decision` publishes no descriptor `configSchema`), and the expression ledger's `predicate` arm skipped every non-string as "a type violation for the schema pass to report" — a schema pass that, for those node types, does not exist. `registerFlow` accepted the flow, `objectstack validate` reported nothing, and the evaluator was the only layer that ever read the predicate. + +- `resolveFlowNodeExpressions` now emits a non-string sitting in a `predicate` slot, and the new `predicateSlotRefusal` / `PREDICATE_SLOT_STRING_REFUSAL` say why it is refused — one notion, derived once, read by both validators so build time and author time cannot disagree about the shape. `flow-template` slots keep the old rule: no validator implements that dialect, so a finding there is one nobody could judge. +- `registerFlow` throws, naming the node, the slot and the index, and attributing the finding to the envelope's own `source`. `objectstack validate` reports the same refusal as a located `error`. + +**String predicates are untouched, deliberately.** A whitespace-only string still means "not authored" on both sides, exactly as before; what a non-empty string *says* is still judged by `validateExpression('predicate', …)`, brace trap and all. Only the shape moved. + +An app that authored an envelope in one of these slots now fails to register with a message naming the slot; the fix is to write the predicate as bare CEL text (`record.rating >= 4`). The `{ dialect, source }` envelope remains the `value`-role spelling, on the `assignment` node's `assignments` map. diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index f122a45ead..8a3544333d 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -11,7 +11,7 @@ import { FieldSchema, ObjectSchema, SelectOptionSchema } from '@objectstack/spec import { SharingRuleSchema } from '@objectstack/spec/security'; // [#15137] The published refusal sentence a `value`-slot finding must lead // with — asserted from the spec's own export, never re-spelled in a test. -import { ASSIGNMENT_VALUE_ENVELOPE_REFUSAL } from '@objectstack/spec/automation'; +import { ASSIGNMENT_VALUE_ENVELOPE_REFUSAL, PREDICATE_SLOT_STRING_REFUSAL } from '@objectstack/spec/automation'; import { validateStackExpressions, @@ -2117,6 +2117,69 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { expect(issues.filter(i => i.where.includes('conditions'))).toHaveLength(0); }); + /** + * [#15572] The same slot, authored as a CEL **envelope** rather than as the + * bare CEL text it is declared to hold (`DecisionConditionSchema.expression` + * is `z.string()`). Before this, `objectstack validate` reported nothing: + * the ledger's `predicate` arm emitted strings only, skipping the envelope + * as "a type violation for the schema pass to report" — and `decision` + * publishes no descriptor `configSchema`, so no schema pass ever ran. + * + * ⚠️ Read the RED CONTROL below before trusting any zero here. The reading + * this card was filed on used a second envelope as its control — a control + * that could itself have been the answer, which makes it no control at all + * — so this test drives a form that DOES report (the brace-trap string, on + * this very slot, through this very call) in the same run as the forms + * under test. Without it, a harness that reached the slot and a harness + * that reached nothing would read identically. + */ + describe('a predicate slot authored as an expression envelope (#15572)', () => { + const atSlot = (expression: unknown) => + validateStackExpressions(decisionFlow(expression as string)) + .filter(i => i.where.includes('conditions[0].expression')); + + it('RED CONTROL — the brace-trap string on this slot still reports', () => { + // Not an envelope, so it cannot be "the answer" to what is being + // measured; it proves only that this call reaches this slot and can + // emit. If this ever goes to zero, every zero below is void. + const control = atSlot("{lead_record.status} == 'converted'"); + expect(control).toHaveLength(1); + expect(control[0].severity).toBe('error'); + expect(control[0].message).toContain('template brace'); + }); + + it('reports a `{ dialect, source }` envelope, whatever the source says', () => { + for (const envelope of [ + { dialect: 'cel', source: ' ' }, // the silent-`false` shape + { dialect: 'cel', source: 'rows.map(r,' }, // the throwing shape + { dialect: 'cel', ast: { kind: 'const' } }, // no `source` at all + ]) { + const found = atSlot(envelope); + expect(found).toHaveLength(1); + expect(found[0].severity).toBe('error'); + expect(found[0].message.startsWith(PREDICATE_SLOT_STRING_REFUSAL)).toBe(true); + expect(found[0].where).toContain('decision branch expression'); + } + }); + + it('attributes the finding to the envelope’s own source when it has one', () => { + expect(atSlot({ dialect: 'cel', source: 'rows.map(r,' })[0].source).toBe('rows.map(r,'); + expect(atSlot({ dialect: 'cel', ast: { kind: 'const' } })[0].source).toBe(''); + }); + + it('reports every other non-string on the same rule, naming what it found', () => { + expect(atSlot(42)[0].message).toContain('Found a number'); + expect(atSlot(['a > 1'])[0].message).toContain('Found an array'); + }); + + it('leaves string predicates alone — including the whitespace-only one', () => { + // The card states this boundary explicitly so nobody "fixes" it: a + // whitespace-only STRING is "not authored" on both sides and stays so. + expect(atSlot(' ')).toHaveLength(0); + expect(atSlot("lead_record.status == 'converted'")).toHaveLength(0); + }); + }); + it('leaves a correct single-brace loop collection alone', () => { // `loop.collection` is the single-brace `{var}` flow-interpolation dialect, // where braces are CORRECT. It is recorded in the ledger as `flow-template` @@ -2850,6 +2913,12 @@ describe('validateStackExpressions — reads only keys the spec declares (meta-t // keys; the metadata keys it judges are `AssignmentValueSchema`'s, read // by the schema and not by name here. 'shape', + // [#15572] The spec's shared refusal for a non-string in a predicate + // slot. Its keys are that helper's own `{ message, source }` — never + // metadata keys — and it is named to stay clear of the `message` / + // `source` receivers for the reason the entry above it records: a local + // called `message` here would be excused into masking a genuine read. + 'shapeRefusal', // [#14089] NOT a receiver at all — the tail of the `'./flow-variable-scope.js'` // import specifier, which this scan cannot tell from `scope.j…`. The two // entries above it in this set (`fields`, `guards`) are the same artefact diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index 49411669ec..b2ffa07470 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -79,7 +79,7 @@ */ import { validateExpression, collectCelRootIdentifiers, parseCelToAst, SCOPE_ROOTS } from '@objectstack/formula'; -import { collectFlowGraphs, resolveFlowNodeExpressions } from '@objectstack/spec/automation'; +import { collectFlowGraphs, predicateSlotRefusal, resolveFlowNodeExpressions } from '@objectstack/spec/automation'; // [#15137] The `value`-role half. Same two published primitives the engine // composes at `registerFlow` (`AutomationEngine.valueEnvelopeRefusals`), in the // same order: the SHAPE rule lives in the spec's `AssignmentValueSchema` (it @@ -1065,11 +1065,22 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { * 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. */ - const checkDeclaredPredicate = (where: string, raw: unknown): void => { - if (raw == null) return; + const checkDeclaredPredicate = (where: string, raw: unknown): { refused: boolean } => { + if (raw == null) return { refused: false }; + // [#15572] The slot is declared bare CEL TEXT, so a non-string — the + // `{ dialect, source }` envelope above all — is refused on SHAPE before + // anything tries to read a source out of it. The refusal is the spec's, + // shared with the engine's `registerFlow` pass: `error`, because that pass + // throws, and a shape build refuses must not pass author time. + const shapeRefusal = predicateSlotRefusal(raw); + if (shapeRefusal) { + issues.push({ where, message: shapeRefusal.message, source: shapeRefusal.source, severity: 'error' }); + return { refused: true }; + } 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' }); + return { refused: false }; }; /** @@ -1184,7 +1195,10 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { continue; } if (found.entry.role !== 'predicate') continue; - checkDeclaredPredicate(slotWhere, found.value); + // [#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; // [#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 diff --git a/packages/services/service-automation/src/builtin/config-expression-ledger.test.ts b/packages/services/service-automation/src/builtin/config-expression-ledger.test.ts index cef90e3dc9..72625d2214 100644 --- a/packages/services/service-automation/src/builtin/config-expression-ledger.test.ts +++ b/packages/services/service-automation/src/builtin/config-expression-ledger.test.ts @@ -281,17 +281,26 @@ describe('resolveFlowNodeExpressions — path resolution (#4027)', () => { expect(found[0].entry.role).toBe('flow-template'); }); - it('skips absent, empty and non-string values rather than inventing findings', () => { + it('skips absent and empty values rather than inventing findings', () => { expect(resolveFlowNodeExpressions('screen', {})).toEqual([]); expect(resolveFlowNodeExpressions('screen', { fields: [] })).toEqual([]); expect(resolveFlowNodeExpressions('screen', { fields: [{ visibleWhen: ' ' }] })).toEqual([]); - // A non-string in an expression slot is a type violation for the schema pass - // to report — not something to hand to a parser. - expect(resolveFlowNodeExpressions('screen', { fields: [{ visibleWhen: true }] })).toEqual([]); // A repeater authored as a non-array must not throw. expect(resolveFlowNodeExpressions('screen', { fields: 'nope' })).toEqual([]); }); + // [#15572] A NON-string in a predicate slot is no longer skipped. It was + // skipped as "a type violation for the schema pass to report", and for the + // schemaless node types — `decision` publishes no descriptor `configSchema`, + // so `validateNodeConfigKeys` exempts it and nothing parses its config + // against `DecisionConfigSchema` — that schema pass does not exist. The value + // is emitted so `registerFlow` and `objectstack validate` can refuse it. + it('emits a non-string in a predicate slot for the consumer to refuse (#15572)', () => { + expect(resolveFlowNodeExpressions('screen', { fields: [{ visibleWhen: true }] }) + .map((f) => [f.path, f.value, f.entry.role])) + .toEqual([['fields[0].visibleWhen', true, 'predicate']]); + }); + it('resolves each decision branch predicate, with its index (#4439)', () => { const found = resolveFlowNodeExpressions('decision', { conditions: [ diff --git a/packages/services/service-automation/src/decision-predicate-envelope.test.ts b/packages/services/service-automation/src/decision-predicate-envelope.test.ts new file mode 100644 index 0000000000..b08c49c3f6 --- /dev/null +++ b/packages/services/service-automation/src/decision-predicate-envelope.test.ts @@ -0,0 +1,174 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, beforeEach } from 'vitest'; +import { LiteKernel } from '@objectstack/core'; +// The published refusal sentence, asserted from the spec's own export and +// never re-spelled here: one notion of "a predicate slot takes text", derived +// once and read by both validators. +import { PREDICATE_SLOT_STRING_REFUSAL } from '@objectstack/spec/automation'; + +import { AutomationServicePlugin } from './plugin.js'; +import type { AutomationEngine } from './engine.js'; + +/** + * [#15572] A `decision` branch predicate authored as a CEL **envelope** — + * `{ dialect, source }` in a slot declared `z.string()`. + * + * Four declarations left it invisible to everyone with an opinion: + * `DecisionConditionSchema.expression` is `z.string()` but a node's `config` is + * an open `z.record` that schema is never parsed against; the unknown-key walk + * exempts the schemaless node types (`decision` publishes no descriptor + * `configSchema` on purpose); the expression ledger's `predicate` arm emitted + * strings only, skipping the envelope as "a type violation for the schema pass" + * that, for those types, does not exist; and `evaluateCondition` accepts the + * envelope. So the flow registered clean, `objectstack validate` reported + * nothing, and the only layer that ever read the predicate was the evaluator. + * + * Measured on the unfixed engine, driven end to end (`registerFlow` then + * `execute`), and worth recording because it corrects the filed symptom: through + * the DECISION door the run did not take the other branch silently — the + * executor wraps the authored value as `{ dialect: 'cel', source: }`, + * so `evaluateCondition` read a non-string `source` and the run failed with + * `exprStr.trim is not a function`, an unattributed TypeError with no source and + * no location. The silent `false` is real one door over — `evaluateCondition` + * called with the envelope directly, which is how an `edge.condition` reaches it + * — and that arm is deliberately NOT touched here: it is the shared evaluator, + * and the whitespace-source envelope it turns on belongs to #15430. + * + * The fix is at the producer, per the contract-first rule: the flow does not + * register. Both run-time shapes are then unreachable from authored metadata. + */ +describe('decision branch predicate — envelope in a `z.string()` slot (#15572)', () => { + let engine: AutomationEngine; + + beforeEach(async () => { + const kernel = new LiteKernel(); + kernel.use(new AutomationServicePlugin()); + await kernel.bootstrap(); + engine = kernel.getService('automation') as AutomationEngine; + }); + + const decisionFlow = (name: string, expression: unknown) => ({ + name, + label: name, + type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'check', type: 'decision', label: 'Check', config: { conditions: [{ label: 'yes', expression }] } }, + { id: 'yes_node', type: 'assignment', label: 'Yes', config: { assignments: { took: 'yes' } } }, + { id: 'no_node', type: 'assignment', label: 'No', config: { assignments: { took: 'no' } } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e0', source: 'start', target: 'check' }, + { id: 'e1', source: 'check', target: 'yes_node', label: 'yes' }, + { id: 'e2', source: 'check', target: 'no_node', label: 'default', isDefault: true }, + { id: 'e3', source: 'yes_node', target: 'end' }, + { id: 'e4', source: 'no_node', target: 'end' }, + ], + }); + + /** + * The serious half as filed: a whitespace-only SOURCE inside the envelope. + * Nothing downstream can tell it apart from "no predicate at all", so the + * only place it can be caught is here, on the shape. + */ + it('refuses a whitespace-source envelope at registration', () => { + const register = () => engine.registerFlow('ws_flow', decisionFlow('ws_flow', { dialect: 'cel', source: ' ' })); + expect(register).toThrow(PREDICATE_SLOT_STRING_REFUSAL); + // Located: which node, which slot, which index. + expect(register).toThrow(/node 'check' \(decision\) decision branch expression at config\.conditions\[0\]\.expression/); + expect(register).toThrow(/an expression envelope/); + }); + + it('refuses an un-parseable-source envelope at registration', () => { + const register = () => engine.registerFlow('bad_flow', decisionFlow('bad_flow', { dialect: 'cel', source: 'rows.map(r,' })); + expect(register).toThrow(PREDICATE_SLOT_STRING_REFUSAL); + // The envelope's own source is attributed, so the author is pointed at + // the text they wrote rather than at an empty string. + expect(register).toThrow(/source: `rows\.map\(r,`/); + }); + + it('refuses an `ast`-only envelope, and every other non-string, on the same rule', () => { + // NOT a claim about `ExpressionSchema` accepting `source`-or-`ast` + // (#15430, a different card and a different mechanism): this refusal is + // about the SHAPE reaching a slot declared as text, whatever is in it. + expect(() => engine.registerFlow('ast_flow', decisionFlow('ast_flow', { dialect: 'cel', ast: { kind: 'const' } }))) + .toThrow(PREDICATE_SLOT_STRING_REFUSAL); + expect(() => engine.registerFlow('num_flow', decisionFlow('num_flow', 42))) + .toThrow(/Found a number/); + expect(() => engine.registerFlow('bool_flow', decisionFlow('bool_flow', true))) + .toThrow(/Found a boolean/); + }); + + /** + * The boundary the card states explicitly, so nobody "fixes" it: a + * whitespace-only STRING predicate behaves consistently on both sides — + * "not authored" at the ledger, `false` at the evaluator — and is left + * exactly as it was. Only the envelope shape moved. + */ + it('leaves string predicates alone — including the whitespace-only one', () => { + expect(() => engine.registerFlow('str_ok', decisionFlow('str_ok', 'record.rating >= 4'))).not.toThrow(); + expect(() => engine.registerFlow('str_ws', decisionFlow('str_ws', ' '))).not.toThrow(); + expect(() => engine.registerFlow('str_absent', decisionFlow('str_absent', undefined))).not.toThrow(); + }); + + /** + * The pre-existing arm still issues its own verdict: a refusal on shape + * must not swallow the #1491 brace-trap a malformed STRING earns. + */ + it('still reports a brace-in-CEL string predicate as the brace trap, not as a shape refusal', () => { + const register = () => engine.registerFlow('brace_flow', decisionFlow('brace_flow', '{record.rating} >= 4')); + expect(register).toThrow(/template braces|bare CEL/); + expect(register).not.toThrow(PREDICATE_SLOT_STRING_REFUSAL); + }); + + /** One class, not one node type — `screen.fields[].visibleWhen` is the other predicate slot. */ + it('refuses an envelope in a screen field `visibleWhen` on the same rule', () => { + const register = () => engine.registerFlow('screen_flow', { + name: 'screen_flow', + label: 'screen_flow', + type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'form', type: 'screen', label: 'Form', + config: { fields: [{ name: 'amount', type: 'number', visibleWhen: { dialect: 'cel', source: 'x == 1' } }] }, + }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e0', source: 'start', target: 'form' }, + { id: 'e1', source: 'form', target: 'end' }, + ], + }); + expect(register).toThrow(PREDICATE_SLOT_STRING_REFUSAL); + expect(register).toThrow(/screen field visibleWhen at config\.fields\[0\]\.visibleWhen/); + }); + + /** + * The run-time consequence, closed at the producer: the flow never enters + * the registry, so there is nothing to execute. Asserting through + * `execute()` rather than by re-reading the registry keeps the pin on the + * behaviour that mattered — a run that decided a branch on a predicate no + * validator had read. + */ + it('leaves nothing to run: the refused flow is not registered', async () => { + expect(() => engine.registerFlow('never', decisionFlow('never', { dialect: 'cel', source: ' ' }))).toThrow(); + const result = await engine.execute('never'); + expect(result.success).toBe(false); + expect(String(result.error)).toMatch(/not found|not registered|Flow/i); + }); + + /** + * The same flow with a plain string predicate still registers AND still + * runs to completion — the refusal is scoped to the shape, and a run + * through the decision executor is what it is scoped away from. + */ + it('still registers and runs a plain string predicate', async () => { + expect(() => engine.registerFlow('runs', decisionFlow('runs', 'true'))).not.toThrow(); + const result = await engine.execute('runs'); + expect(result.success).toBe(true); + expect(result.error).toBeUndefined(); + }); +}); diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index d765ab2e0c..f8fc575ee2 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -25,7 +25,7 @@ import { FlowSchema, FLOW_STRUCTURAL_NODE_TYPES, validateControlFlow, collectFlo // `validate-flow-trigger-readiness`, so the runtime cannot drift from what // authoring accepted. See `resolveTriggerBinding`. import { resolveFlowTriggerKind } from '@objectstack/spec/automation'; -import { resolveFlowNodeExpressions } from '@objectstack/spec/automation'; +import { predicateSlotRefusal, resolveFlowNodeExpressions } from '@objectstack/spec/automation'; // [#15137] The `value`-role half of the ledger. Both halves of "is this envelope // well-formed?" are IMPORTED, never re-spelled here: the shape rule is // `AssignmentValueSchema` (spec, #14149 — it refuses a non-`cel` dialect and the @@ -7152,6 +7152,24 @@ export class AutomationEngine implements IAutomationService { continue; } if (found.entry.role !== 'predicate') continue; + // [#15572] A predicate slot is declared bare CEL TEXT + // (`z.string()`), and until now a NON-string there was seen + // by nobody: the resolver skipped it for "the schema pass" + // to report, `validateNodeConfigKeys` exempts the schemaless + // types that own two of these slots, and `evaluateCondition` + // accepted the `{ dialect, source }` envelope regardless — + // so a `decision` branch predicate written as an envelope + // registered clean and only the evaluator ever read it. + // Same severity as a malformed predicate (this throw): the + // reject set of registration and the reject set of + // evaluation must be one set, and the refusal itself is the + // spec's, shared with `objectstack validate` so build and + // author time cannot disagree about the shape. + const shapeRefusal = predicateSlotRefusal(found.value); + if (shapeRefusal) { + failures.push(` • ${slotWhere}: ${shapeRefusal.message}\n source: \`${shapeRefusal.source}\``); + continue; + } // No schema hint: a screen's `visibleWhen` binds the screen's OWN // collected values, not the trigger record's fields, so the // field-existence pass would report every field name as unknown. diff --git a/packages/spec/api-surface/automation.json b/packages/spec/api-surface/automation.json index df0e204a82..c430989812 100644 --- a/packages/spec/api-surface/automation.json +++ b/packages/spec/api-surface/automation.json @@ -178,6 +178,7 @@ "ORG_MEMBERSHIP_LEVELS (const)", "OS_CONSTRUCT_EXT (const)", "PARALLEL_NODE_TYPE (const)", + "PREDICATE_SLOT_STRING_REFUSAL (const)", "ParallelBranch (type)", "ParallelBranchParsed (type)", "ParallelBranchSchema (const)", @@ -259,6 +260,7 @@ "normalizeDecisionOutputs (function)", "normalizeFlowFunctionEntry (function)", "parseFlowNodeRegions (function)", + "predicateSlotRefusal (function)", "resolveFlowNodeExpressions (function)", "resolveFlowTriggerKind (function)", "validateControlFlow (function)" diff --git a/packages/spec/export-origins/automation.json b/packages/spec/export-origins/automation.json index 95da3dce08..e5eed32d4d 100644 --- a/packages/spec/export-origins/automation.json +++ b/packages/spec/export-origins/automation.json @@ -178,6 +178,7 @@ "ORG_MEMBERSHIP_LEVELS": "src/automation/approval.zod.ts#ORG_MEMBERSHIP_LEVELS (const)", "OS_CONSTRUCT_EXT": "src/automation/bpmn-mapping.ts#OS_CONSTRUCT_EXT (const)", "PARALLEL_NODE_TYPE": "src/automation/control-flow.zod.ts#PARALLEL_NODE_TYPE (const)", + "PREDICATE_SLOT_STRING_REFUSAL": "src/automation/flow-node-expression-paths.ts#PREDICATE_SLOT_STRING_REFUSAL (const)", "ParallelBranch": "src/automation/control-flow.zod.ts#ParallelBranch (type)", "ParallelBranchParsed": "src/automation/control-flow.zod.ts#ParallelBranchParsed (type)", "ParallelBranchSchema": "src/automation/control-flow.zod.ts#ParallelBranchSchema (const)", @@ -259,6 +260,7 @@ "normalizeDecisionOutputs": "src/automation/approval.zod.ts#normalizeDecisionOutputs (function)", "normalizeFlowFunctionEntry": "src/automation/flow-function.zod.ts#normalizeFlowFunctionEntry (function)", "parseFlowNodeRegions": "src/automation/control-flow.zod.ts#parseFlowNodeRegions (function)", + "predicateSlotRefusal": "src/automation/flow-node-expression-paths.ts#predicateSlotRefusal (function)", "resolveFlowNodeExpressions": "src/automation/flow-node-expression-paths.ts#resolveFlowNodeExpressions (function)", "resolveFlowTriggerKind": "src/automation/flow-trigger-kind.ts#resolveFlowTriggerKind (function)", "validateControlFlow": "src/automation/control-flow.zod.ts#validateControlFlow (function)" diff --git a/packages/spec/src/automation/flow-node-expression-paths.test.ts b/packages/spec/src/automation/flow-node-expression-paths.test.ts index f85cb50da6..af4e05d062 100644 --- a/packages/spec/src/automation/flow-node-expression-paths.test.ts +++ b/packages/spec/src/automation/flow-node-expression-paths.test.ts @@ -20,6 +20,8 @@ import { FLOW_NODE_EXPRESSION_PATHS, isExpressionEnvelopeShaped, resolveFlowNodeExpressions, + predicateSlotRefusal, + PREDICATE_SLOT_STRING_REFUSAL, type FlowNodeExpressionPath, type FlowNodeExpressionRole, } from './flow-node-expression-paths.js'; @@ -178,16 +180,66 @@ describe('every pre-#14149 entry resolves byte-identically (the ratchet\'s fixtu expect(found.every((f) => f.entry.role === 'predicate')).toBe(true); }); - it('absent, empty and non-string values in a string-role slot are still skipped', () => { + it('absent and empty values in a string-role slot are skipped', () => { expect(resolveFlowNodeExpressions('screen', {})).toEqual([]); expect(resolveFlowNodeExpressions('screen', { fields: [] })).toEqual([]); + // A whitespace-only STRING is "not authored", on this side and at the + // evaluator alike — consistent on both sides, and deliberately left alone + // (#15572 changed the non-string rule, never the string one). expect(resolveFlowNodeExpressions('screen', { fields: [{ visibleWhen: ' ' }] })).toEqual([]); - expect(resolveFlowNodeExpressions('screen', { fields: [{ visibleWhen: true }] })).toEqual([]); expect(resolveFlowNodeExpressions('screen', { fields: 'nope' })).toEqual([]); - // An ENVELOPE in a predicate-role slot is a non-string — skipped here, a - // type violation for the schema pass, exactly as before: the value-role - // emission rule is scoped to `value` entries and leaks into no other role. + // A `flow-template` slot keeps the old rule: no validator implements that + // dialect, so emitting a non-string there would hand every consumer a + // finding none of them can judge. expect(resolveFlowNodeExpressions('loop', { collection: { dialect: 'cel', source: 'x' } })).toEqual([]); expect(resolveFlowNodeExpressions('decision', { condition: 'a == b' })).toEqual([]); }); + + /** + * [#15572] A NON-string in a `predicate` slot is emitted, so a consumer can + * refuse it. It used to be skipped as "a type violation for the schema pass + * to report" — and for `decision`, a schemaless node type whose config is + * never parsed against any Zod schema, there is no schema pass, so the value + * reached the evaluator with no validator having ever seen it. + */ + it('emits a non-string in a predicate slot, so a consumer can refuse it (#15572)', () => { + const envelope = { dialect: 'cel', source: ' ' }; + const decision = resolveFlowNodeExpressions('decision', { + conditions: [{ label: 'Yes', expression: envelope }], + }); + expect(decision.map((f) => [f.path, f.value, f.entry.role])) + .toEqual([['conditions[0].expression', envelope, 'predicate']]); + // Same rule on the other predicate slot — one class, not one node type. + expect(resolveFlowNodeExpressions('screen', { fields: [{ visibleWhen: true }] }) + .map((f) => [f.path, f.value])).toEqual([['fields[0].visibleWhen', true]]); + // `null` / absent stay "not authored" — a refusal needs something authored. + expect(resolveFlowNodeExpressions('screen', { fields: [{ visibleWhen: null }] })).toEqual([]); + }); + + describe('predicateSlotRefusal (#15572)', () => { + it('says nothing about a string — what it SAYS is validateExpression\'s business', () => { + expect(predicateSlotRefusal('record.rating >= 4')).toBeUndefined(); + // Including a string that is itself malformed: the shape is right, so + // this function is done and the CEL parse issues the verdict. + expect(predicateSlotRefusal('{record.rating} >= 4')).toBeUndefined(); + expect(predicateSlotRefusal('')).toBeUndefined(); + }); + + it('refuses an envelope and attributes it to the envelope\'s own source', () => { + const refusal = predicateSlotRefusal({ dialect: 'cel', source: 'rows.map(r,' }); + expect(refusal?.message.startsWith(PREDICATE_SLOT_STRING_REFUSAL)).toBe(true); + expect(refusal?.message).toContain('an expression envelope'); + expect(refusal?.source).toBe('rows.map(r,'); + }); + + it('refuses every other non-string, naming what it found', () => { + expect(predicateSlotRefusal(42)?.message).toContain('a number'); + expect(predicateSlotRefusal(true)?.message).toContain('a boolean'); + expect(predicateSlotRefusal(['a'])?.message).toContain('an array'); + expect(predicateSlotRefusal({ source: 'x' })?.message).toContain('an object'); + // No `dialect` ⇒ not envelope-shaped, so no source is claimed from it. + expect(predicateSlotRefusal({ source: 'x' })?.source).toBe('x'); + expect(predicateSlotRefusal(42)?.source).toBe(''); + }); + }); }); diff --git a/packages/spec/src/automation/flow-node-expression-paths.ts b/packages/spec/src/automation/flow-node-expression-paths.ts index 1354d27894..6c8b487777 100644 --- a/packages/spec/src/automation/flow-node-expression-paths.ts +++ b/packages/spec/src/automation/flow-node-expression-paths.ts @@ -279,11 +279,20 @@ export function isExpressionEnvelopeShaped(value: unknown): value is { dialect: * * What counts as "an authored expression" depends on the role: * - * - `predicate` / `flow-template`: the slot IS the expression, so a non-empty - * string is emitted. Absent, `null` and non-string values are skipped: "not - * authored" is not a malformed expression, and a non-string in an expression - * slot is a *type* violation for the schema pass to report, not something to - * hand to a parser. + * - `predicate`: the slot IS the expression, so a non-empty string is emitted. + * Absent and `null` are skipped — "not authored" is not a malformed + * expression — and so is a whitespace-only string, which both validators and + * the evaluator already agree means the same thing. A **non-string** is + * emitted too (#15572), for the consumer to refuse through + * {@link predicateSlotRefusal}: it used to be skipped as "a type violation + * for the schema pass to report", and for a schemaless node type there is no + * schema pass — `decision` publishes no descriptor `configSchema`, and + * `DecisionConditionSchema` is never parsed against a node's open + * `z.record` config — so nothing reported it and the value ran anyway. + * - `flow-template`: same string rule, minus the refusal. No validator + * implements that dialect (see {@link FlowNodeExpressionRole}), so emitting a + * non-string there would hand every consumer a finding none of them can + * judge. * - `value`: the slot holds a VALUE that may be spelled as an expression, so * only envelope-shaped objects ({@link isExpressionEnvelopeShaped}) are * emitted. A string there is `{token}` interpolation — the `flow-template` @@ -302,7 +311,11 @@ export function resolveFlowNodeExpressions( walk(config as Record, entry.path.split('.'), '', (path, value) => { if (entry.role === 'value') { if (isExpressionEnvelopeShaped(value)) out.push({ entry, path, value }); - } else if (typeof value === 'string' && value.trim()) { + } else if (typeof value === 'string') { + if (value.trim()) out.push({ entry, path, value }); + } else if (entry.role === 'predicate' && value != null) { + // #15572 — a non-string in a predicate slot. Emitted so a consumer can + // REFUSE it (see `predicateSlotRefusal`), never so it can be parsed. out.push({ entry, path, value }); } }); @@ -310,6 +323,69 @@ export function resolveFlowNodeExpressions( return out; } +/** + * The one sentence a refused `predicate` slot leads with (#15572) — the same + * words however the value is wrong, so an author (or an agent reading the + * failure) learns the rule before the detail. Mirrors + * `ASSIGNMENT_VALUE_ENVELOPE_REFUSAL`, the `value` role's equivalent. + */ +export const PREDICATE_SLOT_STRING_REFUSAL = + 'A predicate slot holds BARE CEL TEXT — it is declared `z.string()` — so an expression envelope, or any other ' + + 'non-string, is not authorable there.'; + +/** + * Why a value sitting in a `predicate`-role slot is not authorable at all — + * the SINGLE notion both consumers apply, derived once (#15572). + * + * `undefined` for every string: what a string *says* is judged by + * `validateExpression('predicate', …)`, unchanged, and a whitespace-only string + * never reaches here (the resolver treats it as "not authored", the same way + * the evaluator does — deliberately, and not a defect on either side). + * + * ## Why this is a refusal and not a parse + * + * The `{ dialect, source }` envelope is the `value` role's spelling + * (`assignment.assignments.*`, the 2026-09-02 ruling on #14149). In a predicate + * slot it was a shape NOBODY could see: the slot is declared `z.string()`, but + * a node's `config` is an open `z.record(z.unknown())` that no Zod schema is + * parsed against, the unknown-key walk exempts the schemaless node types + * (`decision` publishes no descriptor `configSchema` on purpose), and this + * resolver skipped every non-string as "a type violation for the schema pass to + * report" — a schema pass that, for those types, does not exist. The evaluator + * accepted it regardless, so the one layer with an opinion was the last one. + * + * Refusing it here is the producer-side half of that: the flow does not + * register and `objectstack validate` locates it, instead of the flow running + * on a predicate no validator ever read. + * + * @returns the refusal and the source to attribute it to, or `undefined` when + * the value is a string and therefore this function's business is done. + */ +export function predicateSlotRefusal(value: unknown): { message: string; source: string } | undefined { + if (typeof value === 'string') return undefined; + const envelope = isExpressionEnvelopeShaped(value); + const found = envelope + ? 'an expression envelope (an object naming a `dialect`)' + : Array.isArray(value) + ? 'an array' + : value === null + ? '`null`' + : typeof value === 'object' + ? 'an object' + : `a ${typeof value}`; + // The envelope's own `source`, when it has one, so the finding still points + // at the text the author wrote rather than at an empty string. + const rawSource = (value as { source?: unknown } | null | undefined)?.source; + return { + message: + `${PREDICATE_SLOT_STRING_REFUSAL} Found ${found}. Write the predicate as bare CEL text ` + + '(e.g. `record.rating >= 4`); the `{ dialect, source }` envelope is the `value`-role spelling ' + + '(the `assignment` node\'s `assignments` map), and in a predicate slot it is read by the evaluator ' + + 'but by neither validator.', + source: typeof rawSource === 'string' ? rawSource : '', + }; +} + /** * Descend `segments` through `node`, expanding a `key[]` segment over every * element of that array and a `*` segment over every own key of that object,