Skip to content

Commit 30a0d57

Browse files
committed
fix(spec): let a schemaless node own an expression-ledger entry (#4439)
`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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8as8yR67v41xEdomiTba9
1 parent 5293114 commit 30a0d57

9 files changed

Lines changed: 413 additions & 45 deletions

File tree

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
---
2+
"@objectstack/spec": minor
3+
"@objectstack/service-automation": patch
4+
"@objectstack/lint": patch
5+
---
6+
7+
fix(spec): a node that publishes no descriptor configSchema can now own an expression-ledger entry (#4439)
8+
9+
`FLOW_NODE_EXPRESSION_PATHS` is the #4027 ledger that tells `registerFlow` and
10+
`objectstack validate` which config keys hold expressions, and in which dialect.
11+
Its ratchet (`config-expression-ledger.test.ts`) derives what it expects from
12+
descriptor `configSchema` `xExpression` markers, and fails in **both**
13+
directions — an undeclared marker, or a ledger entry nothing declares.
14+
15+
`decision` / `script` / `subflow` publish **no** descriptor `configSchema` on
16+
purpose: a published partial schema would drop the editors their hand-written
17+
Studio forms need (the #4210 incident), so their contract lives in
18+
`schemaless-node-config.zod.ts`. Those two rules compose into a hole — an
19+
expression slot on a schemaless node is structurally unreachable by the ratchet,
20+
and because the reverse direction rejects unclaimed entries, it cannot be
21+
entered by hand either.
22+
23+
`decision.conditions[].expression` sat in that hole. Its own schema says
24+
*"Bare CEL predicate deciding this branch"* and its own comment names `{…}` as
25+
the #1491 trap, and no validator walked it — so `{lead_record.status} ==
26+
'converted'` passed `tsc`, passed `objectstack validate`, passed registration.
27+
#4414 made that fail loudly at run time; this makes it fail at build time,
28+
which is the delay #4027 exists to remove.
29+
30+
## The fix
31+
32+
The ratchet now reads **both** declaration channels:
33+
34+
- **descriptor `configSchema`** — unchanged, enumerated from the live registry;
35+
- **`schemaless-node-config.zod.ts`** — the marker rides
36+
`.meta({ xExpression })` through `z.toJSONSchema`, the same channel
37+
`loop.collection` has used since objectui#2670.
38+
39+
Spec hands the second channel over as JSON Schema
40+
(`getSchemalessNodeConfigJsonSchemas()`, memoized, `input` mode — the shape a
41+
descriptor's `configSchema` already is), so the ratchet walks both with the
42+
*same* function. No second notion of "a declared expression property", which is
43+
the duplication a ledger exists to remove, and no `zod` dependency added to
44+
`service-automation`. Each channel is separately asserted non-empty, so a broken
45+
derivation on one side cannot hide behind the other's results.
46+
47+
`SCHEMALESS_NODE_CONFIG_SCHEMAS` is also exported for anything else that needs
48+
to reason about all node config contracts. Additive — objectui's
49+
`flow-node-config` reconciliation imports each schema by name and is unaffected.
50+
51+
## The sweep
52+
53+
The other schemaless slots were checked and deliberately carry no marker:
54+
`script.template` is a template **id**, not a body; `script.inputs` /
55+
`script.variables` / `subflow.input` are values that interpolate `{token}`
56+
text-with-holes, the shape essentially every node config string has, already
57+
covered generically by `validate-flow-template-paths` and the CLI flow linter.
58+
A `flow-template` ledger entry means something narrower: a *reference that must
59+
resolve to a value*, like `loop.collection`. So `decision.conditions[]
60+
.expression` is the only genuinely declared expression slot on the class — now
61+
recorded in the ledger's header so it is not re-derived.
62+
63+
## Docs corrected
64+
65+
The flows guide taught the **wrong dialect** for decision predicates in three
66+
places (`'{order_amount} > 10000'`), plus a "braces missing in a decision
67+
expression" warning that inverted after #4414 — and `FlowNodeSchema`'s own
68+
`@example` did the same. All corrected to bare CEL, with the history stated so
69+
an author with a braced predicate knows what changed and why their build now
70+
fails. The dialect table drops from three dialects to two: predicates never take
71+
braces, values always do.
72+
73+
Verified: 13 new/updated tests across the ratchet, the engine's registration
74+
pass and `@objectstack/lint` (including the exact app-crm predicate rejected at
75+
both `registerFlow` and `objectstack validate`); `pnpm build`, `pnpm typecheck`
76+
(122 tasks), `pnpm lint` and `check:docs` clean.

content/docs/automation/flows.mdx

Lines changed: 22 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,10 @@ const approvalFlow = {
3131
type: 'decision',
3232
label: 'Check Amount',
3333
config: {
34-
// decision expressions use {braces} around variables — see Expressions in flows
34+
// decision expressions are bare CEL — no {braces}; see Expressions in flows
3535
conditions: [
36-
{ label: 'High Value', expression: '{order_amount} > 10000' },
37-
{ label: 'Standard', expression: '{order_amount} <= 10000' },
36+
{ label: 'High Value', expression: 'order_amount > 10000' },
37+
{ label: 'Standard', expression: 'order_amount <= 10000' },
3838
],
3939
},
4040
},
@@ -168,9 +168,10 @@ or missing-`required` violation (#4277). A node type that publishes no
168168
type: 'decision',
169169
label: 'Check Status',
170170
config: {
171+
// Bare CEL — the labels must match this node's out-edge labels exactly.
171172
conditions: [
172-
{ label: 'Approved', expression: "{status} == 'approved'" },
173-
{ label: 'Rejected', expression: "{status} == 'rejected'" },
173+
{ label: 'Approved', expression: "status == 'approved'" },
174+
{ label: 'Rejected', expression: "status == 'rejected'" },
174175
],
175176
},
176177
}
@@ -972,30 +973,34 @@ failures so one broken flow does not abort startup.
972973

973974
## Expressions in flows
974975

975-
A flow mixes **three expression dialects**, and using the wrong one is the
976-
single most common way a flow silently misbehaves. Which dialect applies is
977-
decided by *where* the expression sits — not by what it looks like:
976+
A flow mixes **two expression dialects**, and using the wrong one is the single
977+
most common way a flow silently misbehaves. Which dialect applies is decided by
978+
*where* the expression sits — not by what it looks like:
978979

979980
| Where | Dialect | Write it like | Bindings |
980981
|:---|:---|:---|:---|
981982
| Start-node `condition` | **CEL** (bare, no braces) | `record.amount > 500` | `record.*`, `previous.*`, bare field names, `vars.*` |
982983
| Edge `condition` | **CEL** (bare, no braces) | `record.status == 'open'` | same as above |
983-
| Decision-node `conditions[].expression` | **Template compare** (braces required) | `{order_amount} > 10000` | flow variables by name, in `{…}` |
984+
| Decision-node `conditions[].expression` | **CEL** (bare, no braces) | `order_amount > 10000` | flow variables by name, and `vars.*` |
984985
| 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) |
985986

986987
<Callout type="warn">
987-
**The two failure modes to memorize:**
988+
**The rule: predicates never take braces; values always do.**
988989

989-
1. **Braces missing in a decision expression**`'order_amount > 10000'` isn't
990-
evaluated as a variable at all. It compares the *string* `"order_amount"`
991-
against `"10000"`, which is **always true**, so the flow always takes the
992-
first branch and never tells you. Write `'{order_amount} > 10000'`.
990+
1. **Braces in a predicate**`'{order_amount} > 10000'` in a decision, edge or
991+
start condition is rejected at `os build` / `registerFlow` with an error that
992+
names the slot and tells you to drop the braces. Write
993+
`'order_amount > 10000'`.
993994
2. **Braces missing in a field value**`due_date: 'TODAY() + 7'` writes the
994995
literal text `TODAY() + 7` into the field. Write `'{TODAY() + 7}'`.
995996

996-
The mirror mistake is putting braces *into* a CEL condition
997-
(`'{record.amount} > 500'`) — CEL conditions fail loudly rather than silently,
998-
with an error that tells you to drop the braces.
997+
Decision expressions were the exception until v17: they were evaluated on the
998+
legacy template path, so `'{order_amount} > 10000'` was the documented spelling
999+
and a bare `order_amount > 10000` compared the *string* `"order_amount"` against
1000+
`"10000"` — always true, always the first branch, never a word about it. Both
1001+
halves are gone: the slot is evaluated as the bare CEL it was always declared to
1002+
be (#4414), and the brace spelling now fails the build (#4439). If you have a
1003+
braced decision predicate, drop the braces.
9991004
</Callout>
10001005

10011006
CEL conditions that fail to evaluate raise an error and stop the run — they

packages/lint/src/validate-expressions.test.ts

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -578,6 +578,43 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => {
578578
expect(issues).toHaveLength(0);
579579
});
580580

581+
/**
582+
* #4439 — `decision.conditions[].expression` reaches the ledger through the
583+
* SCHEMALESS channel (`decision` publishes no descriptor `configSchema`, so
584+
* the marker rides `.meta({ xExpression })` on the Zod contract). Until then
585+
* the ratchet could only see descriptor-declared slots, so this predicate —
586+
* documented bare CEL, evaluated as bare CEL since #4414 — was checked by
587+
* nobody and a `{…}` spelling passed `objectstack validate`.
588+
*/
589+
const decisionFlow = (expression: string) => ({
590+
objects,
591+
flows: [{
592+
name: 'convert_lead',
593+
nodes: [
594+
{ id: 'start', type: 'start', config: { objectName: 'crm_lead' } },
595+
{ id: 'check', type: 'decision', config: { conditions: [{ label: 'Yes', expression }] } },
596+
],
597+
edges: [],
598+
}],
599+
});
600+
601+
it('flags a `{var}` template dialect in a decision branch expression (#4439)', () => {
602+
// The exact predicate app-crm shipped (#4414).
603+
const issues = validateStackExpressions(decisionFlow("{lead_record.status} == 'converted'"));
604+
const found = issues.filter(i => i.where.includes('conditions[0].expression'));
605+
expect(found).toHaveLength(1);
606+
expect(found[0].severity).toBe('error');
607+
expect(found[0].where).toContain("flow 'convert_lead'");
608+
expect(found[0].where).toContain("node 'check'");
609+
expect(found[0].where).toContain('decision branch expression');
610+
expect(found[0].source).toBe("{lead_record.status} == 'converted'");
611+
});
612+
613+
it('passes the corrected bare-CEL decision predicate (#4439)', () => {
614+
const issues = validateStackExpressions(decisionFlow("lead_record.status == 'converted'"));
615+
expect(issues.filter(i => i.where.includes('conditions'))).toHaveLength(0);
616+
});
617+
581618
it('leaves a correct single-brace loop collection alone', () => {
582619
// `loop.collection` is the single-brace `{var}` flow-interpolation dialect,
583620
// where braces are CORRECT. It is recorded in the ledger as `flow-template`

packages/services/service-automation/src/builtin/config-expression-ledger.test.ts

Lines changed: 98 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import { describe, it, expect } from 'vitest';
2929
import {
3030
FLOW_NODE_EXPRESSION_PATHS,
31+
getSchemalessNodeConfigJsonSchemas,
3132
resolveFlowNodeExpressions,
3233
type FlowNodeExpressionRole,
3334
} from '@objectstack/spec/automation';
@@ -93,29 +94,67 @@ function collectExpressionProps(
9394
const engine = new AutomationEngine(silentLogger());
9495
installBuiltinNodes(engine, ctx());
9596

96-
/** Every declared expression slot, derived from the live descriptors. */
97-
function declaredFromDescriptors(): { nodeType: string; path: string; role: FlowNodeExpressionRole }[] {
98-
const found: { nodeType: string; path: string; role: FlowNodeExpressionRole }[] = [];
97+
type DeclaredSlot = { nodeType: string; path: string; role: FlowNodeExpressionRole };
98+
99+
/** Resolve an `xExpression` marker to its ledger role, failing loudly on an unknown one. */
100+
function roleOf(nodeType: string, path: string, marker: string): FlowNodeExpressionRole {
101+
const role = ROLE_BY_MARKER[marker];
102+
expect(
103+
role,
104+
`${nodeType}.${path} declares an unknown xExpression marker '${marker}' — ` +
105+
`add it to ROLE_BY_MARKER and teach the validators which dialect it takes`,
106+
).toBeDefined();
107+
return role!;
108+
}
109+
110+
/** Declared expression slots on builtins that publish a descriptor `configSchema`. */
111+
function declaredFromDescriptors(): DeclaredSlot[] {
112+
const found: DeclaredSlot[] = [];
99113
for (const descriptor of engine.getActionDescriptors()) {
100114
const schema = descriptor.configSchema as SchemaNode | undefined;
101115
for (const { path, marker } of collectExpressionProps(schema)) {
102-
const role = ROLE_BY_MARKER[marker];
103-
expect(
104-
role,
105-
`${descriptor.type}.${path} declares an unknown xExpression marker '${marker}' — ` +
106-
`add it to ROLE_BY_MARKER and teach the validators which dialect it takes`,
107-
).toBeDefined();
108-
found.push({ nodeType: descriptor.type, path, role: role! });
116+
found.push({ nodeType: descriptor.type, path, role: roleOf(descriptor.type, path, marker) });
117+
}
118+
}
119+
return found;
120+
}
121+
122+
/**
123+
* Declared expression slots on the builtins that publish NO descriptor
124+
* `configSchema` (#4439).
125+
*
126+
* `script` / `subflow` / `decision` keep their contract in
127+
* `schemaless-node-config.zod.ts` on purpose, so deriving only from descriptors
128+
* made their expression slots structurally unreachable by this ratchet — and
129+
* because the reverse direction fails on a ledger entry nothing declares, they
130+
* could not be entered by hand either. `decision.conditions[].expression` sat
131+
* in that hole.
132+
*
133+
* Spec hands these over as JSON Schema — the same shape a descriptor's
134+
* `configSchema` is — so the marker walk below is literally the same function.
135+
* No second notion of "a declared expression property", which is the
136+
* duplication a ledger exists to remove.
137+
*/
138+
function declaredFromSchemalessConfigs(): DeclaredSlot[] {
139+
const found: DeclaredSlot[] = [];
140+
for (const [nodeType, json] of Object.entries(getSchemalessNodeConfigJsonSchemas())) {
141+
for (const { path, marker } of collectExpressionProps(json as SchemaNode)) {
142+
found.push({ nodeType, path, role: roleOf(nodeType, path, marker) });
109143
}
110144
}
111145
return found;
112146
}
113147

148+
/** Every declared expression slot, from BOTH declaration channels. */
149+
function declaredEverywhere(): DeclaredSlot[] {
150+
return [...declaredFromDescriptors(), ...declaredFromSchemalessConfigs()];
151+
}
152+
114153
const key = (e: { nodeType: string; path: string; role: string }) => `${e.nodeType}.${e.path} (${e.role})`;
115154

116155
describe('configSchema ↔ expression-ledger reconciliation (#4027)', () => {
117156
it('every xExpression property a builtin declares is in the ledger', () => {
118-
const declared = declaredFromDescriptors();
157+
const declared = declaredEverywhere();
119158
// Sanity: if this ever empties, the derivation broke and the whole ratchet
120159
// would pass vacuously — the failure mode a ledger test must not have.
121160
expect(declared.length, 'no xExpression properties found — derivation is broken').toBeGreaterThan(0);
@@ -129,13 +168,39 @@ describe('configSchema ↔ expression-ledger reconciliation (#4027)', () => {
129168
).toEqual([]);
130169
});
131170

171+
// Each channel must be non-empty on its own. Merging them into one list would
172+
// let a broken derivation on either side hide behind the other's results —
173+
// which is exactly how the schemaless channel went unnoticed until #4439.
174+
it.each([
175+
['descriptor configSchema', declaredFromDescriptors],
176+
['schemaless-node-config.zod.ts', declaredFromSchemalessConfigs],
177+
] as const)('derives at least one slot from the %s channel', (_channel, derive) => {
178+
expect(derive().length).toBeGreaterThan(0);
179+
});
180+
132181
it('the ledger carries no path a builtin no longer declares', () => {
133-
const declared = new Set(declaredFromDescriptors().map(key));
182+
const declared = new Set(declaredEverywhere().map(key));
134183
// Structural predicate surfaces (`config.condition`, `edge.condition`) are
135-
// not descriptor properties and are deliberately absent from the ledger, so
136-
// every ledger entry must correspond to a real declared property.
184+
// not declared config properties on either channel and are deliberately
185+
// absent from the ledger, so every ledger entry must correspond to a real
186+
// declared property.
137187
const stale = FLOW_NODE_EXPRESSION_PATHS.map(key).filter((k) => !declared.has(k));
138-
expect(stale, 'stale ledger entries — the descriptor no longer declares these').toEqual([]);
188+
expect(stale, 'stale ledger entries — no descriptor or schemaless schema declares these').toEqual([]);
189+
});
190+
191+
it('decision.conditions[].expression is covered — the #4439 hole', () => {
192+
const decision = FLOW_NODE_EXPRESSION_PATHS.find(
193+
(e) => e.nodeType === 'decision' && e.path === 'conditions[].expression',
194+
);
195+
expect(
196+
decision,
197+
'the slot a schemaless node could not own: declared bare CEL, walked by neither validator',
198+
).toBeDefined();
199+
expect(decision!.role).toBe('predicate');
200+
// And it must reach the ledger through the schemaless channel specifically —
201+
// `decision` publishes no descriptor configSchema, by design.
202+
expect(declaredFromSchemalessConfigs().map(key)).toContain(key(decision!));
203+
expect(declaredFromDescriptors().map(key)).not.toContain(key(decision!));
139204
});
140205

141206
it('screen.fields[].visibleWhen is covered — the #3528 regression', () => {
@@ -185,7 +250,25 @@ describe('resolveFlowNodeExpressions — path resolution (#4027)', () => {
185250
expect(resolveFlowNodeExpressions('screen', { fields: 'nope' })).toEqual([]);
186251
});
187252

253+
it('resolves each decision branch predicate, with its index (#4439)', () => {
254+
const found = resolveFlowNodeExpressions('decision', {
255+
conditions: [
256+
{ label: 'Yes', expression: "lead.status == 'converted'" },
257+
{ label: 'No', expression: 'true' },
258+
],
259+
});
260+
expect(found.map((f) => f.path)).toEqual([
261+
'conditions[0].expression',
262+
'conditions[1].expression',
263+
]);
264+
expect(found.every((f) => f.entry.role === 'predicate')).toBe(true);
265+
});
266+
188267
it('returns nothing for a node type with no declared slots', () => {
268+
// `config.condition` is a STRUCTURAL surface both validators already walk,
269+
// deliberately not a ledger entry — and `assignment` declares no slots.
270+
expect(resolveFlowNodeExpressions('assignment', { condition: 'a == b' })).toEqual([]);
271+
// A decision branching purely on its edges declares no predicate here.
189272
expect(resolveFlowNodeExpressions('decision', { condition: 'a == b' })).toEqual([]);
190273
});
191274
});

0 commit comments

Comments
 (0)