Skip to content

Commit cafec0a

Browse files
os-helpclaude
andauthored
fix(formula): give an over-budget expression a size prescription, not the dialect trailer (#7073) (#7209)
`validateExpression` appended the same dialect trailer — "`predicate`s are bare CEL (e.g. `record.rating >= 4`)" — to every `celEngine.compile` refusal, byte for byte, including a `bounds` refusal. For a syntactically perfect, over-budget CEL (an 80-clause conjunction past `maxAstNodes` 256) that sentence sends the author to change the one thing that was never wrong; an AI author, which obeys the last sentence it is handed, rewrites the dialect and regresses. The refusal is unchanged. Only the prescription is now class-aware: on the engine's own `kind: 'bounds'` verdict the trailer becomes a size prescription naming the exceeded bound and its value (read from `parseCelToAstWithReason`, without `admitOverLimit` so no unbounded parse or measurement is paid for), with shrink / denormalise remedies and a caveat on splitting. A genuine dialect or syntax fault keeps the old trailer verbatim, and the #1491 braces hint is untouched — both directions pinned. Fixed once at the producer, so all ~10 ADR-0032 expression slots benefit. Also completes, text-only, `cel-pushdown-limits.ts`'s "nothing else needs to move at GA": a third lint gate (`validateStackExpressions`) reaches the same `sharingRules[].condition` and is mode-agnostic, so lint is stricter than the runtime during the rc grace window — benign, tightening-direction, self-healing at GA. No behaviour change there. Claude-Session: https://claude.ai/code/session_016R9de1FqP7NvwKvqXi92Gh Co-authored-by: Claude <noreply@anthropic.com>
1 parent dff4433 commit cafec0a

4 files changed

Lines changed: 215 additions & 2 deletions

File tree

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
---
2+
'@objectstack/formula': patch
3+
---
4+
5+
`validateExpression`: give an over-budget expression a SIZE prescription instead of the dialect trailer (#7073)
6+
7+
ADR-0032's shared validator appended one trailer to every `celEngine.compile`
8+
refusal, byte for byte — `— predicates are bare CEL (e.g. \`record.rating >= 4\`).`
9+
That sentence is right for a dialect mistake and actively wrong for a **bounds**
10+
refusal: an 80-clause conjunction is already bare CEL, perfectly good syntax, and
11+
merely over the platform's parse budget. The author was told to change the one
12+
thing that was never wrong; an AI author, which obeys the last sentence it was
13+
handed, rewrites the dialect and regresses. Reported by #6833's measurement.
14+
15+
The refusal is unchanged — same inputs refused, same `Exceeded maxAstNodes (256)`
16+
front half from cel-js. Only the prescription is now class-aware: a `bounds`
17+
verdict (read off the engine's own `error.kind`, with the exceeded bound named by
18+
`parseCelToAstWithReason`) produces
19+
20+
> invalid CEL predicate: Exceeded maxAstNodes (256) … — this is valid CEL that
21+
> exceeds the `maxAstNodes` budget (limit 256) — a SIZE fault, not a dialect
22+
> mistake, so re-spelling the expression will not fix it. Shrink it (fewer
23+
> clauses, shallower nesting, fewer list elements), or precompute the heavy part
24+
> into a stored field and reference that field instead. …
25+
26+
while a genuine dialect/syntax fault keeps the old trailer verbatim. Fixed once at
27+
the producer, so all ~10 expression slots benefit — build, metadata registration,
28+
lint's `validateStackExpressions`, and the `validate_expression` tool. The
29+
remedies are deliberately slot-generic: the slots' combination semantics differ,
30+
so PR #6831's RLS-specific "splitting the top-level `&&` widens the grant" is not
31+
portable and splitting is offered only with a caveat.
32+
33+
Also documents, text-only, the completeness gap in `cel-pushdown-limits.ts`'s
34+
"nothing else needs to move at GA": a third lint gate (`validateStackExpressions`)
35+
covers the same `sharingRules[].condition` and is mode-agnostic, so during the
36+
rc grace window lint is stricter than the runtime — benign, tightening-direction,
37+
and self-healing at GA. No behaviour change there.

packages/formula/src/cel-pushdown-limits.ts

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,32 @@
5959
* lint suites pin "the lint verdict IS the consumer's verdict" in both
6060
* directions — so authoring-time reporting flips with the runtime, by
6161
* construction, and cannot drift from it.
62+
*
63+
* ### The third gate is NOT downstream of this switch — and that is fine (#7073)
64+
*
65+
* "Nothing else needs to move" is the right conclusion but the two-gate list
66+
* above is not the whole set. A **third** lint gate reaches the same
67+
* `sharingRules[].condition` field: `validateStackExpressions`, which goes
68+
* through ADR-0032's shared `validateExpression` → `celEngine.compile`. That
69+
* path applies {@link DEFAULT_LIMITS} **unconditionally** and never reads this
70+
* switch (measured on #6833: `celPushdownLimitsMode` appears nowhere in
71+
* `validate.ts` or `cel-engine.ts`'s compile path), so it is mode-agnostic by
72+
* construction rather than by oversight.
73+
*
74+
* The consequence, stated plainly so the next reader does not "discover" it as
75+
* a bug: **during the grace window lint is STRICTER than the runtime.** An
76+
* over-budget `condition` is a gating lint ERROR today, while the pushdown path
77+
* still compiles it under `rc-grace`. That divergence runs in the tightening
78+
* direction — the author is told at authoring time about a source that will be
79+
* refused at GA — and it **self-heals at GA**, when the runtime catches up to
80+
* the position lint already holds. #6833's measurement graded it benign on
81+
* exactly those grounds. Loosening lint to chase the grace window would be a
82+
* regression, not a fix: it would restore the silent acceptance #6132 closed.
83+
*
84+
* So the GA checklist is unchanged. What #7073 corrected on that third gate is
85+
* the message's PRESCRIPTION, not its verdict: an over-budget expression used
86+
* to be told "predicates are bare CEL", the dialect trailer, which sends the
87+
* author to rewrite a dialect that was never wrong.
6288
*/
6389

6490
/** How the pushdown path answers a source that overruns a `DEFAULT_LIMITS` bound. */

packages/formula/src/validate.test.ts

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,93 @@ describe('validateExpression (ADR-0032)', () => {
5050
});
5151
});
5252

53+
// #7073 — the trailer used to be undifferentiated: EVERY `celEngine.compile`
54+
// refusal, bounds included, ended with the dialect prescription
55+
// "`predicate`s are bare CEL (e.g. `record.rating >= 4`)". For a
56+
// syntactically perfect but over-budget expression that sentence is advice
57+
// that cannot succeed — the source IS bare CEL — and an author who follows
58+
// the last sentence they were handed (an LLM author above all) rewrites the
59+
// dialect and regresses.
60+
//
61+
// Both directions are pinned, deliberately. A test asserting only "the
62+
// bounds message changed" would stay green on a fix that ALSO stripped the
63+
// dialect trailer from genuine dialect faults, i.e. that shrank the refusal
64+
// surface while appearing to widen it.
65+
describe('bounds vs dialect: the prescription follows the fault class (#7073)', () => {
66+
/** 80-term conjunction — the escalation's `maxAstNodes` shape (#6833's fixture). */
67+
const OVER_AST_NODES = Array.from({ length: 80 }, (_, i) => `record.f${i} == ${i}`).join(' && ');
68+
/** 60-level parenthesis nest — `maxDepth`. Counts recursion that leaves no AST node. */
69+
const OVER_DEPTH = `${'('.repeat(60)}record.a${')'.repeat(60)} == 1`;
70+
/** 200-element list literal — `maxListElements`. */
71+
const OVER_LIST = `record.id in [${Array.from({ length: 200 }, (_, i) => `'u${i}'`).join(',')}]`;
72+
73+
/** The byte-for-byte dialect trailer, per role. Must survive on dialect faults. */
74+
const dialectTrailer = (role: 'predicate' | 'value') =>
75+
` — ${role}s are bare CEL (e.g. \`record.rating >= 4\`).`;
76+
77+
describe.each([
78+
{ name: 'maxAstNodes (80-term conjunction)', source: OVER_AST_NODES, limit: 'maxAstNodes' },
79+
{ name: 'maxDepth (60-level nest)', source: OVER_DEPTH, limit: 'maxDepth' },
80+
{ name: 'maxListElements (200-element list)', source: OVER_LIST, limit: 'maxListElements' },
81+
])('an over-budget but valid CEL $name', ({ source, limit }) => {
82+
it('is refused, names the exceeded bound and its value, and never says "bare CEL"', () => {
83+
const r = validateExpression('predicate', source);
84+
expect(r.ok).toBe(false);
85+
expect(r.errors).toHaveLength(1);
86+
const { message } = r.errors[0];
87+
// The front half — cel-js's own reason — was always right; keep it.
88+
expect(message).toMatch(/^invalid CEL predicate:/);
89+
expect(message).toMatch(/Exceeded/);
90+
// The bound is NAMED with the platform's value for it.
91+
expect(message).toContain(`\`${limit}\` budget (limit `);
92+
// …and the prescription is a size prescription, not a dialect one.
93+
expect(message).toMatch(/SIZE fault, not a dialect mistake/);
94+
expect(message).toMatch(/Shrink it/);
95+
// ⛔ The defect itself: the dialect trailer must NOT reach this class.
96+
expect(message).not.toContain(dialectTrailer('predicate'));
97+
expect(message).not.toMatch(/bare CEL/);
98+
});
99+
100+
it('applies to the `value` role too — one producer, all ~10 slots', () => {
101+
const r = validateExpression('value', source);
102+
expect(r.ok).toBe(false);
103+
expect(r.errors[0].message).toMatch(/^invalid CEL value:/);
104+
expect(r.errors[0].message).toMatch(/SIZE fault, not a dialect mistake/);
105+
expect(r.errors[0].message).not.toContain(dialectTrailer('value'));
106+
});
107+
});
108+
109+
// The flipped pin. The refusal surface may never shrink: a genuine dialect
110+
// or syntax fault keeps the ADR-0032 §1d trailer, byte for byte.
111+
it.each([
112+
{ name: 'an unterminated comparison', source: 'record.stage ==' },
113+
{ name: 'a stray token', source: 'record.stage @@ "won"' },
114+
{ name: 'a SQL-dialect predicate', source: "stage = 'won' AND rating >= 4" },
115+
])('keeps the dialect trailer verbatim on $name', ({ source }) => {
116+
const r = validateExpression('predicate', source);
117+
expect(r.ok).toBe(false);
118+
expect(r.errors[0].message).toContain(dialectTrailer('predicate'));
119+
expect(r.errors[0].message).not.toMatch(/SIZE fault/);
120+
});
121+
122+
it('keeps the #1491 braces hint on a brace fault (it outranks no bounds fault)', () => {
123+
const r = validateExpression('predicate', '{record.rating} >= 4');
124+
expect(r.ok).toBe(false);
125+
expect(r.errors[0].message).toMatch(/template brace was used inside a CEL expression/);
126+
expect(r.errors[0].message).not.toMatch(/SIZE fault/);
127+
});
128+
129+
it('offers no split remedy without a caveat — combination semantics differ per slot', () => {
130+
// PR #6831's RLS sentence ("splitting the top-level `&&` widens the
131+
// grant") is TRUE for a security predicate and wrong-to-meaningless for a
132+
// formula value. This shared producer therefore ships the caveat, not the
133+
// slot-specific claim.
134+
const message = validateExpression('predicate', OVER_AST_NODES).errors[0].message;
135+
expect(message).toMatch(/changes how they combine at this authoring site/);
136+
expect(message).not.toMatch(/widen|grant|permission/i);
137+
});
138+
});
139+
53140
describe('templates', () => {
54141
it('accepts a valid {{ path }} template', () => {
55142
const r = validateExpression('template', 'Hot lead: {{ record.full_name }}');

packages/formula/src/validate.ts

Lines changed: 65 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,14 @@
1717
* This validator detects that specific mistake and returns the exact fix.
1818
*/
1919

20-
import { celEngine, firstUndeclaredReference, firstTypeMismatch, inferCelType, type FieldCelType } from './cel-engine';
20+
import {
21+
celEngine,
22+
firstUndeclaredReference,
23+
firstTypeMismatch,
24+
inferCelType,
25+
parseCelToAstWithReason,
26+
type FieldCelType,
27+
} from './cel-engine';
2128
import { templateEngine } from './template-engine';
2229

2330
export type FieldRole = 'predicate' | 'value' | 'template';
@@ -195,6 +202,58 @@ function bracesHint(source: string): string | null {
195202
);
196203
}
197204

205+
/**
206+
* The prescription for a **bounds** refusal — an expression that is perfectly
207+
* good CEL and merely too big for the platform's parse budget (#7073).
208+
*
209+
* Until #7073 every `celEngine.compile` refusal got the same dialect trailer
210+
* ("`predicate`s are bare CEL (e.g. `record.rating >= 4`)"), byte for byte,
211+
* including this class. That sentence is actively wrong here: the source is
212+
* already bare CEL, so an author who obeys the last sentence they were given —
213+
* an LLM author above all — rewrites the dialect, learns nothing, and comes
214+
* back with the same 80-clause conjunction. The front half of the message
215+
* (cel-js's own `Exceeded maxAstNodes (256)`) was right all along; only the
216+
* prescription lied.
217+
*
218+
* The class comes from `celEngine.compile`'s own `kind: 'bounds'`; WHICH bound
219+
* and its value come from {@link parseCelToAstWithReason}, the same
220+
* reason-carrying entrance `@objectstack/lint`'s RLS gate reads (#6778 /
221+
* PR #6831 — the consumer-side instance of this same defect family). Called
222+
* WITHOUT `admitOverLimit`, so it takes neither the unbounded parse nor the
223+
* overrun measurement: this path needs only the bound's NAME, and a refusal
224+
* must not pay to re-parse a source it has just judged too large.
225+
*
226+
* ### Why the remedies are generic
227+
*
228+
* `validateExpression` is ADR-0032's shared validator: one message serves all
229+
* ~10 expression slots (flow/automation conditions, `Field.formula`, validation
230+
* rules, `visibleWhen`, `sharingRules[].condition`, the `validate_expression`
231+
* tool …). Their COMBINATION semantics differ, so PR #6831's RLS-specific
232+
* sentence ("splitting the top-level `&&` widens the grant") is not portable —
233+
* it is true for a security predicate and false, or merely meaningless, for a
234+
* formula value. Shrinking and denormalising are safe everywhere; splitting is
235+
* offered only with the caveat that the site decides what splitting means.
236+
*/
237+
function boundsHint(source: string): string | null {
238+
const parsed = parseCelToAstWithReason(source);
239+
// `celEngine.compile` said `bounds`, and both verdicts are graded by the same
240+
// `classifyCelFault`, so this holds — but a narrowing that ever stopped
241+
// holding must degrade to the old trailer, never to a wrong bound name.
242+
if (parsed.ok || parsed.kind !== 'bounds') return null;
243+
const { limit, limitValue } = parsed.overrun;
244+
const bound =
245+
limit && limitValue != null
246+
? `the \`${limit}\` budget (limit ${limitValue})`
247+
: "one of the platform's parse budgets";
248+
return (
249+
`this is valid CEL that exceeds ${bound} — a SIZE fault, not a dialect mistake, so ` +
250+
`re-spelling the expression will not fix it. Shrink it (fewer clauses, shallower nesting, ` +
251+
`fewer list elements), or precompute the heavy part into a stored field and reference that ` +
252+
`field instead. Splitting it into several expressions changes how they combine at this ` +
253+
`authoring site, so check that site's semantics before doing that.`
254+
);
255+
}
256+
198257
function checkFieldExistence(source: string, schema: ExprSchemaHint | undefined, errors: ExprValidationError[]): void {
199258
if (!schema?.fields || schema.fields.length === 0) return;
200259
const known = new Set(schema.fields);
@@ -339,7 +398,11 @@ export function validateExpression(
339398
}
340399
const compiled = celEngine.compile(source);
341400
if (!compiled.ok) {
342-
const hint = bracesHint(source);
401+
// #7073 — a bounds refusal gets the SIZE prescription, never the dialect
402+
// trailer: the source is already bare CEL, so "write bare CEL" is advice
403+
// that cannot succeed. Checked first because the class is certain (it comes
404+
// from the engine's own verdict) while the braces hint is a heuristic.
405+
const hint = (compiled.error.kind === 'bounds' ? boundsHint(source) : null) ?? bracesHint(source);
343406
errors.push({
344407
source,
345408
message:

0 commit comments

Comments
 (0)