From fb7cabf6fd00496fae2856f75a25b24cb22f6662 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 10:40:24 +0000 Subject: [PATCH 1/2] fix(formula): classify cel-js ParseError as kind=parse, not runtime (#6133) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `classifyError` decided between `parse` / `type` / `runtime` / `bounds` by regex-matching the error text. cel-js 8.0.0 has ~19 distinct parse-time wordings and only three contain `parse` / `unexpected` / `syntax`, so the rest — unbalanced parens/brackets/braces (`Expected RPAREN, got EOF`), unterminated strings, every escape-sequence fault, reserved identifiers — fell through to the default `runtime`. `kind` is author-facing: it is interpolated verbatim into the write-rejection sentence (`@objectstack/objectql`'s `rule-validator` / `cel-fault`) and into the REST error body's `reason`. A missing closing paren was reported as `runtime`, pointing the author at their data instead of their expression — the opposite of ADR-0032 D1d's self-correcting messages. Classify off the error CLASS instead: `ParseError` -> `parse`, with `code: 'limit_exceeded'` -> `bounds` read first (cel-js raises every bounds violation through the parser). This also closes a hole no keyword table could: cel-js embeds the author's own source line in `message` (`formatErrorWithHighlight`), so a field name could pick the error kind — measured, `((record.type_id)` was graded `type` purely because the echoed source contains "type". The `type` / `runtime` arms deliberately stay on the keyword table: cel-js's `TypeChecker` picks its error class by PHASE, not by fault (`isEvaluating ? evaluationError : typeError`), so `unknown_variable` is a `TypeError` at check time and an `EvaluationError` at evaluate time; routing `EvaluationError` wholesale would silently re-grade faults the table gets right today. Audit recorded in #6133. No change to the `kind` vocabulary and no consumer changed. The objectql test is a read-only pin on the author-visible string. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We --- .changeset/cel-parse-fault-kind.md | 15 ++ packages/formula/src/cel-engine.ts | 61 ++++++- .../src/cel-error-classification.test.ts | 169 ++++++++++++++++++ .../rule-unevaluable-fault-kind.test.ts | 98 ++++++++++ 4 files changed, 338 insertions(+), 5 deletions(-) create mode 100644 .changeset/cel-parse-fault-kind.md create mode 100644 packages/formula/src/cel-error-classification.test.ts create mode 100644 packages/objectql/src/validation/rule-unevaluable-fault-kind.test.ts diff --git a/.changeset/cel-parse-fault-kind.md b/.changeset/cel-parse-fault-kind.md new file mode 100644 index 0000000000..dc8f651ac0 --- /dev/null +++ b/.changeset/cel-parse-fault-kind.md @@ -0,0 +1,15 @@ +--- +'@objectstack/formula': patch +--- + +fix(formula): 括号/引号/转义等 parse 期错误不再被误报为 `runtime` + +`celEngine` 的错误分类此前完全靠**错误文案关键词**判定,而 cel-js 8.0.0 的 parse 期错误有约 19 种措辞,只有 3 种含 `parse` / `unexpected` / `syntax`。其余整类 —— 最典型的括号/方括号/花括号不配对(`Expected RPAREN, got EOF`)、未闭合字符串、非法转义、保留字 —— 全部落到默认值 `runtime`。 + +`kind` 不是内部字段:它被原样拼进作者可见的写入拒绝文案(`@objectstack/objectql` 的 `rule-validator` / `cel-fault`)与 REST 错误响应体的 `reason`。少写一个右括号的校验规则,作者读到的是 `(runtime: …)` —— 指向数据与求值期,而真正该改的是表达式本身,与 ADR-0032 D1d 的"消息面向自纠"相悖。 + +改为按 cel-js 抛出的**错误类**判定:`ParseError` → `parse`(其中 `code: 'limit_exceeded'` 仍 → `bounds`,cel-js 的越界一律由 parser 抛出)。这一层不再读文案,因此也修掉了关键词方案无法修的一格:cel-js 会把**作者自己的源码行**嵌进 `message`(`formatErrorWithHighlight`),于是字段名能决定错误分类 —— 实测 `((record.type_id)` 这条普通的括号不配对,此前被判为 `type`,只因回显的源码里含子串 "type"。 + +`type` / `runtime` 两支暂仍走原关键词表:cel-js 的 `TypeChecker` 按**阶段**而非按故障选择错误类(`isEvaluating ? evaluationError : typeError`),同一个 `unknown_variable` 在 check 期是 `TypeError`、在 eval 期是 `EvaluationError`,整体结构化会改变这些既有判定。审计见 #6133。 + +kind 词表本身(`parse` / `type` / `runtime` / `bounds` / `dialect`)未变,消费方未改。 diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index 0544838e28..757a742b1a 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, serialize } from '@marcbachmann/cel-js'; +import { Environment, ParseError, serialize } from '@marcbachmann/cel-js'; import type { ASTNode } from '@marcbachmann/cel-js'; import type { Expression } from '@objectstack/spec'; @@ -800,12 +800,63 @@ function hydrateOverloadStrings(value: unknown): unknown { return value; } +/** + * cel-js's code for a bounds violation. Raised **only** from the parser + * (`Parser#limitExceeded`, one call site per limit key), so it always arrives as + * a {@link ParseError} and must be read before the ParseError → `parse` rule + * below — otherwise every `maxAstNodes` / `maxDepth` overrun would be reported + * as a syntax fault. + */ +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. + * + * 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. + * + * 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. + * + * 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. + */ +function classifyCelParseFault(err: unknown): 'parse' | 'bounds' | undefined { + if (!(err instanceof ParseError)) return undefined; + return err.code === CEL_LIMIT_EXCEEDED_CODE ? 'bounds' : 'parse'; +} + function classifyError(err: unknown): EvalResult { const message = err instanceof Error ? err.message : String(err); - let kind: 'parse' | 'type' | 'runtime' | 'bounds' = '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'; + 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'; + } 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 new file mode 100644 index 0000000000..ec61b9f739 --- /dev/null +++ b/packages/formula/src/cel-error-classification.test.ts @@ -0,0 +1,169 @@ +/** + * #6133 — `EvalResult.error.kind` is an author-facing field, not an internal + * one. It is interpolated verbatim into the write-rejection text + * (`@objectstack/objectql`'s `rule-validator` / `cel-fault`) and into the REST + * error body's `reason`. A missing closing paren reported as `runtime` points + * the author at their data instead of at their expression, which is exactly the + * misdirection ADR-0032 D1d asks these messages to avoid. + * + * cel-js 8.0.0 has ~19 distinct parse-time wordings; only three of them contain + * `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. + */ +import { describe, expect, it } from 'vitest'; + +import { celEngine } from './cel-engine'; +import type { Expression } from '@objectstack/spec'; + +const cel = (source: string): Expression => ({ dialect: 'cel', source }); + +/** The kind both entry points report for `source`, asserted to agree. */ +function kindOf(source: string): string { + const compiled = celEngine.compile(source); + const evaluated = celEngine.evaluate(cel(source), {}); + expect(compiled.ok).toBe(false); + expect(evaluated.ok).toBe(false); + if (compiled.ok || evaluated.ok) throw new Error('expected both entries to fault'); + // `classifyError` serves compile() and evaluate() alike — build-time + // (`os build` / `os validate` / `os lint`) and run-time (write rejection, + // REST) must never disagree about what kind of mistake the author made. + expect(evaluated.error.kind).toBe(compiled.error.kind); + return compiled.error.kind; +} + +describe('celEngine error classification (#6133)', () => { + describe('unbalanced delimiters are syntax faults, not runtime faults', () => { + // The headline regression: cel-js reports these as `Expected , got + // EOF`, which contains none of `parse` / `unexpected` / `syntax`, so the + // keyword table dropped the whole family to the `runtime` default. + it.each([ + ['unclosed paren', '((record.a)', 'RPAREN'], + ['unclosed bracket', '[1, 2', 'RBRACKET'], + ['unclosed brace', '{"a": 1', 'RBRACE'], + ['ternary missing colon', 'record.a ? 1', 'COLON'], + ])('%s → kind=parse', (_label, source, token) => { + const compiled = celEngine.compile(source); + expect(compiled.ok).toBe(false); + if (compiled.ok) return; + expect(compiled.error.kind).toBe('parse'); + // The message was always right; only the classification was wrong. Pin + // that the fix did not "fix" it by rewriting the author's diagnostic. + expect(compiled.error.message).toContain(`Expected ${token}`); + expect(kindOf(source)).toBe('parse'); + }); + }); + + describe('every other cel-js parse-time wording (audited on 8.0.0)', () => { + // Codes that also missed every keyword and defaulted to `runtime`. + it.each([ + ['unterminated_string', '"abc'], + ['unterminated_triple_quoted_string', '"""abc'], + ['newline_in_string', "'a\nb'"], + ['invalid_hex_integer', '0xZZ'], + ['invalid_escape_sequence', '"\\q"'], + ['invalid_unicode_escape', '"\\u12"'], + ['invalid_hex_escape', '"\\xZZ"'], + ['invalid_octal_escape', '"\\07"'], + ['reserved_identifier', 'record.a && package'], + ])('%s → kind=parse', (_code, source) => { + expect(kindOf(source)).toBe('parse'); + }); + + // Codes the keyword table already graded correctly — they must stay put. + it.each([ + ['unexpected_token (trailing operator)', 'record.budget >'], + ['unexpected_token (QUESTION)', 'record.a ?? 3'], + ['unexpected_character', 'record.a $$ 1'], + ])('%s → kind=parse (unchanged)', (_code, source) => { + expect(kindOf(source)).toBe('parse'); + }); + }); + + it('does not read the author source echoed into the message (#6133)', () => { + // cel-js embeds the offending source line in `message` + // (`formatErrorWithHighlight`), so a keyword classifier is matching text + // the AUTHOR controls. Before the fix this exact fixture — an ordinary + // unbalanced paren — was graded `type`, purely because the echoed line + // contains the substring "type". A field name must not be able to pick the + // error kind. + const compiled = celEngine.compile('((record.type_id)'); + expect(compiled.ok).toBe(false); + if (compiled.ok) return; + expect(compiled.error.message).toContain('record.type_id'); + expect(compiled.error.kind).toBe('parse'); + + // Same shape, a field name containing "parse": it must land on `parse` for + // the structural reason, not by accident of spelling. + const alsoParse = celEngine.compile('((record.parsed_at)'); + expect(alsoParse.ok).toBe(false); + if (alsoParse.ok) return; + expect(alsoParse.error.kind).toBe('parse'); + }); + + describe('the kinds that are NOT parse keep their verdicts', () => { + it('bounds: limit_exceeded is a ParseError but must stay kind=bounds', () => { + // cel-js raises every bounds violation through the parser, so the + // structured route has to read `code` before it reads the class. + const overNodes = Array.from({ length: 500 }, (_, i) => `${i}`).join(' + '); + const r = celEngine.compile(overNodes); + expect(r.ok).toBe(false); + if (!r.ok) { + expect(r.error.kind).toBe('bounds'); + expect(r.error.message).toContain('Exceeded maxAstNodes'); + } + + const overList = `[${Array.from({ length: 200 }, (_, i) => i).join(',')}]`; + const rl = celEngine.compile(overList); + expect(rl.ok).toBe(false); + if (!rl.ok) expect(rl.error.kind).toBe('bounds'); + }); + + it('type: an unknown function is still kind=type (#1877)', () => { + const r = celEngine.compile('PRIOR(status) != "promoted"'); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.error.kind).toBe('type'); + }); + + 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. + const r = celEngine.evaluate(cel('nope.a'), {}); + expect(r.ok).toBe(false); + if (!r.ok) { + expect(r.error.message).toContain('Unknown variable'); + expect(r.error.kind).toBe('type'); + } + }); + + it('runtime: a genuine evaluation fault is still kind=runtime', () => { + const r = celEngine.evaluate(cel('1 / 0'), {}); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.error.kind).toBe('runtime'); + + const overload = celEngine.evaluate(cel('1 + "a"'), {}); + expect(overload.ok).toBe(false); + if (!overload.ok) expect(overload.error.kind).toBe('runtime'); + }); + + it('dialect: a mismatched dialect is untouched', () => { + const r = celEngine.evaluate({ dialect: 'cron', source: 'x' }, {}); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.error.kind).toBe('dialect'); + }); + }); + + it('parseCelToAst never reaches the classifier — it returns null (#4812)', async () => { + // Recorded because the classification fix has a natural blast radius + // question: does the #4812 canonical parse entry re-emit `kind`? It does + // not — it swallows the fault and answers `null`, leaving the verdict to + // compile()/validateExpression. Nothing here changes for it. + const { parseCelToAst } = await import('./cel-engine'); + expect(parseCelToAst('((record.a)')).toBeNull(); + expect(parseCelToAst('record.a > 1')).not.toBeNull(); + }); +}); diff --git a/packages/objectql/src/validation/rule-unevaluable-fault-kind.test.ts b/packages/objectql/src/validation/rule-unevaluable-fault-kind.test.ts new file mode 100644 index 0000000000..497c9e3f2d --- /dev/null +++ b/packages/objectql/src/validation/rule-unevaluable-fault-kind.test.ts @@ -0,0 +1,98 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #6133 — consumer-side pin for the CEL fault `kind`. + * + * `@objectstack/formula` decides the kind; THIS is where an author reads it. + * `unevaluableRuleError` puts `faultSummary(error)` — literally + * `` `${kind}: ${first line}` `` — into both the rejection sentence and + * `constraint.fault`, and `packages/rest` re-emits the same kind as the HTTP + * body's `reason`. So a misclassification upstream is not an internal detail: + * it is the word the author is handed when their write is refused. + * + * Before #6133 an unbalanced paren produced `(runtime: Expected RPAREN, got + * EOF)` — "runtime" pointing the author at their DATA when the fault is in + * their expression. This file reads the consumer without changing it, so the + * formula-side fix is pinned where it is actually visible. + */ +import { describe, it, expect } from 'vitest'; + +import { evaluateValidationRules } from './rule-validator'; +import { ValidationError } from './record-validator'; + +const schemaWithPredicate = (source: string) => ({ + fields: { + status: { name: 'status', label: 'Status', type: 'text' }, + amount: { name: 'amount', label: 'Amount', type: 'number' }, + }, + validations: [ + { + type: 'script' as const, + name: 'broken_predicate', + condition: { dialect: 'cel', source }, + message: 'Amount is out of range.', + events: ['insert', 'update'] as Array<'insert' | 'update'>, + }, + ], +}); + +/** Run the write and hand back the rejection the author would see. */ +function rejectionOf(source: string): ValidationError { + try { + evaluateValidationRules(schemaWithPredicate(source), { status: 'open', amount: 10 }, 'insert'); + } catch (err) { + if (err instanceof ValidationError) return err; + throw err; + } + throw new Error(`expected the write to be rejected for predicate: ${source}`); +} + +describe('#6133 — the kind an author reads for an unparseable predicate', () => { + it('reports an unbalanced paren as a SYNTAX fault, not a runtime one', () => { + const err = rejectionOf('((record.amount > 100)'); + + // The sentence the author reads. + expect(err.fields[0]?.message).toContain('parse: Expected RPAREN'); + expect(err.fields[0]?.message).not.toContain('runtime:'); + + // The machine-readable twin, and the value `packages/rest` re-emits as the + // HTTP `reason`. + expect(err.fields[0]?.constraint).toMatchObject({ + rule: 'broken_predicate', + reason: 'unevaluable', + fault: expect.stringMatching(/^parse: Expected RPAREN/), + }); + }); + + it('reports an unbalanced bracket the same way', () => { + const err = rejectionOf('record.amount in [1, 2'); + expect(err.fields[0]?.constraint).toMatchObject({ + fault: expect.stringMatching(/^parse: Expected RBRACKET/), + }); + }); + + it('still fails CLOSED — a broken predicate rejects the write (#4649)', () => { + // The classification fix must not soften the fail-closed guarantee: an + // unevaluable rule still refuses the write, it just names the fault + // correctly now. + expect(() => + evaluateValidationRules( + schemaWithPredicate('((record.amount > 100)'), + { status: 'open', amount: 10 }, + 'insert', + ), + ).toThrow(ValidationError); + }); + + it('leaves a genuine RUNTIME fault reported as runtime', () => { + // The counter-case: a predicate that parses fine and faults while + // evaluating must keep pointing at evaluation. Ordering a text field + // against a number is the cleanest such fault — the record is total, so no + // missing key, and the message carries no `null`, so `describeCelFault`'s + // null-comparison sentence does not claim it either. + const err = rejectionOf('record.status > 1'); + expect(err.fields[0]?.constraint).toMatchObject({ + fault: expect.stringMatching(/^runtime:/), + }); + }); +}); From 3b2edfca11f2f4ce68afd7094633197954aeb878 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 11:00:51 +0000 Subject: [PATCH 2/2] fix(formula): use the static import in the #6133 test (NodeNext extension) `await import('./cel-engine')` needs an explicit `.js` extension under `moduleResolution: nodenext`, so the new test added one TS2835 to the package's TEST_DEBT ledger (17 -> 18) and tripped `check:type-check-debt`. The ledger is a shrink-only ratchet (#5278), and the error is trivially removable rather than irreducible: `parseCelToAst` is exported from the module the file already imports statically. Re-measured back to 17, the recorded value, with zero errors attributable to this PR's files. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We --- packages/formula/src/cel-error-classification.test.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/formula/src/cel-error-classification.test.ts b/packages/formula/src/cel-error-classification.test.ts index ec61b9f739..fca598f7a3 100644 --- a/packages/formula/src/cel-error-classification.test.ts +++ b/packages/formula/src/cel-error-classification.test.ts @@ -15,7 +15,7 @@ */ import { describe, expect, it } from 'vitest'; -import { celEngine } from './cel-engine'; +import { celEngine, parseCelToAst } from './cel-engine'; import type { Expression } from '@objectstack/spec'; const cel = (source: string): Expression => ({ dialect: 'cel', source }); @@ -157,12 +157,11 @@ describe('celEngine error classification (#6133)', () => { }); }); - it('parseCelToAst never reaches the classifier — it returns null (#4812)', async () => { + 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 // not — it swallows the fault and answers `null`, leaving the verdict to // compile()/validateExpression. Nothing here changes for it. - const { parseCelToAst } = await import('./cel-engine'); expect(parseCelToAst('((record.a)')).toBeNull(); expect(parseCelToAst('record.a > 1')).not.toBeNull(); });