diff --git a/.changeset/18554-expr-schema-hint-surface-roots.md b/.changeset/18554-expr-schema-hint-surface-roots.md new file mode 100644 index 0000000000..f1302ca576 --- /dev/null +++ b/.changeset/18554-expr-schema-hint-surface-roots.md @@ -0,0 +1,22 @@ +--- +"@objectstack/formula": minor +--- + +`ExprSchemaHint` gains `roots` — an authoring surface naming the binding roots it mounts beyond the platform baseline, so `validateExpression` can accept them without standing down on everything else (#18554). + +A page component's `visibleWhen` binds three roots at runtime, and `ExprSchemaHint` could express neither of the two shapes it needs: `scope: 'record'` refused `page.selectedProjectId != ''` — the worked example `packages/spec/src/ui/page.zod.ts`'s own `visibleWhen` describe ends with, under a sentence naming the contract-bound roots as `record`, `current_user` and page state as `page.` — and prescribed `record.page`, which names nothing on any layer; `scope: 'flattened'` accepted that example and accepted a bare `status == 'done'` with it, which is the shorthand the narrowing exists to catch. Downstream the refusal is not cosmetic: an editor that lints a page block on the `record` face disables Save for the author who wrote the platform's own documented spelling. + +```ts +validateExpression('predicate', "page.selectedProjectId != ''", { + scope: 'record', + roots: ['page'], // what this surface mounts beyond the baseline +}); // -> ok; `status == 'done'` at the same site is still an error +``` + +- **It only ever adds.** A root listed in `roots` is declared alongside `SCOPE_ROOTS`, never instead of it, so passing the key can turn a refusal into an acceptance and never the reverse — a caller adopting it cannot silently lose a check it has today, and a call site that does not pass it gets the verdict and the prescription it got before, byte for byte. +- **Declaring a root is not becoming permissive.** The bare-field shorthand, an undeclared root, and a typo of a declared root are all still hard errors at a surface that declares `page`. Trading a false refusal for a silent acceptance is the worse of the two directions, so the surface says *which* roots it binds rather than asking the validator to stop checking. +- **A mistyped root is sent to the root, not to `record.`.** When a surface has declared its roots, a namespace reference within edit distance of one of them (`pge.selectedProjectId`) is named as an unbound root and pointed at `page`. Every other shape — a bare value reference, a known field used as a JSON namespace, any site with no declared roots — keeps the existing `record.` prescription, which is the right fix for the case it was written for. +- **`introspectScope` advertises what the validator accepts.** Declared roots join the roots it hands an author, from the same declaration, so a root that is accepted is never one an author has no way to discover. +- **Not a closed-set mechanism.** A surface that must *refuse* a baseline root it never mounts still says so with `collectCelRootIdentifiers`, which reads the AST and is independent of this key. The two directions stay two mechanisms. + +Clause-②: yes (widening) diff --git a/packages/formula/src/validate-surface-roots.test.ts b/packages/formula/src/validate-surface-roots.test.ts new file mode 100644 index 0000000000..5e5ba57cdc --- /dev/null +++ b/packages/formula/src/validate-surface-roots.test.ts @@ -0,0 +1,212 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `ExprSchemaHint.roots` — an authoring surface naming the binding roots it + * mounts beyond the platform baseline, so the validator can accept them + * WITHOUT standing down on everything else. + * + * ## The defect this closes + * + * A page component's `visibleWhen` binds three roots at runtime. Before this + * key the hint could express two shapes and neither was that surface: + * + * - `scope: 'record'` refused `page.selectedProjectId != ''` — the example + * `packages/spec/src/ui/page.zod.ts`'s own `visibleWhen` + * describe ends with, under a sentence naming the + * contract-bound roots as `record`, `current_user` and + * 「page state as `page.`」 — and prescribed + * `record.page`, which names nothing on any layer. + * Downstream that refusal disables Save in the designer. + * - `scope: 'flattened'` accepted it, and accepted a bare `status` with it, + * which is the shorthand the narrowing exists to catch. + * + * ⛔ The repair is NOT "make the validator permissive at that surface": trading + * a false refusal for a silent acceptance is the worse of the two directions + * here. A surface declares WHICH roots it binds, and every other name keeps the + * verdict it had. + * + * ## What this file pins, and what it deliberately does not + * + * The assertions are about the NAMED SUBJECT of each verdict — which reference + * is judged, and which spelling is prescribed — never the sentence around it. + * `ExprValidationError` carries no code or status, so the prescribed spelling + * IS the machine-readable part of the contract for the two rows where a + * prescription is the defect; the rest of the wording is free to change. + */ + +import { describe, it, expect } from 'vitest'; + +import { SCOPE_ROOTS } from './cel-engine'; +import { introspectScope, validateExpression } from './validate'; + +/** + * The spec's own worked example for `PageComponent.visibleWhen`, copied from + * the `.describe()` in `packages/spec/src/ui/page.zod.ts` rather than invented + * here. It is the sharpest form of the defect: the one spelling the platform + * publishes for this key was the one the `record` scope refused. + */ +const SPEC_PAGE_EXAMPLE = "page.selectedProjectId != ''"; + +/** The bare-field shorthand a page block's `visibleWhen` must keep refusing. */ +const BARE_FIELD = "status == 'done'"; + +/** What the page surface mounts beyond the platform baseline. */ +const PAGE_SURFACE_ROOTS = ['page'] as const; + +describe('ExprSchemaHint.roots — a surface declaring the roots it binds', () => { + it('`page` is not in the platform baseline, and that is why the hint is needed', () => { + // Read from the list, never copied: if `page` is ever added to the baseline + // this row goes red and the premise below has to be re-argued. + expect(SCOPE_ROOTS as readonly string[]).not.toContain('page'); + expect(SCOPE_ROOTS as readonly string[]).toContain('current_user'); + }); + + describe("the card's repro — the spec's own `page.var` example", () => { + it('is refused under `record` scope with no declared roots (the defect)', () => { + const r = validateExpression('predicate', SPEC_PAGE_EXAMPLE, { scope: 'record' }); + expect(r.ok).toBe(false); + // The named subject is `page`, and the prescription is the meaningless one. + expect(r.errors[0].message).toContain('`page`'); + expect(r.errors[0].message).toContain('record.page'); + }); + + it('resolves once the surface declares `page` as one of its roots', () => { + const r = validateExpression('predicate', SPEC_PAGE_EXAMPLE, { + scope: 'record', + roots: PAGE_SURFACE_ROOTS, + }); + expect(r.ok).toBe(true); + expect(r.errors).toEqual([]); + expect(r.warnings).toEqual([]); + }); + }); + + describe('declaring a root does NOT make the surface permissive', () => { + it('still refuses the bare-field shorthand, with the `record.` prescription', () => { + const r = validateExpression('predicate', BARE_FIELD, { + scope: 'record', + roots: PAGE_SURFACE_ROOTS, + }); + expect(r.ok).toBe(false); + expect(r.errors[0].message).toContain('`status`'); + expect(r.errors[0].message).toContain('record.status'); + }); + + it('still refuses a root the surface did NOT declare', () => { + const r = validateExpression('predicate', "wizard.step == 2", { + scope: 'record', + roots: PAGE_SURFACE_ROOTS, + }); + expect(r.ok).toBe(false); + expect(r.errors[0].message).toContain('`wizard`'); + }); + + it('judges the declared root and the bare field in one source, refusing on the field', () => { + const r = validateExpression('predicate', `${SPEC_PAGE_EXAMPLE} && ${BARE_FIELD}`, { + scope: 'record', + roots: PAGE_SURFACE_ROOTS, + }); + expect(r.ok).toBe(false); + expect(r.errors[0].message).toContain('`status`'); + }); + }); + + describe('the refusal names the roots the surface does bind', () => { + it('sends a typo of a declared root to that root, not to `record.`', () => { + const r = validateExpression('predicate', "pge.selectedProjectId != ''", { + scope: 'record', + roots: PAGE_SURFACE_ROOTS, + }); + expect(r.ok).toBe(false); + const [{ message }] = r.errors; + expect(message).toContain('`pge`'); + expect(message).toContain('`page`'); + // ⛔ The prescription the card calls meaningless must not be the one an + // author is handed for a mistyped ROOT. + expect(message).not.toContain('record.pge'); + }); + + it('leaves a bare VALUE reference on the generic message even when roots are declared', () => { + // `pge` here is not written as a namespace, so nothing says it is a + // mistyped root rather than a mistyped field. + const r = validateExpression('predicate', "pge == 'x'", { + scope: 'record', + roots: PAGE_SURFACE_ROOTS, + }); + expect(r.ok).toBe(false); + expect(r.errors[0].message).toContain('record.pge'); + }); + + it('keeps `record.` for a known field used as a namespace (a JSON member)', () => { + const r = validateExpression('predicate', "address.city == 'SF'", { + scope: 'record', + roots: PAGE_SURFACE_ROOTS, + fields: ['address'], + }); + expect(r.ok).toBe(false); + expect(r.errors[0].message).toContain('record.address'); + }); + }); + + describe('every existing call site is unmoved', () => { + it.each([ + ['no hint at all', undefined], + ['`roots` absent', { scope: 'record' as const }], + ['`roots` empty', { scope: 'record' as const, roots: [] }], + ])('%s → the pre-existing verdict and prescription', (_label, schema) => { + const r = validateExpression('predicate', SPEC_PAGE_EXAMPLE, schema); + if (schema === undefined) { + // No `scope` ⇒ the bare-ref check does not run at all; unchanged. + expect(r.ok).toBe(true); + return; + } + expect(r.ok).toBe(false); + expect(r.errors[0].message).toContain('record.page'); + }); + }); + + describe('the flattened face', () => { + it('does not report a declared root as a typo of a near-miss field', () => { + // Without the declaration this warns 「did you mean `pages`?」 — advice on + // a root the surface really does bind. + const r = validateExpression('predicate', SPEC_PAGE_EXAMPLE, { + scope: 'flattened', + fields: ['pages', 'status'], + objectName: 'project', + roots: PAGE_SURFACE_ROOTS, + }); + expect(r.ok).toBe(true); + expect(r.warnings).toEqual([]); + }); + + it('still warns for that same near-miss when the root is NOT declared', () => { + const r = validateExpression('predicate', SPEC_PAGE_EXAMPLE, { + scope: 'flattened', + fields: ['pages', 'status'], + objectName: 'project', + }); + expect(r.warnings).toHaveLength(1); + expect(r.warnings[0].message).toContain('`pages`'); + }); + }); + + describe('introspection advertises what the validator accepts', () => { + it('adds the declared roots to the authoring vocabulary', () => { + const { roots } = introspectScope('predicate', { scope: 'record', roots: PAGE_SURFACE_ROOTS }); + expect(roots).toContain('page'); + expect(roots).toContain('record'); + }); + + it('is unchanged when no roots are declared', () => { + expect(introspectScope('predicate').roots).toEqual( + introspectScope('predicate', { scope: 'record' }).roots, + ); + expect(introspectScope('predicate').roots).not.toContain('page'); + }); + + it('does not duplicate a root that is already advertised', () => { + const { roots } = introspectScope('predicate', { roots: ['record', 'page'] }); + expect(roots.filter((r) => r === 'record')).toHaveLength(1); + }); + }); +}); diff --git a/packages/formula/src/validate.ts b/packages/formula/src/validate.ts index fb85ec3b5c..e751cb547c 100644 --- a/packages/formula/src/validate.ts +++ b/packages/formula/src/validate.ts @@ -80,6 +80,57 @@ export interface ExprSchemaHint { * did-you-mean *warning*. (Default.) */ scope?: 'record' | 'flattened'; + /** + * Binding roots THIS authoring surface mounts **beyond** the platform + * baseline (`SCOPE_ROOTS`) — the surface naming the roots it binds, so the + * validator keeps refusing every other one instead of standing down. + * + * ## What it is for + * + * `scope` answers "are the record's fields flattened here?". It cannot answer + * "what else is mounted here?", and some surfaces mount something else. A + * page component's `visibleWhen` is the worked case: `page.zod.ts`'s describe + * states its contract-bound roots as `record`, `current_user` **and page + * state as `page.`**, and ends with the example `page.selectedProjectId + * != ''`. `page` is not in the platform baseline and has no business being + * there — a hook condition or a validation rule binds nothing of the sort — + * so before this key the two faces of `scope` were the only choices, and + * neither is correct for that surface: + * + * - `'record'` refuses `page.selectedProjectId != ''` — the spec's own + * worked example — and prescribes `record.page`, which + * names nothing on any layer; + * - `'flattened'` accepts it, and accepts a bare `status` with it, which is + * the narrowing the surface wanted in the first place. + * + * Declaring `roots: ['page']` keeps the narrowing and stops the false + * refusal: `page.selectedProjectId != ''` resolves, `status == 'done'` is + * still the hard error it should be. + * + * ## Direction — this key only ADDS, and that is deliberate + * + * A root listed here is declared alongside the baseline, never instead of it: + * passing `roots` can only turn a refusal into an acceptance, never the + * reverse, so a caller that adopts it cannot silently lose a check it has + * today. The opposite direction — a surface that binds a **closed** set and + * must refuse a baseline root (`os`, `vars`) it never mounts — is NOT this + * key's job and must not be bolted onto it: that surface says so with + * `collectCelRootIdentifiers`, which reads the AST and is independent of any + * declaration here. `SCOPE_ROOTS`'s own docblock is the authority on why + * those are two mechanisms rather than one. + * + * ## Shape + * + * Plain declarative data — a list of names the caller already knows, not a + * resolver callback the validator would have to invoke. `@objectstack/lint`'s + * view/page gate already supplies exactly such a list to + * {@link firstUndeclaredReference} (`VIEW_PAGE_EXTRA_ROOTS`); this key is the + * same vocabulary reaching the shared validator, so a surface can declare its + * roots without dropping out of `validateExpression` — and losing the compile + * check, the braces hint, field existence, the role catalog and the + * type-soundness pass with it. + */ + roots?: readonly string[]; /** * ADR-0068 D4 — the closed catalog of valid role names (built-in + declared). * When supplied, a role-membership predicate testing a role NOT in this set @@ -507,6 +558,65 @@ function checkFieldExistence(source: string, schema: ExprSchemaHint | undefined, } } +/** + * Is `name` written as a NAMESPACE in `source` — `name.x`, `name?.x`, + * `name['x']`, `name.fn(…)` — rather than as a bare value (`name == 'x'`)? + * + * The distinction is what lets a refusal speak about roots at all: an author + * who wrote `pge.selectedProjectId` is treating `pge` as a namespace, and a + * bare value reference (`stat == 'done'`) is not that, whatever it is named. + */ +function isNamespaceUse(name: string, source: string): boolean { + // `name` reaches here from cel-js's `Unknown variable: X` capture, so it is + // `[A-Za-z_$][\w$]*` — `$` is the only regex metacharacter it can carry. + const escaped = name.replace(/\$/g, '\\$'); + return new RegExp(`(?` prescription exactly as they were. + * + * Three guards keep this from ever prescribing the wrong fix, and each one is + * load-bearing: + * + * - **No declared roots ⇒ null.** Every call site that does not pass + * `schema.roots` — which is all of them until one opts in — gets the message + * it got before, byte for byte. The message change rides entirely inside the + * new opt-in. + * - **A known field ⇒ null.** `record.` is the right fix for a bare + * field, JSON member access included (`address.city` → `record.address.city`), + * and a field named in `schema.fields` is exactly that case. + * - **Not a near-miss of a declared root ⇒ null.** Without a field catalog a + * namespace reference is genuinely ambiguous — a record field written bare, + * or a root that does not exist — and prescribing either one would be a + * guess. A name within the shared edit-distance threshold of a root the + * surface *does* mount is the one shape that is not ambiguous, so it is the + * only one that gets a second prescription. ⛔ Never widen this to emit both + * prescriptions at once: two findings pointing opposite ways is the shape + * the author cannot act on. + */ +function mistypedRootMessage( + bare: string, + source: string, + surfaceRoots: readonly string[], + fields: readonly string[] | undefined, +): string | null { + if (surfaceRoots.length === 0) return null; + if (fields?.includes(bare)) return null; + if (!isNamespaceUse(bare, source)) return null; + const suggestion = nearest(bare, surfaceRoots); + if (!suggestion) return null; + return ( + `unbound root \`${bare}\` — beyond the record this authoring surface binds ` + + `\`${surfaceRoots.join('`, `')}\`, and \`${bare}\` is none of them, so \`${bare}.…\` ` + + `resolves to nothing and the expression silently evaluates to null. ` + + `Did you mean \`${suggestion}\`?` + ); +} + /** Cheap edit-distance suggestion for typo'd field names. */ /** * The closest candidate to `name`, or `undefined` when nothing is close enough @@ -685,11 +795,18 @@ export function validateExpression( if (schema?.scope === 'record') { // In a `record`-scoped site a bare top-level identifier is a silent bug — // it must be `record.` (#1928). Hard error. - const bare = firstUndeclaredReference(source); + // + // `schema.roots` are declared alongside the platform baseline, so a root + // this surface really does mount is not read as a bare field: that is the + // whole of the accept-side change, and an absent/empty `roots` reproduces + // the previous call byte for byte. + const surfaceRoots = schema.roots ?? []; + const bare = firstUndeclaredReference(source, surfaceRoots); if (bare) { errors.push({ source, message: + mistypedRootMessage(bare, source, surfaceRoots, schema.fields) ?? `bare reference \`${bare}\` — a formula/validation expression binds the record as the ` + `\`record\` namespace, not at top level, so \`${bare}\` resolves to nothing and the ` + `expression silently evaluates to null. Write \`record.${bare}\`.`, @@ -710,7 +827,10 @@ export function validateExpression( // identifier is either a flow variable or a typo. When it is a near-miss // of a known field, warn (did-you-mean) WITHOUT failing the build — // a genuine flow variable won't be edit-distance-close to a field. (#1928) - const unknown = firstUndeclaredReference(source, schema.fields); + // `roots` joins the declared set here for the same reason it does in the + // `record` arm: a root the surface mounts is not a typo'd field, and + // suggesting one for it would be a did-you-mean nobody can act on. + const unknown = firstUndeclaredReference(source, [...schema.fields, ...(schema.roots ?? [])]); if (unknown) { const suggestion = nearest(unknown, schema.fields); if (suggestion) { @@ -746,6 +866,12 @@ function bracesHintForTemplate(source: string): string { * Introspect what an author (esp. an agent) may use in a field (Decision 1e): * the expected dialect, the in-scope field references, and the callable * functions. Feeds the authoring context so the model does not guess. + * + * A surface that declares `schema.roots` gets those roots ADVERTISED here as + * well as accepted by {@link validateExpression}. The two faces are fed from + * one declaration on purpose: a root the validator accepts but this list never + * names is a spelling the author has no way to discover, and a second hand-kept + * list is how the two drift apart. */ export function introspectScope(role: FieldRole, schema?: ExprSchemaHint): { dialect: 'cel' | 'template'; @@ -757,7 +883,10 @@ export function introspectScope(role: FieldRole, schema?: ExprSchemaHint): { return { dialect: expectedDialect(role), fields: [...(schema?.fields ?? [])], - roots: ['record', 'previous', 'input', 'os', 'current_user', 'user', 'vars'], + roots: [...new Set([ + 'record', 'previous', 'input', 'os', 'current_user', 'user', 'vars', + ...(schema?.roots ?? []), + ])], roles: [...(schema?.roleCatalog ?? [])], functions: CEL_STDLIB_FUNCTIONS, };