diff --git a/.changeset/formula-bounds-prescription.md b/.changeset/formula-bounds-prescription.md new file mode 100644 index 0000000000..382cd88787 --- /dev/null +++ b/.changeset/formula-bounds-prescription.md @@ -0,0 +1,37 @@ +--- +'@objectstack/formula': patch +--- + +`validateExpression`: give an over-budget expression a SIZE prescription instead of the dialect trailer (#7073) + +ADR-0032's shared validator appended one trailer to every `celEngine.compile` +refusal, byte for byte — `— predicates are bare CEL (e.g. \`record.rating >= 4\`).` +That sentence is right for a dialect mistake and actively wrong for a **bounds** +refusal: an 80-clause conjunction is already bare CEL, perfectly good syntax, and +merely over the platform's parse budget. The author was told to change the one +thing that was never wrong; an AI author, which obeys the last sentence it was +handed, rewrites the dialect and regresses. Reported by #6833's measurement. + +The refusal is unchanged — same inputs refused, same `Exceeded maxAstNodes (256)` +front half from cel-js. Only the prescription is now class-aware: a `bounds` +verdict (read off the engine's own `error.kind`, with the exceeded bound named by +`parseCelToAstWithReason`) produces + +> invalid CEL predicate: Exceeded maxAstNodes (256) … — this is valid CEL that +> exceeds the `maxAstNodes` budget (limit 256) — a SIZE fault, not a dialect +> mistake, so re-spelling the expression will not fix it. Shrink it (fewer +> clauses, shallower nesting, fewer list elements), or precompute the heavy part +> into a stored field and reference that field instead. … + +while a genuine dialect/syntax fault keeps the old trailer verbatim. Fixed once at +the producer, so all ~10 expression slots benefit — build, metadata registration, +lint's `validateStackExpressions`, and the `validate_expression` tool. The +remedies are deliberately slot-generic: the slots' combination semantics differ, +so PR #6831's RLS-specific "splitting the top-level `&&` widens the grant" is not +portable and splitting is offered only with a caveat. + +Also documents, text-only, the completeness gap in `cel-pushdown-limits.ts`'s +"nothing else needs to move at GA": a third lint gate (`validateStackExpressions`) +covers the same `sharingRules[].condition` and is mode-agnostic, so during the +rc grace window lint is stricter than the runtime — benign, tightening-direction, +and self-healing at GA. No behaviour change there. diff --git a/packages/formula/src/cel-pushdown-limits.ts b/packages/formula/src/cel-pushdown-limits.ts index 00b32ee2d3..eb097bc1e7 100644 --- a/packages/formula/src/cel-pushdown-limits.ts +++ b/packages/formula/src/cel-pushdown-limits.ts @@ -59,6 +59,32 @@ * lint suites pin "the lint verdict IS the consumer's verdict" in both * directions — so authoring-time reporting flips with the runtime, by * construction, and cannot drift from it. + * + * ### The third gate is NOT downstream of this switch — and that is fine (#7073) + * + * "Nothing else needs to move" is the right conclusion but the two-gate list + * above is not the whole set. A **third** lint gate reaches the same + * `sharingRules[].condition` field: `validateStackExpressions`, which goes + * through ADR-0032's shared `validateExpression` → `celEngine.compile`. That + * path applies {@link DEFAULT_LIMITS} **unconditionally** and never reads this + * switch (measured on #6833: `celPushdownLimitsMode` appears nowhere in + * `validate.ts` or `cel-engine.ts`'s compile path), so it is mode-agnostic by + * construction rather than by oversight. + * + * The consequence, stated plainly so the next reader does not "discover" it as + * a bug: **during the grace window lint is STRICTER than the runtime.** An + * over-budget `condition` is a gating lint ERROR today, while the pushdown path + * still compiles it under `rc-grace`. That divergence runs in the tightening + * direction — the author is told at authoring time about a source that will be + * refused at GA — and it **self-heals at GA**, when the runtime catches up to + * the position lint already holds. #6833's measurement graded it benign on + * exactly those grounds. Loosening lint to chase the grace window would be a + * regression, not a fix: it would restore the silent acceptance #6132 closed. + * + * So the GA checklist is unchanged. What #7073 corrected on that third gate is + * the message's PRESCRIPTION, not its verdict: an over-budget expression used + * to be told "predicates are bare CEL", the dialect trailer, which sends the + * author to rewrite a dialect that was never wrong. */ /** How the pushdown path answers a source that overruns a `DEFAULT_LIMITS` bound. */ diff --git a/packages/formula/src/validate.test.ts b/packages/formula/src/validate.test.ts index ac02ec2634..c0f6b4fd1e 100644 --- a/packages/formula/src/validate.test.ts +++ b/packages/formula/src/validate.test.ts @@ -50,6 +50,93 @@ describe('validateExpression (ADR-0032)', () => { }); }); + // #7073 — the trailer used to be undifferentiated: EVERY `celEngine.compile` + // refusal, bounds included, ended with the dialect prescription + // "`predicate`s are bare CEL (e.g. `record.rating >= 4`)". For a + // syntactically perfect but over-budget expression that sentence is advice + // that cannot succeed — the source IS bare CEL — and an author who follows + // the last sentence they were handed (an LLM author above all) rewrites the + // dialect and regresses. + // + // Both directions are pinned, deliberately. A test asserting only "the + // bounds message changed" would stay green on a fix that ALSO stripped the + // dialect trailer from genuine dialect faults, i.e. that shrank the refusal + // surface while appearing to widen it. + describe('bounds vs dialect: the prescription follows the fault class (#7073)', () => { + /** 80-term conjunction — the escalation's `maxAstNodes` shape (#6833's fixture). */ + const OVER_AST_NODES = Array.from({ length: 80 }, (_, i) => `record.f${i} == ${i}`).join(' && '); + /** 60-level parenthesis nest — `maxDepth`. Counts recursion that leaves no AST node. */ + const OVER_DEPTH = `${'('.repeat(60)}record.a${')'.repeat(60)} == 1`; + /** 200-element list literal — `maxListElements`. */ + const OVER_LIST = `record.id in [${Array.from({ length: 200 }, (_, i) => `'u${i}'`).join(',')}]`; + + /** The byte-for-byte dialect trailer, per role. Must survive on dialect faults. */ + const dialectTrailer = (role: 'predicate' | 'value') => + ` — ${role}s are bare CEL (e.g. \`record.rating >= 4\`).`; + + describe.each([ + { name: 'maxAstNodes (80-term conjunction)', source: OVER_AST_NODES, limit: 'maxAstNodes' }, + { name: 'maxDepth (60-level nest)', source: OVER_DEPTH, limit: 'maxDepth' }, + { name: 'maxListElements (200-element list)', source: OVER_LIST, limit: 'maxListElements' }, + ])('an over-budget but valid CEL $name', ({ source, limit }) => { + it('is refused, names the exceeded bound and its value, and never says "bare CEL"', () => { + const r = validateExpression('predicate', source); + expect(r.ok).toBe(false); + expect(r.errors).toHaveLength(1); + const { message } = r.errors[0]; + // The front half — cel-js's own reason — was always right; keep it. + expect(message).toMatch(/^invalid CEL predicate:/); + expect(message).toMatch(/Exceeded/); + // The bound is NAMED with the platform's value for it. + expect(message).toContain(`\`${limit}\` budget (limit `); + // …and the prescription is a size prescription, not a dialect one. + expect(message).toMatch(/SIZE fault, not a dialect mistake/); + expect(message).toMatch(/Shrink it/); + // ⛔ The defect itself: the dialect trailer must NOT reach this class. + expect(message).not.toContain(dialectTrailer('predicate')); + expect(message).not.toMatch(/bare CEL/); + }); + + it('applies to the `value` role too — one producer, all ~10 slots', () => { + const r = validateExpression('value', source); + expect(r.ok).toBe(false); + expect(r.errors[0].message).toMatch(/^invalid CEL value:/); + expect(r.errors[0].message).toMatch(/SIZE fault, not a dialect mistake/); + expect(r.errors[0].message).not.toContain(dialectTrailer('value')); + }); + }); + + // The flipped pin. The refusal surface may never shrink: a genuine dialect + // or syntax fault keeps the ADR-0032 §1d trailer, byte for byte. + it.each([ + { name: 'an unterminated comparison', source: 'record.stage ==' }, + { name: 'a stray token', source: 'record.stage @@ "won"' }, + { name: 'a SQL-dialect predicate', source: "stage = 'won' AND rating >= 4" }, + ])('keeps the dialect trailer verbatim on $name', ({ source }) => { + const r = validateExpression('predicate', source); + expect(r.ok).toBe(false); + expect(r.errors[0].message).toContain(dialectTrailer('predicate')); + expect(r.errors[0].message).not.toMatch(/SIZE fault/); + }); + + it('keeps the #1491 braces hint on a brace fault (it outranks no bounds fault)', () => { + const r = validateExpression('predicate', '{record.rating} >= 4'); + expect(r.ok).toBe(false); + expect(r.errors[0].message).toMatch(/template brace was used inside a CEL expression/); + expect(r.errors[0].message).not.toMatch(/SIZE fault/); + }); + + it('offers no split remedy without a caveat — combination semantics differ per slot', () => { + // PR #6831's RLS sentence ("splitting the top-level `&&` widens the + // grant") is TRUE for a security predicate and wrong-to-meaningless for a + // formula value. This shared producer therefore ships the caveat, not the + // slot-specific claim. + const message = validateExpression('predicate', OVER_AST_NODES).errors[0].message; + expect(message).toMatch(/changes how they combine at this authoring site/); + expect(message).not.toMatch(/widen|grant|permission/i); + }); + }); + describe('templates', () => { it('accepts a valid {{ path }} template', () => { const r = validateExpression('template', 'Hot lead: {{ record.full_name }}'); diff --git a/packages/formula/src/validate.ts b/packages/formula/src/validate.ts index 1b9999e97b..65048b5185 100644 --- a/packages/formula/src/validate.ts +++ b/packages/formula/src/validate.ts @@ -17,7 +17,14 @@ * This validator detects that specific mistake and returns the exact fix. */ -import { celEngine, firstUndeclaredReference, firstTypeMismatch, inferCelType, type FieldCelType } from './cel-engine'; +import { + celEngine, + firstUndeclaredReference, + firstTypeMismatch, + inferCelType, + parseCelToAstWithReason, + type FieldCelType, +} from './cel-engine'; import { templateEngine } from './template-engine'; export type FieldRole = 'predicate' | 'value' | 'template'; @@ -195,6 +202,58 @@ function bracesHint(source: string): string | null { ); } +/** + * The prescription for a **bounds** refusal — an expression that is perfectly + * good CEL and merely too big for the platform's parse budget (#7073). + * + * Until #7073 every `celEngine.compile` refusal got the same dialect trailer + * ("`predicate`s are bare CEL (e.g. `record.rating >= 4`)"), byte for byte, + * including this class. That sentence is actively wrong here: the source is + * already bare CEL, so an author who obeys the last sentence they were given — + * an LLM author above all — rewrites the dialect, learns nothing, and comes + * back with the same 80-clause conjunction. The front half of the message + * (cel-js's own `Exceeded maxAstNodes (256)`) was right all along; only the + * prescription lied. + * + * The class comes from `celEngine.compile`'s own `kind: 'bounds'`; WHICH bound + * and its value come from {@link parseCelToAstWithReason}, the same + * reason-carrying entrance `@objectstack/lint`'s RLS gate reads (#6778 / + * PR #6831 — the consumer-side instance of this same defect family). Called + * WITHOUT `admitOverLimit`, so it takes neither the unbounded parse nor the + * overrun measurement: this path needs only the bound's NAME, and a refusal + * must not pay to re-parse a source it has just judged too large. + * + * ### Why the remedies are generic + * + * `validateExpression` is ADR-0032's shared validator: one message serves all + * ~10 expression slots (flow/automation conditions, `Field.formula`, validation + * rules, `visibleWhen`, `sharingRules[].condition`, the `validate_expression` + * tool …). Their COMBINATION semantics differ, so PR #6831's RLS-specific + * sentence ("splitting the top-level `&&` widens the grant") is not portable — + * it is true for a security predicate and false, or merely meaningless, for a + * formula value. Shrinking and denormalising are safe everywhere; splitting is + * offered only with the caveat that the site decides what splitting means. + */ +function boundsHint(source: string): string | null { + const parsed = parseCelToAstWithReason(source); + // `celEngine.compile` said `bounds`, and both verdicts are graded by the same + // `classifyCelFault`, so this holds — but a narrowing that ever stopped + // holding must degrade to the old trailer, never to a wrong bound name. + if (parsed.ok || parsed.kind !== 'bounds') return null; + const { limit, limitValue } = parsed.overrun; + const bound = + limit && limitValue != null + ? `the \`${limit}\` budget (limit ${limitValue})` + : "one of the platform's parse budgets"; + return ( + `this is valid CEL that exceeds ${bound} — a SIZE fault, not a dialect mistake, so ` + + `re-spelling the expression will not fix it. Shrink it (fewer clauses, shallower nesting, ` + + `fewer list elements), or precompute the heavy part into a stored field and reference that ` + + `field instead. Splitting it into several expressions changes how they combine at this ` + + `authoring site, so check that site's semantics before doing that.` + ); +} + function checkFieldExistence(source: string, schema: ExprSchemaHint | undefined, errors: ExprValidationError[]): void { if (!schema?.fields || schema.fields.length === 0) return; const known = new Set(schema.fields); @@ -339,7 +398,11 @@ export function validateExpression( } const compiled = celEngine.compile(source); if (!compiled.ok) { - const hint = bracesHint(source); + // #7073 — a bounds refusal gets the SIZE prescription, never the dialect + // trailer: the source is already bare CEL, so "write bare CEL" is advice + // that cannot succeed. Checked first because the class is certain (it comes + // from the engine's own verdict) while the braces hint is a heuristic. + const hint = (compiled.error.kind === 'bounds' ? boundsHint(source) : null) ?? bracesHint(source); errors.push({ source, message: