Skip to content

Commit 78f4c4c

Browse files
committed
fix(formula): converge the CEL pushdown parser onto the canonical front 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
1 parent a5d2573 commit 78f4c4c

9 files changed

Lines changed: 1230 additions & 26 deletions
Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
---
2+
"@objectstack/formula": minor
3+
---
4+
5+
fix(formula): the CEL pushdown compiler parses through the canonical front end, so `DEFAULT_LIMITS` finally apply to RLS/sharing predicates (#6132)
6+
7+
`cel-to-filter.ts` — the ONE canonical CEL → `FilterCondition` pushdown compiler
8+
(ADR-0058 D1/D2/D6), consumed by the RLS path (`plugin-security`'s
9+
`RLSCompiler`), the sharing seeder (`plugin-sharing`), and the analytics SQL
10+
backend — kept a **private, limitless** parse environment of its own:
11+
12+
```ts
13+
new Environment({ unlistedVariablesAreDyn: true, enableOptionalTypes: true })
14+
```
15+
16+
no `limits`, no stdlib, no `rewriteNullableTernary`. That made the pushdown path
17+
the one place on the platform that answered a *different* question from
18+
`celEngine.compile()` about what parses. Measured: a 300-term addition, a
19+
60-level parenthesis nest and a 200-element list literal all parsed there while
20+
the interpreter refused each one outright (`Exceeded maxAstNodes (256)` /
21+
`maxDepth (32)` / `maxListElements (64)`). Escalated: an 80-term conjunction, a
22+
40-level nest and a 200-element `$in` all reached **real pushdown SQL**,
23+
silently — and `isSupportedRlsExpression`, the ADR-0056 D4 authoring gate, was a
24+
thin wrapper over the same limitless environment, so it was no independent check
25+
either.
26+
27+
It now parses through `parseCelToAstWithReason`#4812's canonical entry, with
28+
`DEFAULT_LIMITS`, the stdlib and the #3306 null-guard rewrite. "What parses" has
29+
one answer again.
30+
31+
**Within the limits nothing moves, and that is measured, not asserted.** Across
32+
the 710 sources of the pushdown corpus that both front ends accept, the only AST
33+
difference is `rewriteNullableTernary`'s `dyn(…)` wrap on the three null-guard
34+
ternaries — and a ternary faults on its own `?:` node before the lowerer
35+
descends into a branch, so verdict *and* detail come out byte-identical. Pinned
36+
in `cel-to-filter-parse-convergence.test.ts`, which rebuilds the old environment
37+
to compare against.
38+
39+
**Over the limits, behaviour changes — in two dated steps.**
40+
41+
- **Now, during `17.0.0-rc.x` (`rc-grace`):** an over-limit predicate **still
42+
compiles** — nothing that enforces today stops enforcing on this upgrade — and
43+
emits one WARN per predicate naming the bound that was exceeded
44+
(`maxAstNodes` / `maxDepth` / `maxListElements` / …), the platform's value for
45+
it, and what the predicate itself measures (cel-js's own accounting: the
46+
smallest bound it parses under), plus what will happen at GA.
47+
- **At v17.0.0 GA (`fail-closed`):** the same predicate is **refused**
48+
`{ ok: false, reason: 'parse-error', detail: 'Exceeded maxAstNodes (256)' }`
49+
and the RLS path turns that into `RLS_DENY_FILTER`, i.e. zero rows, fail
50+
closed. A sharing rule with such a condition is not seeded.
51+
52+
**The flip is one line.** `CEL_PUSHDOWN_LIMITS_MODE` in
53+
`packages/formula/src/cel-pushdown-limits.ts` — the single dated switch,
54+
shipping as `'rc-grace'`, to be set to `'fail-closed'` at the v17.0.0 GA release
55+
(i.e. when this package's version leaves `17.0.0-rc.x`). Both positions are
56+
exercised in CI today, in `@objectstack/formula` and in
57+
`@objectstack/plugin-security` (where the `RLS_DENY_FILTER` outcome lives), so
58+
the GA half is proven before it ships rather than after. Two tests are written
59+
to go red on that line so the flip cannot be silent.
60+
61+
**If you author RLS or sharing predicates:** a predicate over any of these
62+
bounds is already refused everywhere else on the platform (`os build`,
63+
`os validate`, the interpreter). Split it, or move the logic into a hook/action
64+
body (`ScriptBody { language: 'js' }`), before upgrading past the rc line. The
65+
WARN names the predicate and its measure so you can find them.
66+
67+
**New public surface**, for consumers that must *report* a refusal rather than
68+
merely react to one:
69+
70+
- `parseCelToAstWithReason(source, opts?)` — the reason-carrying sister entrance
71+
to `parseCelToAst`. Same front end, same verdict, but it distinguishes
72+
`'parse'` (not valid CEL) from `'bounds'` (valid CEL, over budget) and names
73+
the exceeded limit, its platform value, and the source's measure. Graded by
74+
the same by-class/by-code classifier `celEngine.compile` uses (#6223) — never
75+
by error prose. `parseCelToAst` is unchanged and still collapses every refusal
76+
to `null`.
77+
- `CelParseResult`, `CelBoundsOverrun`, `CelLimitKey`, `ParseCelToAstOptions`.
78+
- `CEL_PUSHDOWN_LIMITS_MODE`, `celPushdownLimitsMode()`,
79+
`setCelPushdownLimitsModeForTests()`, `CelPushdownLimitsMode`.
80+
81+
`@objectstack/lint` needs no change, at either position of the switch. Its two
82+
enforceability gates read `isSupportedRlsExpression` and `compileCelToFilter`,
83+
both downstream of this switch, and both suites pin "the lint verdict IS the
84+
consumer's verdict" in both directions — so authoring-time reporting flips with
85+
the runtime by construction. An over-limit sharing `condition` is in fact
86+
already an authoring **error** today (`expression-invalid`, from the general
87+
expression rule, quoting `Exceeded maxAstNodes (256)`), because that rule has
88+
always gone through the canonical front end.

packages/formula/src/cel-engine.ts

Lines changed: 224 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -271,14 +271,234 @@ let canonicalParseEnv: Environment | undefined;
271271
* asymmetry so neither side drifts.
272272
*/
273273
export function parseCelToAst(source: string): CelAstNode | null {
274-
if (typeof source !== 'string' || !source.trim()) return null;
274+
const parsed = parseCelToAstWithReason(source);
275+
return parsed.ok ? parsed.ast : null;
276+
}
277+
278+
// ---------------------------------------------------------------------------
279+
// The reason-carrying sister entrance (#6132)
280+
// ---------------------------------------------------------------------------
281+
282+
/** A key of {@link DEFAULT_LIMITS} — the platform bounds a source can overrun. */
283+
export type CelLimitKey = keyof typeof DEFAULT_LIMITS;
284+
285+
const CEL_LIMIT_KEYS = Object.keys(DEFAULT_LIMITS) as readonly CelLimitKey[];
286+
287+
/** How far past a {@link DEFAULT_LIMITS} bound a source actually reaches. */
288+
export interface CelBoundsOverrun {
289+
/** WHICH bound was exceeded — `maxAstNodes` / `maxDepth` / `maxListElements` / … */
290+
limit: CelLimitKey;
291+
/** The platform's value for that bound, i.e. what the source had to stay under. */
292+
limitValue: number;
293+
/**
294+
* What the source itself measures on that axis: the smallest value of
295+
* `limits[limit]` under which it parses, every OTHER bound lifted, so the
296+
* number is cel-js's own accounting rather than a second implementation of
297+
* it. `null` when the measurement was capped (see {@link MEASURE_CAP_FACTOR})
298+
* or is not being taken — a bounds *refusal* never measures, because
299+
* measuring means re-parsing a source we have just decided is too big.
300+
*/
301+
measured: number | null;
302+
/**
303+
* cel-js's own one-line summary — `Exceeded maxAstNodes (256)`. Taken from
304+
* `ParseError#summary`, NOT `#message`: the latter is
305+
* `formatErrorWithHighlight`'s rendering, which interpolates the author's own
306+
* source line (the #6223 hazard).
307+
*/
308+
summary: string;
309+
}
310+
311+
/**
312+
* The verdict {@link parseCelToAstWithReason} returns — the same three-way
313+
* answer {@link classifyCelFault} already grades a thrown fault into, made
314+
* available to a caller that has to ACT differently on `bounds` than on
315+
* `parse`, rather than collapsing both to `null`.
316+
*/
317+
export type CelParseResult =
318+
| { ok: true; ast: CelAstNode }
319+
/** Empty / whitespace-only source. Not a fault — "no expression". */
320+
| { ok: false; kind: 'empty'; message: string }
321+
/** A syntax fault. `message` is cel-js's rendered message, verbatim. */
322+
| { ok: false; kind: 'parse'; message: string }
323+
| {
324+
ok: false;
325+
kind: 'bounds';
326+
/** cel-js's rendered message, verbatim — same string `parse` carries. */
327+
message: string;
328+
/** WHICH bound, and by how much. */
329+
overrun: CelBoundsOverrun;
330+
/**
331+
* The AST an otherwise-identical but **unbounded** parse yields, when the
332+
* caller asked for it (`{ admitOverLimit: true }`) — the 17.0.0-rc.x
333+
* grace window's input, and nothing else's. `null` otherwise.
334+
*/
335+
unboundedAst: CelAstNode | null;
336+
};
337+
338+
export interface ParseCelToAstOptions {
339+
/**
340+
* Also perform the unbounded parse and hand back its AST + the measured
341+
* overrun. **Only** the 17.0.0-rc.x pushdown grace window sets this (see
342+
* `cel-pushdown-limits.ts`); it is what lets that window keep compiling a
343+
* predicate the platform's bounds refuse, while still naming the bound. It
344+
* disappears with the grace window at v17 GA.
345+
*
346+
* Off by default, deliberately: an unbounded parse of a source we have just
347+
* measured as over-budget is work proportional to the source, so a caller
348+
* that only wants the verdict must not pay for it.
349+
*/
350+
admitOverLimit?: boolean;
351+
}
352+
353+
/**
354+
* How far above the exceeded bound {@link measureOverrun} will search before it
355+
* gives up and reports `measured: null`. Bounds the diagnostic's own cost:
356+
* without a cap, describing a pathological source means parsing it at whatever
357+
* size it happens to be.
358+
*/
359+
const MEASURE_CAP_FACTOR = 64;
360+
361+
/**
362+
* The canonical env with ONE bound lifted, used only to measure an overrun.
363+
* Configured identically to {@link canonicalParseEnv} in every other respect —
364+
* same stdlib, same `unlistedVariablesAreDyn`, same `enableOptionalTypes` — so
365+
* "the smallest bound this source parses under" is a fact about the source and
366+
* not about a second, differently-shaped front end.
367+
*/
368+
function buildProbeEnv(limits: Record<string, number>): Environment {
369+
const env = new Environment({
370+
unlistedVariablesAreDyn: true,
371+
enableOptionalTypes: true,
372+
limits: limits as unknown as typeof DEFAULT_LIMITS,
373+
});
374+
return registerNumericCoercions(registerStdLib(env, () => new Date(0), 'UTC'));
375+
}
376+
377+
/** Every bound lifted out of the way except `key`, which is set to `value`. */
378+
function probeLimits(key: CelLimitKey, value: number): Record<string, number> {
379+
const limits: Record<string, number> = {};
380+
for (const k of CEL_LIMIT_KEYS) limits[k] = Number.MAX_SAFE_INTEGER;
381+
limits[key] = value;
382+
return limits;
383+
}
384+
385+
function parsesUnder(source: string, key: CelLimitKey, value: number): boolean {
386+
try {
387+
buildProbeEnv(probeLimits(key, value)).parse(source);
388+
return true;
389+
} catch {
390+
return false;
391+
}
392+
}
393+
394+
/**
395+
* The smallest `limits[key]` under which `source` parses — i.e. what the source
396+
* measures on that axis, in cel-js's own units.
397+
*
398+
* Measured rather than computed. cel-js decrements each counter at its own call
399+
* sites (`Parser#node` for `maxAstNodes`, three separate recursion points for
400+
* `maxDepth`, …), and `maxDepth` in particular counts parenthesised recursion
401+
* that leaves no AST node behind — so a node-walk over the parsed tree would
402+
* report `3` for a 60-deep parenthesis nest. Asking the parser is the only way
403+
* the number in the WARN means what it says.
404+
*
405+
* Exponential probe from the exceeded bound, then binary search: `O(log n)`
406+
* parses, capped at {@link MEASURE_CAP_FACTOR}× the bound.
407+
*/
408+
function measureOverrun(source: string, key: CelLimitKey, limitValue: number): number | null {
409+
const cap = limitValue * MEASURE_CAP_FACTOR;
410+
let hi = limitValue * 2;
411+
while (hi <= cap && !parsesUnder(source, key, hi)) hi *= 2;
412+
if (hi > cap) return null;
413+
// It parses at `hi` and (by construction) not at `lo`. Narrow to the boundary.
414+
let lo = hi / 2;
415+
while (hi - lo > 1) {
416+
const mid = Math.floor((lo + hi) / 2);
417+
if (parsesUnder(source, key, mid)) hi = mid;
418+
else lo = mid;
419+
}
420+
return hi;
421+
}
422+
423+
/** Read the exceeded bound's key out of cel-js's structured limit fault. */
424+
function limitKeyOf(err: ParseError): CelLimitKey | null {
425+
// `summary` is `Exceeded ${limitKey} (${limit})`, built by `Parser#limitExceeded`
426+
// from a fixed set of keys — the author's source never reaches it (that is
427+
// `message`, via `formatErrorWithHighlight`). We still validate the capture
428+
// against `DEFAULT_LIMITS` rather than trusting the shape, so a cel-js that
429+
// rephrases the sentence degrades to "we know it was a bounds fault, not which
430+
// bound" instead of inventing a limit name.
431+
const key = /^Exceeded (\w+) /.exec(err.summary ?? '')?.[1];
432+
return key && (CEL_LIMIT_KEYS as readonly string[]).includes(key) ? (key as CelLimitKey) : null;
433+
}
434+
435+
/**
436+
* {@link parseCelToAst}, but it says WHY it refused (#6132).
437+
*
438+
* `parseCelToAst` collapses "this is not valid CEL" and "this is valid CEL that
439+
* is over the platform's budget" into the same `null`, which is right for a
440+
* caller whose job is not to adjudicate syntax. It is wrong for a caller whose
441+
* job is to *report* the refusal: the RLS / sharing pushdown path fails closed
442+
* on a refusal, and "your policy was rejected: parse error" for a predicate
443+
* that is perfectly well-formed but 431 AST nodes long sends the author
444+
* hunting for a typo that does not exist. This entrance names the bound
445+
* (`maxAstNodes` / `maxDepth` / `maxListElements` / …), the platform's value
446+
* for it, and what their source actually measures.
447+
*
448+
* The verdict is graded by the SAME {@link classifyCelFault} the engine's
449+
* `compile` / `evaluate` use — error class plus structured `code`, never prose
450+
* (#6223). A `bounds` verdict here and a `bounds` verdict from
451+
* `celEngine.compile()` are therefore the same judgement of the same fault,
452+
* which is the property `cel-parse-reason.test.ts` pins.
453+
*/
454+
export function parseCelToAstWithReason(
455+
source: string,
456+
opts: ParseCelToAstOptions = {},
457+
): CelParseResult {
458+
if (typeof source !== 'string' || !source.trim()) {
459+
return { ok: false, kind: 'empty', message: 'empty expression' };
460+
}
461+
// The #3306 rewrite is part of the canonical front end, so it happens before
462+
// the parse whose verdict we are reporting — and the measurement below probes
463+
// the SAME rewritten source, so the number describes what actually parsed.
464+
const rewritten = rewriteNullableTernary(source);
275465
try {
276466
// A wall-clock-free `now()` — the stdlib is registered for parse-time shape
277467
// only and is never called on this path.
278468
canonicalParseEnv ??= buildEnv(() => new Date(0));
279-
return canonicalParseEnv.parse(rewriteNullableTernary(source)).ast;
280-
} catch {
281-
return null;
469+
return { ok: true, ast: canonicalParseEnv.parse(rewritten).ast };
470+
} catch (err) {
471+
const message = err instanceof Error ? err.message : String(err);
472+
if (classifyCelFault(err) !== 'bounds') return { ok: false, kind: 'parse', message };
473+
const parseErr = err as ParseError;
474+
const limit = limitKeyOf(parseErr);
475+
const limitValue = limit ? DEFAULT_LIMITS[limit] : Number.NaN;
476+
const summary = parseErr.summary ?? message.split('\n')[0];
477+
if (!limit) {
478+
// A bounds fault we cannot name. Report it as bounds (the class is not in
479+
// doubt) with no measure — never as a syntax fault, which is the exact
480+
// mislabel this entrance exists to stop.
481+
return {
482+
ok: false,
483+
kind: 'bounds',
484+
message,
485+
overrun: { limit: 'maxAstNodes', limitValue: DEFAULT_LIMITS.maxAstNodes, measured: null, summary },
486+
unboundedAst: null,
487+
};
488+
}
489+
let unboundedAst: CelAstNode | null = null;
490+
let measured: number | null = null;
491+
if (opts.admitOverLimit) {
492+
try {
493+
unboundedAst = buildProbeEnv(probeLimits(limit, Number.MAX_SAFE_INTEGER)).parse(rewritten).ast;
494+
measured = measureOverrun(rewritten, limit, limitValue);
495+
} catch {
496+
// Unbounded still refuses ⇒ a second, non-`limit` bound or a fault the
497+
// bounded parse never got far enough to raise. No AST to admit.
498+
unboundedAst = null;
499+
}
500+
}
501+
return { ok: false, kind: 'bounds', message, overrun: { limit, limitValue, measured, summary }, unboundedAst };
282502
}
283503
}
284504

0 commit comments

Comments
 (0)