Skip to content

Commit 078002f

Browse files
committed
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
1 parent 501c4d6 commit 078002f

2 files changed

Lines changed: 28 additions & 13 deletions

File tree

packages/formula/src/cel-engine.ts

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -286,10 +286,15 @@ const CEL_LIMIT_KEYS = Object.keys(DEFAULT_LIMITS) as readonly CelLimitKey[];
286286

287287
/** How far past a {@link DEFAULT_LIMITS} bound a source actually reaches. */
288288
export interface CelBoundsOverrun {
289-
/** WHICH bound was exceeded — `maxAstNodes` / `maxDepth` / `maxListElements` / … */
290-
limit: CelLimitKey;
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;
291296
/** The platform's value for that bound, i.e. what the source had to stay under. */
292-
limitValue: number;
297+
limitValue: number | null;
293298
/**
294299
* What the source itself measures on that axis: the smallest value of
295300
* `limits[limit]` under which it parses, every OTHER bound lifted, so the
@@ -472,20 +477,24 @@ export function parseCelToAstWithReason(
472477
if (classifyCelFault(err) !== 'bounds') return { ok: false, kind: 'parse', message };
473478
const parseErr = err as ParseError;
474479
const limit = limitKeyOf(parseErr);
475-
const limitValue = limit ? DEFAULT_LIMITS[limit] : Number.NaN;
476480
const summary = parseErr.summary ?? message.split('\n')[0];
477481
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.
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.
481489
return {
482490
ok: false,
483491
kind: 'bounds',
484492
message,
485-
overrun: { limit: 'maxAstNodes', limitValue: DEFAULT_LIMITS.maxAstNodes, measured: null, summary },
493+
overrun: { limit: null, limitValue: null, measured: null, summary },
486494
unboundedAst: null,
487495
};
488496
}
497+
const limitValue = DEFAULT_LIMITS[limit];
489498
let unboundedAst: CelAstNode | null = null;
490499
let measured: number | null = null;
491500
if (opts.admitOverLimit) {

packages/formula/src/cel-to-filter.ts

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -172,13 +172,19 @@ function warnOverLimitPushdown(source: string, overrun: CelBoundsOverrun): void
172172
if (warnedOverLimit.has(source)) return;
173173
if (warnedOverLimit.size >= WARNED_OVER_LIMIT_MAX) warnedOverLimit.clear();
174174
warnedOverLimit.add(source);
175-
const measure = overrun.measured === null
176-
? `over ${overrun.limitValue * CEL_BOUNDS_MEASURE_CAP_FACTOR} (measurement capped)`
177-
: String(overrun.measured);
175+
// `limit` / `limitValue` are non-null on this path by construction: a bounds
176+
// fault whose key could not be named yields no unbounded AST, so it never
177+
// reaches the grace window. The fallbacks keep the sentence readable rather
178+
// than printing `null` if that ever stops being true.
179+
const measure = overrun.measured !== null
180+
? String(overrun.measured)
181+
: overrun.limitValue === null
182+
? 'over the measurement cap'
183+
: `over ${overrun.limitValue * CEL_BOUNDS_MEASURE_CAP_FACTOR} (measurement capped)`;
178184
const shown = source.length > 200 ? `${source.slice(0, 197)}...` : source;
179185
hostConsole()?.warn?.(
180-
`[cel-to-filter] pushdown predicate exceeds the platform CEL bound ${overrun.limit} ` +
181-
`(limit ${overrun.limitValue}, this predicate measures ${measure}): ${overrun.summary}. ` +
186+
`[cel-to-filter] pushdown predicate exceeds the platform CEL bound ${overrun.limit ?? '(unnamed)'} ` +
187+
`(limit ${overrun.limitValue ?? 'unknown'}, this predicate measures ${measure}): ${overrun.summary}. ` +
182188
`It still compiles during 17.0.0-rc.x; at v17 GA it will be REFUSED (parse-error) and the ` +
183189
`RLS/sharing pushdown path will fail closed (RLS_DENY_FILTER). Split it or move the logic ` +
184190
`to a hook/action body before upgrading. Predicate: ${shown}`,

0 commit comments

Comments
 (0)