Skip to content

Commit 9cea1d3

Browse files
committed
test(spec,service-automation,lint): pin the absent decision-branch predicate at all three doors; ADR-0087 entry; changeset (#19961)
Claude-Session: https://claude.ai/code/session_01Rjy9MeetSfq34PKn81CRiN Co-authored-by: Claude <noreply@anthropic.com>
1 parent d4776b3 commit 9cea1d3

9 files changed

Lines changed: 651 additions & 12 deletions
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
---
2+
'@objectstack/spec': minor
3+
'@objectstack/lint': minor
4+
---
5+
6+
fix(spec)!: a `decision` branch with no `expression` — the key absent, or `null` — is refused at authoring (#19961)
7+
8+
Clause-②: no (narrowing)
9+
10+
<!-- adr-0087: registered flow-decision-branch-expression-absent-refused -->
11+
12+
**BREAKING** — an accept-set narrowing on one authored flow-node slot, shipped as
13+
`minor` under the launch-window convention (`check-changeset-no-major` refuses
14+
`major` until GA; breaking-ness is carried by this banner and the ADR-0087
15+
disposition above, not by the level).
16+
17+
**What changed.** `DecisionConditionSchema` declares a branch `{ label, expression }`
18+
with `expression` a required `z.string()`. Nothing enforced that: a decision node's
19+
`config` is an open record no schema is parsed against, and the expression ledger's
20+
resolver skipped an absent value as "not authored". So `conditions: [{ label: 'y' }]`
21+
passed `FlowSchema.parse`, `AutomationEngine.registerFlow` and `objectstack validate`,
22+
and the run then failed at that branch — the executor evaluates every branch it
23+
reaches, and a branch with no `expression` is a condition with no `source`, which
24+
`evaluateCondition` refuses. The ledger now marks the slot `required` (reconciled
25+
against the schema's own `required` list), and the branch is refused at all three
26+
doors through the walk and the function that already refuse a blank one — by
27+
`FlowSchema.parse` with a `custom` issue anchored at the slot (for example
28+
`nodes.1.config.conditions.0.expression`), by `registerFlow` and `objectstack validate`
29+
through that same parse, and by `validateStackExpressions` for a stack handed to it
30+
directly — with one message, led by the published `PREDICATE_SLOT_STRING_REFUSAL`
31+
sentence. `expression: null` is refused the same way, and so is a branch that wrote
32+
its predicate under `condition` (the edge's spelling), which has no `expression`
33+
either. The Studio flow designer writes the refused shape when a branch row's
34+
expression cell is left empty. Where such a branch already sits, the whole flow is
35+
refused: registered from the metadata
36+
registry or `sys_metadata` at boot, it is skipped with a `failed to register flow`
37+
warn naming it while the flows beside it register; a `defineStack({ flows })` source
38+
throws `StackSchemaInvalidError` for the whole stack; an artifact file is refused
39+
whole at load.
40+
41+
## FROM → TO
42+
43+
| you wrote | write instead |
44+
|:--|:--|
45+
| `conditions: [{ label: 'high' }]` on a `decision` node | the predicate you meant — `{ label: 'high', expression: 'record.amount > 10000' }` |
46+
| `conditions: [{ label: 'high', condition: 'record.amount > 10000' }]` | the same predicate under `expression` |
47+
| `conditions: [{ label: 'high', expression: null }]` | the predicate you meant, or `expression: 'false'` to keep the branch and never take it |
48+
49+
**One-line fix:** write the predicate under `expression`. `expression: 'false'` keeps
50+
the branch and its label and never takes it — a change of behaviour, not a preserved
51+
one: a run that reached the branch used to FAIL there, and now routes on to the next
52+
branch or the declared fallback. ⚠️ Do not drop a decision's only branch: the node
53+
then routes by its out-edges alone, and the out-edge that branch labelled is no
54+
longer held back.
55+
56+
**Unchanged.** A branch carrying a non-blank predicate parses, registers and
57+
validates as before; a blank one keeps its refusal and its own prescription
58+
(`flow-predicate-slot-blank-string-refused`); a `decision` with no `conditions`, or
59+
an empty list, still routes by its out-edges; an absent screen field `visibleWhen`
60+
is still legal (that slot is not required); and `PREDICATE_SLOT_STRING_REFUSAL`
61+
keeps its name and its text.

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

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {
1515
ASSIGNMENT_VALUE_ENVELOPE_REFUSAL,
1616
PREDICATE_SLOT_STRING_REFUSAL,
1717
STRUCTURAL_CONDITION_SHAPE_REFUSAL,
18+
predicateSlotRefusal,
1819
} from '@objectstack/spec/automation';
1920

2021
import {
@@ -4398,6 +4399,75 @@ describe('a blank string in a ledger predicate slot (#17493)', () => {
43984399
});
43994400
});
44004401

4402+
/**
4403+
* [#19961] A `decision` branch with no `expression`, at the THIRD door:
4404+
* `objectstack validate`'s expression pass.
4405+
*
4406+
* `DecisionConditionSchema` declares `expression` `z.string()`, not optional,
4407+
* but `conditions: [{ label: 'y' }]` reported NOTHING here: the resolver skipped
4408+
* the absent value as "not authored", and `checkDeclaredPredicate` returned
4409+
* early on `null` / absent besides. The run then failed at the branch. The
4410+
* ledger now marks the slot `required`, the resolver emits the absent value
4411+
* there, and this pass refuses it through `predicateSlotRefusal` — the same
4412+
* call, the same message, as `FlowSchema.parse` and `registerFlow`.
4413+
*
4414+
* The table is the one those two doors run (in `spec` and `service-automation`):
4415+
* nothing, a blank string, a real predicate — asserted by `where`, severity and
4416+
* the full message, read off the spec's own function.
4417+
*
4418+
* ⚠️ Through the CLI, `objectstack validate` meets the absent value first at
4419+
* its schema step (`FlowSchema.parse` refuses it there, with the same message).
4420+
* This pass is what answers for a stack handed to `validateStackExpressions`
4421+
* directly, and it is what these pins drive.
4422+
*/
4423+
describe('a decision branch with no `expression` (#19961)', () => {
4424+
const flowStack = (...branches: Record<string, unknown>[]) => ({
4425+
flows: [{
4426+
name: 'absent_flow',
4427+
nodes: [{ id: 'start', type: 'start' }, { id: 'check', type: 'decision', config: { conditions: branches } }],
4428+
edges: [],
4429+
}],
4430+
});
4431+
const errorsOf = (stack: unknown) =>
4432+
validateStackExpressions(stack as never).filter((i) => (i.severity ?? 'error') === 'error');
4433+
const WHERE_0 = "flow 'absent_flow' · node 'check' (decision) decision branch expression at config.conditions[0].expression";
4434+
4435+
it.each([
4436+
{ name: 'no `expression` key — the #19961 shape', branch: { label: 'y' }, refused: true, refusedWith: undefined },
4437+
{ name: '`expression: null`', branch: { label: 'y', expression: null }, refused: true, refusedWith: null },
4438+
{ name: 'the predicate under the edge\'s spelling `condition`', branch: { label: 'y', condition: 'true' }, refused: true, refusedWith: undefined },
4439+
{ name: 'a blank string — the #17493 control', branch: { label: 'y', expression: ' ' }, refused: true, refusedWith: ' ' },
4440+
{ name: 'a real predicate — the accept control', branch: { label: 'y', expression: 'true' }, refused: false, refusedWith: undefined },
4441+
] as Array<{ name: string; branch: Record<string, unknown>; refused: boolean; refusedWith: unknown }>)('$name', ({ branch, refused, refusedWith }) => {
4442+
const found = errorsOf(flowStack(branch));
4443+
if (!refused) {
4444+
expect(found).toHaveLength(0);
4445+
return;
4446+
}
4447+
expect(found).toHaveLength(1);
4448+
expect(found[0].severity).toBe('error');
4449+
expect(found[0].where).toBe(WHERE_0);
4450+
expect(found[0].message).toBe(predicateSlotRefusal(refusedWith)!.message);
4451+
expect(found[0].message.startsWith(PREDICATE_SLOT_STRING_REFUSAL)).toBe(true);
4452+
});
4453+
4454+
it('names WHICH branch: an absent second branch is located at index 1, the valid first one is not', () => {
4455+
const found = errorsOf(flowStack({ label: 'a', expression: 'amount > 1' }, { label: 'b' }));
4456+
expect(found.map((i) => i.where)).toEqual([
4457+
"flow 'absent_flow' · node 'check' (decision) decision branch expression at config.conditions[1].expression",
4458+
]);
4459+
expect(found[0].source).toBe('');
4460+
});
4461+
4462+
it('CONTROL — a decision with no branch, and a screen field with no `visibleWhen`, report nothing', () => {
4463+
expect(errorsOf({ flows: [{ name: 'f', nodes: [{ id: 'check', type: 'decision', config: {} }], edges: [] }] })).toHaveLength(0);
4464+
expect(errorsOf(flowStack())).toHaveLength(0);
4465+
expect(errorsOf({
4466+
flows: [{ name: 'f', nodes: [{ id: 'form', type: 'screen', config: { fields: [{ name: 'amount', type: 'number' }] } }], edges: [] }],
4467+
})).toHaveLength(0);
4468+
});
4469+
});
4470+
44014471
/**
44024472
* [#20078] A field-level predicate that reads THROUGH a reference field is
44034473
* refused at authoring, with the repair that is true for the root it reads.

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

Lines changed: 33 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,8 @@ interface SchemaNode {
4848
items?: SchemaNode;
4949
/** `true` = an open map with untyped values; an object = the schema every value takes. */
5050
additionalProperties?: boolean | SchemaNode;
51+
/** The keys of `properties` this object requires (JSON Schema `required`). */
52+
required?: string[];
5153
xExpression?: string;
5254
}
5355

@@ -83,9 +85,9 @@ const ROLE_BY_MARKER: Record<string, FlowNodeExpressionRole> = {
8385
function collectExpressionProps(
8486
schema: SchemaNode | undefined,
8587
prefix = '',
86-
): { path: string; marker: string }[] {
88+
): { path: string; marker: string; required: boolean }[] {
8789
if (!schema || typeof schema !== 'object') return [];
88-
const out: { path: string; marker: string }[] = [];
90+
const out: { path: string; marker: string; required: boolean }[] = [];
8991

9092
if (schema.properties) {
9193
for (const [key, prop] of Object.entries(schema.properties)) {
@@ -96,7 +98,11 @@ function collectExpressionProps(
9698
const here = prefix
9799
? `${prefix}.${key}${isObjectArray ? '[]' : ''}`
98100
: `${key}${isObjectArray ? '[]' : ''}`;
99-
if (typeof prop.xExpression === 'string') out.push({ path: here, marker: prop.xExpression });
101+
// [#19961] Whether the declaring object REQUIRES the slot — read off the
102+
// same schema the marker is, so the ledger's `required` flag is
103+
// reconciled against the contract rather than restated beside it.
104+
const required = Array.isArray(schema.required) && schema.required.includes(key);
105+
if (typeof prop.xExpression === 'string') out.push({ path: here, marker: prop.xExpression, required });
100106
if (isObjectArray) out.push(...collectExpressionProps(prop.items, here));
101107
else out.push(...collectExpressionProps(prop, here));
102108
}
@@ -110,7 +116,8 @@ function collectExpressionProps(
110116
const values = schema.additionalProperties;
111117
if (values && typeof values === 'object') {
112118
const here = prefix ? `${prefix}.*` : '*';
113-
if (typeof values.xExpression === 'string') out.push({ path: here, marker: values.xExpression });
119+
// A map value is never "required": the map's keys are the author's own.
120+
if (typeof values.xExpression === 'string') out.push({ path: here, marker: values.xExpression, required: false });
114121
out.push(...collectExpressionProps(values, here));
115122
}
116123
return out;
@@ -119,7 +126,7 @@ function collectExpressionProps(
119126
const engine = new AutomationEngine(silentLogger());
120127
installBuiltinNodes(engine, ctx());
121128

122-
type DeclaredSlot = { nodeType: string; path: string; role: FlowNodeExpressionRole };
129+
type DeclaredSlot = { nodeType: string; path: string; role: FlowNodeExpressionRole; required: boolean };
123130

124131
/** Resolve an `xExpression` marker to its ledger role, failing loudly on an unknown one. */
125132
function roleOf(nodeType: string, path: string, marker: string): FlowNodeExpressionRole {
@@ -137,8 +144,8 @@ function declaredFromDescriptors(): DeclaredSlot[] {
137144
const found: DeclaredSlot[] = [];
138145
for (const descriptor of engine.getActionDescriptors()) {
139146
const schema = descriptor.configSchema as SchemaNode | undefined;
140-
for (const { path, marker } of collectExpressionProps(schema)) {
141-
found.push({ nodeType: descriptor.type, path, role: roleOf(descriptor.type, path, marker) });
147+
for (const { path, marker, required } of collectExpressionProps(schema)) {
148+
found.push({ nodeType: descriptor.type, path, role: roleOf(descriptor.type, path, marker), required });
142149
}
143150
}
144151
return found;
@@ -163,8 +170,8 @@ function declaredFromDescriptors(): DeclaredSlot[] {
163170
function declaredFromSchemalessConfigs(): DeclaredSlot[] {
164171
const found: DeclaredSlot[] = [];
165172
for (const [nodeType, json] of Object.entries(getSchemalessNodeConfigJsonSchemas())) {
166-
for (const { path, marker } of collectExpressionProps(json as SchemaNode)) {
167-
found.push({ nodeType, path, role: roleOf(nodeType, path, marker) });
173+
for (const { path, marker, required } of collectExpressionProps(json as SchemaNode)) {
174+
found.push({ nodeType, path, role: roleOf(nodeType, path, marker), required });
168175
}
169176
}
170177
return found;
@@ -213,6 +220,23 @@ describe('configSchema ↔ expression-ledger reconciliation (#4027)', () => {
213220
expect(stale, 'stale ledger entries — no descriptor or schemaless schema declares these').toEqual([]);
214221
});
215222

223+
/**
224+
* [#19961] The ledger's `required` flag is what makes the resolver emit an
225+
* ABSENT value for the doors to refuse — so it must say exactly what the
226+
* declaring channel's `required` list says, in both directions. A flag the
227+
* contract does not back would refuse a legal omission (an absent
228+
* `visibleWhen` shows the field); a requirement the ledger misses is the
229+
* #19961 shape again — declared required, admitted absent at every door.
230+
*/
231+
it('the ledger marks `required` exactly the slots the declaring channel requires (#19961)', () => {
232+
const declared = declaredEverywhere();
233+
const requiredByChannel = declared.filter((d) => d.required).map(key).sort();
234+
const requiredByLedger = FLOW_NODE_EXPRESSION_PATHS.filter((e) => e.required).map(key).sort();
235+
expect(requiredByLedger, 'ledger `required` flags disagree with the declaring channel').toEqual(requiredByChannel);
236+
// Non-vacuous: the one required slot there is today is derived, not assumed.
237+
expect(requiredByChannel).toEqual(['decision.conditions[].expression (predicate)']);
238+
});
239+
216240
it('decision.conditions[].expression is covered — the #4439 hole', () => {
217241
const decision = FLOW_NODE_EXPRESSION_PATHS.find(
218242
(e) => e.nodeType === 'decision' && e.path === 'conditions[].expression',

0 commit comments

Comments
 (0)