Skip to content

fix(formula): grade CEL faults by error class + code, not by the message (#6223) - #6677

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-6223-classify-error-by-code
Aug 8, 2026
Merged

fix(formula): grade CEL faults by error class + code, not by the message (#6223)#6677
os-zhuang merged 2 commits into
mainfrom
claude/issue-6223-classify-error-by-code

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes #6223

The defect

EvalResult.error.kind is author-facing. @objectstack/objectql's cel-fault puts it in front of the author as `${kind}: ${first line}` (faultSummary), and packages/rest re-emits the same value as the HTTP body's reason. cel-js embeds the author's own source line in message (formatErrorWithHighlight, lib/errors.js), so a classifier that regex-matches that text is matching text the author writes.

PR #6202 (#6133) closed the ParseError arm structurally and deliberately left type / runtime on the keyword table pending a per-code audit. This PR is that audit.

Measured on origin/main through the real engine — 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)

All four carry the identical first line; they differ only in the echoed source. parse is the inverse of #6133's misdirection: the expression is syntactically perfect and failed on the data, and the author is told to go fix an expression that has nothing wrong with it. ADR-0032 D1d is unmet either way.

The fix

classifyError (packages/formula/src/cel-engine.ts) now reads only structured contract:

input verdict
ParseError, code === 'limit_exceeded' bounds
ParseError, otherwise parse
EvaluationError, code in CEL_DECLARATION_CODES type
EvaluationError, otherwise runtime
not a cel-js error runtime

The keyword table is deleted. No branch of the function reads text an author — or a row — can write.

CEL_DECLARATION_CODES holds exactly one code, unknown_variable: the root identifier the expression names is not bound in this scope at all, which is a property of the expression against the call site's contract rather than of any row. cel-fault already gives that fault its own author advice. The set is documented per rejected candidate in the source.

Answering the PM's mechanism assumption: the table goes outright, and here is why the partial route was not enough

The dispatch asked whether a by-code mapping could replace the keyword table outright, and invited falsification. Two measurements, both in the direction of "remove more, not less":

1. The residual arm was never dormant — and it reads data, not just author text. The issue's "未主张" section wondered whether the bounds branch was reachable only for non-cel-js errors. It is reachable, and so is the whole residual arm: matches() is an ObjectStack stdlib binding over new RegExp(...), so an uncompilable pattern escapes cel-js unwrapped as a native SyntaxError whose message echoes the pattern. Measured on origin/main:

matches(record.name, "type(")                  ->  type     (one native regex failure)
matches(record.name, "Exceeded maxAstNodes(")  ->  bounds   (same failure)
matches(record.name, "unexpected(")            ->  parse    (same failure)
matches(record.name, "syntax[")                ->  parse    (same failure)
matches(record.name, record.re)                ->  type     (same failure — pattern read off the ROW)

The last row is the sharp one: a value on a record was choosing the error kind. Keeping a "smaller non-author-text table" was therefore not available — the residual arm was the largest author-text reader left, not the safe remainder.

2. There is no reachable cel-js TypeError, so half the suggested route would have been dead code. The issue and the triage both proposed EvaluationError -> runtime, TypeError -> type. cel-js raises TypeError only from its non-evaluating TypeChecker (createError = isEvaluating ? evaluationError : typeError, lib/type-checker.js:16), and that instance runs only inside Environment#check — which catches it and returns { valid: false, error } rather than throwing (lib/evaluator.js #checkAST). At evaluate time cel-js uses #evalTypeChecker = new TypeChecker(opts, true), so every evaluate-time fault, checker-raised ones included, arrives as an EvaluationError. A TypeError arm in classifyError could never fire. The check-time TypeError -> type mapping already exists and is untouched: celEngine.compile reads that returned object (#1877). This is recorded in the source so the next reader does not "restore" the missing arm.

Consequence for the by-code table: the phase cannot separate declaration faults from data faults, so the code has to — which is what the table does.

Verdicts that change, and why each is a fix

Six evaluate-time codes move from type to runtime. Every one of them was on type only because cel-js happens to use the word "type" in its prose:

code cel-js wording was now
int_conversion_error int() type error: cannot convert to int type runtime
uint_conversion_error uint() type error: … type runtime
double_conversion_error double() type error: … type runtime
invalid_index_type Cannot index type 'bytes' … type runtime
heterogeneous_list_element List elements must have the same type … type runtime
invalid_comprehension_range Expression of type 'double' cannot be range … type runtime

Each is decided against the row, so runtime is the honest verdict.

Deliberately unchanged: no_matching_overload stays runtime. It conflates an unknown function (PRIOR(x)) with a known one called on the wrong runtime types (size(record.x) on a scalar), and under unlistedVariablesAreDyn the second is data-dependent. Its unknown-function half is already caught earlier and louder, at build time, by compile()'s check() read (#1877). No re-grade, and the rationale is in the source.

Tests

packages/formula/src/cel-error-classification.test.ts gains a #6223 block: the four field names from the issue plus type_code, each asserted to compile clean and then evaluate to runtime; one fixture per evaluate-time cel-js code the engine can reach, each pinning its kind and a message fragment naming the fault; the unknown_variable exception plus a record.unknown_variable field-name case proving it is held back by code and not by phrase; the five residual-arm fixtures above; and a pin that bounds cannot be forged from prose.

Reverse verification. Direction predicted before running (the ordinary one — red): restoring origin/main's cel-engine.ts under the new tests turns the #6223 block red and leaves the #6133 pins green. Result — 9 failed / 40 passed, and the reds are exactly the headline case (expected 'parse' to be 'runtime'), the six re-graded codes, the residual arm (expected 'type' to be 'runtime') and the forged bounds (expected 'bounds' to be 'runtime'). One honest deviation from the prediction: the record.unknown_variable > 1 sub-assertion stayed green on the old code too — the old regex was /unknown variable/i with a space, which an underscored field name never matched. That sub-case is a pin of behaviour that was already right, not a regression fixture, and is reported as such rather than claimed as a catch.

Suites (real output): @objectstack/formula 450/450, @objectstack/lint 1599/1599, @objectstack/objectql 2519/2519, @objectstack/rest 1027/1027; pnpm --filter @objectstack/formula typecheck clean; workspace turbo run typecheck clean. Every check:* step enumerated from .github/workflows/lint.yml was run one by one and passes.

Scope

Classifier only. packages/formula/src/cel-to-filter.ts and getParseEnv are untouched — frozen pending the #6132 maintainer ruling. isNumericOverloadError (the ADR-0032 §1c hydration-retry trigger, same file) still matches on /no such overload/i; it is a retry trigger rather than a classification and a false positive is harmless (the retry rethrows the original), but it is the same family and is filed separately rather than fixed here.


Generated by Claude Code

claude added 2 commits August 8, 2026 09:38
…age (#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 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 = "(?<type>"` 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MwoubC3jL271FYt9rGXwxb
… arm (#6223)

`(?<type>` 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MwoubC3jL271FYt9rGXwxb
@vercel

vercel Bot commented Aug 8, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Aug 8, 2026 10:21am

Request Review

@github-actions github-actions Bot added the size/m label Aug 8, 2026
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/formula.

6 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/formulas.mdx (via @objectstack/formula)
  • content/docs/data-modeling/validation.mdx (via @objectstack/formula)
  • content/docs/plugins/packages.mdx (via @objectstack/formula)
  • content/docs/protocol/objectui/record-alert.mdx (via @objectstack/formula)
  • content/docs/releases/v15.mdx (via @objectstack/formula)
  • content/docs/releases/v16.mdx (via @objectstack/formula)

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Aug 8, 2026
@os-zhuang
os-zhuang marked this pull request as ready for review August 8, 2026 12:14
@os-zhuang
os-zhuang added this pull request to the merge queue Aug 8, 2026
Merged via the queue into main with commit b230e5e Aug 8, 2026
25 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-6223-classify-error-by-code branch August 8, 2026 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

2 participants