diff --git a/.changeset/6505-predicate-valued-gate-rules.md b/.changeset/6505-predicate-valued-gate-rules.md new file mode 100644 index 0000000000..67f1d4c700 --- /dev/null +++ b/.changeset/6505-predicate-valued-gate-rules.md @@ -0,0 +1,42 @@ +--- +'@object-ui/core': patch +--- + +Dev-mode `validateSchema` no longer reports every expression-valued `visible` / `disabled` +gate as an invalid schema (objectui#6505). `BASE_SCHEMA_RULES` declared both keys +`typeof value === 'boolean'`, so the exact authoring form the docs teach — +`{ "type": "button", "disabled": "${record.stage == 'closed'}" }` — printed +`disabled must be a boolean` and its host element got `data-obj-schema-invalid`, the cue +apps are told to hang a red outline off. + +**The accept set widens to what the protocol already declares and the runtime already +accepts, not beyond it.** `AGENTS.md` §4 declares both keys as expressions, `SchemaRenderer` +evaluates them through `hasDeclaredPredicate` + `evaluateCondition`, `@objectstack/spec` +normalizes every authored predicate into a `{ dialect, source }` envelope, and the +objectui#3862 / objectui#3955 rulings are entirely about which expression spellings count +as declared. This table was the one place in the repo that disagreed, so this restores +declared = enforced rather than changing a contract. + +**The rule still bites, and that half is pinned separately.** The two keys stay in +`BASE_SCHEMA_RULES`: a number, `null`, `{}`, an array, `''`, whitespace-only predicate text +and the empty / blank-`source` envelope (objectui#3960) are all still reported at their own +path with `INVALID_TYPE`. Every one of those was reported before this change too — the +accept set is a strict superset of the old one, so nothing that validated stops validating +and nothing refused becomes accepted. The message now names both halves of the accept set +instead of only the half that did not change. + +The verdict is delegated to `hasDeclaredPredicate` (`evaluator/declaredPredicate.ts`), the +repo's single definition of "is a predicate gate declared on this value?" +(objectui#3850's ruling), rather than answered a second time in the validator — a +hand-rolled twin that agrees today and drifts tomorrow is the defect class this rule was +already an instance of. `packages/core/src/validation/__tests__/predicate-valued-gate-rules.test.ts` +pins the delegation behaviourally: the rule's verdict must equal +`boolean || hasDeclaredPredicate(value)` across every probe in the file. + +The explicit boolean arm is kept even though `hasDeclaredPredicate` already subsumes it, so +the superset relationship is provable locally: a future narrowing on the declaredness side +cannot silently start reporting `disabled: false` — the most explicit gate an author can +write — as an invalid schema. + +The zod `safeValidateSchema` surface (`@object-ui/types/zod`, objectui#6318) is a different +validator and is untouched. diff --git a/packages/core/src/validation/__tests__/predicate-valued-gate-rules.test.ts b/packages/core/src/validation/__tests__/predicate-valued-gate-rules.test.ts new file mode 100644 index 0000000000..a488cb2dd9 --- /dev/null +++ b/packages/core/src/validation/__tests__/predicate-valued-gate-rules.test.ts @@ -0,0 +1,133 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#6505 — `BASE_SCHEMA_RULES` used to declare `visible` / `disabled` + * "must be a boolean", so every EXPRESSION-valued gate — the authoring form + * `AGENTS.md` §4 documents and `SchemaRenderer` evaluates — was reported as an + * invalid schema by the dev-mode validator. + * + * The two directions this file exists to hold apart, because a fix that only + * proves the first is indistinguishable from having DELETED the rule: + * + * 1. the false positive is gone — a declared predicate validates; + * 2. the rule STILL BITES — a value that is neither a boolean nor a declared + * predicate is still reported, at its own path, with `INVALID_TYPE`. + */ + +import { describe, it, expect } from 'vitest'; +import { validateSchema } from '../schema-validator'; +import { hasDeclaredPredicate } from '../../evaluator/declaredPredicate'; + +/** The two keys this card is about, asserted identically — they are one rule. */ +const GATE_KEYS = ['visible', 'disabled'] as const; + +/** Every error message the validator produced for `schema.`. */ +function gateMessages(key: string, value: unknown): string[] { + const result = validateSchema({ type: 'button', [key]: value }); + return result.errors.filter((e) => e.path === `schema.${key}`).map((e) => e.message); +} + +/** + * Predicate spellings the runtime ALREADY accepts — `toPredicateInput` folds + * none of them to `undefined`, so `evaluateCondition` reaches a verdict on each. + * Named individually rather than as "any string" so a narrowing of the accept + * set cannot pass by keeping one spelling alive. + */ +const DECLARED_PREDICATES: ReadonlyArray = [ + ['a bare CEL expression (the docs shorthand)', "record.stage == 'closed'"], + ['a `${…}` template string (AGENTS.md section 4 spelling)', "${record.stage == 'closed'}"], + ['a `cel` envelope (what `@objectstack/spec` normalizes to)', { dialect: 'cel', source: "record.stage == 'closed'" }], + ['a non-`cel` envelope (flattened onto the legacy path)', { dialect: 'template', source: '${record.locked}' }], +]; + +/** + * The OLD accept set. Kept as its own list, asserted separately, so this file + * pins that the fix WIDENS rather than moves: every value that validated before + * still validates. + */ +const BOOLEANS: ReadonlyArray = [ + ['`true`', true], + ['`false` — a verdict, not an absent gate (objectui#3812)', false], +]; + +/** + * Values that are neither a boolean nor a declared predicate. `hasDeclaredPredicate` + * answers `false` for each — the junk arm of objectui#3850's ruling — and the + * rule must keep reporting them. Every entry here was reported BEFORE this fix + * too: the accept set widens, and nothing that was refused becomes accepted. + */ +const NOT_A_GATE: ReadonlyArray = [ + ['a number', 0], + ['a truthy number', 1], + ['`null` — no predicate at all (objectui#3862)', null], + ['an empty object', {}], + ['an array', []], + ['an empty string (objectui#3492 / objectui#3842)', ''], + ['whitespace-only predicate text (objectui#3960)', ' '], + ['an empty `cel` envelope (objectui#3850)', { dialect: 'cel', source: '' }], + ['a blank-`source` envelope (objectui#3960)', { dialect: 'cel', source: ' ' }], +]; + +describe.each(GATE_KEYS)('#6505 — `%s` accepts every DECLARED predicate spelling', (key) => { + it.each(DECLARED_PREDICATES)('accepts %s', (_label, value) => { + expect(gateMessages(key, value)).toEqual([]); + }); + + it.each(BOOLEANS)('still accepts %s — the old accept set is a SUBSET of the new one', (_label, value) => { + expect(gateMessages(key, value)).toEqual([]); + }); + + it('validates the node this card is named after, end to end', () => { + const result = validateSchema({ type: 'button', [key]: "${record.stage == 'closed'}" }); + expect(result.valid).toBe(true); + // The message this card is named after, asserted by NAME rather than by + // count: `valid: true` alone would also go green on a deleted rule. + expect(result.errors.map((e) => e.message)).not.toContain(`${key} must be a boolean`); + }); +}); + +describe.each(GATE_KEYS)('#6505 — the `%s` rule STILL BITES', (key) => { + it.each(NOT_A_GATE)('reports %s', (_label, value) => { + const result = validateSchema({ type: 'button', [key]: value }); + expect(result.valid).toBe(false); + const errors = result.errors.filter((e) => e.path === `schema.${key}`); + expect(errors).toHaveLength(1); + expect(errors[0].code).toBe('INVALID_TYPE'); + // The message must name BOTH halves of the accept set, or an author reading + // it learns only the half that did not change. + expect(errors[0].message).toContain(key); + expect(errors[0].message).toContain('boolean'); + expect(errors[0].message).toContain('expression'); + }); + + it('the rule is still IN the table — a deleted rule reports nothing at all', () => { + // The forbidden option, pinned as a behaviour: dropping `visible` / + // `disabled` from `BASE_SCHEMA_RULES` makes this whole describe block go + // green by reporting NOTHING, so one cell asserts the rule fires at all. + expect(validateSchema({ type: 'button', [key]: 0 }).errors).not.toHaveLength(0); + }); +}); + +/** + * The drift pin. The repo owns ONE definition of "is this a declared + * predicate?" (objectui#3850) and this validator must not grow a second + * answer — a hand-rolled twin agreeing today and drifting tomorrow is the + * defect class this card belongs to. + */ +describe('#6505 — the validator delegates to `hasDeclaredPredicate`, it does not re-answer', () => { + const ALL = [...DECLARED_PREDICATES, ...BOOLEANS, ...NOT_A_GATE]; + + it.each(GATE_KEYS)('the `%s` verdict equals `boolean || hasDeclaredPredicate` for every probe', (key) => { + for (const [label, value] of ALL) { + const accepted = gateMessages(key, value).length === 0; + const expected = typeof value === 'boolean' || hasDeclaredPredicate(value); + expect({ label, accepted }).toEqual({ label, accepted: expected }); + } + }); +}); diff --git a/packages/core/src/validation/schema-validator.ts b/packages/core/src/validation/schema-validator.ts index 6f09ac68ad..e27f547b7d 100644 --- a/packages/core/src/validation/schema-validator.ts +++ b/packages/core/src/validation/schema-validator.ts @@ -17,6 +17,7 @@ */ import type { BaseSchema } from '@object-ui/types'; +import { hasDeclaredPredicate } from '../evaluator/declaredPredicate.js'; /** * One issue found while walking an ObjectUI schema TREE — `path` locates the @@ -53,6 +54,69 @@ export interface SchemaNodeValidationResult { warnings: SchemaNodeValidationError[]; } +/** + * The rule for a PREDICATE GATE key (`visible` / `disabled`) — objectui#6505. + * + * ## What this rule used to say, and what it cost + * + * Both keys read `typeof value === 'boolean'`, message `" must be a + * boolean"`. Both keys are EXPRESSIONS everywhere else in the system: AGENTS.md + * §4 declares the protocol as `hidden?: string; // expression` / + * `disabled?: string; // expression`, `SchemaRenderer` evaluates them through + * `hasDeclaredPredicate` + `evaluateCondition`, `@objectstack/spec` normalizes + * every authored predicate into a `{ dialect, source }` envelope, and the + * objectui#3862 / objectui#3955 rulings are entirely about which EXPRESSION + * spellings count as declared. So the node the docs teach — + * `{ type: 'button', disabled: "${record.stage == 'closed'}" }` — was reported + * `disabled must be a boolean` by the dev-mode validator and its host element + * got `data-obj-schema-invalid`, the cue apps hang a red outline off. + * + * `BASE_SCHEMA_RULES` was the ONE place in the repo that disagreed with the + * protocol, so this restores declared = enforced rather than widening a + * contract: the accept set moves to exactly what the runtime already accepts. + * + * ## Why `hasDeclaredPredicate` and not a check written here + * + * `evaluator/declaredPredicate.ts` is the repo's single definition of "is a + * predicate gate DECLARED on this value?" (objectui#3850's ruling; nothing in + * the repo asks that question anywhere else). A second, hand-rolled answer in + * this table is how the validator and the renderer come to disagree about the + * same value — which is the defect class this rule was already an instance of. + * The delegation is behavioural, so it is pinned behaviourally: the drift pin in + * `__tests__/predicate-valued-gate-rules.test.ts` asserts this rule's verdict + * equals `boolean || hasDeclaredPredicate(value)` across every probe. + * + * ## Why the boolean arm survives even though it is subsumed + * + * `hasDeclaredPredicate(true)` and `hasDeclaredPredicate(false)` are both + * `true` today (`toPredicateInput` returns a boolean unchanged), so the first + * arm changes no verdict. It is kept because it makes this rule's accept set a + * provable SUPERSET of the one it replaces, locally: a future narrowing on the + * declaredness side cannot silently start reporting `disabled: false` — the + * most explicit gate an author can write — as an invalid schema. + * + * ## What it still REFUSES (the half that is not negotiable) + * + * Everything `hasDeclaredPredicate` calls junk: a number, `null`, `{}`, an + * array, `''`, whitespace-only predicate text, and the empty / blank-`source` + * envelope (objectui#3960). Every one of those was refused before this change + * too — the accept set widens and nothing refused becomes accepted. Dropping + * the two keys from this table was the option this fix was explicitly forbidden + * to take: an absent rule reports nothing at all, which is gate weakening + * wearing the same green. + */ +function predicateGateRule(key: 'visible' | 'disabled') { + return { + required: false, + validate: (value: unknown): boolean => + typeof value === 'boolean' || hasDeclaredPredicate(value), + message: + `${key} must be a boolean or a declared predicate: an expression string ` + + `(bare, e.g. "record.stage == 'closed'", or the "\${...}" template ` + + `spelling), or a { dialect, source } expression envelope`, + }; +} + /** * Validation rules for base schema */ @@ -72,16 +136,10 @@ const BASE_SCHEMA_RULES = { validate: (value: any) => typeof value === 'string', message: 'className must be a string' }, - visible: { - required: false, - validate: (value: any) => typeof value === 'boolean', - message: 'visible must be a boolean' - }, - disabled: { - required: false, - validate: (value: any) => typeof value === 'boolean', - message: 'disabled must be a boolean' - } + // objectui#6505 — both keys are EXPRESSIONS in the protocol, not booleans. + // See `predicateGateRule` above for the accept set and what it still refuses. + visible: predicateGateRule('visible'), + disabled: predicateGateRule('disabled') }; /** diff --git a/packages/react/src/__tests__/SchemaRenderer.disabledGateFaultDiagnostic.test.tsx b/packages/react/src/__tests__/SchemaRenderer.disabledGateFaultDiagnostic.test.tsx index 37f67850ed..079a6f947b 100644 --- a/packages/react/src/__tests__/SchemaRenderer.disabledGateFaultDiagnostic.test.tsx +++ b/packages/react/src/__tests__/SchemaRenderer.disabledGateFaultDiagnostic.test.tsx @@ -141,19 +141,24 @@ const visibilityReports = (warn: WarnSpy): string[] => const allWarnings = (warn: WarnSpy): string[] => warn.mock.calls.map((c) => String(c[0])); /** - * The ONE pre-existing line a development build prints beside these - * diagnostics, and it is not ours. + * The line a development build could print beside these diagnostics without it + * being ours — `SchemaRenderer` runs `validateSchemaOnce` in dev only. * - * `SchemaRenderer` runs `validateSchemaOnce` in dev only, and core's - * `BASE_SCHEMA_RULES` declares `disabled` "must be a boolean" — so every - * EXPRESSION-valued `disabled`, the authoring form this whole card is about, - * is also reported as an invalid schema. That is a false positive predating - * this card and filed separately (objectui#6505); it is named here rather than - * absorbed into a loose assertion, because "the total was 2" would go equally - * green on a build that printed our line twice. + * HISTORY, because the numbers below moved with it (objectui#6505): when this + * file was written, core's `BASE_SCHEMA_RULES` declared `disabled` "must be a + * boolean", so every EXPRESSION-valued `disabled` — the authoring form this + * whole card is about — was ALSO reported as an invalid schema. That false + * positive is gone: the rule now accepts a declared predicate, and the dev + * build prints nothing here. Production was never affected — that validator is + * a no-op there — which is why the production cases below always pinned the + * RAW total. * - * Production is unaffected — that validator is a no-op there — which is why - * the production cases below can still pin the RAW total. + * The by-name subtraction stays, and is not now-redundant belt-and-braces: it + * is what makes every `nonValidatorWarnings` assertion in this file invariant + * under whatever core's validator decides to say. That property is why this + * file survived objectui#6505 with ONE number changed instead of a re-audit — + * "the total was 2" would have gone equally green on a build that printed our + * line twice, and would have said nothing about which line was which. */ const DEV_SCHEMA_VALIDATOR_NOISE = '[ObjectUI] Invalid schema detected:'; /** Everything printed that is NOT the known dev-only validator line. */ @@ -364,12 +369,18 @@ describe('#6445 group 1 — a faulting `disabled` gate is loud, and still disabl expect(disabledProp()).toBe('true'); expect(reports(warn)).toHaveLength(1); expect(reports(warn)[0]).toContain(FAULT_BARE_DEV); - // ONE fault, ONE line of ours. The dev build also prints core's - // `disabled must be a boolean` validator line for this very node - // (objectui#6505) — subtracted by NAME rather than by count, so a second - // copy of OUR line could never hide inside the allowance. + // ONE fault, ONE line of ours — subtracted by NAME rather than by count, + // so a second copy of OUR line could never hide inside the allowance. expect(nonValidatorWarnings(warn)).toHaveLength(1); - expect(allWarnings(warn)).toHaveLength(2); + // The RAW total was 2 when this cell was written: ours, plus core's + // `disabled must be a boolean` line for this very node. That second line + // was a FALSE POSITIVE — an expression-valued `disabled` is the authoring + // form the protocol declares — and objectui#6505 removed it, so the dev + // build now prints exactly one line here. The count is lowered to the + // measured reality and NOT loosened: this still pins that nothing else + // reaches the console, which is the whole reason the raw total is worth + // asserting beside the by-name one above. + expect(allWarnings(warn)).toHaveLength(1); }); }); });