diff --git a/.changeset/flow-variable-default-value.md b/.changeset/flow-variable-default-value.md new file mode 100644 index 0000000000..939c5e3346 --- /dev/null +++ b/.changeset/flow-variable-default-value.md @@ -0,0 +1,60 @@ +--- +"@objectstack/spec": minor +"@objectstack/service-automation": minor +--- + +feat(spec,service-automation): a flow variable can declare a `defaultValue`, so "declared" means "bound" (#4697) + +Declaring a flow variable used to guarantee nothing at run time. The engine bound +an `isInput` variable **only** when the caller actually supplied it +(`params[name] !== undefined`), so every path that omitted the parameter left the +name unbound — and a flow condition is strict CEL, where an unbound name does not +read as `false`, it **aborts the predicate and stops the run**. The declaration was +documentation, not a guarantee, and there was no metadata form that said "this +variable always has a value". + +`FlowVariableSchema` now takes an optional `defaultValue`, and the engine binds it +whenever no parameter supplies one: + +```typescript +variables: [ + { name: 'createOpportunity', type: 'boolean', isInput: true, defaultValue: false }, +] +``` + +The rules: + +- **A supplied parameter always wins**, including a falsy one — the boundary is + `!== undefined`, so `false`, `null`, `0` and `''` are answers rather than + absences, and only a genuinely missing parameter falls through to the default. +- **A non-input declaration takes its default too.** `isInput: false` means no + parameter can reach the name, so the default is the only thing that can bind it. +- **A declared variable shadows a trigger-record field of the same name**, whether + it was bound from a parameter or from its default — the rule a parameter already + followed. A name cannot resolve out of a different source depending on whether + the caller passed it. + +Both run entry points seed from one shared site, so the retry path behaves +identically to the first attempt. + +**Additive and opt-in.** A declaration without `defaultValue` behaves exactly as +before, so existing flows parse and run unchanged. The value is not cross-checked +against the declared `type` — `type` is an open string with no vocabulary to check +against, the same posture as every other `defaultValue` on the authoring surface. + +The case this closes came from a screen flow (hotcrm#643): a screen collects an +optional checkbox, the client returns only the fields the user actually touched, +so on the untouched path the variable was never bound, the outgoing edge aborted, +and a lead conversion persisted nothing. The workaround was an `assignment` node +before every screen mirroring the screen field's own `defaultValue`; a declared +default replaces that ceremony. + +The docs half of the same gap is now written down too +(`content/docs/automation/flows.mdx`): under strict CEL the guard an author +reaches for first — `has(X.f)` — **aborts** on an unbound `X`, the very case it is +written for. Only the `vars.`-scoped `has(vars.X)` tests bindedness. That truth +table is measured against the live evaluator in +`service-automation/src/flow-variable-default.test.ts` rather than asserted, so a +prescription nothing executes cannot quietly stop being true. Prefer +`defaultValue` over either guard: a guard encodes "unanswered means no" into the +predicate and leaves the graph defect in place. diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index 363ec28cdd..acc5881c98 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -336,7 +336,10 @@ Two things worth knowing: - **Give a `required` boolean a `defaultValue`.** An untouched checkbox holds no value at all, which counts as unanswered — without `defaultValue: false` the - user cannot express "no" by leaving it clear. + user cannot express "no" by leaving it clear. Mirror it on the **flow + variable** as well ([`defaultValue`](#defaultvalue--declaring-a-variable-is-not-the-same-as-binding-it)): + the client returns only the fields the user touched, so on the untouched path + the name is never bound and a downstream condition reading it aborts the run. - **A broken predicate fails open** (the field stays visible) rather than hiding an input the flow may be waiting on. `registerFlow()` does check the predicate as bare CEL — a `{var}` template or a syntax error is a loud registration @@ -951,6 +954,7 @@ variables: [ { name: 'input_id', type: 'text', isInput: true, isOutput: false }, { name: 'result', type: 'object', isInput: false, isOutput: true }, { name: 'counter', type: 'number', isInput: false, isOutput: false }, + { name: 'createOpportunity', type: 'boolean', isInput: true, defaultValue: false }, ] ``` @@ -960,6 +964,96 @@ variables: [ | `type` | `string` | `'text'`, `'number'`, `'boolean'`, `'object'`, `'list'` | | `isInput` | `boolean` | Available as input parameter | | `isOutput` | `boolean` | Available as output parameter | +| `defaultValue` | `unknown` | Bound at run start when no parameter supplies one — this is what makes the declaration a guarantee. Optional | + +### `defaultValue` — declaring a variable is not the same as binding it + +Without `defaultValue`, a declaration is documentation. The engine binds an +`isInput` variable **only** when the caller actually passed it +(`params[name] !== undefined`), so every path that omits the parameter leaves +the name unbound — and an unbound name in a condition does not read as `false`, +it **aborts the predicate and stops the run**. + +Declare the default and the name is bound on every path: + +{/* os:check */} +```typescript +import { defineFlow } from '@objectstack/spec/automation'; + +export const LeadConversion = defineFlow({ + name: 'lead_conversion', + label: 'Lead Conversion', + type: 'screen', + variables: [ + // Bound to `false` on every path the caller leaves it out of. + { name: 'createOpportunity', type: 'boolean', isInput: true, defaultValue: false }, + ], + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [{ id: 'e1', source: 'start', target: 'end' }], +}); +``` + +The rules, all three worth knowing: + +- **A supplied parameter always wins** — including a falsy one. The boundary is + `!== undefined`, so `false`, `null`, `0` and `''` are answers, not absences, + and only a genuinely missing parameter falls through to the default. +- **A non-input variable takes its default too.** `isInput: false` means no + parameter can reach the name, so the default is the only thing that can bind + it. +- **A declared variable shadows a trigger-record field of the same name** — + whether it was bound from a parameter or from its default. That is the rule a + parameter already followed; extending it to defaults is what keeps a name from + resolving out of a different source depending on whether the caller passed it. + +The case this exists for is a screen that collects something optional. The +client returns only the fields the user actually touched, so an untouched +checkbox arrives as nothing at all — and the edge reading it aborts. Give the +variable a `defaultValue` mirroring the screen field's own, and the untouched +path routes: + +```typescript +variables: [ + { name: 'createOpportunity', type: 'boolean', isInput: true, defaultValue: false }, +], +nodes: [ + { id: 'collect', type: 'screen', label: 'Conversion Details', config: { + fields: [{ name: 'createOpportunity', label: 'Create Opportunity?', + type: 'boolean', defaultValue: false }], + } }, +], +edges: [ + { id: 'e_no', source: 'collect', target: 'skip', condition: 'createOpportunity == false' }, +] +``` + +The value is **not** checked against the declared `type` — `type` is an open +string, so there is no vocabulary to check against. It behaves like every other +`defaultValue` on the authoring surface. + + +**Reading a variable that might be unbound: only the `vars.`-scoped guard +works.** Conditions are strict CEL, and `has()` is not the escape hatch it looks +like — measured on the live evaluator (and pinned as a test): + +| expression | `X` unbound | `X = null` | `X = {}` | `X = {f: 1}` | +| :--- | :--- | :--- | :--- | :--- | +| `X.f == 1` | **aborts** `Unknown variable: X` | **aborts** `No such key: f` | **aborts** | `true` | +| `has(X.f)` | **aborts** `Unknown variable: X` | `false` | `false` | `true` | +| `has(vars.X)` | `false` | `true` | `true` | `true` | + +So the natural spelling `has(X.f)` fails on exactly the case it was written for, +and `has(vars.X) && X.f == 1` is the guard that holds (CEL short-circuits, so +the right-hand read never runs on the unbound leg). + +Reach for `defaultValue` first anyway. A guard says "unanswered means no" inside +the predicate and leaves the run's shape depending on what the client happened +to send; a default removes the unbound state, and every later reader of the flow +can see it in the declaration. + ## Error Handling @@ -1152,7 +1246,10 @@ curl -b cookies.txt -X POST \ **Passing inputs.** Declare variables with `isInput: true`, then send them in `params` under the same names — only declared inputs are bound. `recordId` and `objectName` are lifted into `params` for you (plus an `Id` alias), -and `event` defaults to `'manual'`. +and `event` defaults to `'manual'`. A param you *omit* leaves the variable +unbound unless its declaration carries a +[`defaultValue`](#defaultvalue--declaring-a-variable-is-not-the-same-as-binding-it); +a param you send explicitly always wins, `false` and `null` included. **Identity.** The caller's identity (user, positions, permissions, tenant) is forwarded into the run, so a `runAs: 'user'` flow executes under that caller's diff --git a/content/docs/references/api/automation-api.mdx b/content/docs/references/api/automation-api.mdx index e4e720a9e5..616b427227 100644 --- a/content/docs/references/api/automation-api.mdx +++ b/content/docs/references/api/automation-api.mdx @@ -94,7 +94,7 @@ const result = AutomationApiErrorCode.parse(data); | **status** | `Enum<'draft' \| 'active' \| 'obsolete' \| 'invalid'>` | optional | Deployment status | | **template** | `never` | optional | [REMOVED] `flow.template` was removed in @objectstack/spec 17.0.0 (#3896 audit close-out) — no designer or engine path ever read it, so flagging a flow as a template/subflow did nothing. Delete the key. Shared logic is invoked via a subflow NODE referencing the flow by name. | | **type** | `Enum<'autolaunched' \| 'record_change' \| 'schedule' \| 'screen' \| 'api'>` | ✅ | Flow type | -| **variables** | `{ name: string; type: string; isInput?: boolean; isOutput?: boolean }[]` | optional | Flow variables | +| **variables** | `{ name: string; type: string; isInput?: boolean; isOutput?: boolean; … }[]` | optional | Flow variables | | **nodes** | `{ id: string; type: string; label: string; config?: Record; … }[]` | ✅ | Flow nodes | | **edges** | `{ id: string; source: string; target: string; condition?: string \| object; … }[]` | ✅ | Flow connections | | **active** | `never` | optional | [REMOVED] `flow.active` was removed in @objectstack/spec 17.0.0 (#3896 audit close-out) — it never had an effect: the engine arms flows from `status`, and `active: false` did NOT stop a flow (worse, the default read as disabled while the engine treated unset as enabled). Delete the key. Use `status: 'obsolete'` (or 'invalid') to unbind and disable a flow, `status: 'active'` to arm it. | diff --git a/content/docs/references/automation/flow.mdx b/content/docs/references/automation/flow.mdx index 3968fb1cb9..5199535b2a 100644 --- a/content/docs/references/automation/flow.mdx +++ b/content/docs/references/automation/flow.mdx @@ -48,7 +48,7 @@ const result = FlowSchema.parse(data); | **status** | `Enum<'draft' \| 'active' \| 'obsolete' \| 'invalid'>` | optional | Deployment status | | **template** | `never` | optional | [REMOVED] `flow.template` was removed in @objectstack/spec 17.0.0 (#3896 audit close-out) — no designer or engine path ever read it, so flagging a flow as a template/subflow did nothing. Delete the key. Shared logic is invoked via a subflow NODE referencing the flow by name. | | **type** | `Enum<'autolaunched' \| 'record_change' \| 'schedule' \| 'screen' \| 'api'>` | ✅ | Flow type | -| **variables** | `{ name: string; type: string; isInput?: boolean; isOutput?: boolean }[]` | optional | Flow variables | +| **variables** | `{ name: string; type: string; isInput?: boolean; isOutput?: boolean; … }[]` | optional | Flow variables | | **nodes** | `{ id: string; type: string; label: string; config?: Record; … }[]` | ✅ | Flow nodes | | **edges** | `{ id: string; source: string; target: string; condition?: string \| object; … }[]` | ✅ | Flow connections | | **active** | `never` | optional | [REMOVED] `flow.active` was removed in @objectstack/spec 17.0.0 (#3896 audit close-out) — it never had an effect: the engine arms flows from `status`, and `active: false` did NOT stop a flow (worse, the default read as disabled while the engine treated unset as enabled). Delete the key. Use `status: 'obsolete'` (or 'invalid') to unbind and disable a flow, `status: 'active'` to arm it. | @@ -142,6 +142,7 @@ const result = FlowSchema.parse(data); | **type** | `string` | ✅ | Data type (text, number, boolean, object, list) | | **isInput** | `boolean` | ✅ | Is input parameter | | **isOutput** | `boolean` | ✅ | Is output parameter | +| **defaultValue** | `any` | optional | Value bound at run start when no parameter supplies one — this is what makes a declared variable always bound. An explicitly supplied param wins, including `false` and `null`; the boundary is `params[name] !== undefined`. | --- diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index f41e7d45b6..152b6fb069 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -2762,14 +2762,7 @@ export class AutomationEngine implements IAutomationService { if (reentryKey) this.activeRecordFlows.add(reentryKey); // Initialize variable context - const variables = new Map(); - if (flow.variables) { - for (const v of flow.variables) { - if (v.isInput && context?.params?.[v.name] !== undefined) { - variables.set(v.name, context.params[v.name]); - } - } - } + const variables = this.seedDeclaredVariables(flow, context); // Inject trigger record. `$record` is the canonical handle; `record` is a // friendlier alias so templates/conditions can write `{record.title}` and // `record.status`. We also flatten the record's own fields to top-level @@ -5707,6 +5700,56 @@ export class AutomationEngine implements IAutomationService { return { success: false, error: lastError, durationMs: Date.now() - startTime }; } + /** + * Seed a run's variable map from the flow's DECLARED variables — the one + * place `declared` is turned into `bound` (#4697). + * + * Two sources, in this precedence: + * + * 1. `context.params[name]`, for an `isInput` variable, when the caller + * supplied it. The boundary is `!== undefined`, so an explicit `false`, + * `null`, `0` or `''` is a supplied value and wins over the default — + * only *absence* falls through. + * 2. `defaultValue`, when the declaration carries one. This is the half + * that did not exist before #4697: a declared variable the caller left + * out stayed **unbound**, and conditions are strict CEL, where reading + * an unbound name ABORTS the predicate (`Unknown variable: X`) instead + * of yielding `false`. A screen flow collecting an optional checkbox hit + * exactly that — the runner returns only the fields the user touched, so + * the untouched path aborted the outgoing edge and the run stopped + * (hotcrm#643). The workaround was an `assignment` node per screen, + * mirroring the screen field's own `defaultValue`. + * + * `defaultValue` is honoured for a NON-input variable too: params are not + * readable there by definition, so the default is the only thing that can + * bind it, and "declared means bound" would otherwise hold for half the + * declarations. A declaration with no `defaultValue` behaves exactly as + * before — existing flows are untouched. + * + * Seeding happens BEFORE the trigger record is flattened to top-level + * names, and that flattening skips names already present. So a declared + * variable — bound from a param or from its default — shadows a record + * field of the same name, which is the rule params already followed; a + * default cannot make the same name resolve from a different source + * depending on whether the caller passed it. + */ + private seedDeclaredVariables( + flow: FlowParsed, + context?: AutomationContext, + ): Map { + const variables = new Map(); + if (!flow.variables) return variables; + for (const v of flow.variables) { + const supplied = v.isInput ? context?.params?.[v.name] : undefined; + if (supplied !== undefined) { + variables.set(v.name, supplied); + } else if (v.defaultValue !== undefined) { + variables.set(v.name, v.defaultValue); + } + } + return variables; + } + /** * Execute a flow without triggering retry logic (used by retryExecution to prevent recursion). */ @@ -5724,14 +5767,7 @@ export class AutomationEngine implements IAutomationService { return { success: false, error: `Flow '${flowName}' is disabled` }; } - const variables = new Map(); - if (flow.variables) { - for (const v of flow.variables) { - if (v.isInput && context?.params?.[v.name] !== undefined) { - variables.set(v.name, context.params[v.name]); - } - } - } + const variables = this.seedDeclaredVariables(flow, context); if (context?.record) { variables.set('$record', context.record); } diff --git a/packages/services/service-automation/src/flow-variable-default.test.ts b/packages/services/service-automation/src/flow-variable-default.test.ts new file mode 100644 index 0000000000..390b1b01a5 --- /dev/null +++ b/packages/services/service-automation/src/flow-variable-default.test.ts @@ -0,0 +1,250 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, beforeEach } from 'vitest'; +import { AutomationEngine } from './engine.js'; +import { registerScreenNodes } from './builtin/screen-nodes.js'; + +/** + * #4697 — a flow variable declared with `defaultValue` is bound on EVERY path, + * so "declared" finally means "bound". + * + * Why that mattered enough to change the contract: conditions are strict CEL, + * and an unbound name does not read as `false` there — it ABORTS the whole + * predicate, which stops the run. `describes the truth table` below measures + * that on the live evaluator rather than restating it, because the docs half of + * this issue (`content/docs/automation/flows.mdx`) prescribes the one guard + * spelling that survives, and a prescription nothing executes is the kind that + * silently stops being true. + */ + +const createTestLogger = () => { + const logger: Record = { + info: () => {}, warn: () => {}, error: () => {}, debug: () => {}, + }; + logger.child = () => logger; + return logger as any; +}; + +const fakeScreenCtx = () => ({ logger: { info() {}, warn() {}, error() {} } }) as any; + +describe('#4697 flow variable defaultValue', () => { + let engine: AutomationEngine; + /** What the `capture` node saw, and whether the name was bound at all. */ + let seen: { value: unknown; bound: boolean }; + + beforeEach(() => { + engine = new AutomationEngine(createTestLogger()); + seen = { value: 'NEVER-RAN', bound: false }; + engine.registerNodeExecutor({ + type: 'capture', + async execute(_node, variables) { + seen = { value: variables.get('createOpportunity'), bound: variables.has('createOpportunity') }; + return { success: true }; + }, + }); + }); + + /** A one-variable flow: start → capture → end. */ + const registerFlow = (name: string, variable: Record) => { + engine.registerFlow(name, { + name, label: name, type: 'autolaunched', + variables: [variable as never], + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'cap', type: 'capture', label: 'Capture' }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'cap' }, + { id: 'e2', source: 'cap', target: 'end' }, + ], + }); + }; + + describe('binding', () => { + it('binds the declared default when the caller supplies no param', async () => { + registerFlow('f_default', { name: 'createOpportunity', type: 'boolean', isInput: true, defaultValue: false }); + + const r = await engine.execute('f_default'); + + expect(r.success).toBe(true); + expect(seen).toEqual({ value: false, bound: true }); + }); + + it('leaves the variable UNBOUND when no default is declared — the pre-#4697 behaviour, unchanged', async () => { + // The reverse direction of the pin above: a declaration WITHOUT + // `defaultValue` must keep behaving exactly as it did, or this + // change would be a silent migration of every existing flow. + registerFlow('f_nodefault', { name: 'createOpportunity', type: 'boolean', isInput: true }); + + const r = await engine.execute('f_nodefault'); + + expect(r.success).toBe(true); + expect(seen).toEqual({ value: undefined, bound: false }); + }); + + it('binds a NON-input declaration from its default (params cannot reach it at all)', async () => { + registerFlow('f_noninput', { name: 'createOpportunity', type: 'boolean', isInput: false, defaultValue: true }); + + const r = await engine.execute('f_noninput', { params: { createOpportunity: false } } as never); + + expect(r.success).toBe(true); + // `isInput: false` means the param is not readable — the default is + // the only thing that can bind the name, and it does. + expect(seen).toEqual({ value: true, bound: true }); + }); + }); + + describe('the `params[name] !== undefined` boundary', () => { + // The whole point of the boundary is that a FALSY supplied value is + // still a supplied value. Each of these would be swallowed by a truthy + // check (`params[name] || defaultValue`), and each is a real answer a + // screen can collect. + const falsy: Array<[string, unknown]> = [ + ['false', false], + ['null', null], + ['0', 0], + ['empty string', ''], + ]; + + for (const [label, supplied] of falsy) { + it(`an explicitly supplied ${label} WINS over the default`, async () => { + registerFlow(`f_boundary_${label.replace(/\W+/g, '_')}`, { + name: 'createOpportunity', type: 'boolean', isInput: true, defaultValue: 'THE-DEFAULT', + }); + + const r = await engine.execute(`f_boundary_${label.replace(/\W+/g, '_')}`, { + params: { createOpportunity: supplied }, + } as never); + + expect(r.success).toBe(true); + expect(seen).toEqual({ value: supplied, bound: true }); + }); + } + + it('an explicitly declared `defaultValue: null` binds null (absence is `undefined`, not `null`)', async () => { + registerFlow('f_null_default', { name: 'createOpportunity', type: 'object', isInput: true, defaultValue: null }); + + const r = await engine.execute('f_null_default'); + + expect(r.success).toBe(true); + expect(seen).toEqual({ value: null, bound: true }); + }); + }); + + describe('precedence against the flattened trigger record', () => { + it('a declared default shadows a record field of the same name — the rule a supplied param already followed', async () => { + // `execute` flattens the trigger record to top-level names but skips + // names already seeded from the declarations. Defaults join that + // seeding, so a name resolves from its declaration whether or not + // the caller passed it — rather than switching source silently. + registerFlow('f_shadow', { name: 'createOpportunity', type: 'boolean', isInput: true, defaultValue: false }); + + const r = await engine.execute('f_shadow', { record: { id: 'r1', createOpportunity: true } } as never); + + expect(r.success).toBe(true); + expect(seen).toEqual({ value: false, bound: true }); + }); + }); + + describe('the defect this closes, end to end', () => { + /** + * hotcrm#643, reduced: a screen collects an optional checkbox, the + * runner returns only the fields the user touched, and the outgoing + * edge reads the variable. On the untouched path the name was unbound + * and the edge ABORTED — a lead conversion that persisted nothing. + */ + const registerScreenFlow = (name: string, variable: Record | null) => { + engine.registerFlow(name, { + name, label: name, type: 'screen', + ...(variable ? { variables: [variable as never] } : {}), + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'collect', type: 'screen', label: 'Conversion Details', + config: { fields: [{ name: 'notes', label: 'Notes', type: 'text' }] }, + }, + { id: 'cap', type: 'capture', label: 'Capture' }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'collect' }, + // The predicate the untouched path used to abort on. + { id: 'e2', source: 'collect', target: 'cap', condition: 'createOpportunity == false' }, + { id: 'e3', source: 'cap', target: 'end' }, + ], + }); + }; + + it('WITHOUT a default the untouched path aborts the outgoing edge', async () => { + registerScreenNodes(engine, fakeScreenCtx()); + registerScreenFlow('hotcrm_broken', { name: 'createOpportunity', type: 'boolean', isInput: true }); + + const paused = await engine.execute('hotcrm_broken'); + expect(paused.status).toBe('paused'); + + // Resume the way the runner does on the untouched path: it returns + // only the fields the user actually touched. + const resumed = await engine.resume(paused.runId!, { variables: { notes: 'left the box alone' } }); + + expect(resumed.success).toBe(false); + expect(resumed.error).toContain('Unknown variable: createOpportunity'); + expect(seen.value).toBe('NEVER-RAN'); // downstream never reached + }); + + it('WITH a default the same untouched path routes and the run completes', async () => { + registerScreenNodes(engine, fakeScreenCtx()); + registerScreenFlow('hotcrm_fixed', { name: 'createOpportunity', type: 'boolean', isInput: true, defaultValue: false }); + + const paused = await engine.execute('hotcrm_fixed'); + expect(paused.status).toBe('paused'); + + const resumed = await engine.resume(paused.runId!, { variables: { notes: 'left the box alone' } }); + + expect(resumed.success).toBe(true); + expect(seen).toEqual({ value: false, bound: true }); + }); + }); + + describe('the strict-CEL truth table the docs half prescribes from', () => { + // Measured, not restated. `content/docs/automation/flows.mdx` tells an + // author that `has(X.f)` does NOT survive an unbound variable and that + // `has(vars.X)` is the form that does; this is that claim, executable. + const evalCel = (source: string, vars: Map) => { + try { + return { ok: true as const, value: engine.evaluateCondition({ dialect: 'cel', source }, vars) }; + } catch (err) { + return { ok: false as const, message: (err as Error).message.replace(/\s+/g, ' ') }; + } + }; + const unbound = () => new Map(); + const bound = (v: unknown) => new Map([['X', v]]); + + it('a bare read of an unbound variable aborts — it does not read as false', () => { + const r = evalCel('X.f == 1', unbound()); + expect(r.ok).toBe(false); + expect(r.ok === false && r.message).toContain('Unknown variable: X'); + }); + + it('`has(X.f)` — the guard an author reaches for first — aborts on the very case it is written for', () => { + const r = evalCel('has(X.f)', unbound()); + expect(r.ok).toBe(false); + expect(r.ok === false && r.message).toContain('Unknown variable: X'); + }); + + it('`has(vars.X)` is the only spelling that tests bindedness', () => { + expect(evalCel('has(vars.X)', unbound())).toEqual({ ok: true, value: false }); + expect(evalCel('has(vars.X)', bound(null))).toEqual({ ok: true, value: true }); + expect(evalCel('has(vars.X)', bound({ f: 1 }))).toEqual({ ok: true, value: true }); + }); + + it('the guarded composite short-circuits, so `has(vars.X) && X.f == 1` is safe on the unbound leg', () => { + expect(evalCel('has(vars.X) && X.f == 1', unbound())).toEqual({ ok: true, value: false }); + expect(evalCel('has(vars.X) && X.f == 1', bound({ f: 1 }))).toEqual({ ok: true, value: true }); + }); + + it('binding a variable — which a declared default now always does — makes the bare read work', () => { + expect(evalCel('X.f == 1', bound({ f: 1 }))).toEqual({ ok: true, value: true }); + }); + }); +}); diff --git a/packages/spec/authorable-surface/automation.json b/packages/spec/authorable-surface/automation.json index 1da490999a..8b1e9e3eec 100644 --- a/packages/spec/authorable-surface/automation.json +++ b/packages/spec/authorable-surface/automation.json @@ -202,6 +202,7 @@ "automation/FlowRunSummary:selected", "automation/FlowRunSummary:skipped", "automation/FlowRunSummary:unmeasured", + "automation/FlowVariable:defaultValue", "automation/FlowVariable:isInput", "automation/FlowVariable:isOutput", "automation/FlowVariable:name", diff --git a/packages/spec/src/automation/flow.test.ts b/packages/spec/src/automation/flow.test.ts index 4d45125797..acb30f7605 100644 --- a/packages/spec/src/automation/flow.test.ts +++ b/packages/spec/src/automation/flow.test.ts @@ -83,7 +83,7 @@ describe('FlowVariableSchema', () => { it('should accept various data types', () => { const types = ['text', 'number', 'boolean', 'object', 'list']; - + types.forEach(type => { const variable = { name: 'testVar', @@ -92,6 +92,64 @@ describe('FlowVariableSchema', () => { expect(() => FlowVariableSchema.parse(variable)).not.toThrow(); }); }); + + // ── #4697: `defaultValue` — the key that makes "declared" mean "bound" ── + // + // The engine binds an `isInput` variable only when `params[name] !== undefined`, + // so before this key a declaration guaranteed nothing at run time. Conditions + // are strict CEL, where an unbound name ABORTS the predicate instead of reading + // as `false` — the whole run stops (hotcrm#643). The engine half of the contract + // is pinned in `service-automation/src/flow-variable-default.test.ts`; this half + // is the authorable surface. + describe('defaultValue (#4697)', () => { + it('accepts a declared default, and keeps it as authored', () => { + const parsed = FlowVariableSchema.parse({ + name: 'createOpportunity', type: 'boolean', isInput: true, defaultValue: false, + }); + expect(parsed.defaultValue).toBe(false); + // Not coerced away by the `isInput`/`isOutput` defaults sitting beside it. + expect(parsed.isInput).toBe(true); + expect(parsed.isOutput).toBe(false); + }); + + it('is OPTIONAL — a variable without it parses exactly as before, and reads back absent', () => { + const parsed = FlowVariableSchema.parse({ name: 'v', type: 'text' }); + expect(parsed.defaultValue).toBeUndefined(); + expect('defaultValue' in parsed).toBe(false); + // The distinction the engine's `!== undefined` boundary rests on: an + // absent key and an authored `null` are different declarations. + expect(FlowVariableSchema.parse({ name: 'v', type: 'object', defaultValue: null }).defaultValue).toBeNull(); + }); + + it('carries a value of any shape — `type` is an open string, so there is no vocabulary to check against', () => { + // Same posture as every other `defaultValue` on the authoring surface (a + // mapping's, an action param's, a page state slot's, a screen field's): + // the value is not cross-validated against the declared `type`. Pinned so + // that adding such a check reads as the deliberate new validation surface + // it would be, rather than as a tightening nobody notices. + for (const defaultValue of [false, 0, '', 'text', [], {}, { nested: { deep: 1 } }, [1, 2, 3]]) { + expect(() => FlowVariableSchema.parse({ name: 'v', type: 'boolean', defaultValue })).not.toThrow(); + } + }); + + it('a flow carries variables with defaults through FlowSchema', () => { + const flow = FlowSchema.parse({ + name: 'lead_conversion', + label: 'Lead Conversion', + type: 'screen', + variables: [ + { name: 'createOpportunity', type: 'boolean', isInput: true, defaultValue: false }, + { name: 'attempts', type: 'number', defaultValue: 0 }, + ], + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [{ id: 'e1', source: 'start', target: 'end' }], + }); + expect(flow.variables?.map(v => v.defaultValue)).toEqual([false, 0]); + }); + }); }); describe('FlowNodeSchema', () => { @@ -1550,6 +1608,17 @@ describe('unknown keys are rejected, not stripped (#4001)', () => { expect(unknownKeyIssue(FlowVariableSchema, { name: 'v', type: 'text', is_input: true })!.message) .toContain('`is_input` → `isInput`'); }); + + it('points `default` / `initialValue` at `defaultValue` (#4697)', () => { + // The two words an author reaches for — `default` is what a page state + // slot and an action param already alias, and `initialValue` is what the + // designer calls it. Still REJECTED; the alias only makes the rejection + // say where to go. + for (const key of ['default', 'initialValue']) { + expect(unknownKeyIssue(FlowVariableSchema, { name: 'v', type: 'boolean', [key]: false })!.message) + .toContain(`\`${key}\` → \`defaultValue\``); + } + }); }); // ── batch 11: the INNER blocks ──────────────────────────────────────────── diff --git a/packages/spec/src/automation/flow.zod.ts b/packages/spec/src/automation/flow.zod.ts index eeebc6d15d..1692a30ec9 100644 --- a/packages/spec/src/automation/flow.zod.ts +++ b/packages/spec/src/automation/flow.zod.ts @@ -112,11 +112,42 @@ export const FLOW_STRUCTURAL_NODE_TYPES: readonly string[] = ['start', 'end']; /** * Flow Variable Schema * Variables available within the flow execution context. + * + * `defaultValue` is what makes **declared mean bound** (#4697). Without it a + * declaration is documentation only: the engine binds an `isInput` variable + * just when `params[name] !== undefined`, so every path that omits the + * parameter leaves the name *unbound* — and conditions are strict CEL, where + * reading an unbound name aborts the whole predicate rather than yielding + * `false`. Measured on 17.0.0-rc.1 and re-measured on this shape: + * + * | expression | X unbound | X = null | X = {f:1} | + * | --------------- | ------------------------- | -------- | --------- | + * | `X.f == 1` | ABORT `Unknown variable` | ABORT | `true` | + * | `has(X.f)` | ABORT `Unknown variable` | `false` | `true` | + * | `has(vars.X)` | `false` | `true` | `true` | + * + * i.e. the guard an author reaches for first — `has(X.f)` — does not survive + * the very case it is written for; only the `vars.`-scoped `has(vars.X)` tests + * bindedness. That is why the answer here is a declared default rather than a + * guard: a guard encodes "unanswered means no" into the predicate and leaves + * the graph defect in place, while a default removes the unbound state. + * + * Reported from HotCRM (hotcrm#643): a screen collects a checkbox into + * `createOpportunity`, the runner returns only the fields the user touched, and + * the untouched path aborted the outgoing edge — a lead conversion that + * persisted nothing. The workaround was an `assignment` node before every + * screen, mirroring the screen field's own `defaultValue`. + * + * The value is **not** cross-checked against `type` — same posture as every + * other `defaultValue` on the authoring surface (`mapping`, an action param, a + * page state slot, a screen field): the declared `type` is itself an open + * `string`, so there is no closed vocabulary to check against, and inventing + * one here would be a new validation surface rather than this additive key. */ export const FlowVariableSchema = lazySchema(() => strictObject( { surface: 'this flow variable', - aliases: { input: 'isInput', output: 'isOutput' }, + aliases: { input: 'isInput', output: 'isOutput', default: 'defaultValue', initialValue: 'defaultValue' }, history: 'Until #4001 these were dropped silently — the variable still parsed, so a ' + 'mis-declared input/output contract shipped without a diagnostic.', @@ -126,6 +157,12 @@ export const FlowVariableSchema = lazySchema(() => strictObject( type: z.string().describe('Data type (text, number, boolean, object, list)'), isInput: z.boolean().default(false).describe('Is input parameter'), isOutput: z.boolean().default(false).describe('Is output parameter'), + defaultValue: z.unknown().optional() + .describe( + 'Value bound at run start when no parameter supplies one — this is what makes a ' + + 'declared variable always bound. An explicitly supplied param wins, including ' + + '`false` and `null`; the boundary is `params[name] !== undefined`.', + ), })); /**