Skip to content

Commit f3141d8

Browse files
os-zhuangclaude
andauthored
fix(spec): let a schemaless node own an expression-ledger entry (objectstack-ai#4439) (objectstack-ai#4474)
* fix(spec): let a schemaless node own an expression-ledger entry (objectstack-ai#4439) `FLOW_NODE_EXPRESSION_PATHS` (objectstack-ai#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 objectstack-ai#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 objectstack-ai#1491 trap, and no validator walked it — so a braced predicate passed tsc, passed validate, passed registration. objectstack-ai#4414 made that loud at run time; this makes it a build failure, which is the delay objectstack-ai#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 objectstack-ai#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 * docs(automation): state the stored-flow impact, and pin the shape Studio emits Two gaps found while verifying objectstack-ai#4439. The objectstack-ai#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 objectstack-ai#4414. Pins that exact emitted shape as a regression test, both directions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q8as8yR67v41xEdomiTba9 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent de0464a commit f3141d8

11 files changed

Lines changed: 523 additions & 45 deletions

File tree

.changeset/decision-branch-routing-enforced.md

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,21 @@ sibling — the actual hole), `flow-default-edge-with-condition` and
7474
Both of the first two fire on the pre-fix `convert-lead.flow.ts` and are silent
7575
after it.
7676

77+
## Effect on flows that already exist
78+
79+
Enforcing `isDefault` changes how a **stored** flow behaves, and the flows it
80+
changes are mostly Studio's own. `objectui`'s flow edge inspector has always
81+
written `isDefault: true` when you bind an out-edge to a decision's default/else
82+
branch — into a key with zero readers, so that edge ran unconditionally, in
83+
parallel with whichever branch actually matched. Those flows now take exactly
84+
one branch. That is the fix, but it is a behaviour change on existing data
85+
rather than only on newly authored metadata, so it is worth knowing before
86+
upgrading: a flow that quietly ran two paths will now run one.
87+
88+
Nothing changes for an edge that never carried the marker — `isDefault` defaults
89+
to `false`, and an ordinary unconditional out-edge still fans out in parallel
90+
exactly as before.
91+
7792
## The example app
7893

7994
`crm_convert_lead_wizard`'s guard is now a plain exclusive gateway: the
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: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,8 @@ const approvalFlow = {
3131
type: 'decision',
3232
label: 'Check Amount',
3333
config: {
34-
// decision expressions are bare CEL, like every other condition — no braces
34+
// decision expressions are bare CEL, like every other condition — no
35+
// braces; see Expressions in flows
3536
conditions: [
3637
{ label: 'High Value', expression: 'order_amount > 10000' },
3738
{ label: 'Standard', expression: 'order_amount <= 10000' },
@@ -168,6 +169,7 @@ or missing-`required` violation (#4277). A node type that publishes no
168169
type: 'decision',
169170
label: 'Check Status',
170171
config: {
172+
// Bare CEL — the labels must match this node's out-edge labels exactly.
171173
conditions: [
172174
{ label: 'Approved', expression: "status == 'approved'" },
173175
{ label: 'Rejected', expression: "status == 'rejected'" },
@@ -782,9 +784,16 @@ sibling), `flow-default-edge-with-condition` and `flow-multiple-default-edges`.
782784

783785
<Callout type="warn">
784786
A decision node that declares **no** `conditions` reports no branch at all — it
785-
is a plain gateway and its out-edges do the routing. Do not declare both:
786-
`config.conditions` *and* per-edge `condition`s on the same node means the node
787-
picks a branch, and then that branch's edge re-decides.
787+
is a plain gateway and its out-edges do the routing.
788+
789+
Declaring **both**`config.conditions` *and* per-edge `condition`s — is
790+
redundant but not wrong: the node picks a branch, and then that branch's edge
791+
re-decides with the same predicate. The Studio flow designer emits exactly this
792+
(it copies each branch's expression and label onto the edge it wires), and it
793+
routes correctly because the two are kept in sync by construction. Hand-written
794+
metadata has no such guarantee, which is the whole of #4414: when the two
795+
disagree, the node's branch wins the narrowing and the edge's predicate decides
796+
what actually runs. If you are writing the flow by hand, pick one.
788797
</Callout>
789798

790799
### Fault edges — handling a failed node
@@ -979,7 +988,7 @@ condition is CEL; braces are for values.**
979988
|:---|:---|:---|:---|
980989
| Start-node `condition` | **CEL** (bare, no braces) | `record.amount > 500` | `record.*`, `previous.*`, bare field names, `vars.*` |
981990
| Edge `condition` | **CEL** (bare, no braces) | `record.status == 'open'` | same as above |
982-
| Decision-node `conditions[].expression` | **CEL** (bare, no braces) | `order_amount > 10000` | same as above |
991+
| Decision-node `conditions[].expression` | **CEL** (bare, no braces) | `order_amount > 10000` | flow variables by name, and `vars.*` |
983992
| 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) |
984993

985994
<Callout type="warn">
@@ -1001,11 +1010,19 @@ a field access on an object variable is not one. Both reported `success`. They
10011010
are bare CEL now, so the spellings in the table above are the correct ones and
10021011
`lead_record.status == 'converted'` resolves the field.
10031012

1004-
The `{var}` form still works where it always did — `{amount} > 100`,
1005-
`{status} == 'active'` — but the two ways it used to answer `false` without
1006-
saying so are now **loud errors** naming the reference: a `{…}` hole that
1007-
matches no flow variable, and a substituted value that is neither a boolean, a
1008-
number, nor part of a comparison (#4336).
1013+
The `{var}` form still works where a condition is a plain authored string — a
1014+
start node's `config.condition`: `{amount} > 100`, `{status} == 'active'`. The
1015+
two ways it used to answer `false` without saying so are now **loud errors**
1016+
naming the reference: a `{…}` hole that matches no flow variable, and a
1017+
substituted value that is neither a boolean, a number, nor part of a comparison
1018+
(#4336).
1019+
1020+
**A decision's `conditions[].expression` is the exception — it is always CEL.**
1021+
The slot is declared bare CEL and is on the expression ledger as a predicate
1022+
(#4439), so a braced spelling there is not the `{var}` dialect but a build
1023+
failure: `os build` and `registerFlow` reject it, naming
1024+
`config.conditions[N].expression`. That is deliberate — the alternative is a
1025+
build that refuses what run time would happily execute.
10091026
</Callout>
10101027

10111028
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)