From 2d5068a5b1991bdaad9e0fa159432ae5191ec024 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 21:26:09 +0000 Subject: [PATCH 1/3] docs(core): name the three field-rule FAULT directions and the four other fallback policies (objectui#8069) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolveFieldRuleState` handed `evalFieldPredicate` three bare positional booleans — `true` / `false` / `false` — and answered the adjacent "no rule declared" case with the SAME literal, so each permissive value was written twice and nothing in the source said which of the two questions either copy answered. Zero behaviour change, and the values are untouched. Six module-private constants now spell the two questions apart (`*_WHEN_FAULTED` vs `*_WHEN_ABSENT`) and carry, in one place, what the direction is, that the three faults compose rather than cancel, and what the history does and does not record about why they point that way (objectui#1578 and ADR-0036 record a per-key "a fault is safe" rationale; no commit puts the case where all three faults arrive from one typo). `evalFieldPredicate`'s docblock gains the call-site policy table: five distinct fault policies share this one helper, two of which — `evalCel` and `ExpressionEvaluator.evaluateCelCondition` under `throwOnError` — detect a fault by calling it twice with OPPOSITE fallbacks, and therefore depend on `fallback` staying freely specifiable. Part of objectui#8069. Q2 (does the direction belong in the authored contract) and Q3 (a loud-but-safe middle) stay undecided; no fallback value moves here. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01611D6ZaRaMmwTNQmSbk8MH --- packages/core/src/evaluator/fieldRules.ts | 121 ++++++++++++++++++++-- 1 file changed, 115 insertions(+), 6 deletions(-) diff --git a/packages/core/src/evaluator/fieldRules.ts b/packages/core/src/evaluator/fieldRules.ts index fb3a940f54..ef553636db 100644 --- a/packages/core/src/evaluator/fieldRules.ts +++ b/packages/core/src/evaluator/fieldRules.ts @@ -138,6 +138,40 @@ function warnPredicateFailure( /** * Evaluate a field-rule CEL predicate against a record. * + * ## `fallback` is a per-CALL-SITE policy, not a default this helper owns + * + * The parameter is required and carries no default value, so every call site + * states a direction of its own — and the directions in this repo do not + * agree. Five distinct fault policies share this one helper today, which is + * readable nowhere but here (objectui#8069): + * + * 1. **Fixed permissive, paired with an equal "no rule" literal.** + * {@link resolveFieldRuleState} (`true` / `false` / `false`) and + * `resolveVisibleOptions` (`evaluator/optionRules.ts`, `true`). A fault + * produces the permissive verdict, indistinguishable from the verdict for + * "the author wrote no rule" — see the `…_FAULTED` / `…_ABSENT` note below. + * 2. **Fixed permissive, unguarded — ONE literal answers absent *and* + * faulted.** Every visibility call site outside core: the form renderer + * (`components/renderers/form/form.tsx`), console's `FormPage`, app-shell's + * `ScreenView`, `plugin-form`'s `WizardForm`. All of them pass `true`, and + * none has a separate absent branch at all. + * 3. **Caller-parameterised.** `evalRowPredicate`'s single-eval fast route + * (`evaluator/listConditional.ts`) forwards `opts.fallback`, so the + * direction is the mounting surface's to state. + * 4. **Divergence probe → fault FLAG.** `evalCel` + * (`evaluator/listConditional.ts`) calls this helper once per direction with + * `warn: false` + `onFault`: a verdict that tracks the fallback in BOTH runs + * is a fault, and the caller emits its own labelled warning and returns its + * own fallback. + * 5. **Divergence probe → THROW.** `ExpressionEvaluator.evaluateCelCondition` + * under `throwOnError` runs the same two-call trick and converts the + * disagreement into `throw new Error('CEL predicate failed to evaluate: …')`. + * + * ⚠️ The last two policies exist **because** `fallback` is free to specify: they + * detect a fault by disagreeing with themselves. Any proposal to fix a + * direction *inside* this helper — objectui#8069's open question — removes the + * mechanism they are built on, so read them before writing one. + * * @param pred The `visibleWhen` / `readonlyWhen` / `requiredWhen` predicate. * @param record The live form values (overlays prior persisted record). * @param fallback Value to return when the predicate is absent or fails to @@ -195,6 +229,60 @@ export function evalFieldPredicate( return value; } +/** + * The verdict {@link resolveFieldRuleState} applies when a rule's predicate + * CANNOT BE EVALUATED — parse error, unbound identifier, engine fault, or a + * predicate the author left blank. + * + * ⚠️ These answer **"what should the form do when this rule is BROKEN?"** — + * *not* "what should the form do when the author declared NO rule?" That + * second question is answered separately, by the `…_ABSENT` set below. The two + * sets hold pairwise EQUAL values today, and that equality is a copy, not an + * implication: each `rules. != null` ternary answers the absent case with + * its own literal and hands `evalFieldPredicate` the same literal as the fault + * fallback beside it, so every permissive value is written twice and the "it + * broke" answer was chosen by aligning it with the "it is absent" answer next + * to it. Nothing was *inherited* from a shared default — `evalFieldPredicate`'s + * `fallback` parameter is required and has none (objectui#8069 measured both + * halves). + * + * All three directions are the PERMISSIVE one, so a single mistyped column in + * one authored predicate yields a form that shows more, locks less and demands + * less — three faults from one typo, composing rather than cancelling. That is + * objectui#8069's open question and it is deliberately NOT decided here. + * + * **What the history records** (read on full, unshallowed history — a shallow + * clone answers this with one commit and no warning): the direction *was* + * reasoned about, once, when the helper landed (objectui#1578). This module's + * head and ADR-0036 both record the same rationale — the fallbacks are "chosen + * so a fault is *safe*: `true` for visibility (don't hide content on error), + * `false` for required/readonly (don't block submit or lock a field on + * error)". What no commit records is the COMPOSITION: every recorded argument + * is per-key, and the case where all three faults arrive from one typo was + * never put. objectstack#5149 then removed the SILENCE and left the direction + * explicitly undecided ("appeal 1"). + * + * ⛔ These values are shipped behaviour. Changing one is not a refactor, it is + * objectui#8069's decision — and objectui#6958 leans on the `visibleWhen` half + * staying fail-open (a broken predicate must never silently null a stored + * column). + */ +const VISIBLE_WHEN_FAULTED = true; +const READONLY_WHEN_FAULTED = false; +const REQUIRED_WHEN_FAULTED = false; + +/** + * The verdict {@link resolveFieldRuleState} applies when the author declared NO + * rule for that key at all — the `: ` arm of each `!= null` ternary. + * + * Spelled apart from the `…_FAULTED` set above because they answer a different + * question, not because they differ: changing one of THESE changes what an + * unconditional field does, which is a louder and quite separate decision. + */ +const VISIBLE_WHEN_ABSENT = true; +const READONLY_WHEN_ABSENT = false; +const REQUIRED_WHEN_ABSENT = false; + /** * Resolve the effective `{ visible, readonly, required }` state for a field * given its conditional rules and the live record. Each `*When` rule, when @@ -243,14 +331,28 @@ export function resolveFieldRuleState( const visible = rules.visibleWhen != null - ? evalFieldPredicate(rules.visibleWhen, record, true, previous, scope, diag('visibleWhen')) - : true; + ? evalFieldPredicate( + rules.visibleWhen, + record, + VISIBLE_WHEN_FAULTED, + previous, + scope, + diag('visibleWhen'), + ) + : VISIBLE_WHEN_ABSENT; const readonly = statics.readonly === true || (rules.readonlyWhen != null - ? evalFieldPredicate(rules.readonlyWhen, record, false, previous, scope, diag('readonlyWhen')) - : false); + ? evalFieldPredicate( + rules.readonlyWhen, + record, + READONLY_WHEN_FAULTED, + previous, + scope, + diag('readonlyWhen'), + ) + : READONLY_WHEN_ABSENT); // Short-circuited, not evaluated-and-discarded: the verdict cannot depend on // the predicate, so running it would only spend an engine call per field per @@ -262,8 +364,15 @@ export function resolveFieldRuleState( ? false : statics.required === true || (rules.requiredWhen != null - ? evalFieldPredicate(rules.requiredWhen, record, false, previous, scope, diag('requiredWhen')) - : false); + ? evalFieldPredicate( + rules.requiredWhen, + record, + REQUIRED_WHEN_FAULTED, + previous, + scope, + diag('requiredWhen'), + ) + : REQUIRED_WHEN_ABSENT); return { visible, readonly, required }; } From 61b389f4bae7d8e44823a953d6336fb1f549d5b7 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 21:33:42 +0000 Subject: [PATCH 2/3] fix(core): a declared-but-blank field-rule predicate is no longer silent (objectui#8069) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `''` and `' '` are authorable — `ExpressionWireSchema` is a bare `z.string()` with no `.min(1)`, and `resolveFieldRuleState`'s guard is `!= null`, so a blank predicate passes both. `evalFieldPredicate` then returned the caller's permissive fallback on its first line, BEFORE `warnPredicateFailure` or `onFault` could fire. That is a third state, not a spelling of either neighbour: the key is present (so it is not "the author wrote no rule") and nothing evaluates (so no engine fault is raised). The one state an author reaches by starting a rule and not finishing it was the one state that said nothing at all — the exact silence objectui#4051 / objectstack#5149 ruled out for every other fault. Both spellings now report `[blank] the predicate is declared but empty — nothing to evaluate` through the same single reporting site as every other fault, on both channels (the built-in warning and the `onFault` passback, so the fault-probing callers that pass `warn: false` are not silenced either). Every verdict is unchanged, the envelope spelling included: `{ source: '' }` used to reach the engine and come back "AST-only evaluation not yet supported; persist `source`" and `{ source: ' ' }` "Unexpected token: EOF" — two misleading reasons for one author mistake, both already resolving to the same fallback this keeps. Blankness is decided by `isBlankPredicateText` (`evaluator/declaredPredicate.ts`), the repo's one definition of that question since objectui#3960, exported for this second consumer rather than copied — a fourth local `trim()` here is the drift it was consolidated to stop. For this one fault class the once-per-predicate dedupe key joins the caller's LOCATOR: a blank predicate has no distinguishing text, so every blank rule in an app shares the key `""` and the first would silence every other author's. Non-blank keys are unchanged. Part of objectui#8069. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01611D6ZaRaMmwTNQmSbk8MH --- ...069-field-rule-fault-direction-declared.md | 48 +++++ .../evaluator/__tests__/fieldRules.test.ts | 192 +++++++++++++++++- .../core/src/evaluator/declaredPredicate.ts | 10 +- packages/core/src/evaluator/fieldRules.ts | 95 +++++++-- 4 files changed, 320 insertions(+), 25 deletions(-) create mode 100644 .changeset/8069-field-rule-fault-direction-declared.md diff --git a/.changeset/8069-field-rule-fault-direction-declared.md b/.changeset/8069-field-rule-fault-direction-declared.md new file mode 100644 index 0000000000..9ff69391f0 --- /dev/null +++ b/.changeset/8069-field-rule-fault-direction-declared.md @@ -0,0 +1,48 @@ +--- +'@object-ui/core': minor +--- + +A field-rule predicate the author **declared and left blank** is no longer silent, and the +three fault directions in `resolveFieldRuleState` are now named and documented instead of +being bare positional booleans (objectui#8069). **No fallback value moves.** + +**The blank hole.** `''` and `' '` are authorable — `ExpressionWireSchema` is a bare +`z.string()` with no `.min(1)`, and `resolveFieldRuleState`'s own guard is `!= null`, so a +blank predicate passes both. `evalFieldPredicate` then returned the caller's fallback on its +first line, *before* `warnPredicateFailure` or `onFault` could fire. That is a third state, +not a spelling of either neighbour: the key is present (so it is not "the author wrote no +rule") and nothing evaluates (so no engine fault is raised). The one state an author reaches +by *starting* a rule and not finishing it was the one state that said nothing at all — which +is exactly what objectui#4051 / objectstack#5149 ruled out for every other fault. + +Both spellings now report `[blank] the predicate is declared but empty — nothing to evaluate` +through the same single reporting site as every other fault, on both channels (the built-in +`console.warn` and the `onFault` passback, so the fault-probing callers that pass +`warn: false` are not silenced either). **Every verdict is unchanged**, including the +envelope spelling: `{ source: '' }` used to reach the engine and come back +"AST-only evaluation not yet supported; persist `source`" and `{ source: ' ' }` +"Unexpected token: EOF" — two misleading reasons for one author mistake, both already +resolving to the same fallback this change keeps. Blankness is decided by +`isBlankPredicateText` (`evaluator/declaredPredicate.ts`), the repo's one definition of that +question since objectui#3960, now exported for this second consumer rather than copied. + +For this one fault class the once-per-predicate dedupe key joins the caller's **locator**: a +blank predicate has no distinguishing text, so every blank rule in an app shares the key +`""` and the first one would silence every other author's. Non-blank keys are unchanged. + +**The named directions.** `resolveFieldRuleState` passed `true` / `false` / `false` as bare +third arguments and answered the adjacent "no rule declared" case with the *same* literal — +so every permissive value was written twice, and the "the rule broke" answer was chosen by +aligning it with the "the rule is absent" answer beside it. Six module-private constants now +spell the two questions apart (`*_WHEN_FAULTED` / `*_WHEN_ABSENT`), with one docblock +recording the direction, the fact that all three point the permissive way so a single +mistyped column yields a form that shows more, locks less and demands less at once, and what +the history does and does not record about why (objectui#1578 and ADR-0036 both carry a +per-key "a fault is safe" rationale; no commit puts the case where all three faults arrive +from one typo). `evalFieldPredicate`'s docblock gains the call-site policy table — five +distinct fault policies share this one helper, two of which detect a fault by calling it +twice with *opposite* fallbacks and therefore depend on `fallback` staying freely +specifiable. + +Whether the direction belongs in the authored contract, and whether a loud-but-safe middle +should exist, remain open on objectui#8069. diff --git a/packages/core/src/evaluator/__tests__/fieldRules.test.ts b/packages/core/src/evaluator/__tests__/fieldRules.test.ts index e442ca23d9..19be61d89e 100644 --- a/packages/core/src/evaluator/__tests__/fieldRules.test.ts +++ b/packages/core/src/evaluator/__tests__/fieldRules.test.ts @@ -248,11 +248,15 @@ describe('failure diagnostics — loud fail-open (objectstack#5149, appeal 2)', expect(String(warn.mock.calls[0][0])).toContain(JSON.stringify(pred)); }); - it('does not warn for a healthy, absent, or blank predicate', () => { + // A BLANK predicate used to be listed here too — it was the negative + // baseline objectui#8069 measured, and it is now loud; the case moved to the + // blank-predicate describe below rather than being edited in place, because + // what it pinned is the branch that card removes. An ABSENT predicate keeps + // its silence: nothing was authored, so there is nothing to report. + it('does not warn for a healthy or absent predicate', () => { expect(evalFieldPredicate("record.ok_5149 == 'y'", { ok_5149: 'y' }, false)).toBe(true); expect(evalFieldPredicate(undefined, {}, true)).toBe(true); expect(evalFieldPredicate(null, {}, false)).toBe(false); - expect(evalFieldPredicate(' ', {}, true)).toBe(true); expect(warn).not.toHaveBeenCalled(); }); @@ -356,3 +360,187 @@ describe('failure diagnostics — loud fail-open (objectstack#5149, appeal 2)', expect(String(warn.mock.calls[0][0])).toContain('[throw] engine exploded'); }); }); + +/** + * objectui#8069, deliverable 1. The three fault directions are now named + * constants; these pin the VALUES through the public surface, so a future edit + * that moves one goes red here instead of shipping. ⛔ They are not an + * endorsement of the direction — that is the card's open question. Each case + * pairs the FAULTED verdict with the ABSENT one to keep visible the fact the + * card turns on: the two questions have equal answers today, and only because + * one was copied from the other. + */ +describe('fault directions — what a BROKEN rule decides (objectui#8069)', () => { + let warn: ReturnType; + beforeEach(() => { + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + afterEach(() => warn.mockRestore()); + + const broken = 'unbound_direction_8069 =='; + + it('a faulted visibleWhen SHOWS the field — same verdict as no visibleWhen at all', () => { + expect(resolveFieldRuleState({ visibleWhen: broken }, {}, {}).visible).toBe(true); + expect(resolveFieldRuleState({}, {}, {}).visible).toBe(true); + }); + + it('a faulted readonlyWhen leaves the field EDITABLE — same verdict as no readonlyWhen', () => { + expect(resolveFieldRuleState({ readonlyWhen: broken }, {}, {}).readonly).toBe(false); + expect(resolveFieldRuleState({}, {}, {}).readonly).toBe(false); + }); + + it('a faulted requiredWhen demands NOTHING — same verdict as no requiredWhen', () => { + expect(resolveFieldRuleState({ requiredWhen: broken }, {}, {}).required).toBe(false); + expect(resolveFieldRuleState({}, {}, {}).required).toBe(false); + }); + + it('all three compose from ONE broken predicate: shows more, locks less, demands less', () => { + // The card's thesis in one assertion — the three faults do not cancel. + expect( + resolveFieldRuleState( + { visibleWhen: broken, readonlyWhen: broken, requiredWhen: broken }, + {}, + {}, + ), + ).toEqual({ visible: true, readonly: false, required: false }); + }); +}); + +/** + * objectui#8069, deliverable 2. A predicate the author DECLARED and left blank + * is a third state: the key is present, so it is not "no rule", and nothing + * evaluates, so no engine fault is raised. It used to return the permissive + * fallback before any warning could fire — silent, which is the one thing + * objectui#4051 / objectstack#5149 ruled out. The VERDICT is deliberately + * unchanged; only the silence is. + * + * Every case below uses a UNIQUE context locator: for this class the locator + * joins the once-per-predicate dedupe key (a blank predicate has no + * distinguishing text), so a reused locator would spend its warning earlier. + */ +describe('blank predicate — declared but empty (objectui#8069)', () => { + let warn: ReturnType; + beforeEach(() => { + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + afterEach(() => warn.mockRestore()); + + it('a whitespace-only bare string warns instead of defaulting in silence', () => { + expect( + evalFieldPredicate(' ', {}, true, undefined, undefined, { context: 'blank_ws_8069' }), + ).toBe(true); + expect(warn).toHaveBeenCalledTimes(1); + const msg = String(warn.mock.calls[0][0]); + expect(msg).toContain('[blank]'); + expect(msg).toContain('blank_ws_8069'); + }); + + it('the empty-string spelling warns too', () => { + expect(evalFieldPredicate('', {}, true, undefined, undefined, { context: 'blank_empty_8069' })).toBe( + true, + ); + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0][0])).toContain('[blank]'); + }); + + it('the ENVELOPE spelling reports the same reason, not the engine parse fault it used to', () => { + // Before: `{ source: '' }` reached the engine and came back + // "AST-only evaluation not yet supported; persist `source`", and + // `{ source: ' ' }` came back "Unexpected token: EOF" — two different + // reasons for one author mistake, neither of them naming it. + expect( + evalFieldPredicate({ dialect: 'cel', source: ' ' }, {}, true, undefined, undefined, { + context: 'blank_envelope_8069', + }), + ).toBe(true); + const msg = String(warn.mock.calls[0][0]); + expect(msg).toContain('[blank]'); + expect(msg).not.toContain('AST-only'); + expect(msg).not.toContain('Unexpected token'); + }); + + it('the VERDICT is unchanged — a blank predicate still returns the caller\'s fallback either way', () => { + expect( + evalFieldPredicate(' ', {}, true, undefined, undefined, { context: 'blank_verdict_t_8069' }), + ).toBe(true); + expect( + evalFieldPredicate(' ', {}, false, undefined, undefined, { context: 'blank_verdict_f_8069' }), + ).toBe(false); + }); + + it('`onFault` carries the blank reason, so a caller that silenced the warning is not silenced', () => { + // The fault-probing callers (`evalCel`, `ExpressionEvaluator` under + // `throwOnError`) pass `warn: false` and print their own line; without the + // passback a blank predicate would stay silent for exactly those surfaces. + const reasons: string[] = []; + evalFieldPredicate(' ', {}, true, undefined, undefined, { + warn: false, + onFault: (r) => reasons.push(r), + }); + expect(warn).not.toHaveBeenCalled(); + expect(reasons).toHaveLength(1); + expect(reasons[0]).toContain('[blank]'); + }); + + it('two blank predicates at DIFFERENT locators each warn — text alone would silence the second', () => { + evalFieldPredicate('', {}, true, undefined, undefined, { context: 'blank_site_A_8069' }); + evalFieldPredicate('', {}, true, undefined, undefined, { context: 'blank_site_B_8069' }); + expect(warn).toHaveBeenCalledTimes(2); + }); + + it('the SAME blank predicate at the same locator still warns once across re-renders', () => { + const diag = { context: 'blank_rerender_8069' }; + evalFieldPredicate(' ', {}, true, undefined, undefined, diag); + evalFieldPredicate(' ', {}, true, undefined, undefined, diag); + evalFieldPredicate(' ', {}, true, undefined, undefined, diag); + expect(warn).toHaveBeenCalledTimes(1); + }); + + // ── controls: a control that reds when the subject reds is not a control ── + + it('control — an ABSENT predicate stays silent and keeps its verdict', () => { + expect(evalFieldPredicate(undefined, {}, true, undefined, undefined, { context: 'absent_8069' })).toBe( + true, + ); + expect(evalFieldPredicate(null, {}, false, undefined, undefined, { context: 'absent_8069' })).toBe( + false, + ); + expect(warn).not.toHaveBeenCalled(); + }); + + it('control — a HEALTHY predicate is silent and its verdict ignores the fallback', () => { + const pred = "record.state_8069 == 'paid'"; + expect(evalFieldPredicate(pred, { state_8069: 'paid' }, false)).toBe(true); + expect(evalFieldPredicate(pred, { state_8069: 'paid' }, true)).toBe(true); + expect(evalFieldPredicate(pred, { state_8069: 'draft' }, true)).toBe(false); + expect(evalFieldPredicate(pred, { state_8069: 'draft' }, false)).toBe(false); + expect(warn).not.toHaveBeenCalled(); + }); + + it('control — a genuinely BROKEN predicate still warns with the engine reason, not [blank]', () => { + const pred = 'still_faults_8069 =='; + expect(evalFieldPredicate(pred, {}, true)).toBe(true); + expect(warn).toHaveBeenCalledTimes(1); + const msg = String(warn.mock.calls[0][0]); + expect(msg).toContain('Reason: ['); + expect(msg).not.toContain('[blank]'); + }); + + it('resolveFieldRuleState: a blank visibleWhen is reported with its field locator, verdict unchanged', () => { + const state = resolveFieldRuleState( + { visibleWhen: ' ' }, + { status: 'x' }, + {}, + undefined, + undefined, + "field 'amount_8069'", + ); + // The `!= null` guard lets `''` / `' '` through — that is how the blank + // reached the permissive fallback in the first place. + expect(state.visible).toBe(true); + expect(warn).toHaveBeenCalledTimes(1); + const msg = String(warn.mock.calls[0][0]); + expect(msg).toContain("visibleWhen of field 'amount_8069'"); + expect(msg).toContain('[blank]'); + }); +}); diff --git a/packages/core/src/evaluator/declaredPredicate.ts b/packages/core/src/evaluator/declaredPredicate.ts index 24fef85432..5b19815ba5 100644 --- a/packages/core/src/evaluator/declaredPredicate.ts +++ b/packages/core/src/evaluator/declaredPredicate.ts @@ -25,8 +25,16 @@ import { toPredicateInput } from './predicateInput.js'; * return true`), `evalRowPredicate` (`listConditional.ts`) — brought to the one * place that answers "is there a condition at all?", so the two halves cannot * disagree about the same blank. + * + * Exported since objectui#8069 for its second consumer, `evalFieldPredicate` + * (`evaluator/fieldRules.ts`), which is the entry this docblock's list did NOT + * name because it did not apply the rule: it trimmed the STRING spelling only, + * so `{ dialect: 'cel', source: ' ' }` went to the engine and came back as a + * parse fault while `' '` returned the caller's fallback in silence. Reusing + * this definition rather than writing a fourth `trim()` is the whole point of + * the paragraph above. */ -function isBlankPredicateText(value: unknown): boolean { +export function isBlankPredicateText(value: unknown): boolean { if (typeof value === 'string') return value.trim() === ''; if (value !== null && typeof value === 'object') { const source = (value as { source?: unknown }).source; diff --git a/packages/core/src/evaluator/fieldRules.ts b/packages/core/src/evaluator/fieldRules.ts index ef553636db..c2d55198ba 100644 --- a/packages/core/src/evaluator/fieldRules.ts +++ b/packages/core/src/evaluator/fieldRules.ts @@ -62,10 +62,23 @@ * logs `treating the field as LOCKED` instead). The *default* stays * fail-open on purpose — flipping it is a shipped-behavior change tracked * separately in objectstack#5149 (appeal 1, undecided). + * + * A predicate the author DECLARED and left BLANK is loud too, since + * objectui#8069 — and it did not used to be. It is a third state, not a + * spelling of either neighbour: the key is present (so it is not "no rule") + * and nothing evaluates (so no engine fault is raised), and it used to return + * the caller's permissive fallback before any warning could fire. `''` and + * `' '` are authorable — `ExpressionWireSchema` is a bare `z.string()`, and + * `resolveFieldRuleState`'s own guard is `!= null` — so the one state an author + * reaches by *starting* a rule and not finishing it was the one state that said + * nothing at all. Both spellings now report `[blank]` through the same single + * site as every other fault; the VERDICT is unchanged for every input. */ import { ExpressionEngine } from '@objectstack/formula'; import type { Expression } from '@objectstack/spec'; +import { isBlankPredicateText } from './declaredPredicate.js'; + /** A field-rule predicate as authored in metadata. */ export type FieldRulePredicate = string | { dialect?: string; source: string }; @@ -109,12 +122,29 @@ export interface FieldPredicateDiagnostic { const warnedPredicates = new Set(); +/** + * The `reason` reported for a predicate the author DECLARED and left blank. + * + * Tagged like an engine reason (`[parse]`, `[type]`, `[throw]`) because it + * travels the same two channels — the built-in warning and the `onFault` + * passback — and a caller that routes on the tag must be able to tell this + * apart from a predicate that faulted with text in it: the fix for a blank one + * is to finish it or delete the key, never to debug its syntax. + */ +const BLANK_PREDICATE_REASON = '[blank] the predicate is declared but empty — nothing to evaluate'; + /** * One-time warning for a predicate that could not be evaluated. Deduped per * predicate TEXT (dialect + source): a broken predicate is re-evaluated on * every render/keystroke, and the point is one loud line, not a scrolling * wall. The dedupe key is JSON-encoded — never a control-character separator * (objectstack#5450 made a sibling of this file binary to grep that way). + * + * ⚠️ One class joins the LOCATOR to that key: a BLANK predicate. The text is + * what identifies the authoring site for every other fault — it carries the + * typo — but every blank predicate in an app shares the key `""`, so text + * alone would let the first blank rule silence every other author's. Non-blank + * keys are unchanged (objectui#8069). */ function warnPredicateFailure( expr: Expression, @@ -122,7 +152,9 @@ function warnPredicateFailure( reason: string, context?: string, ): void { - const key = JSON.stringify([expr.dialect, expr.source]); + const key = expr.source.trim() + ? JSON.stringify([expr.dialect, expr.source]) + : JSON.stringify([expr.dialect, expr.source, context ?? '']); if (warnedPredicates.has(key)) return; warnedPredicates.add(key); console.warn( @@ -174,10 +206,16 @@ function warnPredicateFailure( * * @param pred The `visibleWhen` / `readonlyWhen` / `requiredWhen` predicate. * @param record The live form values (overlays prior persisted record). - * @param fallback Value to return when the predicate is absent or fails to - * evaluate. Pick the *safe* default for the caller: - * `false` for readonly/required (don't lock/block on error), - * `true` for visibility (don't hide on error). + * @param fallback Value to return when the predicate is ABSENT (`null` / + * `undefined`), and — separately — the value returned when a + * present predicate cannot be evaluated. ⚠️ Those are two + * questions this one parameter answers with one value; a + * caller that wants different answers must branch before the + * call, as {@link resolveFieldRuleState} does. The historic + * advice is to pick the *safe* default (`false` for + * readonly/required — don't lock/block on error; `true` for + * visibility — don't hide on error); whether "safe" is the + * right axis is objectui#8069's open question. * @param previous The prior persisted record, if any (for `previous.*` refs). * @param scope Extra top-level scope variables bound alongside `record` — * e.g. `{ parent }` so an inline line-item cell can reference @@ -196,26 +234,39 @@ export function evalFieldPredicate( scope?: Record, diagnostic?: FieldPredicateDiagnostic, ): boolean { - if (pred == null || (typeof pred === 'string' && !pred.trim())) return fallback; + if (pred == null) return fallback; const expr = toExpression(pred); - // The two fault sources — a not-ok verdict and a throw that slipped past the - // engine's "never throws" contract — converge on one reason string and one - // reporting site below, so the built-in warning and the `onFault` passback - // can never describe the failure differently. + // The fault sources — a blank predicate, a not-ok verdict, and a throw that + // slipped past the engine's "never throws" contract — converge on one reason + // string and one reporting site below, so the built-in warning and the + // `onFault` passback can never describe the failure differently. let reason: string | undefined; let value = fallback; - try { - const res = ExpressionEngine.evaluate(expr, { - record, - previous, - ...(scope ? { extra: scope } : {}), - }); - // Parse error, type error, unbound identifier, engine fault … — every - // not-ok verdict resolves to the fallback, but never silently (#5149). - if (!res.ok) reason = `[${res.error.kind}] ${res.error.message}`; - else value = res.value === true; - } catch (err) { - reason = `[throw] ${err instanceof Error ? err.message : String(err)}`; + if (isBlankPredicateText(pred)) { + // Declared-and-blank, in EITHER spelling. Answered here rather than by the + // engine for two reasons: the bare-string spelling never reached the engine + // at all (this line used to `return fallback` before any warning could + // fire — objectui#8069), and the envelope spelling reached it and came back + // with `AST-only evaluation not yet supported; persist \`source\`` for + // `{ source: '' }`, a reason that sends the author looking for a + // serialization bug. `isBlankPredicateText` is the repo's one definition of + // this question (objectui#3960) — a fourth local `trim()` here is exactly + // the drift it was consolidated to stop. + reason = BLANK_PREDICATE_REASON; + } else { + try { + const res = ExpressionEngine.evaluate(expr, { + record, + previous, + ...(scope ? { extra: scope } : {}), + }); + // Parse error, type error, unbound identifier, engine fault … — every + // not-ok verdict resolves to the fallback, but never silently (#5149). + if (!res.ok) reason = `[${res.error.kind}] ${res.error.message}`; + else value = res.value === true; + } catch (err) { + reason = `[throw] ${err instanceof Error ? err.message : String(err)}`; + } } if (reason !== undefined) { if (diagnostic?.warn !== false) { From d104dc8b606fdb728f58ad0bc5b3dd9e2579ca55 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 21:41:41 +0000 Subject: [PATCH 3/3] fix(core): the blank dedupe key reads an OPTIONAL `Expression['source']` (objectui#8069) `tsc --noEmit` on a built workspace: `expr.source` is `string | undefined` in the spec, so calling `.trim()` on it is TS18048. The absent case is blank by the same rule as a whitespace-only one and lands in the same locator-joined key shape. Part of objectui#8069. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01611D6ZaRaMmwTNQmSbk8MH --- packages/core/src/evaluator/fieldRules.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/core/src/evaluator/fieldRules.ts b/packages/core/src/evaluator/fieldRules.ts index c2d55198ba..6c005f69a1 100644 --- a/packages/core/src/evaluator/fieldRules.ts +++ b/packages/core/src/evaluator/fieldRules.ts @@ -152,7 +152,9 @@ function warnPredicateFailure( reason: string, context?: string, ): void { - const key = expr.source.trim() + // `Expression['source']` is optional in the spec; an absent source is blank + // by the same rule as a whitespace-only one, and lands in the same key shape. + const key = expr.source?.trim() ? JSON.stringify([expr.dialect, expr.source]) : JSON.stringify([expr.dialect, expr.source, context ?? '']); if (warnedPredicates.has(key)) return;