From 37cb9fc2c5c1b5e1b454c1956c576f6ad0d779c3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 09:38:33 +0000 Subject: [PATCH 1/2] fix(formula): grade CEL faults by error class + code, not by the message (#6223) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `EvalResult.error.kind` is author-facing: `@objectstack/objectql`'s `cel-fault` puts it in front of the author as `${kind}: ${first line}` and `packages/rest` re-emits it as the HTTP body's `reason`. cel-js embeds the author's own source line in `message` (`formatErrorWithHighlight`), so a classifier that regexes that text is matching text the author writes. PR #6202 closed the ParseError arm structurally and deliberately left `type` / `runtime` on the keyword table pending a per-code audit. This is that audit; its verdict is that the table goes entirely. Measured on cel-js 8.0.0 — one `no such overload` EVALUATION fault, four field names, three wrong answers: record.status > 1 -> runtime (right) record.parse_status > 1 -> parse (wrong) record.syntax_mode > 1 -> parse (wrong) record.type_code > 1 -> type (wrong) `classifyError` now reads only structured contract: ParseError -> `bounds` when `code === 'limit_exceeded'` else `parse`; EvaluationError -> `type` for the one declaration-class code (`unknown_variable`) else `runtime`; anything that is not a cel-js error -> `runtime`. Two audit findings recorded in the code: - The residual keyword arm was NOT dormant. `matches()` is an ObjectStack stdlib binding over `new RegExp(...)`, so an uncompilable pattern escapes as a native SyntaxError echoing the pattern — and the pattern can come off the ROW. `matches(record.name, record.re)` with `re = "(?"` was graded `type`; `"Exceeded maxAstNodes("` was graded `bounds`. - There is deliberately no TypeError arm: cel-js raises that class only from its non-evaluating TypeChecker, which runs only inside `Environment#check`, and that method catches it and RETURNS `{ valid: false, error }`. The check-time TypeError -> `type` mapping already lives in `celEngine.compile`. Every evaluate-time cel-js code the engine can reach now carries a fixture pinning its `kind`. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01MwoubC3jL271FYt9rGXwxb --- .changeset/cel-classify-error-by-code.md | 57 +++++++ packages/formula/src/cel-engine.ts | 144 ++++++++++++----- .../src/cel-error-classification.test.ts | 149 +++++++++++++++++- 3 files changed, 306 insertions(+), 44 deletions(-) create mode 100644 .changeset/cel-classify-error-by-code.md diff --git a/.changeset/cel-classify-error-by-code.md b/.changeset/cel-classify-error-by-code.md new file mode 100644 index 0000000000..2e468b2548 --- /dev/null +++ b/.changeset/cel-classify-error-by-code.md @@ -0,0 +1,57 @@ +--- +'@objectstack/formula': patch +--- + +fix(formula): `classifyError` grades a CEL fault by error class + code, never by the message (#6223) + +`EvalResult.error.kind` is author-facing — `@objectstack/objectql`'s `cel-fault` +puts it in front of the author as `` `${kind}: ${first line}` `` and +`packages/rest` re-emits it as the HTTP body's `reason`. cel-js embeds the +author's own **source line** in `message` (`formatErrorWithHighlight`), so a +classifier that regex-matches that text is matching text the author writes. +PR #6202 closed the `ParseError` arm this way and left `type` / `runtime` on the +keyword table pending a per-code audit. This is that audit, and its verdict is +that the table goes entirely. + +Measured on cel-js 8.0.0 — one `no such overload` **evaluation** fault, four +field names, three wrong answers: + +```text +record.status > 1 -> runtime (right) +record.parse_status > 1 -> parse (wrong) +record.syntax_mode > 1 -> parse (wrong) +record.type_code > 1 -> type (wrong) +``` + +`parse` is the inverse of the #6133 misdirection: the expression is +syntactically perfect and failed on the data, and the author was told to go fix +an expression that has nothing wrong with it. + +`classifyError` now reads only structured contract: + +- `ParseError` -> `bounds` when `code === 'limit_exceeded'`, else `parse` + (unchanged, from #6202); +- `EvaluationError` -> `type` for the one declaration-class code + (`unknown_variable`, the root identifier is not bound in this scope at all), + else `runtime`; +- anything that is not a cel-js error -> `runtime`. + +Two findings from the audit worth recording. First, the residual keyword arm was +**not** dormant: `matches()` is an ObjectStack stdlib binding over `new +RegExp(...)`, so an uncompilable pattern escapes as a native `SyntaxError` whose +message echoes the pattern — and the pattern can come off the row, not just out +of the source. `matches(record.name, record.re)` with `re = "(?"` was +graded `type`; with `"Exceeded maxAstNodes("` it was graded `bounds`. A data +value was picking the error kind. Second, there is deliberately no `TypeError` +arm: cel-js raises that class only from its non-evaluating `TypeChecker`, which +runs only inside `Environment#check`, and that method catches it and *returns* +`{ valid: false, error }`. The check-time `TypeError -> type` mapping already +lives in `celEngine.compile`, which reads that object. + +Six evaluate-time codes change verdict from `type` to `runtime` +(`int_conversion_error`, `uint_conversion_error`, `double_conversion_error`, +`invalid_index_type`, `heterogeneous_list_element`, +`invalid_comprehension_range`). Each is a fault decided against the row; every +one of them was graded `type` only because cel-js happens to use the word "type" +in its prose (`int() type error: cannot convert to int`). Every evaluate-time +code the engine can reach now has a fixture pinning its `kind`. diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index f702c02add..248822177e 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -13,7 +13,7 @@ * third-party plugins can't ship runaway predicates. */ -import { Environment, ParseError, serialize } from '@marcbachmann/cel-js'; +import { Environment, EvaluationError, ParseError, serialize } from '@marcbachmann/cel-js'; import type { ASTNode } from '@marcbachmann/cel-js'; import type { Expression } from '@objectstack/spec'; @@ -842,53 +842,117 @@ function hydrateOverloadStrings(value: unknown): unknown { const CEL_LIMIT_EXCEEDED_CODE = 'limit_exceeded'; /** - * Grade a cel-js fault off the error **class** the parser threw, not off its - * prose. Returns `undefined` for anything that is not a cel-js error, so the - * caller can fall back to the legacy keyword table. + * The evaluate-time cel-js codes that describe the EXPRESSION rather than the + * DATA, and therefore stay `type` instead of falling to `runtime`. * - * Why the class and not the message (#6133): `classifyError` used to decide - * between `parse` / `type` / `runtime` by regex-matching the error text, and - * cel-js has ~19 distinct parse-time wordings of which only three contain - * `parse` / `unexpected` / `syntax`. Everything else — `Expected RPAREN, got - * EOF` (unbalanced parens), `Expected RBRACKET, got EOF`, `Unterminated - * string`, `Reserved identifier: package`, the seven escape-sequence faults — - * fell through to the default `runtime`, and `kind` is not an internal field: - * it is interpolated verbatim into the author-facing rejection text - * (`objectql`'s `rule-validator` / `cel-fault`) and into the REST `reason`. - * An author who forgot a closing paren was told their *data* was at fault. + * The membership test is "would this fault reproduce on every input?". At + * evaluate time cel-js runs its checker with `isEvaluating: true` + * (`Environment#evaluate` → `#evalTypeChecker`, built as + * `new TypeChecker(opts, true)`), so *every* fault — including the ones the + * checker raises — arrives as an {@link EvaluationError}. The phase therefore + * cannot separate the two, and the code has to. * - * Topping the keyword list up cannot fix this, because cel-js embeds the - * **author's own source line** in `message` (see `formatErrorWithHighlight` in - * `lib/errors.js`), so the author controls the text being matched. Measured on - * cel-js 8.0.0: `((record.type_id)` — a plain unbalanced paren — classified as - * `type`, purely because the echoed source contains the substring "type". - * Classifying on prose is not a table with holes in it; it is the hole. + * Exactly one code qualifies on cel-js 8.0.0, measured per code (#6223): + * `unknown_variable` means the ROOT identifier the expression names is not + * bound in this scope at all — a property of the expression against the call + * site's contract, not of any row. `@objectstack/objectql`'s `cel-fault` + * already gives it its own author advice ("the field is fine; the thing you + * hung it off isn't available here"). * - * Scope note, deliberate: only the ParseError arm is structural here. cel-js's - * `TypeChecker` picks its error class **by phase**, not by fault - * (`this.createError = isEvaluating ? evaluationError : typeError`), so the same - * `unknown_variable` fault is a `TypeError` at check time and an - * `EvaluationError` at evaluate time. Routing `EvaluationError` → `runtime` - * wholesale would therefore silently re-grade faults the keyword table gets - * right today (`Unknown variable: x` → `type`). Those arms stay on the keyword - * table until that mapping is measured per code — see #6133 for the audit. + * Deliberately NOT here, each with the reason: + * - `no_such_key` — a record may carry a key on one row and not the next, so + * it is a fact about the data. `cel-fault` gives it its own sentence too. + * - `no_such_overload` — this is ADR-0032 §1c, the string-serialized numeric / + * date field (`record.rating >= 4` where `rating` is `"5.0"`). Data. + * - `int_conversion_error` / `uint_conversion_error` / + * `double_conversion_error` — cel-js phrases these as `int() type error: …`, + * which is what put them on `type` before #6223. Converting a value that + * cannot convert is a data fault; the word "type" in the prose was the only + * reason they were graded otherwise. + * - `no_matching_overload` — genuinely ambiguous: it covers both an unknown + * function (`PRIOR(x)`) and a known one called with the wrong runtime types + * (`size(record.x)` on a scalar). Under `unlistedVariablesAreDyn` the second + * is data-dependent, so it stays `runtime` — which is also its verdict + * before #6223, i.e. this is not a re-grade. The unknown-function case is + * already caught earlier and louder: {@link celEngine.compile} reads + * `check()`'s `{ valid: false }` and answers `type` at build time (#1877). + * - `heterogeneous_list_element`, `invalid_index_type`, + * `invalid_comprehension_range`, `invalid_condition_type`, + * `invalid_logical_operand` — all "this VALUE has the wrong type", decided + * against the row, not against the source. */ -function classifyCelParseFault(err: unknown): 'parse' | 'bounds' | undefined { - if (!(err instanceof ParseError)) return undefined; - return err.code === CEL_LIMIT_EXCEEDED_CODE ? 'bounds' : 'parse'; +const CEL_DECLARATION_CODES: ReadonlySet = new Set(['unknown_variable']); + +/** + * Grade a cel-js fault off the error **class** it was thrown as and its + * structured `code` — never off its prose. Returns `undefined` for anything + * that is not a cel-js error. + * + * Why not the message (#6133, #6223): `classifyError` used to decide between + * `parse` / `type` / `runtime` / `bounds` by regex-matching the error text, and + * cel-js embeds the **author's own source line** in `message` (see + * `formatErrorWithHighlight` in `lib/errors.js`). The text being matched is + * therefore text the author writes. `kind` is not an internal field — it is + * interpolated verbatim into the author-facing rejection text (`objectql`'s + * `rule-validator` / `cel-fault`) and into the REST error body's `reason` — so + * the author is told which of *their* mistakes this was, by a rule their own + * field names can flip. Measured on cel-js 8.0.0, one `no such overload` + * evaluation fault, four field names, one wrong answer per polluted name: + * + * ```text + * record.status > 1 -> runtime (right) + * record.parse_status > 1 -> parse (wrong — "your expression is broken") + * record.syntax_mode > 1 -> parse (wrong) + * record.type_code > 1 -> type (wrong) + * ``` + * + * #6133 / PR #6202 closed the ParseError arm this way and left `type` / + * `runtime` on the keyword table pending a per-code audit. #6223 is that audit, + * and it deletes the table outright: see {@link CEL_DECLARATION_CODES} for the + * evaluate-time verdicts and {@link classifyError} for why nothing is left for + * a keyword to decide. + * + * There is deliberately no `TypeError` arm. cel-js's `TypeError` is raised only + * by the non-evaluating `TypeChecker` (`createError = isEvaluating ? + * evaluationError : typeError`), which runs only inside `Environment#check` — + * and that method catches it and *returns* `{ valid: false, error }` rather + * than throwing. So a cel-js `TypeError` can never reach a `catch` block here; + * an arm for it would be dead code. The check-time `TypeError → type` mapping + * does exist, in {@link celEngine.compile}, where that returned object is read. + */ +function classifyCelFault(err: unknown): 'parse' | 'bounds' | 'type' | 'runtime' | undefined { + if (err instanceof ParseError) { + return err.code === CEL_LIMIT_EXCEEDED_CODE ? 'bounds' : 'parse'; + } + if (err instanceof EvaluationError) { + return CEL_DECLARATION_CODES.has(err.code) ? 'type' : 'runtime'; + } + return undefined; } +/** + * Resolve a thrown fault into the {@link EvalError} the caller reports. + * + * Everything that is not a cel-js error faulted *while evaluating* and carries + * no structured contract at all — our own stdlib bindings, a caller-supplied + * `os.*` API, a native JS throw — so `runtime` is the honest answer and the + * only one. It is not a fallback worth "improving" with a keyword table: the + * residual arm was never dormant, and its prose is author- and *data*- + * controlled through our own `matches()` binding, which hands cel-js a native + * `SyntaxError` whose message echoes the pattern (measured, #6223): + * + * ```text + * matches(record.name, "(?") -> type (was) + * matches(record.name, "Exceeded maxAstNodes(") -> bounds (was) + * matches(record.name, "unexpected(") -> parse (was) + * matches(record.name, record.re) -> type (was) — from a ROW + * ``` + * + * All four are one native regex-compilation failure, i.e. `runtime`. + */ function classifyError(err: unknown): EvalResult { const message = err instanceof Error ? err.message : String(err); - let kind: 'parse' | 'type' | 'runtime' | 'bounds' | undefined = classifyCelParseFault(err); - if (kind === undefined) { - // Legacy keyword table — the residual path for faults that carry no - // structured contract at all (our own stdlib, a native JS throw). - kind = 'runtime'; - if (/Exceeded max/i.test(message)) kind = 'bounds'; - else if (/parse|unexpected|syntax/i.test(message)) kind = 'parse'; - else if (/type|unknown variable|undeclared/i.test(message)) kind = 'type'; - } + const kind = classifyCelFault(err) ?? 'runtime'; return { ok: false, error: { kind, message } }; } diff --git a/packages/formula/src/cel-error-classification.test.ts b/packages/formula/src/cel-error-classification.test.ts index fca598f7a3..72e7d2eaa4 100644 --- a/packages/formula/src/cel-error-classification.test.ts +++ b/packages/formula/src/cel-error-classification.test.ts @@ -10,12 +10,19 @@ * `parse` / `unexpected` / `syntax`. This file pins **one fixture per wording * class** so a cel-js re-wording can never silently re-open the hole, and pins * the two things the classification must not lose along the way: bounds faults - * (a `ParseError` carrying `code: 'limit_exceeded'`) and the non-parse kinds - * the keyword table still owns. + * (a `ParseError` carrying `code: 'limit_exceeded'`) and the non-parse kinds. + * + * #6223 finished the job on the other side of the same function. PR #6202 left + * `type` / `runtime` on the keyword table pending a per-code audit; the audit + * is the `evaluate-time faults` block below, one fixture per cel-js evaluation + * code, and it deleted the table. `classifyError` now reads only the error + * CLASS and its structured `code` — no branch of it reads text an author (or a + * row) can write. */ import { describe, expect, it } from 'vitest'; import { celEngine, parseCelToAst } from './cel-engine'; +import type { EvalContext } from './types'; import type { Expression } from '@objectstack/spec'; const cel = (source: string): Expression => ({ dialect: 'cel', source }); @@ -130,8 +137,10 @@ describe('celEngine error classification (#6133)', () => { it('type: an undeclared variable at evaluate time is still kind=type', () => { // cel-js's TypeChecker picks its error CLASS by phase // (`isEvaluating ? evaluationError : typeError`), so this fault arrives - // as an EvaluationError. It stays on the keyword table on purpose — the - // structured route only claims the ParseError arm. + // as an EvaluationError. Since #6223 it is graded by CODE, not by the + // words "Unknown variable" in the prose: `unknown_variable` is the one + // evaluate-time code in `CEL_DECLARATION_CODES`, because it is the one + // that describes the expression rather than the row. const r = celEngine.evaluate(cel('nope.a'), {}); expect(r.ok).toBe(false); if (!r.ok) { @@ -157,6 +166,138 @@ describe('celEngine error classification (#6133)', () => { }); }); + describe('evaluate-time faults are graded by code, never by the echoed source (#6223)', () => { + /** The kind `evaluate()` reports for `source` against `ctx`. */ + function evalKind(source: string, ctx: EvalContext = {}): { kind: string; message: string } { + const r = celEngine.evaluate(cel(source), ctx); + expect(r.ok).toBe(false); + if (r.ok) throw new Error(`expected '${source}' to fault`); + return { kind: r.error.kind, message: r.error.message }; + } + + it('one fault, four field names, one verdict — the headline case', () => { + // All four are the SAME `no_such_overload` evaluation fault with the same + // first line (`no such overload: dyn > int`); they differ only in + // the source line cel-js echoes into `message`. Before #6223 the three + // polluted names were graded `parse` — telling the author their + // syntactically PERFECT expression was malformed, which is the inverse of + // #6133's misdirection and equally against ADR-0032 D1d. + for (const field of ['status', 'parse_status', 'syntax_mode', 'unexpected_at', 'type_code']) { + const source = `record.${field} > 1`; + // The expression parses and type-checks: nothing is wrong with it. + expect(celEngine.compile(source).ok).toBe(true); + const { kind, message } = evalKind(source, { record: { [field]: 'open' } }); + expect(message).toContain('no such overload'); + expect(message).toContain(`record.${field}`); // the echoed source IS there… + expect(kind).toBe('runtime'); // …and it does not get a vote. + } + }); + + // One fixture per cel-js 8.0.0 evaluation code reachable through this + // engine, each pinned to `runtime` — the honest verdict for a fault decided + // against the ROW. Audited per code (the measurement #6133 deferred), so a + // cel-js re-wording cannot move any of them and a future `??`-style + // "improvement" to the classifier goes red here rather than in production. + it.each([ + ['division_by_zero', '1 / 0', {}, 'division by zero'], + ['modulo_by_zero', '1 % 0', {}, 'modulo by zero'], + ['numeric_overflow', '9223372036854775807 + 1', {}, 'integer overflow'], + ['no_such_overload', '1 + "a"', {}, 'no such overload'], + ['no_such_key', '{"a": 1}.b', {}, 'No such key'], + ['index_out_of_bounds', '[1,2,3][-1]', {}, 'index out of bounds'], + ['index_out_of_range', '"abc".substring(10)', {}, 'index out of range'], + ['no_matching_overload', 'size(1)', {}, 'found no matching overload'], + ['invalid_regular_expression', '"a".matches("(")', {}, 'Invalid regular expression'], + ['invalid_timestamp', 'timestamp("nope")', {}, 'ISO 8601'], + ['invalid_duration', 'duration("nope")', {}, 'Invalid duration string'], + ['bool_conversion_error', 'bool("abc")', {}, 'conversion error'], + // The four below carry the word "type" in cel-js's own prose, which is + // the ONLY reason the keyword table graded them `type`. Each is a value + // that would not convert / would not index — a fact about the data. + ['int_conversion_error', 'int("abc")', {}, 'int() type error'], + ['uint_conversion_error', 'uint("abc")', {}, 'uint() type error'], + ['double_conversion_error', 'double("abc")', {}, 'double() type error'], + ['invalid_index_type', 'bytes("ab")[9]', {}, 'Cannot index type'], + ['heterogeneous_list_element', '["a", 1].sort()', {}, 'must have the same type'], + [ + 'invalid_comprehension_range', + 'record.n.all(x, x > 0)', + { record: { n: 5 } }, + 'cannot be range of a comprehension', + ], + ['invalid_condition_type', 'record.flag ? 1 : 2', { record: { flag: 3 } }, 'must be bool'], + [ + 'invalid_logical_operand', + 'record.flag && true', + { record: { flag: 3 } }, + 'requires bool operands', + ], + ['optional_value_missing', 'optional.none().value()', {}, 'Optional value is not present'], + ['invalid_macro_argument', 'has(record)', { record: { a: 1 } }, 'has() invalid argument'], + ] as Array<[string, string, EvalContext, string]>)( + '%s → kind=runtime', + (_code, source, ctx, fragment) => { + const { kind, message } = evalKind(source, ctx); + expect(message).toContain(fragment); + expect(kind).toBe('runtime'); + }, + ); + + it('unknown_variable is the ONE evaluate-time code held back at kind=type', () => { + // The whole by-code table, asserted as a table of one. It earns the + // exception because it is the only evaluate-time fault that is a property + // of the EXPRESSION against the call site's binding contract rather than + // of the row — see CEL_DECLARATION_CODES in cel-engine.ts. + const { kind, message } = evalKind('nope.a'); + expect(message).toContain('Unknown variable: nope'); + expect(kind).toBe('type'); + + // And it is held back by CODE, not by the phrase: a record key that reads + // exactly like the fault's prose does not borrow its verdict. + const borrowed = evalKind('record.unknown_variable > 1', { + record: { unknown_variable: 'open' }, + }); + expect(borrowed.message).toContain('record.unknown_variable'); + expect(borrowed.kind).toBe('runtime'); + }); + + it('a NON-cel-js throw is `runtime` — the residual arm read prose too', () => { + // The keyword table's last consumer was never dormant. `matches()` is an + // ObjectStack stdlib binding over `new RegExp(...)`, so an uncompilable + // pattern escapes as a NATIVE SyntaxError whose message echoes the + // pattern — and the pattern is written by the author, or (last fixture) + // read straight off the record. Before #6223 these four one-and-the-same + // regex failures were graded `type` / `bounds` / `parse` / `type`. + for (const [source, ctx] of [ + ['matches(record.name, "(?")', { record: { name: 'x' } }], + ['matches(record.name, "Exceeded maxAstNodes(")', { record: { name: 'x' } }], + ['matches(record.name, "unexpected(")', { record: { name: 'x' } }], + // Not even authored: the pattern is a VALUE on the row. + ['matches(record.name, record.re)', { record: { name: 'x', re: '(?' } }], + ] as Array<[string, EvalContext]>) { + const { kind, message } = evalKind(source, ctx); + expect(message).toContain('Invalid regular expression'); + expect(kind).toBe('runtime'); + } + }); + + it('bounds is decided by ParseError code, so prose cannot forge one', () => { + // `Exceeded max…` is cel-js's bounds wording, raised ONLY by the parser + // (`Parser#limitExceeded`), hence always a ParseError carrying + // `code: 'limit_exceeded'`. A real overrun is still `bounds`… + const overNodes = Array.from({ length: 500 }, (_, i) => `${i}`).join(' + '); + const real = celEngine.compile(overNodes); + expect(real.ok).toBe(false); + if (!real.ok) expect(real.error.kind).toBe('bounds'); + // …and nothing else can claim it, however the message reads. + const forged = evalKind('matches(record.name, "Exceeded maxAstNodes(")', { + record: { name: 'x' }, + }); + expect(forged.message).toMatch(/Exceeded max/i); + expect(forged.kind).toBe('runtime'); + }); + }); + it('parseCelToAst never reaches the classifier — it returns null (#4812)', () => { // Recorded because the classification fix has a natural blast radius // question: does the #4812 canonical parse entry re-emit `kind`? It does From 88f2ab4cb2bdb10ab239eda33ca3db127a4d94bf Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 10:11:05 +0000 Subject: [PATCH 2/2] test(formula): use angle-bracket-free regex fixtures for the residual arm (#6223) `(?` is unquotable in a GitHub issue or PR body: the sanitizer strips `<` followed by a letter as an HTML tag at rest, so the fixture that carries this PR's argument would be destroyed the moment anyone pasted it. `type(` is the same defect with the same native `SyntaxError` (`Invalid regular expression: /type(/: Unterminated group`) and survives the round trip. Adds a `syntax[` fixture for the third keyword while there. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01MwoubC3jL271FYt9rGXwxb --- .changeset/cel-classify-error-by-code.md | 2 +- packages/formula/src/cel-engine.ts | 2 +- packages/formula/src/cel-error-classification.test.ts | 5 +++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/.changeset/cel-classify-error-by-code.md b/.changeset/cel-classify-error-by-code.md index 2e468b2548..82334836cb 100644 --- a/.changeset/cel-classify-error-by-code.md +++ b/.changeset/cel-classify-error-by-code.md @@ -40,7 +40,7 @@ Two findings from the audit worth recording. First, the residual keyword arm was **not** dormant: `matches()` is an ObjectStack stdlib binding over `new RegExp(...)`, so an uncompilable pattern escapes as a native `SyntaxError` whose message echoes the pattern — and the pattern can come off the row, not just out -of the source. `matches(record.name, record.re)` with `re = "(?"` was +of the source. `matches(record.name, record.re)` with `re = "type("` was graded `type`; with `"Exceeded maxAstNodes("` it was graded `bounds`. A data value was picking the error kind. Second, there is deliberately no `TypeError` arm: cel-js raises that class only from its non-evaluating `TypeChecker`, which diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index 248822177e..0499b7bc73 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -942,7 +942,7 @@ function classifyCelFault(err: unknown): 'parse' | 'bounds' | 'type' | 'runtime' * `SyntaxError` whose message echoes the pattern (measured, #6223): * * ```text - * matches(record.name, "(?") -> type (was) + * matches(record.name, "type(") -> type (was) * matches(record.name, "Exceeded maxAstNodes(") -> bounds (was) * matches(record.name, "unexpected(") -> parse (was) * matches(record.name, record.re) -> type (was) — from a ROW diff --git a/packages/formula/src/cel-error-classification.test.ts b/packages/formula/src/cel-error-classification.test.ts index 72e7d2eaa4..36e9f82ae3 100644 --- a/packages/formula/src/cel-error-classification.test.ts +++ b/packages/formula/src/cel-error-classification.test.ts @@ -269,11 +269,12 @@ describe('celEngine error classification (#6133)', () => { // read straight off the record. Before #6223 these four one-and-the-same // regex failures were graded `type` / `bounds` / `parse` / `type`. for (const [source, ctx] of [ - ['matches(record.name, "(?")', { record: { name: 'x' } }], + ['matches(record.name, "type(")', { record: { name: 'x' } }], ['matches(record.name, "Exceeded maxAstNodes(")', { record: { name: 'x' } }], ['matches(record.name, "unexpected(")', { record: { name: 'x' } }], + ['matches(record.name, "syntax[")', { record: { name: 'x' } }], // Not even authored: the pattern is a VALUE on the row. - ['matches(record.name, record.re)', { record: { name: 'x', re: '(?' } }], + ['matches(record.name, record.re)', { record: { name: 'x', re: 'type(' } }], ] as Array<[string, EvalContext]>) { const { kind, message } = evalKind(source, ctx); expect(message).toContain('Invalid regular expression');