diff --git a/.changeset/decision-branch-routing-enforced.md b/.changeset/decision-branch-routing-enforced.md index 623e48e9eb..a546d1638d 100644 --- a/.changeset/decision-branch-routing-enforced.md +++ b/.changeset/decision-branch-routing-enforced.md @@ -74,6 +74,21 @@ sibling — the actual hole), `flow-default-edge-with-condition` and Both of the first two fire on the pre-fix `convert-lead.flow.ts` and are silent after it. +## Effect on flows that already exist + +Enforcing `isDefault` changes how a **stored** flow behaves, and the flows it +changes are mostly Studio's own. `objectui`'s flow edge inspector has always +written `isDefault: true` when you bind an out-edge to a decision's default/else +branch — into a key with zero readers, so that edge ran unconditionally, in +parallel with whichever branch actually matched. Those flows now take exactly +one branch. That is the fix, but it is a behaviour change on existing data +rather than only on newly authored metadata, so it is worth knowing before +upgrading: a flow that quietly ran two paths will now run one. + +Nothing changes for an edge that never carried the marker — `isDefault` defaults +to `false`, and an ordinary unconditional out-edge still fans out in parallel +exactly as before. + ## The example app `crm_convert_lead_wizard`'s guard is now a plain exclusive gateway: the diff --git a/.changeset/schemaless-node-expression-ledger.md b/.changeset/schemaless-node-expression-ledger.md new file mode 100644 index 0000000000..5d3d070181 --- /dev/null +++ b/.changeset/schemaless-node-expression-ledger.md @@ -0,0 +1,76 @@ +--- +"@objectstack/spec": minor +"@objectstack/service-automation": patch +"@objectstack/lint": patch +--- + +fix(spec): a node that publishes no descriptor configSchema can now own an expression-ledger entry (#4439) + +`FLOW_NODE_EXPRESSION_PATHS` is the #4027 ledger that tells `registerFlow` and +`objectstack validate` which config keys hold expressions, and in which dialect. +Its ratchet (`config-expression-ledger.test.ts`) derives what it expects from +descriptor `configSchema` `xExpression` markers, and fails in **both** +directions — an undeclared marker, or a ledger entry nothing declares. + +`decision` / `script` / `subflow` publish **no** descriptor `configSchema` on +purpose: a published partial schema would drop the editors their hand-written +Studio forms need (the #4210 incident), so their contract lives in +`schemaless-node-config.zod.ts`. Those two rules compose into a hole — an +expression slot on a schemaless node is structurally unreachable by the ratchet, +and because the reverse direction rejects unclaimed entries, it cannot be +entered by hand either. + +`decision.conditions[].expression` sat in that hole. Its own schema says +*"Bare CEL predicate deciding this branch"* and its own comment names `{…}` as +the #1491 trap, and no validator walked it — so `{lead_record.status} == +'converted'` passed `tsc`, passed `objectstack validate`, passed registration. +#4414 made that fail loudly at run time; this makes it fail at build time, +which is the delay #4027 exists to remove. + +## The fix + +The ratchet now reads **both** declaration channels: + +- **descriptor `configSchema`** — unchanged, enumerated from the live registry; +- **`schemaless-node-config.zod.ts`** — the marker rides + `.meta({ xExpression })` through `z.toJSONSchema`, the same channel + `loop.collection` has used since objectui#2670. + +Spec hands the second channel over as JSON Schema +(`getSchemalessNodeConfigJsonSchemas()`, memoized, `input` mode — the shape a +descriptor's `configSchema` already is), so the ratchet walks both with the +*same* function. No second notion of "a declared expression property", which is +the duplication a ledger exists to remove, and no `zod` dependency added to +`service-automation`. Each channel is separately asserted non-empty, so a broken +derivation on one side cannot hide behind the other's results. + +`SCHEMALESS_NODE_CONFIG_SCHEMAS` is also exported for anything else that needs +to reason about all node config contracts. Additive — objectui's +`flow-node-config` reconciliation imports each schema by name and is unaffected. + +## The sweep + +The other schemaless slots were checked and deliberately carry no marker: +`script.template` is a template **id**, not a body; `script.inputs` / +`script.variables` / `subflow.input` are values that interpolate `{token}` — +text-with-holes, the shape essentially every node config string has, already +covered generically by `validate-flow-template-paths` and the CLI flow linter. +A `flow-template` ledger entry means something narrower: a *reference that must +resolve to a value*, like `loop.collection`. So `decision.conditions[] +.expression` is the only genuinely declared expression slot on the class — now +recorded in the ledger's header so it is not re-derived. + +## Docs corrected + +The flows guide taught the **wrong dialect** for decision predicates in three +places (`'{order_amount} > 10000'`), plus a "braces missing in a decision +expression" warning that inverted after #4414 — and `FlowNodeSchema`'s own +`@example` did the same. All corrected to bare CEL, with the history stated so +an author with a braced predicate knows what changed and why their build now +fails. The dialect table drops from three dialects to two: predicates never take +braces, values always do. + +Verified: 13 new/updated tests across the ratchet, the engine's registration +pass and `@objectstack/lint` (including the exact app-crm predicate rejected at +both `registerFlow` and `objectstack validate`); `pnpm build`, `pnpm typecheck` +(122 tasks), `pnpm lint` and `check:docs` clean. diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index 8e19cb8868..eda67e7f47 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -31,7 +31,8 @@ const approvalFlow = { type: 'decision', label: 'Check Amount', config: { - // decision expressions are bare CEL, like every other condition — no braces + // decision expressions are bare CEL, like every other condition — no + // braces; see Expressions in flows conditions: [ { label: 'High Value', expression: 'order_amount > 10000' }, { label: 'Standard', expression: 'order_amount <= 10000' }, @@ -168,6 +169,7 @@ or missing-`required` violation (#4277). A node type that publishes no type: 'decision', label: 'Check Status', config: { + // Bare CEL — the labels must match this node's out-edge labels exactly. conditions: [ { label: 'Approved', expression: "status == 'approved'" }, { label: 'Rejected', expression: "status == 'rejected'" }, @@ -782,9 +784,16 @@ sibling), `flow-default-edge-with-condition` and `flow-multiple-default-edges`. A decision node that declares **no** `conditions` reports no branch at all — it -is a plain gateway and its out-edges do the routing. Do not declare both: -`config.conditions` *and* per-edge `condition`s on the same node means the node -picks a branch, and then that branch's edge re-decides. +is a plain gateway and its out-edges do the routing. + +Declaring **both** — `config.conditions` *and* per-edge `condition`s — is +redundant but not wrong: the node picks a branch, and then that branch's edge +re-decides with the same predicate. The Studio flow designer emits exactly this +(it copies each branch's expression and label onto the edge it wires), and it +routes correctly because the two are kept in sync by construction. Hand-written +metadata has no such guarantee, which is the whole of #4414: when the two +disagree, the node's branch wins the narrowing and the edge's predicate decides +what actually runs. If you are writing the flow by hand, pick one. ### Fault edges — handling a failed node @@ -979,7 +988,7 @@ condition is CEL; braces are for values.** |:---|:---|:---|:---| | Start-node `condition` | **CEL** (bare, no braces) | `record.amount > 500` | `record.*`, `previous.*`, bare field names, `vars.*` | | Edge `condition` | **CEL** (bare, no braces) | `record.status == 'open'` | same as above | -| Decision-node `conditions[].expression` | **CEL** (bare, no braces) | `order_amount > 10000` | same as above | +| Decision-node `conditions[].expression` | **CEL** (bare, no braces) | `order_amount > 10000` | flow variables by name, and `vars.*` | | Field values in `create_record` / `update_record` | **Interpolation** (braces required) | `'Follow up on {record.name}'`, `'{TODAY() + 7}'` | `{var}`, `{var.path}`, `{$User.Id}`, `{$User.Email}`, `{NOW()}`, `{TODAY()}`, `{TODAY() + 90}` (whole days) | @@ -1001,11 +1010,19 @@ a field access on an object variable is not one. Both reported `success`. They are bare CEL now, so the spellings in the table above are the correct ones and `lead_record.status == 'converted'` resolves the field. -The `{var}` form still works where it always did — `{amount} > 100`, -`{status} == 'active'` — but the two ways it used to answer `false` without -saying so are now **loud errors** naming the reference: a `{…}` hole that -matches no flow variable, and a substituted value that is neither a boolean, a -number, nor part of a comparison (#4336). +The `{var}` form still works where a condition is a plain authored string — a +start node's `config.condition`: `{amount} > 100`, `{status} == 'active'`. The +two ways it used to answer `false` without saying so are now **loud errors** +naming the reference: a `{…}` hole that matches no flow variable, and a +substituted value that is neither a boolean, a number, nor part of a comparison +(#4336). + +**A decision's `conditions[].expression` is the exception — it is always CEL.** +The slot is declared bare CEL and is on the expression ledger as a predicate +(#4439), so a braced spelling there is not the `{var}` dialect but a build +failure: `os build` and `registerFlow` reject it, naming +`config.conditions[N].expression`. That is deliberate — the alternative is a +build that refuses what run time would happily execute. CEL conditions that fail to evaluate raise an error and stop the run — they diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index 59b78f6a68..9e8d98456f 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -578,6 +578,43 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { expect(issues).toHaveLength(0); }); + /** + * #4439 — `decision.conditions[].expression` reaches the ledger through the + * SCHEMALESS channel (`decision` publishes no descriptor `configSchema`, so + * the marker rides `.meta({ xExpression })` on the Zod contract). Until then + * the ratchet could only see descriptor-declared slots, so this predicate — + * documented bare CEL, evaluated as bare CEL since #4414 — was checked by + * nobody and a `{…}` spelling passed `objectstack validate`. + */ + const decisionFlow = (expression: string) => ({ + objects, + flows: [{ + name: 'convert_lead', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_lead' } }, + { id: 'check', type: 'decision', config: { conditions: [{ label: 'Yes', expression }] } }, + ], + edges: [], + }], + }); + + it('flags a `{var}` template dialect in a decision branch expression (#4439)', () => { + // The exact predicate app-crm shipped (#4414). + const issues = validateStackExpressions(decisionFlow("{lead_record.status} == 'converted'")); + const found = issues.filter(i => i.where.includes('conditions[0].expression')); + expect(found).toHaveLength(1); + expect(found[0].severity).toBe('error'); + expect(found[0].where).toContain("flow 'convert_lead'"); + expect(found[0].where).toContain("node 'check'"); + expect(found[0].where).toContain('decision branch expression'); + expect(found[0].source).toBe("{lead_record.status} == 'converted'"); + }); + + it('passes the corrected bare-CEL decision predicate (#4439)', () => { + const issues = validateStackExpressions(decisionFlow("lead_record.status == 'converted'")); + expect(issues.filter(i => i.where.includes('conditions'))).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` 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 67a42ef97b..4f897fe49a 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 @@ -28,6 +28,7 @@ import { describe, it, expect } from 'vitest'; import { FLOW_NODE_EXPRESSION_PATHS, + getSchemalessNodeConfigJsonSchemas, resolveFlowNodeExpressions, type FlowNodeExpressionRole, } from '@objectstack/spec/automation'; @@ -93,29 +94,67 @@ function collectExpressionProps( const engine = new AutomationEngine(silentLogger()); installBuiltinNodes(engine, ctx()); -/** Every declared expression slot, derived from the live descriptors. */ -function declaredFromDescriptors(): { nodeType: string; path: string; role: FlowNodeExpressionRole }[] { - const found: { nodeType: string; path: string; role: FlowNodeExpressionRole }[] = []; +type DeclaredSlot = { nodeType: string; path: string; role: FlowNodeExpressionRole }; + +/** Resolve an `xExpression` marker to its ledger role, failing loudly on an unknown one. */ +function roleOf(nodeType: string, path: string, marker: string): FlowNodeExpressionRole { + const role = ROLE_BY_MARKER[marker]; + expect( + role, + `${nodeType}.${path} declares an unknown xExpression marker '${marker}' — ` + + `add it to ROLE_BY_MARKER and teach the validators which dialect it takes`, + ).toBeDefined(); + return role!; +} + +/** Declared expression slots on builtins that publish a descriptor `configSchema`. */ +function declaredFromDescriptors(): DeclaredSlot[] { + const found: DeclaredSlot[] = []; for (const descriptor of engine.getActionDescriptors()) { const schema = descriptor.configSchema as SchemaNode | undefined; for (const { path, marker } of collectExpressionProps(schema)) { - const role = ROLE_BY_MARKER[marker]; - expect( - role, - `${descriptor.type}.${path} declares an unknown xExpression marker '${marker}' — ` + - `add it to ROLE_BY_MARKER and teach the validators which dialect it takes`, - ).toBeDefined(); - found.push({ nodeType: descriptor.type, path, role: role! }); + found.push({ nodeType: descriptor.type, path, role: roleOf(descriptor.type, path, marker) }); + } + } + return found; +} + +/** + * Declared expression slots on the builtins that publish NO descriptor + * `configSchema` (#4439). + * + * `script` / `subflow` / `decision` keep their contract in + * `schemaless-node-config.zod.ts` on purpose, so deriving only from descriptors + * made their expression slots structurally unreachable by this ratchet — and + * because the reverse direction fails on a ledger entry nothing declares, they + * could not be entered by hand either. `decision.conditions[].expression` sat + * in that hole. + * + * Spec hands these over as JSON Schema — the same shape a descriptor's + * `configSchema` is — so the marker walk below is literally the same function. + * No second notion of "a declared expression property", which is the + * duplication a ledger exists to remove. + */ +function declaredFromSchemalessConfigs(): DeclaredSlot[] { + const found: DeclaredSlot[] = []; + for (const [nodeType, json] of Object.entries(getSchemalessNodeConfigJsonSchemas())) { + for (const { path, marker } of collectExpressionProps(json as SchemaNode)) { + found.push({ nodeType, path, role: roleOf(nodeType, path, marker) }); } } return found; } +/** Every declared expression slot, from BOTH declaration channels. */ +function declaredEverywhere(): DeclaredSlot[] { + return [...declaredFromDescriptors(), ...declaredFromSchemalessConfigs()]; +} + const key = (e: { nodeType: string; path: string; role: string }) => `${e.nodeType}.${e.path} (${e.role})`; describe('configSchema ↔ expression-ledger reconciliation (#4027)', () => { it('every xExpression property a builtin declares is in the ledger', () => { - const declared = declaredFromDescriptors(); + const declared = declaredEverywhere(); // Sanity: if this ever empties, the derivation broke and the whole ratchet // would pass vacuously — the failure mode a ledger test must not have. expect(declared.length, 'no xExpression properties found — derivation is broken').toBeGreaterThan(0); @@ -129,13 +168,39 @@ describe('configSchema ↔ expression-ledger reconciliation (#4027)', () => { ).toEqual([]); }); + // Each channel must be non-empty on its own. Merging them into one list would + // let a broken derivation on either side hide behind the other's results — + // which is exactly how the schemaless channel went unnoticed until #4439. + it.each([ + ['descriptor configSchema', declaredFromDescriptors], + ['schemaless-node-config.zod.ts', declaredFromSchemalessConfigs], + ] as const)('derives at least one slot from the %s channel', (_channel, derive) => { + expect(derive().length).toBeGreaterThan(0); + }); + it('the ledger carries no path a builtin no longer declares', () => { - const declared = new Set(declaredFromDescriptors().map(key)); + const declared = new Set(declaredEverywhere().map(key)); // Structural predicate surfaces (`config.condition`, `edge.condition`) are - // not descriptor properties and are deliberately absent from the ledger, so - // every ledger entry must correspond to a real declared property. + // not declared config properties on either channel and are deliberately + // absent from the ledger, so every ledger entry must correspond to a real + // declared property. const stale = FLOW_NODE_EXPRESSION_PATHS.map(key).filter((k) => !declared.has(k)); - expect(stale, 'stale ledger entries — the descriptor no longer declares these').toEqual([]); + expect(stale, 'stale ledger entries — no descriptor or schemaless schema declares these').toEqual([]); + }); + + it('decision.conditions[].expression is covered — the #4439 hole', () => { + const decision = FLOW_NODE_EXPRESSION_PATHS.find( + (e) => e.nodeType === 'decision' && e.path === 'conditions[].expression', + ); + expect( + decision, + 'the slot a schemaless node could not own: declared bare CEL, walked by neither validator', + ).toBeDefined(); + expect(decision!.role).toBe('predicate'); + // And it must reach the ledger through the schemaless channel specifically — + // `decision` publishes no descriptor configSchema, by design. + expect(declaredFromSchemalessConfigs().map(key)).toContain(key(decision!)); + expect(declaredFromDescriptors().map(key)).not.toContain(key(decision!)); }); it('screen.fields[].visibleWhen is covered — the #3528 regression', () => { @@ -185,7 +250,25 @@ describe('resolveFlowNodeExpressions — path resolution (#4027)', () => { expect(resolveFlowNodeExpressions('screen', { fields: 'nope' })).toEqual([]); }); + it('resolves each decision branch predicate, with its index (#4439)', () => { + const found = resolveFlowNodeExpressions('decision', { + conditions: [ + { label: 'Yes', expression: "lead.status == 'converted'" }, + { label: 'No', expression: 'true' }, + ], + }); + expect(found.map((f) => f.path)).toEqual([ + 'conditions[0].expression', + 'conditions[1].expression', + ]); + expect(found.every((f) => f.entry.role === 'predicate')).toBe(true); + }); + it('returns nothing for a node type with no declared slots', () => { + // `config.condition` is a STRUCTURAL surface both validators already walk, + // deliberately not a ledger entry — and `assignment` declares no slots. + expect(resolveFlowNodeExpressions('assignment', { condition: 'a == b' })).toEqual([]); + // A decision branching purely on its edges declares no predicate here. expect(resolveFlowNodeExpressions('decision', { condition: 'a == b' })).toEqual([]); }); }); diff --git a/packages/services/service-automation/src/builtin/decision-branch-routing.test.ts b/packages/services/service-automation/src/builtin/decision-branch-routing.test.ts index 55328439ea..426e089967 100644 --- a/packages/services/service-automation/src/builtin/decision-branch-routing.test.ts +++ b/packages/services/service-automation/src/builtin/decision-branch-routing.test.ts @@ -228,14 +228,16 @@ describe('decision branch routing (#4414)', () => { expect(visited).toEqual(['proceed']); }); - it('fails loudly on a brace-in-CEL decision predicate rather than deciding `false`', async () => { - engine.registerFlow('guard', guardFlow({ + it('refuses a brace-in-CEL decision predicate rather than deciding `false`', () => { + // #4414 made this loud (it used to string-compare and decide `false` + // forever); #4439 put the slot on the expression ledger, so the refusal + // now lands at REGISTRATION and never reaches a run. See the + // registration block at the bottom of this file for the located + // diagnostic. + expect(() => engine.registerFlow('guard', guardFlow({ proceedIsDefault: true, conditions: [{ label: 'Yes', expression: "{lead.status} == 'converted'" }], - })); - const result = await run({ status: 'converted' }); - expect(result.success).toBe(false); - expect(String(result.error)).toMatch(/template braces|bare CEL/); + }))).toThrow(/template braces|bare CEL/); }); it("lets the `default` sentinel claim the `isDefault` edge when no condition matched", async () => { @@ -250,3 +252,118 @@ describe('decision branch routing (#4414)', () => { expect(warnings).toHaveLength(0); }); }); + +/** + * #4439 — the decision's branch predicate is now on the expression ledger, so + * a brace-in-CEL predicate is a REGISTRATION error rather than a run-time one. + * + * #4414 made the failure loud; this makes it early. Before both, the raw string + * went to the legacy `{var}` template path, `{lead.status}` never resolved, and + * the branch was decided by string comparison — silently, forever. + */ +describe('decision branch predicate is validated at registration (#4439)', () => { + let engine: AutomationEngine; + + beforeEach(() => { + engine = new AutomationEngine(createTestLogger()); + registerLogicNodes(engine, createCtx()); + }); + + const flowWith = (expression: string) => ({ + name: 'guard', + label: 'Guard', + type: 'autolaunched' as const, + nodes: [ + { id: 'start', type: 'start' as const, label: 'Start' }, + { id: 'check', type: 'decision' as const, label: 'Check', config: { conditions: [{ label: 'Yes', expression }] } }, + { id: 'end', type: 'end' as const, label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'check' }, + { id: 'e2', source: 'check', target: 'end', label: 'Yes' }, + ], + }); + + it('rejects a brace-in-CEL branch predicate, naming the slot', () => { + const register = () => engine.registerFlow('guard', flowWith("{lead.status} == 'converted'")); + expect(register).toThrow(/\{lead\.status\} == 'converted'/); + expect(register).toThrow(/template braces|bare CEL/); + // The diagnostic must locate it — a flow may carry several branches. + expect(register).toThrow(/conditions\[0\]\.expression/); + }); + + it('accepts the bare-CEL spelling', () => { + expect(() => engine.registerFlow('guard', flowWith("lead.status == 'converted'"))).not.toThrow(); + }); +}); + +/** + * The shape objectui's flow designer actually emits, pinned. + * + * `FlowEdgeInspector.applyBranch()` copies a decision branch onto the edge it + * wires: a guarded branch becomes `{ condition, label }`, and the `true`/empty + * branch becomes `{ isDefault: true, label }`. So Studio has been writing + * `isDefault` since long before anything read it (#4414) — every Studio + * "default/else" edge ran unconditionally, in parallel with whichever branch + * matched. These flows are the ones enforcement changes, and they must now take + * exactly one path. + * + * It is also the double declaration the authoring guide tells hand-writers to + * avoid — node `conditions[]` AND per-edge `condition`s. It is correct here + * only because the designer keeps the two in sync by construction, which is + * exactly why it is worth pinning rather than assuming. + */ +describe('objectui-authored decision shape (FlowEdgeInspector.applyBranch)', () => { + let engine: AutomationEngine; + let visited: string[]; + + beforeEach(() => { + warnings.length = 0; + visited = []; + engine = new AutomationEngine(createTestLogger()); + registerLogicNodes(engine, createCtx()); + engine.registerNodeExecutor({ + type: 'mark', + async execute(node) { visited.push(node.id); return { success: true }; }, + }); + engine.registerFlow('studio', { + name: 'studio', + label: 'Studio-authored', + type: 'autolaunched', + variables: [{ name: 'order_amount', type: 'number', isInput: true }], + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'check', type: 'decision', label: 'Check Amount', + config: { + conditions: [ + { label: 'High Value', expression: 'order_amount > 10000' }, + { label: 'Standard', expression: 'true' }, + ], + }, + }, + { id: 'escalate', type: 'mark', label: 'Escalate' }, + { id: 'auto', type: 'mark', label: 'Auto approve' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'check' }, + // The guarded branch: expression + label copied onto the edge. + { id: 'e2', source: 'check', target: 'escalate', label: 'High Value', condition: 'order_amount > 10000', isDefault: false }, + // The `true` branch: written as the BPMN default edge, no condition. + { id: 'e3', source: 'check', target: 'auto', label: 'Standard', isDefault: true }, + ], + }); + }); + + it('takes only the guarded branch when it matches', async () => { + await engine.execute('studio', { params: { order_amount: 20000 } } as any); + expect(visited).toEqual(['escalate']); + expect(warnings).toHaveLength(0); + }); + + it('takes only the default branch when it does not', async () => { + await engine.execute('studio', { params: { order_amount: 5000 } } as any); + expect(visited).toEqual(['auto']); + expect(warnings).toHaveLength(0); + }); +}); diff --git a/packages/services/service-automation/src/builtin/logic-nodes.ts b/packages/services/service-automation/src/builtin/logic-nodes.ts index d69b22dc5e..a1d805a847 100644 --- a/packages/services/service-automation/src/builtin/logic-nodes.ts +++ b/packages/services/service-automation/src/builtin/logic-nodes.ts @@ -50,14 +50,26 @@ export function registerLogicNodes(engine: AutomationEngine, ctx: PluginContext) for (const cond of conditions) { // `DecisionConditionSchema.expression` is declared BARE CEL - // (ADR-0032) — evaluate it as such. Handing the raw string to - // `evaluateCondition` routed it to the legacy `{var}` template - // path instead, which reads a declared-CEL field in a second - // dialect: `lead.status == 'converted'` never resolves there, - // so the branch it decides is decided by string comparison. + // (ADR-0032), so pin the dialect rather than let it be + // inferred. #4453 made `evaluateCondition` sniff a bare + // string — CEL unless it contains a `{var}` hole — which + // already fixes the #4414 case this wrap was added for. + // + // The wrap still earns its place, for a different reason: the + // sniff would route a BRACED predicate to the template + // dialect and happily run it, while #4439 put this slot on + // the expression ledger as a `predicate`, so `registerFlow` + // and `objectstack validate` reject exactly that spelling. + // Without the explicit envelope the two would disagree — + // build refuses what run time accepts, the worst of both. + // An explicit `dialect: 'cel'` keeps braces the #1491 trap + // here, which is what the contract says and what the + // validators enforce. + // // Unlike `edge.condition` this slot has no `ExpressionInput` - // envelope to carry the dialect — the decision descriptor is - // deliberately schemaless — so the executor supplies it. + // envelope of its own to carry the dialect — the decision + // descriptor is deliberately schemaless — so the executor + // supplies it. if (engine.evaluateCondition({ dialect: 'cel', source: cond.expression }, variables)) { return { success: true, branchLabel: cond.label }; } diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index b8a2cdca4a..9d0989802f 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -2287,11 +2287,13 @@ "ResolvedFlowNodeExpression (interface)", "RetryPolicy (type)", "RetryPolicySchema (const)", + "SCHEMALESS_NODE_CONFIG_SCHEMAS (const)", "SCRIPT_BUILTIN_ACTION_TYPES (const)", "SCRIPT_INVOKE_FUNCTION_ACTION_TYPE (const)", "ScheduleState (type)", "ScheduleStateParsed (type)", "ScheduleStateSchema (const)", + "SchemalessNodeType (type)", "ScreenConfig (type)", "ScreenConfigParsed (type)", "ScreenConfigSchema (const)", @@ -2355,6 +2357,7 @@ "findRegionEntry (function)", "flowForm (const)", "getApprovalNodeConfigJsonSchema (function)", + "getSchemalessNodeConfigJsonSchemas (function)", "importBpmnToConstructs (function)", "isFlowFunctionEffect (function)", "normalizeControlFlowRegions (function)", diff --git a/packages/spec/src/automation/flow-node-expression-paths.ts b/packages/spec/src/automation/flow-node-expression-paths.ts index 36cd232e22..b736d65580 100644 --- a/packages/spec/src/automation/flow-node-expression-paths.ts +++ b/packages/spec/src/automation/flow-node-expression-paths.ts @@ -25,11 +25,34 @@ * * This ledger is the declared source both validators now read, mirroring the * dispatcher↔client route ledger of #3569: one list, two consumers, plus a - * reconciliation test that fails when a descriptor's `configSchema` declares an - * `xExpression` property this ledger does not carry + * reconciliation test that fails when a declared `xExpression` property this + * ledger does not carry appears anywhere * (`config-expression-ledger.test.ts` in `service-automation`). A new expression * key can no longer be added to a designer form and silently go unvalidated. * + * ## The two channels a slot can be declared through + * + * A node type declares its config contract one of two ways, and the ratchet + * reads both (#4439): + * + * - **descriptor `configSchema`** — the JSON-Schema literal an executor + * publishes. Its `xExpression` properties are enumerable from the live + * registry. + * - **`schemaless-node-config.zod.ts`** — `script` / `subflow` / `decision` + * publish NO descriptor `configSchema` on purpose (a published partial + * schema would drop the editors their hand-written forms need — the #4210 + * incident), so their contract is a Zod schema in that module and the marker + * rides `.meta({ xExpression })` through `z.toJSONSchema`, the same channel + * `loop.collection` already used. + * + * Until #4439 only the first channel was read, and because the ratchet also + * fails on a ledger entry no channel declares, a schemaless node's expression + * slot could not be entered here even deliberately. `decision`'s + * `conditions[].expression` sat in exactly that hole: declared bare CEL by its + * own schema and its own comments, walked by neither validator, so a `{…}` + * predicate passed `objectstack validate` and surfaced only when the flow ran + * (#4414 made that run-time failure loud; this makes it a build failure). + * * ## Why the dialect must be recorded, not assumed * * `xExpression` takes two values that mean **opposite** things about braces, and @@ -97,7 +120,18 @@ export interface FlowNodeExpressionPath { * * Not listed here (deliberately): `config.condition` and `edge.condition`. Those * are *structural* predicate surfaces on every node and edge rather than - * descriptor-declared config properties, and both validators already walk them. + * declared config properties, and both validators already walk them. + * + * Also deliberately absent: config values that merely INTERPOLATE `{token}` + * templates — `script.inputs` / `script.variables` / `subflow.input`, + * `notify.body`, `create_record.fields.*` and so on. Those are text-with-holes, + * the shape essentially every node config string has, already covered + * generically (`validate-flow-template-paths`, the CLI flow linter's + * `collectTemplateStrings`). A `flow-template` ledger entry means something + * narrower: a slot whose value is a *reference that must resolve to a value*, + * like `loop.collection`. The #4439 sweep of the schemaless class found exactly + * one genuinely declared expression slot — `decision.conditions[].expression` — + * and `script.template` is a template **id**, not a template body. */ export const FLOW_NODE_EXPRESSION_PATHS: readonly FlowNodeExpressionPath[] = [ { @@ -106,6 +140,15 @@ export const FLOW_NODE_EXPRESSION_PATHS: readonly FlowNodeExpressionPath[] = [ role: 'predicate', label: 'screen field visibleWhen', }, + { + // Declared through the schemaless channel — `decision` publishes no + // descriptor `configSchema`, so the marker lives on + // `DecisionConditionSchema.expression`'s `.meta()` (#4439). + nodeType: 'decision', + path: 'conditions[].expression', + role: 'predicate', + label: 'decision branch expression', + }, { nodeType: 'loop', path: 'collection', diff --git a/packages/spec/src/automation/flow.zod.ts b/packages/spec/src/automation/flow.zod.ts index b157d762b0..e06867e9c6 100644 --- a/packages/spec/src/automation/flow.zod.ts +++ b/packages/spec/src/automation/flow.zod.ts @@ -113,10 +113,12 @@ export const FlowVariableSchema = lazySchema(() => z.object({ * type: "decision", * label: "Is High Value?", * config: { + * // Bare CEL — braces are the #1491 trap and fail at registration. + * // Each `label` must match an out-edge's `label` to route anywhere; + * // when nothing matches, the `isDefault` out-edge is the fallback. * conditions: [ - * // Bare CEL, like every other condition — no `{…}` braces (#4336). * { label: "Yes", expression: "amount > 10000" }, - * { label: "No", expression: "true" } // default + * { label: "No", expression: "true" } // catch-all, NOT the default path * ] * }, * position: { x: 300, y: 200 } diff --git a/packages/spec/src/automation/schemaless-node-config.zod.ts b/packages/spec/src/automation/schemaless-node-config.zod.ts index 99cfd81e9e..997e5a3213 100644 --- a/packages/spec/src/automation/schemaless-node-config.zod.ts +++ b/packages/spec/src/automation/schemaless-node-config.zod.ts @@ -195,8 +195,22 @@ export type SubflowConfigParsed = z.infer; export const DecisionConditionSchema = lazySchema(() => z.object({ /** Branch label — must match an out-edge's `label` to route anywhere. */ label: z.string().describe("Branch label; the winning branch resumes down the out-edge with this label (no match → the out-edge marked isDefault, or one labelled 'default')"), - /** Bare-CEL predicate (ADR-0032) — `{…}` template braces are the #1491 trap. */ - expression: z.string().describe('Bare CEL predicate deciding this branch'), + /** + * Bare-CEL predicate (ADR-0032) — `{…}` template braces are the #1491 trap. + * + * `xExpression: 'expression'` is what carries that from a comment into the + * machine-readable contract (#4439): it rides the `.meta()` → JSON-Schema + * channel (same as `loop.collection`'s `'template'` marker), so the + * expression ledger can claim this slot even though `decision` publishes no + * descriptor `configSchema`, and `registerFlow` / `objectstack validate` then + * check it as the bare CEL it is. Before that the declaration was prose only: + * both validators walked a hardcoded list this key was not on, so a + * brace-in-CEL predicate passed the build and was only caught at run time. + */ + expression: z.string().meta({ + description: 'Bare CEL predicate deciding this branch', + xExpression: 'expression', + }), })); export type DecisionCondition = z.input; @@ -226,3 +240,62 @@ export const DecisionConfigSchema = lazySchema(() => z.object({ export type DecisionConfig = z.input; export type DecisionConfigParsed = z.infer; + +// ─── registry ──────────────────────────────────────────────────────── + +/** + * Every schemaless builtin's config contract, keyed by `node.type` (#4439). + * + * The descriptor-schema'd builtins can be enumerated at run time — the engine's + * registry hands out their `configSchema`s — but these three publish none by + * design, so anything that wants to reason about *all* node config contracts + * had to name them one by one. That is how the expression ledger's + * reconciliation ratchet ended up structurally unable to cover them: it derives + * its expectation from descriptor `configSchema`s, and a node that has none + * could never own a ledger entry, no matter what its contract declared. + * + * With this map the ratchet reads BOTH channels — descriptor `xExpression` + * markers and these schemas' `.meta({ xExpression })` markers — so a declared + * expression slot is covered wherever it is declared, and a stale ledger entry + * still fails from either side. + * + * Additive: objectui's `flow-node-config` reconciliation imports each schema by + * name and is unaffected. + */ +export const SCHEMALESS_NODE_CONFIG_SCHEMAS = { + script: ScriptConfigSchema, + subflow: SubflowConfigSchema, + decision: DecisionConfigSchema, +} as const satisfies Record; + +/** Node types whose config contract lives in this module rather than a descriptor. */ +export type SchemalessNodeType = keyof typeof SCHEMALESS_NODE_CONFIG_SCHEMAS; + +/** + * {@link SCHEMALESS_NODE_CONFIG_SCHEMAS} as JSON Schema, memoized — the same + * shape a descriptor's `configSchema` is, so a consumer can read both channels + * with one walk instead of two notions of "a declared config property" (#4439). + * + * Derived in `input` mode like {@link getApprovalNodeConfigJsonSchema}, which + * is what carries `.meta({ xExpression })` markers through verbatim. + * + * These are **not** published on a descriptor — that is the whole point of the + * schemaless class (see this module's header) — so nothing here reaches the + * Studio property form. It exists so validation ledgers and reconciliation + * ratchets can see these contracts at all. + */ +let cachedSchemalessNodeConfigJsonSchemas: Readonly> | undefined; +export function getSchemalessNodeConfigJsonSchemas(): Readonly> { + if (cachedSchemalessNodeConfigJsonSchemas === undefined) { + const out = {} as Record; + for (const [nodeType, schema] of Object.entries(SCHEMALESS_NODE_CONFIG_SCHEMAS)) { + out[nodeType as SchemalessNodeType] = z.toJSONSchema(schema, { + target: 'draft-2020-12', + io: 'input', + unrepresentable: 'any', + }); + } + cachedSchemalessNodeConfigJsonSchemas = out; + } + return cachedSchemalessNodeConfigJsonSchemas; +}