From 10814ab114f9d72af7e67f8ad33f7ee296ca11d5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 09:41:03 +0000 Subject: [PATCH] fix(metadata-admin): diagnose a path on the right side of ==/!= in a visibility predicate (#4049) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The evaluator resolves paths only on the LEFT of ==/!=. The right side goes to parseLiteral, whose tail returns anything it does not recognise as a literal verbatim, so `data.a == data.b` compares against the string "data.b" and is false however equal the two sides are — silently. objectstack#6936's warning hangs on resolveValue, which the right side never enters. Option B per the ruling on #4049: a dev-mode warning at that tail when the returned text is path-shaped, ZERO semantic change. Verdicts pinned identical before and after. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- .changeset/wise-pumas-attack.md | 19 ++++ .../views/metadata-admin/predicate.test.ts | 104 ++++++++++++++++++ .../src/views/metadata-admin/predicate.ts | 102 +++++++++++++++-- 3 files changed, 213 insertions(+), 12 deletions(-) create mode 100644 .changeset/wise-pumas-attack.md diff --git a/.changeset/wise-pumas-attack.md b/.changeset/wise-pumas-attack.md new file mode 100644 index 0000000000..cfe8b8d79c --- /dev/null +++ b/.changeset/wise-pumas-attack.md @@ -0,0 +1,19 @@ +--- +'@object-ui/app-shell': patch +--- + +metadata-admin: diagnose a path on the right-hand side of `==` / `!=` in a visibility predicate + +The predicate evaluator resolves paths only on the LEFT of `==` / `!=`. The right-hand side goes +through `parseLiteral`, which hands back anything it does not recognise as a literal verbatim — so +`data.a == data.b` compares the value of `data.a` against the seven-character string `"data.b"` and +is false however equal the two sides are, with nothing in the console. objectstack#6936's +unresolved-path warning cannot see this: it hangs on `resolveValue`, which the right side never +enters. + +A dev-mode `console.warn` now fires when that tail returns something path-shaped (a dot-separated +identifier chain — the same grammar the left side accepts), naming the text, the predicate carrying +it, and the boundary. **No semantics change**: `data.a == data.b` still evaluates false, and the +before/after verdicts are pinned identical. The semantic fix belongs to publish-time predicate +validation (objectstack#7010) and to the real CEL runtime this file stands in for (ROADMAP M9), with +which this diagnostic retires. diff --git a/packages/app-shell/src/views/metadata-admin/predicate.test.ts b/packages/app-shell/src/views/metadata-admin/predicate.test.ts index c8235eab9b..c031a18e21 100644 --- a/packages/app-shell/src/views/metadata-admin/predicate.test.ts +++ b/packages/app-shell/src/views/metadata-admin/predicate.test.ts @@ -346,3 +346,107 @@ describe('the 16 vanished Studio sub-fields (objectstack#6936 reading)', () => { expect(warn).toHaveBeenCalled(); }); }); + +/* ── 7. a path on the RIGHT of ==/!= is a string literal — say so (#4049) ── */ + +/** + * objectui#4049. `parseLiteral`'s tail `return s` hands back any input it does + * not recognise as a literal, verbatim. The LEFT side of `==` / `!=` resolves + * paths; the RIGHT side does not — so `data.a == data.b` compares the value of + * `data.a` against the seven-character string "data.b" and is FALSE however + * equal the two sides are, with nothing in the console. objectstack#6936's + * warning cannot see this: it hangs on `resolveValue`, which the right side + * never enters (measured — 0 warnings for the whole truth table below). + * + * Maintainer/PM ruling on this card: option B ONLY — a dev-mode diagnostic at + * that tail, ZERO semantic change. The verdicts are therefore pinned IDENTICAL + * before and after (§7.3); the console merely stops being silent. + */ +describe('a path-shaped right-hand side is diagnosed, not resolved (objectui#4049)', () => { + /* 7.1 — it fires, and it names both halves */ + + it('`data.a == data.b` warns, naming the right-hand text and the predicate', () => { + expect(evaluatePredicate('data.a == data.b', scope({ a: 'x', b: 'x' }))).toBe(false); + expect(warn).toHaveBeenCalledTimes(1); + // Both halves or the warning sends nobody anywhere — same bar as #6936. + expect(warnings()).toContain('`data.b`'); + expect(warnings()).toContain('data.a == data.b'); + }); + + it('`!=` routes through the same tail', () => { + expect(evaluatePredicate('data.a != data.b', scope({ a: 'x', b: 'x' }))).toBe(true); + expect(warnings()).toContain('`data.b`'); + }); + + it('an UNQUOTED bare identifier fires — the A-direction regression shape', () => { + // `data.type == text` "works" today by accident: the row happens to hold the + // string "text". Option A (resolving the right side) would have flipped it + // to fail-open true. It stays exactly as it was — and stops being silent, so + // the accident is discoverable instead of load-bearing. + expect(evaluatePredicate('data.type == text', scope({ type: 'text' }))).toBe(true); + expect(warnings()).toContain('`text`'); + }); + + it('warns ONCE per (right-hand text, predicate) pair', () => { + for (let i = 0; i < 5; i++) evaluatePredicate('data.a == data.b', scope({ a: 'x', b: 'x' })); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it('but a different predicate carrying the same text gets its own warning', () => { + evaluatePredicate('data.a == data.b', scope({ a: 'x', b: 'x' })); + evaluatePredicate('data.c == data.b', scope({ b: 'x', c: 'x' })); + expect(warn).toHaveBeenCalledTimes(2); + }); + + it('the diagnostic is dev-mode only', () => { + const prev = process.env.NODE_ENV; + process.env.NODE_ENV = 'production'; + try { + expect(evaluatePredicate('data.a == data.b', scope({ a: 'x', b: 'x' }))).toBe(false); + expect(warn).not.toHaveBeenCalled(); + } finally { + process.env.NODE_ENV = prev; + } + }); + + /* 7.2 — controls: real literals stay silent */ + + it.each([ + ["a quoted string", "data.type == 'text'", { type: 'text' }], + ['a double-quoted string', 'data.type == "text"', { type: 'text' }], + ['a number', 'data.n == 42', { n: 42 }], + ['a negative number', 'data.n == -1', { n: -1 }], + ['a decimal', 'data.n == 1.5', { n: 1.5 }], + ['true', 'data.flag == true', { flag: true }], + ['false', 'data.flag == false', { flag: false }], + ['null', 'data.v == null', { v: null }], + ['an array', "data.type in ['text','number']", { type: 'text' }], + ['a quoted string that CONTAINS dots', "data.v == 'a.b.c'", { v: 'a.b.c' }], + ])('%s on the right side fires nothing', (_label, expr, row) => { + evaluatePredicate(expr as string, scope(row as Record)); + expect(warn).not.toHaveBeenCalled(); + }); + + it('a dotted NON-identifier is not called a path (grammar boundary)', () => { + // `1.2.3` reaches the same tail (via resolveValue's literal shortcut for + // digit-leading operands) and is likewise compared as text — but it is a + // malformed NUMBER, not a path. Reporting it as one would be a false claim, + // so the grammar is the dot-separated identifier chain the left side + // accepts, not "contains a dot". + expect(evaluatePredicate('data.v == 1.2.3', scope({ v: '1.2.3' }))).toBe(true); + expect(warn).not.toHaveBeenCalled(); + }); + + /* 7.3 — the zero-semantics proof: verdicts identical to pre-change */ + + it.each([ + ['data.a == data.b', { a: 'x', b: 'x' }, false], // equal values, still FALSE + ['data.a != data.b', { a: 'x', b: 'x' }, true], // equal values, still TRUE + ['data.a == data.b', { a: 'x', b: 'y' }, false], // right for the wrong reason + ["data.a == 'x'", { a: 'x', b: 'y' }, true], // literal control + ['data.type == text', { type: 'text' }, true], // unquoted, unchanged + ['data.a == data.b', { a: 'data.b' }, true], // the literal-text comparison itself + ])('%s over %j is still %s — the diagnostic changes no verdict', (expr, row, expected) => { + expect(evaluatePredicate(expr as string, scope(row as Record))).toBe(expected); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/predicate.ts b/packages/app-shell/src/views/metadata-admin/predicate.ts index cb003f781d..1315f5d0b0 100644 --- a/packages/app-shell/src/views/metadata-admin/predicate.ts +++ b/packages/app-shell/src/views/metadata-admin/predicate.ts @@ -80,6 +80,30 @@ * `false && ` is `false` and `true || ` is `true` — * the unresolvable half is short-circuited away, absorbed exactly as CEL absorbs * an erroring branch, and no warning fires because nothing needed it. + * + * ## A path on the RIGHT of `==` / `!=` is a string literal (objectui#4049) + * + * Only the LEFT side resolves. The right side goes to `parseLiteral`, whose tail + * hands back anything it does not recognise as a literal VERBATIM — so + * `data.a == data.b` compares the value of `data.a` against the seven-character + * string "data.b". It is FALSE however equal the two sides are, and + * `data.a != data.b` is correspondingly TRUE. The subset list above is honest + * about this (`path == 'literal'`, never `path == path`), but the boundary was + * enforced by silence: objectstack#6936's warning cannot reach it, because that + * one hangs on `resolveValue` and the right side never enters it. + * + * Ruling on objectui#4049: **option B only — a dev-mode diagnostic here, zero + * semantic change.** Resolving the right side (option A) was rejected: it would + * flip `data.type == text`, the unquoted-string spelling that works today by + * accident, into a fail-open `true`. The verdicts stay bit-for-bit what they + * were and are pinned that way in `predicate.test.ts` §7.3; all that changes is + * that the console stops being silent about the subset boundary. + * + * The semantic fix belongs to the producer — publish-time validation of + * predicate expressions (objectstack#7010) — and to the real CEL runtime: as the + * header says, this file is an interim stand-in for `@objectstack/formula`, so + * **this diagnostic retires with the file** when ROADMAP M9 lands CEL. Do not + * grow it into a second evaluator. */ export function evaluatePredicate( @@ -90,7 +114,7 @@ export function evaluatePredicate( const source = typeof expr === 'string' ? expr : expr.source; if (!source) return true; try { - return evalExpr(source.trim(), ctx); + return evalExpr(source.trim(), ctx, source); } catch (err) { // Fail-open either way; an unresolvable path additionally gets a name. if (err instanceof UnresolvedPathError) warnUnresolvedPath(err.path, source); @@ -119,9 +143,18 @@ class UnresolvedPathError extends Error { // `warnOnUnknownActionKeys` in `@object-ui/core` (`actions/actionKeys.ts`). const warnedUnresolvedPaths = new Set(); -/** Reset the warn-once memo. Exported for tests. */ +/** + * The same warn-once discipline for the right-hand-literal diagnostic + * (objectui#4049), keyed on (right-hand text, predicate) for the same reason: + * keyed on the text alone, a form with fifteen predicates comparing against + * `data.b` would report one of them and hide the rest. + */ +const warnedPathShapedLiterals = new Set(); + +/** Reset the warn-once memos. Exported for tests. */ export function resetPredicateWarnings(): void { warnedUnresolvedPaths.clear(); + warnedPathShapedLiterals.clear(); } const isDev = (): boolean => @@ -144,42 +177,79 @@ function warnUnresolvedPath(path: string, source: string): void { ); } +/** + * The identifier grammar the LEFT side accepts: a root identifier followed by + * dot-separated segments (`text`, `data.type`, `data.config.kind`) — i.e. the + * shape `resolveValue` would have resolved had this text been on the other side + * of the operator. + * + * Deliberately NOT "contains a dot". `1.2.3` also reaches `parseLiteral`'s tail + * (via the digit-leading literal shortcut in `resolveValue`) and is likewise + * compared as text, but it is a malformed NUMBER, not a path; announcing it as + * a path would be a false statement about the author's code. Quoted strings, + * numbers, booleans, null and arrays never reach the tail at all — they return + * from their own branches above. + */ +const PATH_SHAPED_LITERAL = /^[A-Za-z_$][A-Za-z0-9_$]*(?:\.[A-Za-z_$][A-Za-z0-9_$]*)*$/; + +function warnPathShapedLiteral(text: string, source: string): void { + if (!isDev()) return; + const memo = `${text}::${source}`; + if (warnedPathShapedLiterals.has(memo)) return; + warnedPathShapedLiterals.add(memo); + console.warn( + `[metadata-admin] visibility predicate \`${source}\` compares against \`${text}\`, which looks ` + + `like a path but is being used as the literal string "${text}" — this evaluator resolves ` + + 'paths only on the LEFT of `==` / `!=`; the right-hand side is always a literal (supported ' + + "subset: `path == 'literal'`). So `data.a == data.b` is FALSE even when both sides hold the " + + 'same value, and `data.a != data.b` is TRUE — the verdict does not depend on the right-hand ' + + "path at all. If you meant the text, quote it (`== 'text'`). If you meant to compare two " + + 'paths, that is outside this evaluator\'s subset: it is an interim stand-in for ' + + '`@objectstack/formula` until CEL lands (ROADMAP M9), and predicate expressions are ' + + 'validated at publish time (objectstack#7010). objectui#4049.', + ); +} + function evalExpr( expr: string, ctx: { data: Record }, + // The WHOLE predicate, threaded down unchanged so a diagnostic raised deep in + // a sub-expression can name the predicate the author actually wrote — the + // same pairing objectstack#6936's warning makes via UnresolvedPathError. + source: string, ): boolean { // Handle || (lowest precedence) const orParts = splitTopLevel(expr, '||'); if (orParts.length > 1) { - return orParts.some((p) => evalExpr(p.trim(), ctx)); + return orParts.some((p) => evalExpr(p.trim(), ctx, source)); } // Handle && const andParts = splitTopLevel(expr, '&&'); if (andParts.length > 1) { - return andParts.every((p) => evalExpr(p.trim(), ctx)); + return andParts.every((p) => evalExpr(p.trim(), ctx, source)); } // Handle negation if (expr.startsWith('!')) { - return !evalExpr(expr.slice(1).trim(), ctx); + return !evalExpr(expr.slice(1).trim(), ctx, source); } // Handle 'in' const inMatch = expr.match(/^(.+?)\s+in\s+(\[.*\])$/); if (inMatch) { - const left = resolveValue(inMatch[1].trim(), ctx); - const right = parseLiteral(inMatch[2]); + const left = resolveValue(inMatch[1].trim(), ctx, source); + const right = parseLiteral(inMatch[2], source); return Array.isArray(right) && right.includes(left as never); } // Handle == / != (CEL-style loose equality: null == undefined) const eqMatch = expr.match(/^(.+?)\s*(==|!=)\s*(.+)$/); if (eqMatch) { - const left = resolveValue(eqMatch[1].trim(), ctx); - const right = parseLiteral(eqMatch[3].trim()); + const left = resolveValue(eqMatch[1].trim(), ctx, source); + const right = parseLiteral(eqMatch[3].trim(), source); const nullish = (v: unknown) => v === null || v === undefined; const equal = nullish(left) && nullish(right) ? true : left === right; return eqMatch[2] === '==' ? equal : !equal; } // Bare truthy check - return Boolean(resolveValue(expr, ctx)); + return Boolean(resolveValue(expr, ctx, source)); } function splitTopLevel(expr: string, op: string): string[] { @@ -221,10 +291,11 @@ function splitTopLevel(expr: string, op: string): string[] { function resolveValue( path: string, ctx: { data: Record }, + source: string, ): unknown { // Allow literals on the left side too. if (/^['"]/.test(path) || /^-?\d/.test(path) || path === 'true' || path === 'false' || path === 'null') { - return parseLiteral(path); + return parseLiteral(path, source); } const segs = path.split('.'); // The root identifier must be a name the scope actually declares. `hasOwn`, @@ -245,7 +316,7 @@ function resolveValue( return cur; } -function parseLiteral(raw: string): unknown { +function parseLiteral(raw: string, source: string): unknown { const s = raw.trim(); if (s === 'true') return true; if (s === 'false') return false; @@ -266,5 +337,12 @@ function parseLiteral(raw: string): unknown { return []; } } + // The tail: `s` is not a literal this evaluator recognises, so it is handed + // back as itself and compared as text. Every route to this line carries the + // diagnostic (objectui#4049) — the right side of `==` / `!=`, and the + // digit-leading literal shortcut in `resolveValue` used by `in`'s left side, + // a bare truthy check and the left of `==`. NOTE the verdict is untouched: + // `s` is still returned verbatim, exactly as before. + if (PATH_SHAPED_LITERAL.test(s)) warnPathShapedLiteral(s, source); return s; }