Skip to content

Commit f6cd635

Browse files
os-zhuangclaude
andauthored
fix(formula): converge the CEL pushdown parser onto the canonical front end, with an rc grace window (#6132) (#6766)
* 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 * fix(formula): spy the WARN sink through globalThis, and correct the flip 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 * refactor(formula): name the measurement cap once (#6132) 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 * fix(formula): report an unnameable bound as `limit: null`, not as a guessed 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 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent e1e7629 commit f6cd635

9 files changed

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

0 commit comments

Comments
 (0)