From 7fbc7e52aa5f55d84243bb7182c167bd1f43b416 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 08:31:17 +0000 Subject: [PATCH 1/2] feat(spec): declare the settings `visible` grammar the evaluator actually implements (#7327) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both settings-manifest `visible` slots — specifier-level and manifest-level — were typed `ExpressionInputSchema`, whose bare-string arm normalises to `dialect: 'cel'`. Nothing has ever evaluated them as CEL: their only readers are the console's client-side `new Function(...)` and, since #7310, the server-side `evaluateVisibility`, which implements a small closed grammar. `===` / `!==` — used throughout the bundled manifests — are not CEL at all. #7169 measured which side should move: routing the declared CEL into evaluation breaks 93 of the 94 bundled predicates, narrowing the declaration breaks 1, and #7310's relational-operator extension had already taken that 1 to 0. Per the maintainer's 2026-08-10 ruling (and #7071's "each protocol keeps its own spelling"), the declaration moves. Both slots now accept exactly the evaluated grammar: single root `data`, one level of member access, `|| && !`, `=== !== == != >= <= > <`, parentheses and string/number/bool/null literals, optionally `${...}`-wrapped. Bare string and `{ dialect, source }` envelope are both still accepted and a bare string still normalises to the canonical envelope, so the wire shape does not move — only the accepted `source` strings narrow. Real CEL (`data.x in [...]`, `size(data.y) > 0`, `data.a.b == 1`) is refused at publish/parse with a message naming the offending source, the reason, and the grammar that would work. #7310's save-time refusal stays as defense in depth. A second statement of one grammar is the drift that caused #7169, so the two are pinned to each other: `settings-visibility-declaration.pin.test.ts` asserts "the schema accepts it" and "the evaluator can parse it" are the same bit, over an in/out-of-grammar table and over the real corpus — re-measured at 10 manifests / 94 predicates, 0 refused. Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_01VdPj3S347aPWapzTuHCb4N --- .../settings-visible-grammar-declared.md | 66 +++++ .../references/system/settings-manifest.mdx | 4 +- ...ettings-visibility-declaration.pin.test.ts | 163 ++++++++++++ .../spec/src/system/settings-manifest.test.ts | 90 +++++++ .../spec/src/system/settings-manifest.zod.ts | 231 +++++++++++++++++- 5 files changed, 549 insertions(+), 5 deletions(-) create mode 100644 .changeset/settings-visible-grammar-declared.md create mode 100644 packages/services/service-settings/src/settings-visibility-declaration.pin.test.ts diff --git a/.changeset/settings-visible-grammar-declared.md b/.changeset/settings-visible-grammar-declared.md new file mode 100644 index 0000000000..24f195baa7 --- /dev/null +++ b/.changeset/settings-visible-grammar-declared.md @@ -0,0 +1,66 @@ +--- +"@objectstack/spec": minor +--- + +feat(spec): the settings-manifest `visible` slots declare the grammar they are actually evaluated with, instead of claiming CEL (#7327, the alignment half of #7169) + +Both `visible` slots on a settings manifest — specifier-level and +manifest-level — were typed `ExpressionInputSchema`, the shared expression +input whose bare-string arm normalises to `dialect: 'cel'`. Nothing has ever +evaluated them as CEL. Their only two readers are the console's client-side +`new Function(...)` over the raw string and, since #7310, the server-side +`evaluateVisibility` in `@objectstack/service-settings`, which implements a +deliberately tiny closed grammar. So the declared dialect and the evaluated +dialect disagreed, and the disagreement was **not** cosmetic: `===` and `!==`, +which the bundled manifests use throughout, are not CEL operators at all. + +**The measurement decided which side moves.** #7169 counted the corpus — 94 +`visible` predicates across the 10 bundled manifests, 27 distinct sources. +Wiring the *declared* CEL into evaluation breaks **93 of 94**, syntactically +and totally, plus every manifest stored outside this repo. Narrowing the +*declaration* to the grammar already evaluated breaks **1**, and #7310's +relational-operator extension had already absorbed that one, taking it to +**0**. The maintainer's 2026-08-10 ruling took the second direction, and +#7071's ruling on `ExpressionInput` ("each protocol keeps its own spelling") +named this narrowing as the follow-up. + +**After:** both slots accept the grammar the evaluator implements and nothing +else — a single root `data` with one level of member access, the operators +`||` `&&` `!` and `===` `!==` `==` `!=` `>=` `<=` `>` `<`, parentheses, and +string / number / `true` / `false` / `null` literals, optionally wrapped in +`${…}`. A bare string and a `{ dialect, source }` envelope are both still +accepted, and a bare string still normalises to the canonical envelope, so +**the wire shape does not move** — only the set of accepted `source` strings +narrows. + +An author who reaches for real CEL is now told so where it is cheap to fix: + +``` +Unsupported `visible` predicate "data.provider in ['smtp', 'resend']": +unsupported identifier "in" — the only root is `data`. A settings `visible` +predicate is not CEL: … Rewrite CEL membership as an `||` chain +(`${data.x === 'a' || data.x === 'b'}`); function calls, macros and member +paths deeper than one level have no equivalent here. +``` + +Previously that predicate passed every publish-time gate and then failed the +tenant's next save — and before #7310, did not even fail: it silently switched +off `required`, `options`, `pattern`, `valueDomain` and the value window on its +key. #7310's save-time refusal stays exactly where it is, as defense in depth: +this is the producer-side check, that is the consumer-side check. + +**The two sides are pinned to each other**, because a second statement of one +grammar is exactly the drift that caused #7169 in the first place. +`service-settings/src/settings-visibility-declaration.pin.test.ts` asserts that +"the schema accepts it" and "the evaluator can parse it" are the same bit, over +an in-grammar / out-of-grammar table *and* over the real bundled corpus — which +it re-measures at 10 manifests / 94 predicates, 0 refused. + +**Upgrading:** every bundled manifest is unaffected (measured, 0 refusals). A +third-party manifest is affected only if it carries a `visible` predicate the +save path already could not evaluate; the refusal names the predicate, the +reason and the supported grammar. `minor` rather than `major` follows the +repo's precedent for narrowing acceptance on one authorable key +(`action-param-strict-unknown-keys`, `chart-aggregate-groupby-strict`) — this +removes no authorable surface with reachable behaviour, so it is not the +`major` class of #6188 / #6815. diff --git a/content/docs/references/system/settings-manifest.mdx b/content/docs/references/system/settings-manifest.mdx index 08be6df5dd..e85f25cc8d 100644 --- a/content/docs/references/system/settings-manifest.mdx +++ b/content/docs/references/system/settings-manifest.mdx @@ -87,7 +87,7 @@ const result = ResolvedSettingValueSchema.parse(data); | **category** | `string` | optional | Settings hub category | | **order** | `number` | optional | Display order | | **specifiers** | `{ type: Enum<'group' \| 'child_pane' \| 'info_banner' \| 'title_value' \| 'text' \| 'textarea' \| … +13 more>; id?: string; key?: string; label: string \| Record; … }[]` | ✅ | Page contents (ordered) | -| **visible** | `string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }` | optional | Whole-manifest visibility | +| **visible** | `string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }` | optional | Whole-manifest visibility. Grammar is NOT CEL: root `data` with one-level member access, `\|\|` `&&` `!`, `===` `!==` `==` `!=` `>=` `<=` `>` `<`, parentheses and string/number/bool/null literals, optionally wrapped in `${...}`; bare string or `{ dialect, source }` envelope. | | **featureFlag** | `string` | optional | Gate manifest visibility on a feature flag | | **beta** | `boolean` | optional | Show a Beta chip on the page | @@ -119,7 +119,7 @@ const result = ResolvedSettingValueSchema.parse(data); | **description** | `string` | optional | Help text | | **icon** | `string` | optional | Icon name (Lucide) | | **default** | `any` | optional | Default value | -| **visible** | `string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }` | optional | Visibility expression | +| **visible** | `string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }` | optional | Visibility expression evaluated against the namespace value map, e.g. `${data.provider === 'smtp'}`. Hidden specifiers are not rendered and their values are not validated. Grammar is NOT CEL: root `data` with one-level member access, `\|\|` `&&` `!`, `===` `!==` `==` `!=` `>=` `<=` `>` `<`, parentheses and string/number/bool/null literals, optionally wrapped in `${...}`; bare string or `{ dialect, source }` envelope. | | **required** | `boolean` | optional | Required field | | **encrypted** | `boolean` | optional | Encrypt value at rest (forced true for password) | | **scope** | `Enum<'global' \| 'tenant' \| 'user'>` | optional | Override manifest scope for this key | diff --git a/packages/services/service-settings/src/settings-visibility-declaration.pin.test.ts b/packages/services/service-settings/src/settings-visibility-declaration.pin.test.ts new file mode 100644 index 0000000000..110a2d1f64 --- /dev/null +++ b/packages/services/service-settings/src/settings-visibility-declaration.pin.test.ts @@ -0,0 +1,163 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +// +// The producer/consumer pin for settings `visible` (#7327, the alignment half +// of #7169). +// +// There are two checks on a `visible` predicate and they live in different +// packages. The DECLARATION — `SettingsManifestSchema` in `@objectstack/spec` +// — refuses an unsupported predicate at publish/parse. The EVALUATOR — +// `evaluateVisibility` in this package — refuses it at save (#7310, fail +// closed). Those two agreeing is not a nice-to-have: #7169 was caused by a +// producer and a consumer disagreeing about what parses, and answering it by +// adding a SECOND grammar statement would have reproduced the bug class if the +// two were left free to drift. +// +// So this file asserts the pairing directly, from the consumer side (spec +// cannot import a service — Prime Directive #2, the spec declares, it does not +// execute). Two facts: +// +// 1. The corpus. Every bundled manifest still parses. This is the #7169 +// measurement (94 predicates / 10 manifests) carried onto the declaration +// side: narrowing that refused even one of them would brick publishing of +// a builtin namespace, and the whole reason direction (b) was chosen over +// direction (a) is that this number is 0 rather than 93. +// 2. The agreement table. For a spread of in-grammar and out-of-grammar +// sources, "the schema accepts it" and "the evaluator can parse it" are +// the same bit. + +import { describe, it, expect } from 'vitest'; +import { SettingsManifestSchema, SpecifierSchema } from '@objectstack/spec/system'; +import { builtinSettingsManifests } from './manifests/index.js'; +import { evaluateVisibility, visibilitySource } from './visibility-eval.js'; + +/** Wrap a predicate in the smallest specifier that carries one. */ +const specifierWith = (visible: unknown) => ({ + type: 'text' as const, + key: 'some_key', + label: 'Some key', + visible, +}); + +const declarationAccepts = (visible: unknown): boolean => + SpecifierSchema.safeParse(specifierWith(visible)).success; + +const evaluatorParses = (visible: unknown): boolean => { + try { + evaluateVisibility(visible, {}); + return true; + } catch { + return false; + } +}; + +describe('settings `visible` — declaration ⇄ evaluator', () => { + it('the corpus: every bundled manifest parses against the narrowed declaration', () => { + // The #7169 corpus, on the declaration side. Expected refusals: 0. + const refusals: string[] = []; + for (const manifest of builtinSettingsManifests as Array>) { + const parsed = SettingsManifestSchema.safeParse(manifest); + if (!parsed.success) { + refusals.push(`${manifest.namespace}: ${parsed.error.issues.map((i) => `${i.path.join('.')} — ${i.message}`).join(' | ')}`); + } + } + expect(refusals).toEqual([]); + }); + + it('the corpus is still the size the ruling measured — 10 manifests, 94 predicates', () => { + // If a bundled manifest gains or loses a `visible`, this number moves and + // the reader is sent back to the measurement rather than trusting a stale + // one. It is a tripwire on the premise, not a rule about how many + // predicates a manifest may have — move it deliberately. + const predicates = (builtinSettingsManifests as Array>).flatMap((m) => [ + ...(typeof m.visible === 'undefined' ? [] : [m.visible]), + ...(m.specifiers ?? []).filter((s: any) => typeof s.visible !== 'undefined').map((s: any) => s.visible), + ]); + expect(builtinSettingsManifests).toHaveLength(10); + expect(predicates).toHaveLength(94); + }); + + it('every bundled predicate is accepted by BOTH sides', () => { + for (const manifest of builtinSettingsManifests as Array>) { + for (const spec of manifest.specifiers ?? []) { + if (typeof spec.visible === 'undefined') continue; + const where = `${manifest.namespace}.${spec.key ?? '(layout)'}: ${visibilitySource(spec.visible)}`; + expect(declarationAccepts(spec.visible), `declaration refused ${where}`).toBe(true); + expect(evaluatorParses(spec.visible), `evaluator refused ${where}`).toBe(true); + } + } + }); + + describe('the two sides answer the same bit', () => { + const IN_GRAMMAR = [ + // Every shape the bundled corpus actually uses. + "${data.provider === 'smtp'}", + "${data.provider !== 'memory'}", + '${data.email_password_enabled !== false}', + '${data.mfa_required === true}', + '${data.lockout_threshold > 0}', + "${data.provider === 'resend' || data.provider === 'postmark'}", + "${data.embedder_provider && data.embedder_provider !== 'none'}", + "${data.provider !== 'memory' && data.title_generation_enabled !== false}", + // Reachable but unused by the bundled set — declared is declared. + '${!data.disabled}', + '${data.count <= 10 && (data.a == 1 || data.b != null)}', + '${data.ratio >= 0.5}', + "${data.mode < 'z'}", + // Both accepted envelopes, and the unwrapped spelling. + "data.provider === 'smtp'", + { dialect: 'cel' as const, source: "${data.provider === 'smtp'}" }, + { dialect: 'cel' as const, source: "data.provider === 'smtp'" }, + ]; + + const OUT_OF_GRAMMAR = [ + // Real CEL an author would reasonably reach for — the case #7327 exists + // to move from a save-time failure to a publish-time one. + "${data.provider in ['smtp', 'resend']}", + '${size(data.recipients) > 0}', + "${data.provider.startsWith('s')}", + '${data.nested.field === 1}', + "${current_user.role === 'admin'}", + "${has(data.provider) ? 'a' : 'b'}", + // Malformed rather than mis-dialected. + "${data.provider === 'smtp'", + '${data.provider ===}', + "${data.a === 'x'} && ${data.b === 'y'}", + '${data.provider === }{', + '${-5 > data.x}', + ]; + + const labelled = (xs: unknown[]) => xs.map((v) => [JSON.stringify(v), v] as const); + + it.each(labelled(IN_GRAMMAR))('accepts %s on both sides', (_label, visible) => { + expect(evaluatorParses(visible)).toBe(true); + expect(declarationAccepts(visible)).toBe(true); + }); + + it.each(labelled(OUT_OF_GRAMMAR))('refuses %s on both sides', (_label, visible) => { + expect(evaluatorParses(visible)).toBe(false); + expect(declarationAccepts(visible)).toBe(false); + }); + }); + + it('the publish-time refusal prescribes the grammar instead of just naming a violation', () => { + const result = SpecifierSchema.safeParse(specifierWith("${data.provider in ['smtp', 'resend']}")); + expect(result.success).toBe(false); + const message = result.success ? '' : result.error.issues.map((i) => i.message).join('\n'); + // The offending source, why, and — the point of the card — what to write. + expect(message).toContain("data.provider in ['smtp', 'resend']"); + expect(message).toContain('unsupported identifier "in"'); + expect(message).toContain('not CEL'); + expect(message).toContain('`===`'); + expect(message).toContain('one-level member access'); + }); + + it('the manifest-level slot carries the same grammar as the specifier-level one', () => { + const base = { + namespace: 'demo', + label: 'Demo', + specifiers: [{ type: 'text' as const, key: 'some_key', label: 'Some key' }], + }; + expect(SettingsManifestSchema.safeParse({ ...base, visible: "${data.tier === 'pro'}" }).success).toBe(true); + expect(SettingsManifestSchema.safeParse({ ...base, visible: "${data.tier in ['pro']}" }).success).toBe(false); + }); +}); diff --git a/packages/spec/src/system/settings-manifest.test.ts b/packages/spec/src/system/settings-manifest.test.ts index da2713b8cd..edaeb09d67 100644 --- a/packages/spec/src/system/settings-manifest.test.ts +++ b/packages/spec/src/system/settings-manifest.test.ts @@ -302,6 +302,96 @@ describe('Specifier.valueDomain (#5933)', () => { }); }); +describe('`visible` — the settings visibility grammar (#7327)', () => { + const withVisible = (visible: unknown) => + SpecifierSchema.safeParse({ type: 'text', key: 'smtp_host', label: 'Host', visible }); + const firstMessage = (visible: unknown): string => { + const result = withVisible(visible); + return result.success ? '' : result.error.issues[0].message; + }; + + // The shapes the ten bundled manifests are actually written in. The + // cross-package half — that this schema and `evaluateVisibility` accept the + // same set, measured against the real corpus — is pinned from the consumer + // side, in `service-settings/src/settings-visibility-declaration.pin.test.ts`. + it.each([ + "${data.provider === 'smtp'}", + "${data.provider !== 'memory'}", + '${data.email_password_enabled !== false}', + '${data.mfa_required === true}', + '${data.lockout_threshold > 0}', + "${data.provider === 'resend' || data.provider === 'postmark'}", + "${data.embedder_provider && data.embedder_provider !== 'none'}", + '${!data.disabled}', + '${data.count <= 10 && (data.a == 1 || data.b != null)}', + '${data.ratio >= 0.5}', + ])('accepts %s', (visible) => { + expect(withVisible(visible).success).toBe(true); + }); + + it('accepts the unwrapped spelling and the `{ dialect, source }` envelope', () => { + expect(withVisible("data.provider === 'smtp'").success).toBe(true); + expect(withVisible({ dialect: 'cel', source: "${data.provider === 'smtp'}" }).success).toBe(true); + // An `ast`-only envelope is opaque at this layer — nothing to walk. + expect(withVisible({ dialect: 'cel', ast: { kind: 'opaque' } }).success).toBe(true); + }); + + it('normalises a bare string to the canonical envelope, unchanged by the narrowing', () => { + const parsed = SpecifierSchema.parse({ + type: 'text', key: 'smtp_host', label: 'Host', visible: "${data.provider === 'smtp'}", + }); + expect(parsed.visible).toEqual({ dialect: 'cel', source: "${data.provider === 'smtp'}" }); + }); + + // The point of #7327: this slot never was CEL, and an author who wrote CEL + // here used to be told so only by a failing tenant save (#7169 / PR #7310). + it.each([ + ["${data.provider in ['smtp', 'resend']}", 'unsupported identifier "in"'], + ['${size(data.recipients) > 0}', 'unsupported identifier "size"'], + ["${current_user.role === 'admin'}", 'unsupported identifier "current_user.role"'], + ['${data.nested.field === 1}', 'unsupported reference "data.nested.field"'], + ["${data.provider.startsWith('s')}", 'unsupported reference "data.provider.startsWith"'], + ])('refuses CEL %s', (visible, detail) => { + expect(withVisible(visible).success).toBe(false); + expect(firstMessage(visible)).toContain(detail); + }); + + it.each([ + ['${data.provider ===}', 'unexpected end of expression'], + ["${data.a === 'x'} && ${data.b === 'y'}", 'unexpected character "}"'], + ['${-5 > data.x}', 'unexpected character "-"'], + ["${(data.a === 'x'}", 'missing closing parenthesis'], + ["${data.a 'x'}", 'trailing tokens'], + ])('refuses malformed %s', (visible, detail) => { + expect(withVisible(visible).success).toBe(false); + expect(firstMessage(visible)).toContain(detail); + }); + + it('prescribes the grammar rather than only naming the violation', () => { + // Every part an author needs to rewrite the predicate without leaving the + // error: the offending source, the reason, and the grammar itself. + const message = firstMessage("${data.provider in ['smtp']}"); + expect(message).toContain("data.provider in ['smtp']"); + expect(message).toContain('unsupported identifier "in"'); + expect(message).toContain('not CEL'); + expect(message).toContain('one-level member access'); + expect(message).toContain('`===` `!==` `==` `!=` `>=` `<=` `>` `<`'); + expect(message).toContain("`${data.x === 'a' || data.x === 'b'}`"); + }); + + it('applies to the manifest-level slot too', () => { + const base = { + namespace: 'demo', + label: 'Demo', + specifiers: [{ type: 'text', key: 'smtp_host', label: 'Host' }], + }; + expect(SettingsManifestSchema.safeParse({ ...base, visible: "${data.tier === 'pro'}" }).success).toBe(true); + const refused = SettingsManifestSchema.safeParse({ ...base, visible: "${data.tier in ['pro']}" }); + expect(refused.success).toBe(false); + expect(refused.success ? [] : refused.error.issues[0].path).toEqual(['visible']); + }); +}); + describe('valueDomain membership definitions — the measurements service-settings must implement', () => { // These pin the TSDoc on `SpecifierValueDomainSchema`. `packages/spec` does // not enforce a domain (Prime Directive #2) — but the two halves have to agree diff --git a/packages/spec/src/system/settings-manifest.zod.ts b/packages/spec/src/system/settings-manifest.zod.ts index 2dfc25c422..c6815a188b 100644 --- a/packages/spec/src/system/settings-manifest.zod.ts +++ b/packages/spec/src/system/settings-manifest.zod.ts @@ -185,6 +185,220 @@ export const SpecifierValueDomainSchema = z.enum([ ]); export type SpecifierValueDomain = z.input; +// --------------------------------------------------------------------------- +// `visible` — the settings visibility grammar (NOT CEL) +// --------------------------------------------------------------------------- + +/** + * Comparison operators, longest-first so `>=` is never read as `>` plus a + * stray `=`, and `!==` / `!=` win over the unary `!`. Mirrors + * `COMPARISON_OPERATORS` in `visibility-eval.ts`. + */ +const VISIBILITY_COMPARISON_OPERATORS = ['===', '!==', '==', '!=', '>=', '<=', '>', '<'] as const; + +/** + * The prescription every refusal carries. Stated as the grammar rather than + * as "invalid", because the author's next question is always "then what DO I + * write?" — and the honest answer is short enough to print. + */ +const VISIBILITY_GRAMMAR_PRESCRIPTION = + 'A settings `visible` predicate is not CEL: it is read by the save-time evaluator in ' + + '`@objectstack/service-settings`, whose grammar is closed — a single root `data` with ' + + 'one-level member access (`data.some_key`), the operators `||` `&&` `!` and ' + + '`===` `!==` `==` `!=` `>=` `<=` `>` `<`, parentheses, and string / number / `true` / ' + + '`false` / `null` literals. The whole predicate may be wrapped in `${...}`, and both a ' + + 'bare string and a `{ dialect, source }` envelope are accepted. Rewrite CEL membership ' + + 'as an `||` chain (`${data.x === \'a\' || data.x === \'b\'}`); function calls, macros and ' + + 'member paths deeper than one level have no equivalent here.'; + +/** Thrown while walking a predicate; never escapes this module. */ +class VisibilityGrammarViolation extends Error {} + +const violate = (detail: string): never => { + throw new VisibilityGrammarViolation(detail); +}; + +/** + * Unwrap the three authored forms into the raw predicate source — the exact + * unwrapping `visibilitySource()` performs before evaluating. Mirroring it + * matters: `'${a} && ${b}'` starts with `${` and ends with `}`, so BOTH sides + * strip it to `a} && ${b` and refuse, rather than one side quietly repairing + * an expression the other cannot read. + */ +function unwrapVisibilitySource(value: unknown): string | undefined { + let src: string | undefined; + if (typeof value === 'string') src = value; + else if (value && typeof value === 'object' && typeof (value as { source?: unknown }).source === 'string') { + src = (value as { source: string }).source; + } + if (src === undefined) return undefined; + const trimmed = src.trim(); + if (trimmed.startsWith('${') && trimmed.endsWith('}')) return trimmed.slice(2, -1).trim(); + return trimmed; +} + +/** + * Leaves (`data.x`, `'lit'`, `42`, `true`) are collapsed to one token kind: + * the evaluator's `primary()` treats them identically, so distinguishing them + * here could only make this side accept a different set than that side. + */ +type VisibilityToken = { kind: 'punct'; value: string } | { kind: 'value' }; + +function tokenizeVisibility(expr: string): VisibilityToken[] { + const tokens: VisibilityToken[] = []; + let i = 0; + while (i < expr.length) { + const ch = expr[i]; + if (/\s/.test(ch)) { i++; continue; } + if (ch === '(' || ch === ')') { tokens.push({ kind: 'punct', value: ch }); i++; continue; } + const op = [...VISIBILITY_COMPARISON_OPERATORS, '&&', '||'].find(o => expr.startsWith(o, i)); + if (op) { tokens.push({ kind: 'punct', value: op }); i += op.length; continue; } + if (ch === '!') { tokens.push({ kind: 'punct', value: '!' }); i++; continue; } + if (ch === "'" || ch === '"') { + let j = i + 1; + while (j < expr.length && expr[j] !== ch) j += expr[j] === '\\' && j + 1 < expr.length ? 2 : 1; + if (j >= expr.length) violate('unterminated string'); + tokens.push({ kind: 'value' }); + i = j + 1; + continue; + } + if (/[0-9]/.test(ch)) { + const m = /^[0-9]+(\.[0-9]+)?/.exec(expr.slice(i))!; + tokens.push({ kind: 'value' }); + i += m[0].length; + continue; + } + if (/[A-Za-z_]/.test(ch)) { + const word = /^[A-Za-z_][A-Za-z0-9_.]*/.exec(expr.slice(i))![0]; + if (word === 'true' || word === 'false' || word === 'null') { + tokens.push({ kind: 'value' }); + } else if (word.startsWith('data.')) { + // One level only: `data.a.b` and `data.` are both out. + if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(word.slice('data.'.length))) { + violate(`unsupported reference "${word}" — the only root is \`data\`, with exactly one level of member access`); + } + tokens.push({ kind: 'value' }); + } else { + violate(`unsupported identifier "${word}" — the only root is \`data\``); + } + i += word.length; + continue; + } + violate(`unexpected character "${ch}"`); + } + return tokens; +} + +/** + * Parse-only walk of the same recursive descent `evaluateVisibility` runs: + * + * orExpr := andExpr ('||' andExpr)* + * andExpr := unary ('&&' unary)* + * unary := '!' unary | compare + * compare := primary (COMPARISON primary)? + * primary := '(' orExpr ')' | literal | data. + * + * Returns the failure detail, or `undefined` when the source is in grammar. + */ +function visibilityGrammarViolation(src: string): string | undefined { + try { + const tokens = tokenizeVisibility(src); + let pos = 0; + const eat = (value: string): boolean => { + const t = tokens[pos]; + if (t?.kind === 'punct' && t.value === value) { pos++; return true; } + return false; + }; + + const primary = (): void => { + const t = tokens[pos]; + if (!t) violate('unexpected end of expression'); + if (t.kind === 'punct' && t.value === '(') { + pos++; + orExpr(); + if (!eat(')')) violate('missing closing parenthesis'); + return; + } + if (t.kind !== 'value') violate(`unexpected token "${t.value}"`); + pos++; + }; + const compare = (): void => { + primary(); + const t = tokens[pos]; + if (t?.kind === 'punct' && (VISIBILITY_COMPARISON_OPERATORS as readonly string[]).includes(t.value)) { + pos++; + primary(); + } + }; + const unary = (): void => { if (eat('!')) unary(); else compare(); }; + const andExpr = (): void => { unary(); while (eat('&&')) unary(); }; + function orExpr(): void { andExpr(); while (eat('||')) andExpr(); } + + orExpr(); + if (pos !== tokens.length) violate('trailing tokens'); + return undefined; + } catch (err) { + if (err instanceof VisibilityGrammarViolation) return err.message; + throw err; + } +} + +/** + * `visible` on a settings manifest is **not** CEL, and this schema says so. + * + * Every other `visible` / `visibleWhen` in the spec is + * `ExpressionInputSchema` — a bare string normalised to `dialect: 'cel'` and + * handed to `@objectstack/formula`. The settings manifest slot never was: + * its only evaluators are the console's client-side `new Function(...)` over + * the raw string and, since #7169, the server-side `evaluateVisibility` in + * `packages/services/service-settings/src/visibility-eval.ts`, which + * implements a deliberately tiny closed grammar. The two spellings are not + * compatible in either direction — the bundled manifests are written with + * `===` / `!==`, which CEL does not have. + * + * #7169 measured the corpus and the maintainer ruled on 2026-08-10: 94 + * `visible` predicates across the 10 bundled manifests, of which routing them + * through CEL would break 93, while narrowing the declaration to the grammar + * actually evaluated breaks 0. So the declaration moves, not the evaluator + * (and not `ExpressionInputSchema`, which #7071 settled stays CEL-only — + * "each protocol keeps its own spelling"). + * + * What this buys: an author who writes real CEL (`data.x in ['a','b']`, + * `size(data.y) > 0`, `data.a.b == 1`) is refused **here**, at publish/parse, + * with a message naming the grammar that would work — instead of authoring a + * predicate that every gate accepts and that then fails the tenant's next + * save. PR #7310's save-time refusal stays as defense in depth: this slot is + * the producer-side check, that one is the consumer-side check, and they are + * pinned against each other in + * `packages/services/service-settings/src/settings-visibility-declaration.pin.test.ts`. + * + * The wire shape is untouched — bare string and `{ dialect, source }` + * envelope are both still accepted, and the bare string still normalises to + * the canonical envelope. Only the set of accepted `source` strings narrows. + * + * An `ast`-only envelope is passed through — the AST is opaque at this layer, + * and the evaluator reads `source`. + */ +const SettingsVisibilityInputSchema = ExpressionInputSchema.superRefine((value, ctx) => { + const source = unwrapVisibilitySource(value); + // `undefined` is an `ast`-only envelope; `''` is `"${}"`, which the + // evaluator answers with `true` rather than a parse error. Neither reaches + // the grammar walk there, so neither reaches it here. + if (!source) return; + const detail = visibilityGrammarViolation(source); + if (detail === undefined) return; + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: `Unsupported \`visible\` predicate "${source}": ${detail}. ${VISIBILITY_GRAMMAR_PRESCRIPTION}`, + }); +}); + +/** Shared `.describe()` — the grammar, named, on both slots that carry it. */ +const VISIBILITY_DESCRIBE_GRAMMAR = + 'Grammar is NOT CEL: root `data` with one-level member access, `||` `&&` `!`, ' + + '`===` `!==` `==` `!=` `>=` `<=` `>` `<`, parentheses and string/number/bool/null ' + + 'literals, optionally wrapped in `${...}`; bare string or `{ dialect, source }` envelope.'; + // --------------------------------------------------------------------------- // Specifier schema (the unit of UI in a manifest) // --------------------------------------------------------------------------- @@ -230,8 +444,14 @@ export const SpecifierSchema = lazySchema(() => z.object({ * Visibility expression evaluated against the live namespace value map * (e.g. "${data.provider === 'smtp'}"). Hidden specifiers are not * rendered AND their values are not validated. + * + * Not CEL — see {@link SettingsVisibilityInputSchema}. `visible` is the + * gate every other check on this specifier hangs off (`required`, + * `options`, `pattern`, `valueDomain`, the value window), so a predicate + * the evaluator cannot read is refused at parse rather than at save. */ - visible: ExpressionInputSchema.optional().describe('Visibility expression'), + visible: SettingsVisibilityInputSchema.optional() + .describe(`Visibility expression evaluated against the namespace value map, e.g. \`\${data.provider === 'smtp'}\`. Hidden specifiers are not rendered and their values are not validated. ${VISIBILITY_DESCRIBE_GRAMMAR}`), /** Mark the field required (renderer + server-side validation). */ required: z.boolean().default(false).describe('Required field'), @@ -487,8 +707,13 @@ export const SettingsManifestSchema = lazySchema(() => z.object({ /** The ordered list of specifiers that make up the page. */ specifiers: z.array(SpecifierSchema).min(1).describe('Page contents (ordered)'), - /** Visibility predicate for the whole manifest (e.g. license gate). */ - visible: ExpressionInputSchema.optional().describe('Whole-manifest visibility'), + /** + * Visibility predicate for the whole manifest (e.g. license gate). + * Same grammar as the specifier-level slot — not CEL, see + * {@link SettingsVisibilityInputSchema}. + */ + visible: SettingsVisibilityInputSchema.optional() + .describe(`Whole-manifest visibility. ${VISIBILITY_DESCRIBE_GRAMMAR}`), /** * Feature flag key that gates the manifest. When set, the renderer From 60b31207d973643dfdf86486fbd356bae57a1f08 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 08:56:27 +0000 Subject: [PATCH 2/2] fix(qa): keep the settings `visible` slot in the ADR-0058 expression ratchet, classified honestly (#7327) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The expression-surface conformance ratchet discovers surfaces by matching `: ExpressionInputSchema` textually, so narrowing the two settings `visible` slots onto their own schema dropped them out of the scan and turned their ledger entry stale — a live predicate surface silently leaving the ledger, which is the #1887 class the ledger exists to catch. Discovery now reads a registered list of expression-declaring schema names rather than one hardcoded name, with the failure mode written down: a slot narrowed onto its own schema must register that schema on the same commit. The classification is corrected while it is being moved. `settings-manifest visible` sat under `cel-ui` — `dialect: 'cel'`, enforced by the SchemaRenderer and celEngine — and is evaluated by neither. It gets its own `settings-visibility` row naming `evaluateVisibility`, its closed grammar, and its fail-closed policy (#7310), proved by the producer/consumer pin. `ExprDialect` gains a member for it: the ledger records what a surface IS, and spelling this one `cel` would restate in the ledger the exact claim #7327 removes from the schema. Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_01VdPj3S347aPWapzTuHCb4N --- .../test/expression-conformance.ledger.ts | 44 +++++++++++++++++-- .../test/expression-conformance.test.ts | 32 +++++++++++--- 2 files changed, 67 insertions(+), 9 deletions(-) diff --git a/packages/qa/dogfood/test/expression-conformance.ledger.ts b/packages/qa/dogfood/test/expression-conformance.ledger.ts index 0158116d75..069e9a4d94 100644 --- a/packages/qa/dogfood/test/expression-conformance.ledger.ts +++ b/packages/qa/dogfood/test/expression-conformance.ledger.ts @@ -15,13 +15,24 @@ import type { ConformanceRow } from '@objectstack/verify'; // the celEngine). There is NO silent fallback from compile to interpret. // // The companion test (`expression-conformance.test.ts`) RE-DISCOVERS every -// `ExpressionInputSchema` field declaration in `packages/spec/src` (plus the RLS +// expression-declaring field in `packages/spec/src` (plus the RLS // `using`/`check` string predicates) and asserts each is `covers`-ed by exactly // one row. A NEW expression surface that nobody classified — the #1887 class of -// "declared-but-unwired predicate" — breaks the build. +// "declared-but-unwired predicate" — breaks the build. Discovery is by SCHEMA +// NAME (`EXPRESSION_INPUT_SCHEMAS` in that file), so a slot narrowed onto its +// own schema must register that schema there or it drops out of the scan. export type ExprMode = 'compile' | 'interpret'; -export type ExprDialect = 'cel' | 'cron' | 'template' | 'js'; +/** + * What a surface is ACTUALLY evaluated as — deliberately not the spec's + * `ExpressionDialect` enum. `js` outlives its retirement from that enum + * (#3278), and `settings-visibility` never was in it: the settings manifest + * `visible` slots carry a closed hand-rolled grammar with its own evaluator + * (#7169 / #7327). A ledger that could only spell the three declared dialects + * would have to record the settings slot as `cel`, which is the exact + * misclassification it exists to surface. + */ +export type ExprDialect = 'cel' | 'cron' | 'template' | 'js' | 'settings-visibility'; export type ExprState = 'enforced' | 'experimental' | 'removed'; /** ADR-0058 D5 fail-policy tiers. */ export type FailPolicy = 'compile-error' | 'fail-closed' | 'fail-soft-log' | 'throw'; @@ -143,9 +154,34 @@ export const EXPRESSION_SURFACE: ExprSurface[] = [ // this new surface). Interpreted by the objectui page:tabs renderer to // omit the whole tab (header + panel) when FALSE. 'ui/component.zod.ts:visibleWhen', - 'system/settings-manifest.zod.ts:visible', + // `system/settings-manifest.zod.ts:visible` used to sit here. It never + // belonged: this row's dialect is `cel` and its enforcement is the + // SchemaRenderer + celEngine, and the settings slot is evaluated by + // neither. Split out as `settings-visibility` in #7327. ], }, + { + id: 'settings-visibility', + summary: 'settings-manifest `visible` — specifier + whole-manifest gating (a closed non-CEL grammar)', + // The one row in this ledger whose dialect is NOT one of the spec's three. + // That is the finding, not an oversight: the slot was typed + // `ExpressionInputSchema` — which labels its contents CEL — while its only + // two evaluators read a small hand-rolled grammar. Measured in #7169 over + // the 94 bundled predicates: routing them through CEL breaks 93 (`===` and + // `!==` are not CEL operators at all), so the maintainer's 2026-08-10 + // ruling moved the DECLARATION rather than the evaluator. Classifying this + // `cel` under `cel-ui` would restate here the exact claim #7327 removed + // from the schema. + dialect: 'settings-visibility', mode: 'interpret', state: 'enforced', failPolicy: 'fail-closed', + enforcement: + 'service-settings `evaluateVisibility` (visibility-eval.ts), called from `SettingsService.validatePatch` — a closed grammar: single root `data`, one-level member access, `|| && !`, `=== !== == != >= <= > <`, parens, and string/number/bool/null literals, optionally `${…}`-wrapped, as a bare string or a `{dialect, source}` envelope. Fail-closed since #7310: a predicate outside the grammar REFUSES the save (SettingsValidationError, HTTP 400) instead of skipping the specifier — `visible` gates every other check on the key (`required`, `options`, `pattern`, `valueDomain`, the value window), so skipping it switched all of them off at once. The console evaluates the same string client-side through `new Function(...)`. Since #7327 the spec DECLARES that same grammar (`SettingsVisibilityInputSchema`), so it is refused at publish/parse too', + covers: ['system/settings-manifest.zod.ts:visible'], + // Proof is the producer/consumer pin rather than a runtime fixture: the + // failure mode this surface actually has is the two sides disagreeing about + // what parses, which is what that file measures — over the real bundled + // corpus and an in/out-of-grammar table. + proof: 'packages/services/service-settings/src/settings-visibility-declaration.pin.test.ts', + }, { id: 'cel-action-param-option-visible', summary: "action param option-list per-option gating (params[].options[].visibleWhen, #5016)", diff --git a/packages/qa/dogfood/test/expression-conformance.test.ts b/packages/qa/dogfood/test/expression-conformance.test.ts index c59a1f914e..49e30abbe2 100644 --- a/packages/qa/dogfood/test/expression-conformance.test.ts +++ b/packages/qa/dogfood/test/expression-conformance.test.ts @@ -2,9 +2,12 @@ // // ADR-0058 D7 — the Expression Surface Conformance ledger is a CHECKED artifact. // Refactored onto the reusable ADR-0060 `checkLedger` helper: one call asserts -// the shared invariants AND the ratchet (re-discover every ExpressionInputSchema -// field in packages/spec/src + the RLS using/check predicates; fail if any is -// unclassified). The expression-specific invariants (mode/dialect/fail-policy, +// the shared invariants AND the ratchet (re-discover every expression-declaring +// field in packages/spec/src — see EXPRESSION_INPUT_SCHEMAS — plus the RLS +// using/check predicates; fail if any is unclassified). Discovery is by SCHEMA +// NAME, so a slot that moves to a narrower schema leaves the scan unless that +// schema is registered: #7327 is the worked example. The +// expression-specific invariants (mode/dialect/fail-policy, // compile rows name the canonical compiler) stay here. import { describe, expect, it } from 'vitest'; @@ -20,7 +23,26 @@ const SPEC_SRC = join(REPO_ROOT, 'packages/spec/src'); const MODES = new Set(['compile', 'interpret']); const FAIL_POLICIES = new Set(['compile-error', 'fail-closed', 'fail-soft-log', 'throw']); -const DIALECTS = new Set(['cel', 'cron', 'template', 'js']); +// `settings-visibility` is not one of the spec's `ExpressionDialect` members on +// purpose (#7327): it is a closed non-CEL grammar with its own evaluator, and +// the ledger's job is to say what a surface IS, not what its schema used to +// claim. See the `settings-visibility` row. +const DIALECTS = new Set(['cel', 'cron', 'template', 'js', 'settings-visibility']); + +/** + * Schemas that DECLARE an expression surface. `ExpressionInputSchema` is the + * shared one; a slot whose accepted grammar is narrower gets its own schema and + * must be listed here too, or the ratchet silently stops watching it. + * + * That is not hypothetical — it is how this scan behaves by construction, and + * #7327 hit it: narrowing the settings `visible` slots off `ExpressionInputSchema` + * dropped them out of discovery and turned their ledger entry stale. A new + * narrowed alias belongs in this list on the same commit that introduces it. + */ +const EXPRESSION_INPUT_SCHEMAS = ['ExpressionInputSchema', 'SettingsVisibilityInputSchema']; +const DECLARES_EXPRESSION = new RegExp( + String.raw`^\s*([a-zA-Z_][a-zA-Z0-9_]*)\s*:\s*(?:${EXPRESSION_INPUT_SCHEMAS.join('|')})\b`, +); /** Re-discover every expression surface in the spec — the SAME scan the ledger encodes. */ function discoverSurfaces(): Set { @@ -34,7 +56,7 @@ function discoverSurfaces(): Set { else if (ent.isFile() && ent.name.endsWith('.zod.ts')) { const rel = relative(SPEC_SRC, p); for (const line of readFileSync(p, 'utf8').split('\n')) { - const m = line.match(/^\s*([a-zA-Z_][a-zA-Z0-9_]*)\s*:\s*ExpressionInputSchema\b/); + const m = line.match(DECLARES_EXPRESSION); if (m) found.add(`${rel}:${m[1]}`); } }