From aca2b75f4df77703633462175884f5e16fec474c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 2 Jun 2026 02:33:29 +0000 Subject: [PATCH] docs(adr): correct ADR-0032 #1491 root-cause attribution; tighten silent-failure scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verified ADR-0032's factual claims against the code and issue #1491: - Issue #1491 was "record-change trigger loads but flows never fire," reproduced with the condition omitted — not the single-brace '{record.rating} >= 4' the ADR named as root cause. The committed fix (static import in service-automation/engine.ts, replacing a throwing CJS __require stub) attributes it to the formula engine throwing on every CEL eval, masked by evaluateCondition's silent catch -> return false. Context rewritten to that account; single-brace/CEL-map collision kept as a real latent hazard of the same class (engine.ts:1456/1458). - Decision 4 split into its two halves: build-time validation catches the malformed-expression class; removing the silent fallback catches the runtime-fault class (#1491). Build-time validation alone would not have caught #1491. - Noted the silent/inconsistent handling is repo-wide: the same EvalResult is handled five different ways (seed/hook/validation/projection/condition); target is one declared policy. - Fixed precision: coercion transform lives in spec/shared/expression.zod.ts:84 (flow.zod.ts only consumes it). Cited flow.zod.ts:213-214 JSDoc still teaching condition: "{amount} < 500". https://claude.ai/code/session_01Sc2gibgfAH8af75zitRTUh --- docs/adr/0032-unified-expression-layer.md | 24 +++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/docs/adr/0032-unified-expression-layer.md b/docs/adr/0032-unified-expression-layer.md index 69434e4d6..56d4c1740 100644 --- a/docs/adr/0032-unified-expression-layer.md +++ b/docs/adr/0032-unified-expression-layer.md @@ -25,11 +25,14 @@ The deciding lens, as in ADR-0010/0011/0031, is **AI/pro-code authoring**. When ## Context — current state (verified 2026-06-02) -This came out of a real incident (issue #1491). A record-change flow loaded, bound, and fired correctly — but produced **zero** observable effect. Root cause: a decision/edge condition was authored as `'{record.rating} >= 4'`. Evidence, end to end: +This ADR is motivated by issue #1491, where record-change flows **loaded but never fired** on data writes — reproduced even with the **condition omitted**. The committed fix (static `import` of `@objectstack/formula` in `service-automation/engine.ts`, replacing a CJS `require` that bundled to a throwing tsup `__require` stub — see the comment at `engine.ts:11`) traced the symptom to the formula engine **throwing on every CEL evaluation**, which `evaluateCondition`'s `catch → return false` silently converted into "start condition not met → flow skipped, `success:true`." Every flow skipped; no error, no warning, nothing in any log. **That silent `catch → false` is the disease this ADR targets** — it turned an unrelated bundling fault into an invisible, platform-wide no-op. -- **Coercion.** `ExpressionInputSchema` (`spec/automation/flow.zod.ts`) turns any bare string into `{dialect:'cel', source}`. So `'{record.rating} >= 4'` is evaluated **as CEL**. -- **Collision.** CEL reads `{ … }` as a **map literal**, so `{record.rating}` is a parse error (`Expected COLON, got RBRACE`) — confirmed against `@objectstack/formula`. The single-brace template delimiter **directly collides** with CEL syntax. -- **Silent failure.** `engine.ts evaluateCondition` returns **`false`** on eval error. The `decision` node took no branch, the flow ended in ~1ms `success:true`, and the `update_record`/`notify` nodes never ran. No error, no warning, nothing in any log. +The same silent path also swallows a **second, independent hazard** that the contract (not just the bundling) must fix: **single-brace template syntax colliding with CEL**. Tracing it end to end: + +- **Coercion.** `ExpressionInputSchema` (defined in `spec/shared/expression.zod.ts:84`, consumed by `automation/flow.zod.ts`) turns any bare string into `{dialect:'cel', source}`. So a condition like `'{record.rating} >= 4'` is evaluated **as CEL**. +- **Collision.** CEL reads `{ … }` as a **map literal**, so `{record.rating}` (no `:`) is a parse error (`Expected COLON, got RBRACE`). The single-brace template delimiter **directly collides** with CEL syntax. +- **Silent failure (again).** `engine.ts evaluateCondition` returns **`false`** on parse/eval error (`engine.ts:1456`, and the bare `catch { return false }` at `1458`). The `decision` node would take no branch, the flow would end ~1ms `success:true`, and downstream nodes would never run — the *same* invisible failure mode as #1491. +- **The anti-pattern is even taught in-spec.** `automation/flow.zod.ts:213-214`'s own JSDoc example uses `condition: "{amount} < 500"` — the exact single-brace form that silently evaluates to `false`. The hazard is not hypothetical; the reference docs demonstrate it. - **Two template engines already exist.** `service-automation/builtin/template.ts` interpolates **single** `{…}` (`{var}`, `{record.x}`, `{$User.Id}`, `{NOW()}`, `{TODAY()+N}`) for flow node fields; `Object.titleFormat` / notification templates use **double** `{{…}}` mustache. CEL date helpers are **lowercase** `today()` / `daysFromNow(int)`; the template engine uses **uppercase** `TODAY()` / `NOW()`. Same intent, three spellings. Why an AI (and a human) writes it wrong: the author sees `{record.name}` work in `notify.title` and **over-generalizes** the single-brace syntax to the condition — a natural inference the platform does nothing to block, and whose failure is invisible. @@ -78,7 +81,12 @@ The canonical serialized form remains the existing envelope `{ dialect, source, ### 4. Zero silent failure: validate at build time, delete the fallback-to-false path. -`objectstack build` (and `registerFlow` / metadata registration) **parses and validates every expression**, failing with a precise source location on any error. The runtime **"eval error → return false"** branch in `evaluateCondition` is **removed** — it does not exist in the target design. A malformed expression is a **build error**, never a silent runtime no-op. (#1491 would have failed `objectstack build` at the offending line.) +Two complementary halves, catching two distinct failure modes: + +- **Build-time validation.** `objectstack build` (and `registerFlow` / metadata registration) **parses and validates every expression**, failing with a precise source location on any error. This catches the *malformed-expression* class — e.g. the single-brace `{amount} < 500` would fail `objectstack build` at the offending line instead of silently becoming `false` at runtime. +- **No silent runtime fallback.** The **"eval error → return false"** branch in `evaluateCondition` (`engine.ts:1456`/`1458`) is **removed** — it does not exist in the target design. This catches the *runtime-fault* class — e.g. the #1491 bundling fault, where the engine itself threw at runtime on valid expressions, surfaces **loudly** instead of as an invisible platform-wide no-op. (Build-time validation alone would *not* have caught #1491, because the expressions were syntactically valid; the silent `catch` is what hid it.) + +One policy, repo-wide. Today the silent/inconsistent handling is not confined to `evaluateCondition`: the same `EvalResult` is treated five different ways — `seed-loader.ts` fails **loud** (flips `success:false`), `hook-wrappers.ts` warns + treats as **false**, `rule-validator.ts` warns + **skips** (returns `null`, fail-open), `engine.ts` formula projection writes **`null`** with no log, and flow `evaluateCondition` returns **`false`** with no log. The target is a **single, declared failure policy** applied at every expression call site, not five ad-hoc ones. ### 5. Schema-aware validation — the greenfield payoff. @@ -106,7 +114,7 @@ Reserved for future dialects (`sql`, `cron`) via the same envelope; CEL is the d ## Consequences **Positive** -- The #1491 class of bug becomes **unrepresentable** (type error) or **caught at build** (validator) — never a silent runtime no-op. +- The malformed-expression class (single-brace, bad field refs) becomes **unrepresentable** (type error) or **caught at build** (validator); the runtime-fault class (the actual #1491 bundling failure) surfaces **loudly** instead of as a silent no-op. Neither can produce an invisible `success:true` again. - One mental model for every author; the format is LLM-safe and LLM-self-correcting. - Schema-aware errors raise authoring quality across *all* CEL surfaces (flows, validations, sharing, formula fields), not just flows. @@ -119,8 +127,8 @@ Reserved for future dialects (`sql`, `cron`) via the same envelope; CEL is the d ## Sequencing (roadmap) 1. **Contract** — `spec`: expression fields become typed envelopes (`Predicate` / `Template` / `Expr`); remove bare-string acceptance and the string→CEL coercion. Land `` cel`` `` / `` tpl`` `` builders. -2. **Engine** — `formula`: `{{CEL}}` template interpolation; unify date helpers under CEL stdlib (`today()`/`daysFromNow`). `service-automation`: route templates through it; **delete** the eval-error→false fallback in `evaluateCondition`. -3. **Build-time validation** — CLI/registration parses every expression; fail with location. (Catches the whole #1491 class immediately.) +2. **Engine** — `formula`: `{{CEL}}` template interpolation; unify date helpers under CEL stdlib (`today()`/`daysFromNow`). `service-automation`: route templates through it; **delete** the eval-error→false fallback in `evaluateCondition`, and adopt **one declared failure policy** across all five call sites (seed / hook / validation / projection / condition) rather than today's five ad-hoc ones. +3. **Build-time validation** — CLI/registration parses every expression; fail with location. (Catches the malformed-expression class immediately.) Also fix the in-spec authoring examples that still teach the anti-pattern (`automation/flow.zod.ts:213-214` `condition: "{amount} < 500"`) and the skills/docs. 4. **Schema-aware validation** — project object schema into the CEL type env; field-existence + return-type checks (see Open Question 1 for v1 depth). 5. **Designer** — GUI condition/template builders emit the canonical IR; one validator shared with build.