From 30a0d57fbdcfe00281d4e19a218964af9c7a000b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 08:08:31 +0000 Subject: [PATCH 1/2] fix(spec): let a schemaless node own an expression-ledger entry (#4439) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `FLOW_NODE_EXPRESSION_PATHS` (#4027) tells `registerFlow` and `objectstack validate` which config keys hold expressions and in which dialect. Its ratchet derives what it expects from descriptor `configSchema` markers and fails both ways — an undeclared marker, and a ledger entry nothing declares. `decision` / `script` / `subflow` publish no descriptor `configSchema` on purpose (a partial one would drop the editors their hand-written Studio forms need — the #4210 incident). Those two rules compose into a hole: an expression slot on a schemaless node is unreachable by the ratchet, and the reverse direction then refuses to let it be entered by hand either. `decision.conditions[].expression` sat in that hole. Its own schema calls it a bare CEL predicate and its own comment names `{…}` as the #1491 trap, and no validator walked it — so a braced predicate passed tsc, passed validate, passed registration. #4414 made that loud at run time; this makes it a build failure, which is the delay #4027 exists to remove. The ratchet now reads both declaration channels. Spec hands the schemaless one over as JSON Schema (`getSchemalessNodeConfigJsonSchemas()`, memoized, input mode — the shape a descriptor's `configSchema` already is), so both are walked by the same function: no second notion of "a declared expression property", and no `zod` dependency added to service-automation. Each channel is separately asserted non-empty so a broken derivation cannot hide behind the other's results. Swept the rest of the class and recorded the result in the ledger header: `script.template` is a template id, not a body; `script.inputs` / `script.variables` / `subflow.input` interpolate `{token}` like essentially every node config string and are covered generically. The decision predicate is the only genuinely declared expression slot there. Docs: the flows guide taught the wrong dialect for decision predicates in three places, plus a warning that inverted after #4414 — and FlowNodeSchema's own `@example` did the same. Corrected to bare CEL with the history stated, so an author whose build now fails knows what changed. The dialect table goes from three dialects to two: predicates never take braces, values always do. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Q8as8yR67v41xEdomiTba9 --- .../schemaless-node-expression-ledger.md | 76 ++++++++++++ content/docs/automation/flows.mdx | 39 +++--- .../lint/src/validate-expressions.test.ts | 37 ++++++ .../builtin/config-expression-ledger.test.ts | 113 +++++++++++++++--- .../builtin/decision-branch-routing.test.ts | 58 ++++++++- packages/spec/api-surface.json | 3 + .../automation/flow-node-expression-paths.ts | 49 +++++++- packages/spec/src/automation/flow.zod.ts | 6 +- .../automation/schemaless-node-config.zod.ts | 77 +++++++++++- 9 files changed, 413 insertions(+), 45 deletions(-) create mode 100644 .changeset/schemaless-node-expression-ledger.md 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 7d344b7f5a..13e472daa2 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -31,10 +31,10 @@ const approvalFlow = { type: 'decision', label: 'Check Amount', config: { - // decision expressions use {braces} around variables — see Expressions in flows + // decision expressions are bare CEL — no {braces}; see Expressions in flows conditions: [ - { label: 'High Value', expression: '{order_amount} > 10000' }, - { label: 'Standard', expression: '{order_amount} <= 10000' }, + { label: 'High Value', expression: 'order_amount > 10000' }, + { label: 'Standard', expression: 'order_amount <= 10000' }, ], }, }, @@ -168,9 +168,10 @@ 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'" }, + { label: 'Approved', expression: "status == 'approved'" }, + { label: 'Rejected', expression: "status == 'rejected'" }, ], }, } @@ -972,30 +973,34 @@ failures so one broken flow does not abort startup. ## Expressions in flows -A flow mixes **three expression dialects**, and using the wrong one is the -single most common way a flow silently misbehaves. Which dialect applies is -decided by *where* the expression sits — not by what it looks like: +A flow mixes **two expression dialects**, and using the wrong one is the single +most common way a flow silently misbehaves. Which dialect applies is decided by +*where* the expression sits — not by what it looks like: | Where | Dialect | Write it like | Bindings | |:---|:---|:---|:---| | 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` | **Template compare** (braces required) | `{order_amount} > 10000` | flow variables by name, in `{…}` | +| 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) | -**The two failure modes to memorize:** +**The rule: predicates never take braces; values always do.** -1. **Braces missing in a decision expression** — `'order_amount > 10000'` isn't - evaluated as a variable at all. It compares the *string* `"order_amount"` - against `"10000"`, which is **always true**, so the flow always takes the - first branch and never tells you. Write `'{order_amount} > 10000'`. +1. **Braces in a predicate** — `'{order_amount} > 10000'` in a decision, edge or + start condition is rejected at `os build` / `registerFlow` with an error that + names the slot and tells you to drop the braces. Write + `'order_amount > 10000'`. 2. **Braces missing in a field value** — `due_date: 'TODAY() + 7'` writes the literal text `TODAY() + 7` into the field. Write `'{TODAY() + 7}'`. -The mirror mistake is putting braces *into* a CEL condition -(`'{record.amount} > 500'`) — CEL conditions fail loudly rather than silently, -with an error that tells you to drop the braces. +Decision expressions were the exception until v17: they were evaluated on the +legacy template path, so `'{order_amount} > 10000'` was the documented spelling +and a bare `order_amount > 10000` compared the *string* `"order_amount"` against +`"10000"` — always true, always the first branch, never a word about it. Both +halves are gone: the slot is evaluated as the bare CEL it was always declared to +be (#4414), and the brace spelling now fails the build (#4439). If you have a +braced decision predicate, drop the braces. 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..914009ec68 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,47 @@ 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(); + }); +}); diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index 2c68a5880c..9e3d91070e 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -2233,11 +2233,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)", @@ -2301,6 +2303,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 f48cff461f..a455a3371f 100644 --- a/packages/spec/src/automation/flow.zod.ts +++ b/packages/spec/src/automation/flow.zod.ts @@ -113,9 +113,11 @@ 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: [ - * { label: "Yes", expression: "{amount} > 10000" }, - * { label: "No", expression: "true" } // default + * { label: "Yes", expression: "amount > 10000" } * ] * }, * 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; +} From e12c2e15fed29d4323891f7611c60f1005d1e8cb Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 09:04:00 +0000 Subject: [PATCH 2/2] docs(automation): state the stored-flow impact, and pin the shape Studio emits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps found while verifying #4439. The #4414 changeset described the mechanism but not who it moves. `objectui`'s FlowEdgeInspector has always written `isDefault: true` when an out-edge is bound to a decision's default/else branch — into a key with zero readers, so that edge ran unconditionally alongside whichever branch matched. Enforcement therefore changes STORED flows, and mostly Studio's own: they now take exactly one branch. That is the fix, but it lands on existing data, which a reader of the release notes needs to know before upgrading. The same inspector also copies each branch's expression and label onto the edge it wires, so Studio emits the very double declaration the authoring guide told hand-writers to avoid. It routes correctly — the designer keeps the two sides in sync by construction — so the guide now says that plainly instead of forbidding a shape our own tool produces: redundant, not wrong, and dangerous only when the two disagree, which is the whole of #4414. Pins that exact emitted shape as a regression test, both directions. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Q8as8yR67v41xEdomiTba9 --- .../decision-branch-routing-enforced.md | 15 ++++ content/docs/automation/flows.mdx | 13 +++- .../builtin/decision-branch-routing.test.ts | 71 +++++++++++++++++++ 3 files changed, 96 insertions(+), 3 deletions(-) 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/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index 13e472daa2..6707470087 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -783,9 +783,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 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 914009ec68..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 @@ -296,3 +296,74 @@ describe('decision branch predicate is validated at registration (#4439)', () => 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); + }); +});