diff --git a/.changeset/evaluate-condition-shape-refusal.md b/.changeset/evaluate-condition-shape-refusal.md new file mode 100644 index 0000000000..fb872e476d --- /dev/null +++ b/.changeset/evaluate-condition-shape-refusal.md @@ -0,0 +1,17 @@ +--- +"@objectstack/service-automation": minor +--- + +`evaluateCondition` now refuses a malformed condition shape with the same `STRUCTURAL_CONDITION_SHAPE_REFUSAL` registration already raises — evaluation and registration share one refusal, so a shape that slipped past registration can never surface as a raw `TypeError` or as a silent `false`. + +#15662 closed the reject set at the producer: `registerFlow` refuses a structural condition (`config.condition` on a node, `edge.condition`) that is neither CEL text nor an expression envelope. The evaluator was left saying the opposite thing in a different vocabulary, and that half matters because `evaluateCondition` is a **public method on an exported class** — a plugin reaches it directly regardless of what `registerFlow` admits, and a flow stored before that gate landed replays through it. + +The unguarded read had three arms, all of them now refused by the shared `structuralConditionRefusal` — the same call `registerFlow` makes, not a second hand-written envelope that could drift from it: + +- an envelope whose `source` is present and **not a string** (`{ source: 1 }`, `{ dialect: 'cel', source: 1 }`) reached `.trim()` and threw `TypeError: exprStr.trim is not a function`, naming no flow, no node and no expression; +- a value that is neither text nor envelope-shaped (`42`, `true`, `['a']`, `{}`, `{ dialect: 'cel' }`) was read as an **empty condition** and answered `false` — the "an unauthored branch must not open" rule applied to a value that was very much authored, on the same key a start node's **trigger gate** is read from; +- a malformed envelope carrying a non-predicate dialect (`{ dialect: 'cron', source: 1 }`) answered `false` one statement earlier still, at the dialect check, never reaching the source derivation at all. + +**What still evaluates is unchanged, and is pinned as controls.** Bare CEL text and both envelope spellings evaluate exactly as before; an `ast`-only envelope still answers `false`; a well-formed non-predicate dialect (`{ dialect: 'cron', source: '0 0 * * *' }`) still answers `false` rather than being refused; absent, `null`, empty and whitespace-only conditions are still "not authored", not malformed. A malformed **string** still earns its own verdict — the brace trap or the ADR-0032 §1c CEL fault — never the shape refusal. + +An app whose stored flow carries one of the refused shapes in a node or edge `condition` now fails that run loudly with a message carrying the rule, instead of skipping a branch in silence or faulting unattributed; the fix is to write the condition as bare CEL text (`record.rating >= 4`) or as an expression envelope. diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index f7a18c3c4e..213c3d650f 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -8709,8 +8709,45 @@ export class AutomationEngine implements IAutomationService { * throw: an explicit `dialect: 'cel'` is the author saying "this is CEL", and * `{…}` is a map literal there. The sniff only applies where the dialect was * never stated. + * + * ## The shape gate, shared with registration (#16038) + * + * The FIRST statement, above everything else, is `structuralConditionRefusal` + * — the same call `registerFlow` makes on the same slots, not a second + * hand-written envelope that would drift from it. #15662 closed the reject + * set at the producer; this closes it at the evaluator, so the two are one + * set by construction rather than by agreement. It matters because + * `evaluateCondition` is a public method on an exported class: a plugin + * reaches it directly regardless of what `registerFlow` admits, and a flow + * stored before that gate landed replays through here. + * + * It sits ABOVE the dialect check, not at the `exprStr` derivation, because + * the unguarded read has three arms and the derivation is only two of them: + * a non-string `source` threw a bare `TypeError: exprStr.trim is not a + * function` naming nothing; a value that is neither text nor envelope read + * as an EMPTY condition and answered `false` — on the same key a start + * node's trigger gate is read from; and a malformed envelope carrying a + * non-predicate dialect (`{ dialect: 'cron', source: 1 }`) answered `false` + * one statement earlier still, never reaching the derivation at all. + * + * What it does NOT refuse is what the constructor admits, and those are + * controls, not oversights: every string (a malformed one still earns the + * #1491 brace trap or the §1c CEL fault below), absent/`null`, and an + * envelope carrying a string `source` or an `ast` — the `ast`-only arm + * still falls through to `false`, since that population is #15430/#15807's + * and not this ruling's. */ evaluateCondition(expression: string | { dialect?: string; source?: string; ast?: unknown }, variables: Map): boolean { + const shapeRefusal = structuralConditionRefusal(expression); + if (shapeRefusal) { + // ADR-0032 §1d — the error carries its source. `structuralConditionRefusal` + // attributes an empty one for the shape it is refusing here (a non-string + // `source` cannot be the attribution), which is its documented choice. + throw new Error( + `condition evaluation error: ${shapeRefusal.message} — source: \`${shapeRefusal.source}\``, + ); + } + const isEnvelope = typeof expression === 'object' && expression != null && 'dialect' in expression; const dialect = isEnvelope ? (expression as { dialect?: string }).dialect : undefined; const exprStr = typeof expression === 'string' ? expression : ((expression as { source?: string })?.source ?? ''); diff --git a/packages/services/service-automation/src/structural-condition-shape.test.ts b/packages/services/service-automation/src/structural-condition-shape.test.ts index 0f3f7288ba..50cad3f784 100644 --- a/packages/services/service-automation/src/structural-condition-shape.test.ts +++ b/packages/services/service-automation/src/structural-condition-shape.test.ts @@ -144,3 +144,154 @@ describe('#15662 — a structural condition that is neither text nor an expressi }); }); }); + +/** + * #16038 — the EVALUATION half of #15662's principle, ruled by the maintainer + * on 2026-09-06 (decision batch #57, option A): `evaluateCondition` refuses a + * malformed condition shape with the SAME `STRUCTURAL_CONDITION_SHAPE_REFUSAL` + * registration already raises, so a shape that slipped past registration — + * older stored data, or a direct caller of this public method on an exported + * class — can never surface as a raw `TypeError`, nor as a silent `false`. + * + * ## The enumeration, measured on `3e7ef9c23` before the fix + * + * There is ONE unguarded read — `exprStr`, derived at the top of + * `evaluateCondition` — and it has three distinct failure arms. Every helper + * the method delegates to (`templateHoles`, `celScope`, + * `refuseUnresolvedTemplateHole`, `refuseUnresolvedCelOperand`, + * `compareValues`) is handed `exprStr` and nothing else, so a guard above the + * derivation closes the whole delegation tree. One test per arm, below: + * + * - **A — a raw `TypeError`.** An envelope whose `source` is PRESENT and not a + * string, under a predicate dialect. `?? ''` covers only absent/`null`, so + * `exprStr` becomes the non-string value and `.trim()` throws + * `TypeError: exprStr.trim is not a function` — naming no flow, no node and + * no expression. This is the reported arm. + * - **B — a silent `false`.** A value that is neither text nor envelope-shaped: + * the read yields `undefined`, `?? ''` supplies the empty source, and the + * "an unauthored branch must not open" arm answers `false` for a value that + * was very much authored — on the same key a start node's TRIGGER GATE is + * read from. + * - **C — a silent `false` one statement earlier.** A malformed envelope + * carrying a NON-predicate dialect (`{ dialect: 'cron', source: 1 }`) returns + * `false` at the dialect pre-check, BEFORE the trim. This arm is why the + * guard is the method's first statement rather than a patch at the reported + * line: a fix written at `exprStr` never reaches it. + * + * ## The sibling value path is NOT a site — measured, not assumed + * + * `evaluateValueEnvelope` already derives its verdict from + * `valueEnvelopeRefusals`, the same call `registerFlow` makes (#15137), so it + * is already this shape one door over with its own shared constructor. Driven + * before the fix, `{ source: 1 }`, `{ dialect: 'cel', source: 1 }`, + * `{ dialect: 'cel', source: {} }`, `{ ast, source: 1 }`, `{ dialect: 'cel' }`, + * `42`, `['a']` and `{}` each threw an ATTRIBUTED error leading with + * `ASSIGNMENT_VALUE_ENVELOPE_REFUSAL` or a located CEL fault — zero raw + * `TypeError`s. Nothing to move there, which is why nothing here does. + * + * ## No caller depended on the `TypeError` + * + * Also measured, because the ruling's landing shape turns on it: repo-wide, + * every occurrence of `is not a function` on this path is PROSE recording the + * pre-fix symptom, never an assertion and never a `catch` that branches. The + * two engine-internal callers (the start gate and the edge gate) call it bare, + * so a throw propagates to `execute()`'s catch and is recorded as a loud flow + * failure — ADR-0032 §1c's prescribed handling, not a regression. + */ +const REFUSED_AT_EVALUATION: Array<[label: string, value: unknown, arm: string]> = [ + // Arm A — reached `.trim()` and threw a bare `TypeError`. + ['`{ source: 1 }` (the reproduction)', { source: 1 }, 'A'], + ['a `cel` envelope with a number source', { dialect: 'cel', source: 1 }, 'A'], + ['a `cel` envelope with an object source', { dialect: 'cel', source: {} }, 'A'], + ['a `template` envelope with a number source', { dialect: 'template', source: 1 }, 'A'], + // Arm C — returned `false` at the dialect pre-check, one statement earlier. + ['a non-predicate-dialect envelope with a number source', { dialect: 'cron', source: 1 }, 'C'], + // Arm B — returned `false` off the empty-source arm. + ['a number', 42, 'B'], + ['a boolean', true, 'B'], + ['an array', ['a'], 'B'], + ['an object that is neither', {}, 'B'], + ['an envelope with no source and no ast', { dialect: 'cel' }, 'B'], +]; + +describe('#16038 — evaluation refuses the same shapes registration does', () => { + const evaluate = (value: unknown) => () => + new AutomationEngine(silentLogger).evaluateCondition( + value as never, + new Map([['record', { rating: 5 }]]), + ); + + for (const [label, value, arm] of REFUSED_AT_EVALUATION) { + it(`refuses ${label} (arm ${arm})`, () => { + expect(evaluate(value)).toThrow(STRUCTURAL_CONDITION_SHAPE_REFUSAL); + }); + } + + it('never answers a malformed shape with a bare TypeError again', () => { + // The filed symptom, asserted as an absence: the refusal must REPLACE + // the `TypeError`, not sit beside it. Without this an implementation + // that threw the refusal only on the `false` arms would pass every + // assertion above except the arm-A rows. + for (const [, value] of REFUSED_AT_EVALUATION) { + expect(evaluate(value)).not.toThrow('is not a function'); + } + }); + + it('attributes the refusal — ADR-0032 §1d, the error carries its source', () => { + expect(evaluate({ source: 1 })).toThrow(/source:/); + }); + + /** + * The property the ruling is actually about, asserted mechanically rather + * than described: ONE population walked through BOTH doors, refused by both + * with the same published sentence. Two hand-written envelopes that drifted + * apart would fail here while every per-site test above stayed green. + */ + it('the reject set of registration and the reject set of evaluation are ONE set', () => { + for (const [label, value] of REFUSED_AT_EVALUATION) { + expect(register(flowWith({ decisionCondition: value })), `registration: ${label}`) + .toThrow(STRUCTURAL_CONDITION_SHAPE_REFUSAL); + expect(evaluate(value), `evaluation: ${label}`) + .toThrow(STRUCTURAL_CONDITION_SHAPE_REFUSAL); + } + }); + + describe('CONTROLS — the shapes evaluation must still answer, not refuse', () => { + it('bare CEL text, and both envelope spellings, still evaluate', () => { + expect(evaluate('record.rating >= 4')()).toBe(true); + expect(evaluate({ source: 'record.rating >= 4' })()).toBe(true); + expect(evaluate({ dialect: 'cel', source: 'record.rating >= 4' })()).toBe(true); + }); + + it('an `ast`-only envelope still answers `false` — that population is #15430/#15807', () => { + // `structuralConditionRefusal` admits an `ast`, so this must fall + // through to the empty-source arm exactly as before. If this ever + // throws, the guard swallowed a different card's population. + expect(evaluate({ dialect: 'cel', ast: { kind: 'const' } })()).toBe(false); + }); + + it('a WELL-FORMED non-predicate dialect still answers `false`, not a refusal', () => { + // The arm-C boundary: `cron` is not a boolean predicate here, but a + // string source makes the SHAPE authorable, so the pre-existing + // `false` stands. Only the malformed spelling moved. + expect(evaluate({ dialect: 'cron', source: '0 0 * * *' })()).toBe(false); + }); + + it('an unauthored condition is not a malformed one', () => { + expect(evaluate(null)()).toBe(false); + expect(evaluate(undefined)()).toBe(false); + expect(evaluate('')()).toBe(false); + expect(evaluate(' ')()).toBe(false); + }); + + it('a malformed STRING still earns its own verdict, not the shape refusal', () => { + // RED CONTROL — the shape gate must not shadow the #1491 brace trap + // or the §1c CEL fault. If this goes green the guard is refusing + // strings, which `structuralConditionRefusal` admits by design. + expect(evaluate({ dialect: 'cel', source: '{record.rating} >= 4' })) + .toThrow(/template braces|failed to evaluate as CEL/); + expect(evaluate({ dialect: 'cel', source: '{record.rating} >= 4' })) + .not.toThrow(STRUCTURAL_CONDITION_SHAPE_REFUSAL); + }); + }); +});