Skip to content

fix(formula): converge the CEL pushdown parser onto the canonical front end, with an rc grace window (#6132) - #6766

Merged
os-zhuang merged 4 commits into
mainfrom
claude/issue-6132-cel-to-filter-converge
Aug 8, 2026
Merged

fix(formula): converge the CEL pushdown parser onto the canonical front end, with an rc grace window (#6132)#6766
os-zhuang merged 4 commits into
mainfrom
claude/issue-6132-cel-to-filter-converge

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes #6132

Implements the maintainer's A′ ruling of 2026-08-08, recorded verbatim on the issue:

  1. Converge now (rc window): packages/formula/src/cel-to-filter.ts drops its private limitless getParseEnv and parses through packages/lint 绕过 @objectstack/formula 直接 parse CEL —— 两个解析入口对「什么能解析」会给出不同答案 #4812's parseCelToAst(with DEFAULT_LIMITS). Within-limits predicates: byte-identical behavior.
  2. rc grace, GA flip: during 17.0.0-rc.x an over-limit predicate on the pushdown path still compiles + emits a WARN naming the exceeded limit; at v17 GA the runtime flips to fail-closed refusal (the parse-errorRLS_DENY_FILTER path). Implement the flip as a single dated switch (a named const, default = rc-grace) so GA needs a one-line change; record the intended flip point in a comment + the changeset.
  3. Sister entrance: cel-engine gains a reason-carrying variant so a limit refusal names WHICH limit was exceeded (maxAstNodes / maxDepth / maxListElements) instead of a bare parse-error. Wire the WARN (and the future refusal) to use it.

Premises, re-measured on current main (a5d2573)

The issue's facts were measured on bc67c28e2. Re-verified before writing code; every one holds, including after PR #6677's same-day rewrite of cel-engine.ts.

shape old private env parseCelToAst celEngine.compile() isPushdownableCel (before)
300-term addition parses refuses bounds: Exceeded maxAstNodes (256) unsupported (arithmetic)
80-term conjunction parses refuses bounds: Exceeded maxAstNodes (256) ok — reached real pushdown SQL
60-level parens parses refuses bounds: Exceeded maxDepth (32) ok
40-level nesting parses refuses bounds: Exceeded maxDepth (32) ok
200-element $in parses refuses bounds: Exceeded maxListElements (64) ok
64-element $in (AT the bound) parses parses ok ok
65-element $in parses refuses bounds: Exceeded maxListElements (64) ok

What changed

packages/formula/src/cel-engine.ts — the sister entrance (ruling §3).
parseCelToAstWithReason(source, opts?) returns { ok: true, ast } or a discriminated refusal: 'empty' / 'parse' (not valid CEL) / 'bounds' (valid CEL, over budget) carrying { limit, limitValue, measured, summary }. parseCelToAst is now parseCelToAstWithReason(...) collapsed to null — its #4812 contract is untouched, and that is pinned.

The verdict is graded by the same classifyCelFault compile/evaluate use — error class plus structured code, never prose. PR #6677's fresh by-code table is not touched; cel-parse-reason.test.ts pins that a bounds overrun is bounds on both entrances with the same summary, that a syntax fault is parse on both, and that an author-controlled field name spelling Exceeded_maxAstNodes is still graded parse (the #6223 hazard).

The limit key comes from ParseError#summary (Exceeded maxAstNodes (256) — built by Parser#limitExceeded from a fixed key set), never #message (which is formatErrorWithHighlight's rendering of the author's own source line). The captured key is validated against DEFAULT_LIMITS, so a cel-js that rephrases the sentence degrades to "bounds, limit unknown" instead of inventing a limit name.

The measure is cel-js's own accounting, not a second implementation of it: the smallest limits[key] under which the source parses, every other bound lifted, found by exponential probe + binary search (O(log n) parses, capped at 64× the bound). A node-walk would report 3 for a 60-deep parenthesis nest, because maxDepth counts parenthesised recursion that leaves no AST node behind. Measuring happens only on the grace path, where an unbounded parse has already occurred — a bounds refusal never re-parses a source it has just declared too big.

packages/formula/src/cel-pushdown-limits.ts (new) — the dated switch (ruling §2).
CEL_PUSHDOWN_LIMITS_MODE: 'rc-grace' | 'fail-closed', shipping 'rc-grace'. Intended flip point: the v17.0.0 GA release — when packages/formula/package.json leaves 17.0.0-rc.x. Flipping is exactly one line; the module docblock says so, names the two assertions that go red on it, and states that nothing else needs to move.

packages/formula/src/cel-to-filter.ts — the convergence (ruling §1).
getParseEnv is gone. Both entry points (compileCelToFilter, isPushdownableCel) go through parseForPushdown, which asks the canonical entry and then, on a bounds refusal, either WARNs and compiles off the unbounded AST (rc-grace) or fails closed (fail-closed).

The GA refusal is { ok: false, reason: 'parse-error', detail: 'Exceeded maxAstNodes (256)' }deliberately the existing reason, not a fourth one: parse-error is what every consumer already routes to its deny path, and a new reason value would be a new branch none of them has. Which bound was blown rides in detail.

The WARN is emitted once per source (bounded memo), through globalThis.console rather than the bare console global — this package builds with neither the DOM lib nor @types/node, and a bare embedder must degrade to silence, not to a ReferenceError thrown from inside a security compiler.

A1 — within-limits parity: measured, and it holds

Corpus: 1604 candidate strings harvested from every suite that drives compileCelToFilter / isPushdownableCel (formula, lint × 2, plugin-security, plugin-sharing, service-analytics, the dogfood conformance ledger and authz matrix), plus the SQL-bridged form of each. 710 sources parse under both front ends. AST differences: 3 — all of them rewriteNullableTernary's dyn(…) wrap on a null-guard ternary. Zero verdict changes.

The issue flagged this as scope to pin rather than discover: "a ternary was already non-pushdownable, but the REASON it is non-pushdownable changing is behavior". Measured across seven ternary shapes (top-level, as a comparison operand, as an in container, as a receiver of a comparison, as a string-method argument), reason and detail are byte-identical before and after — the ?: node faults in lowerCondition / classify before the lowerer ever descends into a branch, so the wrap the rewrite adds inside a branch is never reached:

record.done ? 'y' : null            old {unsupported, 'unsupported operator "?:"'}  new identical
record.a == (record.b ? 'x' : null) old {unsupported, 'unsupported operand "?:"'}   new identical

cel-to-filter-parse-convergence.test.ts rebuilds the pre-#6132 environment — the one place in the repo that still may — and demands an equal CelFilterCompileResult for all 47 corpus shapes, plus an explicit assertion that the AST really does differ for a ternary (so the pin cannot go vacuous).

No corpus source was newly refused: the corpus contains no over-limit predicate.

A2 — the authoring-time lint face

Confirmed, and no packages/lint edit is needed at either switch position.

  • The general CEL lint face already refuses over-limit sources, independently of this change, because it converged onto parseCelToAst in packages/lint 绕过 @objectstack/formula 直接 parse CEL —— 两个解析入口对「什么能解析」会给出不同答案 #4812/refactor(formula,lint): parseCelToAst 成为唯一的 CEL 解析入口 (#4812) #6130. Measured: a 60-level-nested visibleWhen yields visibility-predicate-syntax (error), message visibility predicate is not valid CEL — Exceeded maxDepth (32).
  • An over-limit sharing condition is already an authoring error today, under the shipping rc-grace default: expression-invalid, quoting Exceeded maxAstNodes (256). validateSharingRuleEnforceability correctly stays silent — it defers parse-error to the rule that owns syntax, and that rule reports it.
  • The two enforceability gates (validateRlsPredicateEnforceabilityisSupportedRlsExpression, validateSharingRuleEnforceabilitycompileCelToFilter) are downstream of this switch, and both suites pin "the lint verdict IS the consumer's verdict" in both directions. Authoring reporting therefore flips with the runtime by construction and cannot drift from it. Measured: flipping the switch to fail-closed turns an 80-term RLS using from clean into rls-predicate-unparseable (error), with no lint change.

One thing worth a follow-up, not touched here (cross-lane surface, and out of this card's scope): at GA an over-limit RLS predicate is reported under the id rls-predicate-unparseable with prose about SQL-vs-CEL syntax, which is off-label for a bounds overrun even though the message does carry the exact Exceeded maxAstNodes (256) detail. A rls-predicate-over-budget id with its own hint would read better. Flagged, not done.

A3 — per-consumer declarations

consumer call verdict
plugin-security RLSCompiler.compileExpression compileCelToFilter(cel, { variables: { current_user } }) already-conforming. Any !oknullRLS_DENY_FILTER when it is the only applicable policy. No source change; new test pins both switch positions end-to-end.
plugin-security RLSCompiler.compileFilter !isSupportedRlsExpression(predicate) (the ADR-0056 D4 drop-warning) already-conforming. Both sides of the comparison move together, so the flip cannot produce a silently dropped policy — pinned: at fail-closed the drop emits DROPPED (no enforcement).
formula isSupportedRlsExpression isPushdownableCel(sqlPredicateToCel(x)) changed at GA (it is the wrapper the issue called out as no independent gate). At rc-grace: unchanged.
plugin-sharing bootstrapDeclaredSharingRules compileCelToFilter(cel, { variables: {} }) already-conforming. !ok → rule skipped + boot WARN. At GA an over-limit condition stops being seeded — and it is already an authoring error today (A2), so it cannot reach boot from a stack that passed os build.
service-analytics read-scope-sql consumes the FilterCondition the RLSCompiler emits already-conforming; no CEL parse of its own.
lint validateRlsPredicateEnforceability isSupportedRlsExpression / isPushdownableCel changed at GA, by construction, with no edit. Clean at rc-grace, rls-predicate-unparseable at fail-closed.
lint validateSharingRuleEnforceability compileCelToFilter already-conforming. Defers parse-error to the syntax rule, which already reports over-limit conditions today.
lint validateVisibilityPredicates, validateNullGuards parseCelToAst out of scope, already-conforming. Already on the canonical front end; untouched by this change.

Tests

Rejection- and WARN-class tests assert specific identity; there is no bare toThrow() anywhere in the diff.

  • cel-parse-reason.test.ts (24) — the sister entrance: which limit, its platform value, the exact summary, the measure (200 for the 200-element list, exactly), no measurement/no unbounded parse unless asked; parseCelToAst's null contract; both-entrance agreement with celEngine.compile; the by-code hazard.
  • cel-to-filter-parse-convergence.test.ts (56) — A1, both directions, against the rebuilt old env.
  • cel-to-filter-limits.test.ts (27) — the switch in both positions on the three measured shapes: rc-grace compiles + WARNs (asserting the limit name, limit <N>, Exceeded <limit> (<N>), a numeric measure strictly over the bound, the GA consequence, once-per-source dedupe, and that the compiled filter is real); fail-closed refuses with the exact { reason, detail } and does not warn; a genuine syntax fault keeps its own detail in both. Boundary shapes at the bound are admitted in both positions. The shipped default is pinned to rc-grace.
  • plugin-security/rls-pushdown-limits.test.ts (20) — the same three shapes on the RLS path: rc-grace → real filter, not the sentinel, no drop-warning, console WARN naming the limit; fail-closedRLS_DENY_FILTER, with an observable DROPPED (no enforcement) log, no grace WARN, and the multi-policy blast radius stated.

No new fake engine was introduced, so the assertEngineDeleteDispatch rule does not apply to this diff.

Reverse verification — direction written before running

Each pin was expected to fail if its subject were removed, and each was run in that state before being run in the passing state:

  1. Revert the convergence (restore the private limitless env in cel-to-filter.ts) → every fail-closed assertion in cel-to-filter-limits.test.ts and rls-pushdown-limits.test.ts must fail, and the grace WARNs must stop firing. (Expected: red.)
  2. Delete the rewriteNullableTernary half of A1 — i.e. assert the ternary AST is equal across front ends → cel-to-filter-parse-convergence.test.ts's "the rewrite really does change the AST" must fail, proving the reason-parity pins below it are not vacuous. (Expected: red.)
  3. Flip CEL_PUSHDOWN_LIMITS_MODE to 'fail-closed' → exactly the two documented assertions go red (the shipped-default pin and the rc-grace suite), and nothing else — the property that makes the GA flip a one-line change with a known blast radius. (Expected: exactly those, red.)
  4. Grade the bounds fault as parse (drop the code === 'limit_exceeded' arm) → the both-entrance agreement test and every "names the limit" assertion must fail. (Expected: red.)

Results are reported in the final comment on #6132.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Bx3H8DJhBsmgDoMp8Tz87T


Generated by Claude Code

…nt end, with an rc grace window (#6132)

`packages/formula/src/cel-to-filter.ts` — the one canonical CEL →
FilterCondition pushdown compiler (ADR-0058 D1/D2/D6) — kept a private,
limitless `new Environment({ unlistedVariablesAreDyn: true, enableOptionalTypes:
true })` with no `limits`, no stdlib and no `rewriteNullableTernary`, and read
`.ast` off it. That made the RLS / sharing pushdown path the one place on the
platform answering a different question from `celEngine.compile()` about what
parses: an 80-term conjunction, a 40-level nest and a 200-element `$in` all
reached REAL pushdown SQL while the interpreter refused each outright, and
`isSupportedRlsExpression` was a thin wrapper over the same env rather than an
independent gate.

It now parses through `parseCelToAstWithReason`, the canonical entry (#4812),
carrying DEFAULT_LIMITS.

Within the limits this is behaviour-preserving, measured rather than asserted:
across the 710 sources of the pushdown corpus both front ends accept, the only
AST difference is `rewriteNullableTernary`'s `dyn(...)` wrap on the null-guard
ternaries, and a ternary faults on its own `?:` node before the lowerer descends
into a branch — so reason AND detail are byte-identical. Pinned in
cel-to-filter-parse-convergence.test.ts, which rebuilds the old env to compare.

Over the limits, the maintainer's A' ruling (2026-08-08, on the issue) is
implemented as a single dated switch, `CEL_PUSHDOWN_LIMITS_MODE` in the new
`cel-pushdown-limits.ts`:

  - `rc-grace` (shipping default, 17.0.0-rc.x): an over-limit predicate still
    compiles, off the unbounded AST the canonical entry hands back for exactly
    this purpose, and WARNs once per predicate naming the exceeded bound, the
    platform's value for it, the predicate's own measure, and the GA
    consequence.
  - `fail-closed` (v17 GA, one line): the predicate is refused as
    `{ reason: 'parse-error', detail: 'Exceeded maxAstNodes (256)' }`, which the
    RLS path already routes to RLS_DENY_FILTER.

Both positions run in CI today — in `@objectstack/formula` and in
`@objectstack/plugin-security`, which owns the deny sentinel — so the GA half is
proven before it ships. Two assertions go red on the flip so it cannot be
silent.

Sister entrance: `parseCelToAstWithReason` separates "not valid CEL" from "valid
CEL, over budget" and names WHICH bound was blown, its platform value, and what
the source measures (cel-js's own accounting — the smallest bound it parses
under, found by probing the parser, because `maxDepth` counts parenthesised
recursion that leaves no AST node behind). Graded by the same by-class/by-code
classifier `compile`/`evaluate` use (#6223), never by error prose; the parity is
pinned. `parseCelToAst` is unchanged and still collapses every refusal to
`null`.

`@objectstack/lint` needs no change at either position: its two enforceability
gates read `isSupportedRlsExpression` / `compileCelToFilter` and both suites pin
"the lint verdict IS the consumer's verdict" in both directions, so authoring
reporting flips with the runtime by construction.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bx3H8DJhBsmgDoMp8Tz87T
@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 3:08pm

Request Review

@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
claude added 3 commits August 8, 2026 15:02
…lip docblock (#6132)

Two follow-ups from running the gate list:

  - `check:type-check-debt` went red: `@objectstack/formula`'s TEST_DEBT ledger
    records 17 raw tsc errors and the new suite made it 18. The added error was
    `TS2584: Cannot find name 'console'` — this package compiles with neither the
    DOM lib nor `@types/node`, which is why the compiler itself reaches the sink
    through `globalThis`. The test now spies on the same object the compiler
    writes to, rather than a differently-obtained one, which is also the only
    version of this spy that cannot go green over a silent sink. Back to 17.
  - The switch docblock named three test files that do not exist (the suites were
    consolidated into one). It now names the real file and states the flip's
    measured blast radius: 10 tests, all in that file, and nothing else.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bx3H8DJhBsmgDoMp8Tz87T
The grace WARN's "measurement capped" branch spelled the cap factor as a literal
`64` while `measureOverrun` read it from `MEASURE_CAP_FACTOR` — two copies of one
number, in two files, where a drift would make the WARN quote a bound the
measurement never used. The constant is now exported from `cel-engine.ts` (not
from the package index: it is an implementation detail of the diagnostic, not
public surface) and read in both places.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bx3H8DJhBsmgDoMp8Tz87T
…uessed key (#6132)

`parseCelToAstWithReason`'s "cel-js raised a limit fault whose key this package
cannot read" branch filled the overrun with `maxAstNodes` while its own comment
said a guessed name is the thing to avoid. It is unreachable on cel-js 8.0.0 —
`Parser#limitExceeded` always phrases it `Exceeded <key> (<n>)` — but a guessed
key is worse than none: the author goes and shortens the wrong axis. The branch
now reports `{ limit: null, limitValue: null }`, which the types carry, and
hands back no unbounded AST, so the pushdown path fails closed on it in either
position of the switch rather than compiling something it cannot describe.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bx3H8DJhBsmgDoMp8Tz87T
@os-zhuang
os-zhuang marked this pull request as ready for review August 8, 2026 17:14
@os-zhuang
os-zhuang added this pull request to the merge queue Aug 8, 2026
Merged via the queue into main with commit f6cd635 Aug 8, 2026
25 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-6132-cel-to-filter-converge branch August 8, 2026 17:25
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/xl tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

formula 内部还剩第三个 CEL 解析入口:cel-to-filter.ts 自建 limitless env,与 celEngine 对「什么能解析」仍不一致

2 participants