From 1f19abf1a89a2c26dd1908e854d91ec2acf2c6c7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 15:05:12 +0000 Subject: [PATCH] fix(formula): arm the CEL hydration retry off cel-js's structured code (#6679) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `isNumericOverloadError` decided whether to run the ADR-0032 §1c hydration retry by testing `/no such overload/i` against `err.message` — the last message-text read in `cel-engine.ts` that armed behaviour after #6223 / PR #6677 closed the same hole in `classifyError`. It now reads `err instanceof EvaluationError && err.code === 'no_such_overload'`, the class-and-code rule `classifyCelFault` already follows one function below. The phrase was reachable from a native throw: the `matches()` stdlib binding is `new RegExp(String(re)).test(...)`, so an uncompilable pattern escapes cel-js unwrapped as a `SyntaxError` echoing the pattern verbatim, from the source or from a row. Measuring that for the fix found a case the filing expected might not exist: when hydration lets the expression short-circuit around the throwing call, the spurious retry succeeds and returns a value where the fault was right. record.s == "5.0" ? matches(record.name, "no such overload(") : false { s: "5.0", name: "x" } -> was: ok/false now: the regex fault record.s == "5.0" ? matches(record.name, "(") : false { s: "5.0", name: "x" } -> the regex fault (unchanged) Both directions pinned in `cel-overload-retry-trigger.test.ts`: a native throw carrying the phrase no longer arms the retry, and a genuine cel-js `no_such_overload` still does (#1530, #1534). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CkomWBsADMsq174GmqhxhC --- .../cel-overload-retry-structured-code.md | 48 +++++ packages/formula/src/cel-engine.ts | 40 +++- .../src/cel-overload-retry-trigger.test.ts | 173 ++++++++++++++++++ 3 files changed, 253 insertions(+), 8 deletions(-) create mode 100644 .changeset/cel-overload-retry-structured-code.md create mode 100644 packages/formula/src/cel-overload-retry-trigger.test.ts diff --git a/.changeset/cel-overload-retry-structured-code.md b/.changeset/cel-overload-retry-structured-code.md new file mode 100644 index 0000000000..d784da692c --- /dev/null +++ b/.changeset/cel-overload-retry-structured-code.md @@ -0,0 +1,48 @@ +--- +"@objectstack/formula": patch +--- + +fix(formula): the CEL hydration retry arms off cel-js's structured code, not the phrase "no such overload" (#6679) + +`celEngine.evaluate` catches a fault and asks `isNumericOverloadError` whether to +hydrate string-serialized numeric / date fields and re-evaluate once — the +ADR-0032 §1c accommodation for `Field.rating` → `"5.0"` and `Field.date` → +`"2026-06-20"` (#1530, #1534). That question was answered by +`/no such overload/i.test(err.message)`: the last message-text read in +`cel-engine.ts` that armed behaviour after #6223 / PR #6677 closed the same hole +in `classifyError`. It now reads +`err instanceof EvaluationError && err.code === 'no_such_overload'`, the same +class-and-code rule `classifyCelFault` already follows one function below. + +The phrase was reachable from a **native** throw, not only from cel-js. Our +`matches()` stdlib binding is `new RegExp(String(re)).test(...)`, so an +uncompilable pattern escapes cel-js unwrapped as a `SyntaxError` echoing the +pattern verbatim — `Invalid regular expression: /no such overload(/` — which +matched. The pattern can be written in the source or read off a row via +`matches(record.name, record.re)`. + +The filing recorded this as observation-class, expecting the consequence to be +nil because the retry re-throws the original error. Measuring it for the fix +found one case where it is not nil, so this ships as a fix rather than a +tolerance removal: when hydration lets the expression short-circuit around the +throwing call, the spurious retry **succeeds** and returns a value where the +fault was the right answer. + +```text +record.s == "5.0" ? matches(record.name, "no such overload(") : false + { s: "5.0", name: "x" } -> was: ok, false now: the regex fault +record.s == "5.0" ? matches(record.name, "(") : false + { s: "5.0", name: "x" } -> the regex fault (unchanged) +``` + +Evaluation 1 takes the `matches(...)` branch and throws natively; the phrase +armed the retry; hydration made `record.s` the number `5`, so `5 == "5.0"` went +false, the ternary took the other branch, and `matches` was never called. Two +expressions that differ only in whether a regex literal happens to contain the +phrase no longer disagree about whether they fault. + +The behaviour change is one-directional and narrow. A genuine cel-js +`no_such_overload` still arms the retry and every §1c hydration behaves exactly +as before; only a native throw whose message merely contains the phrase stops +arming it. Faults are otherwise unchanged — a native throw carries no cel-js +contract, so it is still reported as `runtime` (#6223). diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index a9985b6b60..1d2f594128 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -1036,16 +1036,40 @@ const ISO_TEMPORAL_STRING_RE = /^\d{4}-\d{2}-\d{2}(?:[T ]\d{2}:\d{2}(?::\d{2})?(?:\.\d+)?(?:Z|[+-]\d{2}:?\d{2})?)?$/; /** - * cel-js raises `no such overload: dyn int` (and kin) when a comparison - * or arithmetic operator sees a `string` on one side and a number on the - * other. ADR-0032 §1c — numeric fields that serialize as strings (`Field.rating` - * → `"5.0"`, `Field.currency` → `"250000.00"`, `Field.percent`) trip this in - * flow conditions / formulas (#1530, #1534) even though the schema and the - * build-time validator treat them as numeric. + * cel-js's code for the operand-type fault this engine accommodates. Raised + * from `lib/operators.js` when a comparison or arithmetic operator sees a + * `string` on one side and a number on the other, and phrased + * `no such overload: dyn >= int`. + */ +const CEL_NO_SUCH_OVERLOAD_CODE = 'no_such_overload'; + +/** + * Whether a fault is the one ADR-0032 §1c accommodates — numeric and date + * fields that serialize as strings (`Field.rating` → `"5.0"`, `Field.currency` + * → `"250000.00"`, `Field.date` → `"2026-06-20"`) tripping a cel-js operand + * overload in flow conditions / formulas (#1530, #1534) even though the schema + * and the build-time validator treat them as numeric / temporal. + * + * Read off the error CLASS and its structured `code`, never off its prose — + * the same rule {@link classifyCelFault} follows one function below, and for a + * sharper reason than symmetry (#6679). This predicate was the last + * message-text read in this file that armed behaviour after #6223 / PR #6677, + * and the phrase it matched is reachable from a **native** throw: our own + * `matches()` stdlib binding is `new RegExp(String(re)).test(...)`, so an + * uncompilable pattern escapes cel-js unwrapped as a `SyntaxError` echoing the + * pattern verbatim — `Invalid regular expression: /no such overload(/`. The + * pattern can come from the source or, via `matches(record.name, record.re)`, + * from a row. + * + * That armed the retry on a fault ADR-0032 §1c never claimed, and the + * consequence is not cosmetic: when hydration lets the expression + * short-circuit around the throwing call, the retry *succeeds* and returns a + * value where the fault was the right answer, so two expressions differing + * only in whether a regex literal contains the phrase disagree about whether + * they fault at all. Pinned both ways in `cel-overload-retry-trigger.test.ts`. */ function isNumericOverloadError(err: unknown): boolean { - const message = err instanceof Error ? err.message : String(err); - return /no such overload/i.test(message); + return err instanceof EvaluationError && err.code === CEL_NO_SUCH_OVERLOAD_CODE; } /** diff --git a/packages/formula/src/cel-overload-retry-trigger.test.ts b/packages/formula/src/cel-overload-retry-trigger.test.ts new file mode 100644 index 0000000000..c2dd6a90ea --- /dev/null +++ b/packages/formula/src/cel-overload-retry-trigger.test.ts @@ -0,0 +1,173 @@ +/** + * #6679 — what arms the ADR-0032 §1c hydration retry. + * + * `celEngine.evaluate` catches a fault, and `isNumericOverloadError` decides + * whether to hydrate string-serialized numeric / date fields and re-evaluate + * once (#1530, #1534). Until this card that decision was a text read — + * `/no such overload/i` against `err.message` — the last message-text read in + * `cel-engine.ts` that armed behaviour after #6223 / PR #6677 closed the same + * hole in `classifyError`. + * + * The phrase is reachable from a **native** throw, not only from cel-js. Our + * `matches()` stdlib binding is `new RegExp(String(re)).test(...)`, so an + * uncompilable pattern escapes cel-js unwrapped as a native `SyntaxError` that + * echoes the pattern verbatim (measured on cel-js 8.0.0): + * + * ```text + * matches(record.name, "no such overload(") + * -> SyntaxError: Invalid regular expression: /no such overload(/: Unterminated group + * ``` + * + * That message contains the phrase, so the retry used to arm on it. The filing + * expected the consequence to be nil — the retry re-throws the original error, + * which is classified correctly. It is not nil: if the expression short-circuits + * around the throwing call once a field is hydrated, the retry *succeeds* and + * returns a value where the fault was the right answer. `retry succeeds on a + * native throw` below is that case, and it is why this file pins the trigger by + * value and not only by the predicate's own answer. + * + * Both directions are pinned. Narrowing the trigger must not disarm the thing + * the retry exists for. + */ +import { describe, expect, it } from 'vitest'; +import { Environment, EvaluationError } from '@marcbachmann/cel-js'; + +import { celEngine } from './cel-engine'; +import type { Expression } from '@objectstack/spec'; + +const cel = (source: string): Expression => ({ dialect: 'cel', source }); + +/** + * Evaluate `source` against `record`, counting how many times CEL reads + * `record.`. + * + * The count is the retry's fingerprint and needs no export: one read means the + * expression was evaluated once, i.e. the retry never armed. More than one + * means it did — `hydrateOverloadStrings` walks the live scope object with + * `Object.entries` before re-evaluating, so arming the retry always re-reads + * the record. + */ +function evaluateCountingReads( + source: string, + probe: string, + value: unknown, + rest: Record = {}, +) { + let reads = 0; + const record = { + ...rest, + get [probe]() { + reads++; + return value; + }, + }; + const result = celEngine.evaluate(cel(source), { record }); + return { result, reads }; +} + +describe('ADR-0032 §1c hydration retry — what arms it (#6679)', () => { + describe('a native throw whose message merely contains the phrase does NOT arm it', () => { + it('does not re-evaluate for an uncompilable regex literal echoing the phrase', () => { + const { result, reads } = evaluateCountingReads( + 'matches(record.name, "no such overload(")', + 'name', + 'x', + ); + + expect(result.ok).toBe(false); + if (result.ok) throw new Error('expected the regex compilation to fault'); + // The fault itself is unchanged — a native throw carries no cel-js + // contract, so `runtime` is still the honest verdict (#6223). + expect(result.error.kind).toBe('runtime'); + expect(result.error.message).toContain('Invalid regular expression'); + // …and the retry never ran. Before #6679 this was 2. + expect(reads).toBe(1); + }); + + it('does not re-evaluate when the pattern comes from a ROW rather than the source', () => { + // The author does not have to write the phrase: `record.re` is a column + // value, so any row can put it in the message. + const { result, reads } = evaluateCountingReads( + 'matches(record.name, record.re)', + 'name', + 'x', + { re: 'no such overload(' }, + ); + + expect(result.ok).toBe(false); + expect(reads).toBe(1); + }); + + it('a spuriously-armed retry could SUCCEED and swallow the fault', () => { + // The case the filing did not find and expected might not exist. + // + // Evaluation 1: `record.s == "5.0"` is true, so the ternary takes the + // `matches(...)` branch and it throws natively. The message contains the + // phrase, so (before this card) the retry armed. Hydration turned + // `record.s` into the number 5, `5 == "5.0"` is false, the ternary took + // the OTHER branch, `matches` was never called — and the retry returned + // `false` where the regex fault was the right answer. + // + // Two expressions differing only in whether the regex literal happens to + // contain the phrase must not differ in whether they fault. + const withPhrase = celEngine.evaluate( + cel('record.s == "5.0" ? matches(record.name, "no such overload(") : false'), + { record: { s: '5.0', name: 'x' } }, + ); + const withoutPhrase = celEngine.evaluate( + cel('record.s == "5.0" ? matches(record.name, "(") : false'), + { record: { s: '5.0', name: 'x' } }, + ); + + expect(withPhrase.ok).toBe(false); // was `{ ok: true, value: false }` + expect(withoutPhrase.ok).toBe(false); // the control: always faulted + if (withPhrase.ok || withoutPhrase.ok) throw new Error('expected both to fault'); + expect(withPhrase.error.kind).toBe(withoutPhrase.error.kind); + }); + + it('leaves a native throw that does not contain the phrase exactly as it was', () => { + const { result, reads } = evaluateCountingReads('matches(record.name, "(")', 'name', 'x'); + + expect(result.ok).toBe(false); + if (result.ok) throw new Error('expected the regex compilation to fault'); + expect(result.error.kind).toBe('runtime'); + expect(reads).toBe(1); + }); + }); + + describe('a genuine cel-js `no_such_overload` fault still arms it', () => { + it('hydrates a string-serialized numeric field and re-evaluates (#1534)', () => { + const { result, reads } = evaluateCountingReads('record.rating >= 4', 'rating', '5.0'); + + expect(result).toEqual({ ok: true, value: true }); + expect(reads).toBeGreaterThan(1); // the retry ran + }); + + it('hydrates a string-serialized date field and re-evaluates (#1530)', () => { + const { result, reads } = evaluateCountingReads( + 'record.end_date <= daysFromNow(60)', + 'end_date', + '2026-06-20', + ); + + expect(result.ok).toBe(true); + expect(reads).toBeGreaterThan(1); + }); + + it('the fault the retry keys on really is an EvaluationError carrying `no_such_overload`', () => { + // The structured read the predicate now performs is only as good as this + // pairing. Pinned against cel-js as installed, so a re-code or a re-class + // upstream goes red here rather than by silently disarming the retry. + const env = new Environment({ unlistedVariablesAreDyn: true }); + let caught: unknown; + try { + env.evaluate('record.rating >= 4', { record: { rating: '5.0' } }); + } catch (err) { + caught = err; + } + + expect(caught).toBeInstanceOf(EvaluationError); + expect((caught as EvaluationError).code).toBe('no_such_overload'); + }); + }); +});