Skip to content

Commit 5c083e8

Browse files
fix(ui-core): evaluatePlainCondition reads a bare-string predicate as CEL, as evaluateCondition does (#27773)
<!-- claude-projects-attribution: {"github_login":"os-bill"} --> _Requested by **Bill** · [project thread](https://claude.ai/code/project/chan_016iMXWSZDzqWRWvWzdNo8wg?thread=cmsg_016iMXWSZDzqWRWvWzdNo8wgDFH4S5pR1C8YvYCWWsZfXx)_ Fixes #27605 Clause-②: no (narrowing) Metadata-protocol: narrowed Skills-docs: none — no skill or doc names `evaluatePlainCondition` (repo grep over `skills/`, `content/`, `docs/`: 0 hits); the CEL teaching for bare predicates is #27587's ## 维护者速读 - 改了什么:`@object-ui/core` 对外导出的 `evaluatePlainCondition(condition, record)` 把「裸字符串」条件按 CEL 求值,与 `evaluateCondition` 走同一个 CEL 引擎;只有 `${…}` 模板仍走旧 JS 求值器。 - 为什么改:spec 把裸字符串定义为 CEL 简写,服务端和控制台其余谓词路径(#27584 之后)都按 CEL 读;这个导出却还按旧 JS 语法读,调用它的宿主拿到的是另一种方言。 - 风险与回滚:只依赖 JS 语法的裸字符串(`===`、`.includes()`、`.length` 等)现在返回 false,原来能算出 true;仓内除自身测试外零调用方(实测见下)。回滚即 revert 本 PR。 - 先看:`ui/packages/core/src/evaluator/ExpressionEvaluator.ts`(`evaluatePlainCondition` 函数体)、`ui/packages/core/src/evaluator/__tests__/ExpressionEvaluator.test.ts`(`describe('evaluatePlainCondition')`)、`.changeset/27605-plain-condition-cel.md`。 - 两分钟验证:`pnpm --dir ui exec vitest run packages/core/src/evaluator/__tests__/ExpressionEvaluator.test.ts`,预期 88 个测试全绿。 - 需要你做的:无,席位落地。 ## What changes - `evaluatePlainCondition` (`@object-ui/core`, published through `evaluator/index.ts`): a bare string now goes to `ExpressionEvaluator.evaluateCondition(condition, { throwOnError: true })`, which since #27584 evaluates a bare string on the canonical `@objectstack/formula` CEL engine (`evaluateCelCondition`). The evaluation context is unchanged (`{ ...record, data: record }`), so both `status` and `data.status` resolve in CEL. - The function's fault policy is kept: `throwOnError: true` turns a CEL fault into a throw, which the existing `catch` answers with `false`; a non-boolean result answers `false` (the engine seam already compares `=== true`). A blank condition answers `false` as it did before, by an explicit guard, because `evaluateCondition` answers a blank gate `true` ("no gate"). - A `${…}` template is unchanged: `evaluate(condition, { throwOnError: true }) === true` on the legacy engine. - Changeset `.changeset/27605-plain-condition-cel.md`: `'@object-ui/core': minor`, both declaration lines, a BREAKING note that teaches the CEL spelling with no FROM-TO list and no console-only escape hatch, ADR-0087 disposition `not-required (no-migration-prescription)` as in the precedent's changeset. ## Why `@objectstack/spec` defines a bare string in a predicate slot as `{ dialect: 'cel', source }` (the string arm of `ExpressionInputSchema` in `packages/spec/src/shared/expression.zod.ts`), the server reads it on CEL, and #27584 moved every console predicate path to CEL. This export was the one public reader left on the legacy JavaScript grammar (contract review of #27584, should-fix 3; triage graded it align, not retire). ## Tests All in `ExpressionEvaluator.test.ts`, `describe('evaluatePlainCondition')`: - Rewritten pin: the compound case now uses the CEL spelling (`amount > 1000 && status == 'urgent'`), true and false legs. - New: CEL-only constructs evaluate (`status in [...]`, `size(tags) > 0`), each with a true and a false leg. - New: a JS-only spelling is refused with `false` where the JS grammar would have answered `true` (`===`, `.includes(`), each beside its CEL control (`==`, `.contains(`) answering `true`, and no console line. - New: `data.amount > 1000` as a bare CEL predicate. - New: parity, six source and record pairs, `evaluatePlainCondition` equals `new ExpressionEvaluator({ ...record, data: record }).evaluateCondition(source)`. - New: a `${…}` template still reads `===` on the legacy engine. - New: a blank and a whitespace-only condition answer `false`. - Kept: field reference, numeric comparison, template `data.` reference, invalid syntax, non-boolean result. ## Acceptance notes - Narrowing, declared `Clause-②: no (narrowing)` / `Metadata-protocol: narrowed` as the claim and the precedent (#27584) do: a bare string that only the JS grammar reads now answers `false` from this export. - Caller sweep at `d93d9be5` (`git grep evaluatePlainCondition` over the whole tree, `ui/` and `packages/` included): 0 callers outside its own file and its test file. Sibling repositories (HotCRM, cloud) were not measured: NOT MEASURED, out of this card's scope. - Blank condition: kept at `false` (unchanged), so it deliberately differs from `evaluateCondition`'s diagnosed "no gate" `true`; this export answers "does the condition hold", and a blank one never did. Pinned. - No console line on a fault, as before: the `throwOnError` route passes `warn: false` to the engine seam, so the answer `false` is the whole report. Pinned. ## Verification <details><summary>Ablation (scripts/ablation-replace.mjs under the verify lock, restored to HEAD after)</summary> - Anchor `return evaluator.evaluateCondition(condition, { throwOnError: true });` replaced with `return evaluator.evaluateExpression(condition) === true;` (the legacy route): 3 failed of 88, the CEL-only, JS-only-refused and parity tests. Restore: blob equal to HEAD, `git diff HEAD` empty. </details> <details><summary>Local runs</summary> - `pnpm --dir ui exec vitest run packages/core/`: 218 files passed, 4141 tests passed, 27 skipped. - `pnpm --filter @object-ui/core type-check` (`tsc --noEmit && tsc -p tsconfig.test.json`): exit 0. - eslint on both changed source files: 0 errors (warnings are the file's existing `any`s). - `node ui/scripts/check-new-cross-file-line-citations.mjs`: 0 new citations. </details> <details><summary>Gates</summary> `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derived 34 commands on this diff; each was run with its exit code recorded, and all 34 exit 0 on `873fd637`. `--ran` reconciliation: 34 derived, 34 run, 0 NOT-MEASURED, 0 UNRUN. - `pnpm --dir ui exec vitest run scripts/` first exited 1 with 2 failures, both from this container's build state and not from the diff: `unit-registry-absence-collision` could not import 31 modules because `@objectstack/client` had no `dist/` here, and `check-vi-mock-inherit` timed out at 15 s on the shared box. After `pnpm --filter '@objectstack/client...' build`, the full command re-ran: 169 files passed, 2 skipped, 4902 tests passed, exit 0. That re-run is the recorded line. </details> <details><summary>Dogfood battery</summary> Run once on the final head `873fd637`, after building the closure (`pnpm --filter '@objectstack/dogfood^...' build`, exit 0), under the verify lock: `pnpm --filter @objectstack/dogfood test`, `VERDICT command-exit 0`; Test Files 354 passed, 3 skipped (357); Tests 2859 passed, 20 skipped (2879); 0 failures. </details> --- _Generated by [Claude Code](https://claude.ai/code/session_01Utp6CYKdX5M2WLtcLDH7qF)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 561b764 commit 5c083e8

3 files changed

Lines changed: 92 additions & 8 deletions

File tree

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
---
2+
'@object-ui/core': minor
3+
---
4+
5+
fix(core): `evaluatePlainCondition` reads a bare-string predicate as CEL, the way the spec defines it and `evaluateCondition` reads it
6+
7+
Clause-②: no (narrowing)
8+
Metadata-protocol: narrowed
9+
10+
`@objectstack/spec` defines a bare string in a predicate slot as shorthand for `{ dialect: 'cel', source }`, the server evaluates it on CEL, and `ExpressionEvaluator.evaluateCondition` reads it on CEL. The published `evaluatePlainCondition(condition, record)` export still read a bare string on the legacy JavaScript expression grammar, so a host calling it got the opposite dialect from every other predicate path: a CEL-only predicate such as `status in ['overdue', 'urgent']` or `size(tags) > 0` faulted and answered `false`.
11+
12+
- A bare string handed to `evaluatePlainCondition` is evaluated on the canonical `@objectstack/formula` CEL engine, the route `evaluateCondition` gives it, so a predicate that evaluates gets the same verdict from both. Record fields stay readable both bare (`status`) and under `data.` (`data.status`).
13+
- The function's answer is unchanged in shape: `true` only when the predicate evaluates to `true`; a fault, a non-boolean result and a blank condition answer `false`, with no console line.
14+
- Unchanged: a `${…}` template is still evaluated on the legacy engine.
15+
16+
**BREAKING**: a bare string passed to `evaluatePlainCondition` is read on CEL only, as the spec defines it. A predicate that relied on syntax only the legacy JavaScript grammar reads (for example `===`, `!==`, `?.`, `??`, `.includes(…)` or `.length`) now faults and the function answers `false` where it used to evaluate. Write the predicate in CEL, the one predicate language the spec and the server define (ADR-0032): equality is `==`, membership is `in`, a substring test is `.contains(…)`, a length is `size(…)`, a presence test is `has(…)`. Measured in this repository, nothing outside its own tests calls the export.
17+
18+
<!-- adr-0087: not-required (no-migration-prescription) the narrowed operand is the condition argument a host passes to the evaluatePlainCondition function at runtime, not metadata: no packages/spec schema declares it, no stored row or package source carries it, and its only reader is this function, so there is nothing for objectstack migrate meta to rewrite -->

‎ui/packages/core/src/evaluator/ExpressionEvaluator.ts‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -710,20 +710,31 @@ export function evaluateCondition(
710710
/**
711711
* Convenience function to evaluate a plain condition string against a data record.
712712
* Supports both template expressions (e.g., '${data.amount > 1000}') and
713-
* plain expressions (e.g., "status == 'overdue'").
713+
* bare CEL predicates (e.g., "status == 'overdue'").
714714
* Record fields are available both directly (status) and namespaced (data.status).
715+
*
716+
* A bare string is CEL (objectstack#27605): the spec defines it as shorthand for
717+
* `{ dialect: 'cel', source }`, so it takes the CEL route
718+
* {@link ExpressionEvaluator.evaluateCondition} gives it and a predicate that
719+
* evaluates reaches the same verdict on both. Only a `${…}` template stays on
720+
* the legacy engine. The answer is
721+
* `true` only when the predicate evaluates to `true`: a fault (a JS-only
722+
* spelling such as `===` included), a non-boolean result and a blank condition
723+
* all answer `false`.
715724
*/
716725
export function evaluatePlainCondition(
717726
condition: string,
718727
record: Record<string, any>
719728
): boolean {
729+
if (typeof condition !== 'string' || isBlankPredicateText(condition)) {
730+
return false;
731+
}
720732
const evaluator = new ExpressionEvaluator({ ...record, data: record }, globalCache, globalFormulas);
721733
try {
722-
const isTemplate = /\$\{/.test(condition);
723-
const result = isTemplate
724-
? evaluator.evaluate(condition, { throwOnError: true })
725-
: evaluator.evaluateExpression(condition);
726-
return result === true;
734+
if (/\$\{/.test(condition)) {
735+
return evaluator.evaluate(condition, { throwOnError: true }) === true;
736+
}
737+
return evaluator.evaluateCondition(condition, { throwOnError: true });
727738
} catch {
728739
return false;
729740
}

‎ui/packages/core/src/evaluator/__tests__/ExpressionEvaluator.test.ts‎

Lines changed: 57 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -175,22 +175,77 @@ describe('evaluatePlainCondition', () => {
175175
});
176176

177177
it('should evaluate compound conditions', () => {
178-
expect(evaluatePlainCondition("amount > 1000 && status === 'urgent'", { amount: 2000, status: 'urgent' })).toBe(true);
179-
expect(evaluatePlainCondition("amount > 1000 && status === 'urgent'", { amount: 2000, status: 'normal' })).toBe(false);
178+
expect(evaluatePlainCondition("amount > 1000 && status == 'urgent'", { amount: 2000, status: 'urgent' })).toBe(true);
179+
expect(evaluatePlainCondition("amount > 1000 && status == 'urgent'", { amount: 2000, status: 'normal' })).toBe(false);
180+
});
181+
182+
// objectstack#27605 — a bare string is CEL, as the spec defines it and
183+
// `evaluateCondition` reads it.
184+
it('reads a bare string as CEL: CEL-only constructs evaluate', () => {
185+
expect(evaluatePlainCondition("status in ['overdue', 'urgent']", { status: 'urgent' })).toBe(true);
186+
expect(evaluatePlainCondition("status in ['overdue', 'urgent']", { status: 'active' })).toBe(false);
187+
expect(evaluatePlainCondition('size(tags) > 0', { tags: ['vip'] })).toBe(true);
188+
expect(evaluatePlainCondition('size(tags) > 0', { tags: [] })).toBe(false);
189+
});
190+
191+
it('refuses a JS-only spelling with false, even where it would hold on the JS grammar', () => {
192+
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
193+
try {
194+
expect(evaluatePlainCondition("amount > 1000 && status === 'urgent'", { amount: 2000, status: 'urgent' })).toBe(false);
195+
expect(evaluatePlainCondition("name.includes('Acme')", { name: 'Acme Corp' })).toBe(false);
196+
// Control: the CEL spelling of the same two predicates holds.
197+
expect(evaluatePlainCondition("amount > 1000 && status == 'urgent'", { amount: 2000, status: 'urgent' })).toBe(true);
198+
expect(evaluatePlainCondition("name.contains('Acme')", { name: 'Acme Corp' })).toBe(true);
199+
// The fault is answered by the return value, as before: no console line.
200+
expect(warn).not.toHaveBeenCalled();
201+
} finally {
202+
warn.mockRestore();
203+
}
204+
});
205+
206+
it('reads data.field in a bare CEL predicate as well as the bare field', () => {
207+
expect(evaluatePlainCondition('data.amount > 1000', { amount: 2000 })).toBe(true);
208+
expect(evaluatePlainCondition('data.amount > 1000', { amount: 500 })).toBe(false);
209+
});
210+
211+
it('gives the verdict evaluateCondition gives for the same string and record', () => {
212+
const cases: Array<[string, Record<string, unknown>]> = [
213+
["status == 'overdue'", { status: 'overdue' }],
214+
["status == 'overdue'", { status: 'active' }],
215+
['amount > 1000 && data.amount < 5000', { amount: 2000 }],
216+
['amount > 1000 && data.amount < 5000', { amount: 9000 }],
217+
["status in ['a', 'b']", { status: 'b' }],
218+
['size(tags) > 1', { tags: ['x'] }],
219+
];
220+
for (const [source, record] of cases) {
221+
const viaCondition = new ExpressionEvaluator({ ...record, data: record }).evaluateCondition(source);
222+
expect(evaluatePlainCondition(source, record)).toBe(viaCondition);
223+
}
180224
});
181225

182226
it('should support data.field references in template expressions', () => {
183227
expect(evaluatePlainCondition('${data.amount > 1000}', { amount: 2000 })).toBe(true);
184228
expect(evaluatePlainCondition('${data.amount > 1000}', { amount: 500 })).toBe(false);
185229
});
186230

231+
it('keeps a template on the legacy engine', () => {
232+
// `===` is a JS-only spelling: still read inside a `${…}` template.
233+
expect(evaluatePlainCondition("${data.status === 'urgent'}", { status: 'urgent' })).toBe(true);
234+
expect(evaluatePlainCondition("${data.status === 'urgent'}", { status: 'normal' })).toBe(false);
235+
});
236+
187237
it('should return false for invalid expressions', () => {
188238
expect(evaluatePlainCondition('!!!invalidSyntax', { status: 'ok' })).toBe(false);
189239
});
190240

191241
it('should return false for non-boolean results', () => {
192242
expect(evaluatePlainCondition('status', { status: 'active' })).toBe(false);
193243
});
244+
245+
it('should return false for a blank condition', () => {
246+
expect(evaluatePlainCondition('', { status: 'active' })).toBe(false);
247+
expect(evaluatePlainCondition(' ', { status: 'active' })).toBe(false);
248+
});
194249
});
195250

196251
// ─── CSP Safety Tests ──────────────────────────────────────────────────────

0 commit comments

Comments
 (0)