fix(formula): classifyError 把 cel-js 的 parse 错误按错误类归为 syntax/parse,不再误报 runtime (#6133)

.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`)未变,消费方未改。

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<never> {
   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 } };
 }
 

packages/formula/src/cel-error-classification.test.ts

@@ -0,0 +1,168 @@
+/**
+ * #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, parseCelToAst } 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 <TOKEN>, 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)', () => {
+    // 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.
+    expect(parseCelToAst('((record.a)')).toBeNull();
+    expect(parseCelToAst('record.a > 1')).not.toBeNull();
+  });
+});

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:/),
+    });
+  });
+});