diff --git a/.changeset/predicate-rhs-path-shaped.md b/.changeset/predicate-rhs-path-shaped.md new file mode 100644 index 0000000000..8dd0b603e0 --- /dev/null +++ b/.changeset/predicate-rhs-path-shaped.md @@ -0,0 +1,51 @@ +--- +"@objectstack/lint": minor +--- + +feat(lint): refuse a path-shaped right-hand side in a metadata-form predicate (#7659) + +The metadata-editing form renderer supports a declared subset of predicate +expressions in which the RIGHT side of `==` / `!=` is a **literal**, never a +resolved path. Only the left side resolves; the right side goes to a literal +parser whose tail hands back anything it does not recognise verbatim. So +`data.a == data.b` compares `data.a`'s value against the seven-character +**string** `"data.b"` — false however equal the two sides are, and +`data.a != data.b` correspondingly true. An `==` predicate written that way +hides the element on every row, and nothing says why. + +Nothing at the publish door could see it. #7010's `predicate-path-unresolved` +asks whether a path RESOLVES; `data.a == data.b` answers yes twice and walks +through. The renderer's own diagnostic (objectui#4049) is dev-mode only and +fires at render time — after the metadata is stored — so an AI author or a CI +pipeline publishing forms never sees it. + +**New rule — `predicate-rhs-path-shaped`**, a sibling of the two path-resolution +rules in `validate-predicate-path-refs.ts`, exported from the package root and +run by `os build` / `os lint` / `os validate` and at the runtime `view` publish +gate. It reports a `==` / `!=` right-hand side that is an unquoted identifier +chain, using the same grammar the renderer warns on +(`/^[A-Za-z_$][A-Za-z0-9_$]*(?:\.[A-Za-z_$][A-Za-z0-9_$]*)*$/`) — one grammar, +two enforcement points. The message names both sanctioned spellings: quote the +literal, or restructure so the path is on the left and a literal on the right. + +**Two severities, on one id:** + +- **`error` for a dotted chain** (`data.a == data.b`, or the same with the sides + swapped). Nobody writes a dotted identifier chain meaning the literal text of + it, so there is no reading under which this worked — the same bar + `predicate-path-unresolved` already gates on. +- **`warning` for a bare single word** (`status == active`). This one *works* + today: it compares against the literal string `"active"`, which is very likely + what the author meant, and the renderer's ruling preserved that deliberately. + It is outside the declared subset all the same and stops working when this + surface moves to the real CEL evaluator, so it is reported — but refusing a + `view` write over metadata that renders correctly would be a false build error. + +Measured over the shipped `METADATA_FORM_REGISTRY` (17 forms, 46 predicates): +**0** findings at either severity, with reverse verification (rewriting each +`== 'literal'` into `== data.__rhs__` reports all 45 comparisons). + +Deliberately unchanged: the two path-resolution rules. A predicate that is both +unresolvable and path-shaped on the right reports twice — both statements are +true and their fixes differ. `in`'s array parse is a distinct defect +(objectui#4266) and is not folded in. diff --git a/packages/lint/src/authoring-rules.ts b/packages/lint/src/authoring-rules.ts index 3610efe0b2..66f9aa218f 100644 --- a/packages/lint/src/authoring-rules.ts +++ b/packages/lint/src/authoring-rules.ts @@ -822,7 +822,9 @@ export const AUTHORING_RULES: readonly AuthoringRule[] = [ // in ONE edit, on the maintainer's 2026-08-10 ruling, sequenced after #4717's // `advisories` channel landed (PR #7435). Before that move a `view` written // through Studio / REST `/meta` / MCP — the only door most tenants have, and - // the door AI authors use — was judged by NONE of the family's six rule ids. + // the door AI authors use — was judged by NONE of the family's rule ids (six + // at the time of the move; seven since #7659 added + // `predicate-rhs-path-shaped` inside the second entry). // // They move together on purpose, and the two entries carry one comment because // they are one wall: #7214's implementer wired its own rule here alone and then @@ -870,6 +872,17 @@ export const AUTHORING_RULES: readonly AuthoringRule[] = [ // object's addressable path set is NOT closed (lookup traversal, system // columns, formula outputs), and an `error` gate over an open set generates // false build errors. See the rule's module note. + // + // #7659 adds a THIRD id here, `predicate-rhs-path-shaped`, which is not a + // resolution question at all: the metadata-admin renderer resolves paths only + // on the LEFT of `==` / `!=` and hands the right side to its literal parser, + // so `data.a == data.b` resolves both sides cleanly, passes the two rules + // above, and still compares against the string "data.b" — a constant verdict. + // It carries `error` on a dotted chain (no reading under which it worked) and + // `warning` on a bare word (`status == active` compares as the text today, so + // refusing it would fail a build over metadata that renders correctly). The + // per-finding severity is what gates, exactly as `lintFlowPatterns` has worked + // since #3760; the entry's `gating` tier is unchanged because it already was. { name: 'validatePredicatePathRefs', tier: 'gating', diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index 1b60a5f813..d3d7adb922 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -179,6 +179,7 @@ export { validatePredicatePathRefs, PREDICATE_PATH_UNRESOLVED, PREDICATE_PATH_UNROOTED, + PREDICATE_RHS_PATH_SHAPED, } from './validate-predicate-path-refs.js'; export type { PredicatePathFinding, diff --git a/packages/lint/src/runtime-gate.test.ts b/packages/lint/src/runtime-gate.test.ts index 1e24763bf7..0cb037edab 100644 --- a/packages/lint/src/runtime-gate.test.ts +++ b/packages/lint/src/runtime-gate.test.ts @@ -320,6 +320,40 @@ describe('the views[] visibility-predicate family at the runtime publish gate (# expect(errors.map((e) => e.rule)).toContain('predicate-path-unrooted'); }); + it('REFUSES a path on the RIGHT of `==` — which resolves cleanly and is broken anyway', () => { + // #7659, and the measurement that justifies the id existing: `data.type` and + // `data.label` are both keys of `FieldSchema`, so the rule directly above is + // silent here BY CONSTRUCTION. The renderer resolves only the left side and + // parses the right as a literal, so this compares against the string + // "data.label" and is false on every row. + const { errors } = gateView(schemaBoundForm('data.type == data.label')); + const f = errors.find((e) => e.rule === 'predicate-rhs-path-shaped'); + expect(f, 'the right-hand position never evaluates a path').toBeDefined(); + expect(f!.severity).toBe('error'); + expect( + errors.map((e) => e.rule), + "#7214's check has nothing to say here — that silence is why this rule exists", + ).not.toContain('predicate-path-unresolved'); + }); + + it('sends a BARE unquoted word down the advisory channel, not the refusal one', () => { + // The second severity the same id carries. `== active` is compared as the + // literal string "active" today — very likely what the author meant — so + // this rule does not refuse the write over metadata that renders correctly. + // + // The write IS refused, by `visibility-bare-identifier` from the sibling + // file, which reads `active` as a dropped binding root. Both findings are + // true about the token and they prescribe DIFFERENT fixes (`data.active` vs + // `'active'`), so this pins the pair rather than asserting a clean `errors` + // list that would go stale the moment either side moved. + const result = gateView(schemaBoundForm('data.type == active')); + const f = result.advisories.find((a) => a.rule === 'predicate-rhs-path-shaped'); + expect(f, 'the subset boundary must still reach the author').toBeDefined(); + expect(f!.severity).toBe('warning'); + expect(result.errors.map((e) => e.rule)).not.toContain('predicate-rhs-path-shaped'); + expect(result.errors.map((e) => e.rule)).toContain('visibility-bare-identifier'); + }); + it('reports a MISLAYERED root through the advisory channel, not a refusal', () => { // The one family member that is `warning` on every surface, so it must not // 422 — and must not be silent either. #4717's `advisories` channel (the diff --git a/packages/lint/src/validate-predicate-path-refs.test.ts b/packages/lint/src/validate-predicate-path-refs.test.ts index 577376b50a..1a5bbc1133 100644 --- a/packages/lint/src/validate-predicate-path-refs.test.ts +++ b/packages/lint/src/validate-predicate-path-refs.test.ts @@ -1,7 +1,8 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * Tests for the #7010 predicate PATH-resolution gate. + * Tests for the #7010 predicate PATH-resolution gate, and for #7659's sibling + * rule about the SHAPE of a `==` / `!=` right-hand side. * * The load-bearing block is `#6254 corpus` at the bottom. Everything above it is * unit coverage over a hand-built schema; that block runs the rule over the @@ -20,6 +21,7 @@ import { validatePredicatePathRefs, PREDICATE_PATH_UNRESOLVED, PREDICATE_PATH_UNROOTED, + PREDICATE_RHS_PATH_SHAPED, } from './validate-predicate-path-refs.js'; import { AUTHORING_RULES } from './authoring-rules.js'; @@ -261,6 +263,120 @@ describe('validatePredicatePathRefs — traversal reach', () => { }); }); +// ──────────────────────────────────────────────────────────────────────────── +// #7659 — the RIGHT-hand side of `==` / `!=` +// ──────────────────────────────────────────────────────────────────────────── +// +// A different question from everything above, and the reason it needed its own +// rule rather than a widening of #7214: `data.a == data.b` RESOLVES on both +// sides, so both rules above are silent on it by construction. The first two +// tests pin that pair — the silence, and a control proving the walk that went +// silent can still see. +describe('validatePredicatePathRefs — path-shaped right-hand side (#7659)', () => { + const rhs = (source: string) => + run(form([{ label: 'S', fields: [{ field: 'name', visibleWhen: source }] }])); + + it('reports a path on the RIGHT of `==` — the case #7214 is silent on', () => { + // Both sides resolve against DemoSchema (`name`, `type`), so neither + // resolution limb has anything to say. Asserted as an EQUALITY on the rule + // set rather than a `toContain`: if a resolution limb ever started firing + // here, this card's premise would be gone and the test must say so. + const findings = rhs('data.name == data.type'); + expect(findings.map((f) => f.rule)).toEqual([PREDICATE_RHS_PATH_SHAPED]); + expect(findings[0].severity).toBe('error'); + expect(findings[0].path).toBe('views[0].sections[0].fields[0].visibleWhen'); + expect(findings[0].message).toContain('`data.type`'); + expect(findings[0].message).toMatch(/literal string "data\.type"/); + expect(findings[0].hint).toMatch(/quote it/); + }); + + it('proves that silence is #7214 unable to SEE this, not a walk that reports nothing', () => { + // Same predicate, one segment misspelled: the resolution limb must wake up. + // Without this, the assertion above could be measuring a dead walk. + expect(rhs('data.name == data.tpye').map((f) => f.rule).sort()) + .toEqual([PREDICATE_PATH_UNRESOLVED, PREDICATE_RHS_PATH_SHAPED]); + }); + + it('reports `!=` the same way', () => { + const findings = rhs('data.name != data.type'); + expect(findings.map((f) => f.rule)).toEqual([PREDICATE_RHS_PATH_SHAPED]); + expect(findings[0].message).toContain('`!=`'); + }); + + it('reports a path on the right even when the LITERAL is on the left', () => { + // Sides swapped: the renderer resolves the left and parses the right, so + // this is the same defect and the fix is to swap them back. + expect(rhs("'grid' == data.type").map((f) => f.rule)).toEqual([PREDICATE_RHS_PATH_SHAPED]); + }); + + it('reports a BARE unquoted word, at `warning` — it works today by accident', () => { + // `... == active` compares against the literal string "active", which is + // very likely what the author meant; objectui#4049's ruling preserved that + // deliberately. Reported, because it is outside the declared subset and dies + // when CEL lands; not gated, because refusing a `view` write over metadata + // that renders correctly is a false build error. + const findings = rhs('data.name == active'); + expect(findings.map((f) => f.rule)).toEqual([PREDICATE_RHS_PATH_SHAPED]); + expect(findings[0].severity).toBe('warning'); + expect(findings[0].message).toContain('`active`'); + }); + + it('reports every comparison in a compound predicate', () => { + const findings = rhs("data.name == data.type && data.type == 'formula' || data.name != data.type"); + expect(findings.map((f) => f.rule)).toEqual([ + PREDICATE_RHS_PATH_SHAPED, + PREDICATE_RHS_PATH_SHAPED, + ]); + }); + + // ── Negative controls. Each is either a spelling the rule's own hint + // recommends, or a literal form `parseLiteral` returns from BEFORE its + // path-shaped tail — so the renderer is silent on it too. + it.each([ + ['a quoted literal RHS', "data.type == 'formula'"], + ['a double-quoted literal RHS', 'data.type == "formula"'], + ['a numeric RHS', 'data.name == 3'], + ['a negative numeric RHS', 'data.name != -3'], + ['a boolean RHS', 'data.enable.search == true'], + ['a boolean RHS, negated', 'data.enable.search != false'], + ['a null RHS', 'data.type == null'], + // The restructuring the hint recommends — PATH on the left, literal on the + // right. If the message tells authors to write this, writing it must not be + // reported, or the rule sends them in a circle. + ['the sanctioned restructuring (path LEFT, literal right)', "data.type != 'formula'"], + ['a bare truthy check with no comparison at all', 'data.enable.search'], + ['an `in` membership test (objectui#4266, deliberately not folded in)', "data.type in ['a','b']"], + // Reachable through `parseLiteral`'s tail in the renderer, but NOT + // path-shaped under the shared grammar — the consumer stays silent on both. + ['an indexed RHS', 'data.type == data.rows[0]'], + ['a call-result RHS', 'data.type == size(data.tags)'], + ])('is silent on %s', (_label, source) => { + expect(rhs(source)).toEqual([]); + }); + + it('leaves a comprehension macro BODY alone, and still walks its receiver', () => { + // The interim evaluator supports no macros at all, so a comparison inside + // one is not a statement about this subset. The receiver is still walked — + // the second case carries a real finding in the same predicate. + expect(rhs('data.tags.all(t, t == data.type)')).toEqual([]); + expect(rhs('data.name == data.type && data.tags.all(t, t == data.type)').map((f) => f.rule)) + .toEqual([PREDICATE_RHS_PATH_SHAPED]); + }); + + it('gives no verdict on a source the canonical front end refuses', () => { + // `$` is in the shared grammar and NOT in CEL's identifier syntax, so this + // never parses. One broken predicate, one finding — and that one is + // `visibility-predicate-syntax`'s (#6253), from the sibling file. + expect(rhs('data.name == $b')).toEqual([]); + }); + + it('emits the id the published barrel exports', async () => { + const barrel = await import('./index.js'); + expect(barrel.PREDICATE_RHS_PATH_SHAPED).toBe('predicate-rhs-path-shaped'); + expect(rhs('data.name == data.type')[0].rule).toBe(barrel.PREDICATE_RHS_PATH_SHAPED); + }); +}); + describe('registry wiring', () => { it('is registered in AUTHORING_RULES as a gating rule on all three commands', () => { const entry = AUTHORING_RULES.find((r) => r.name === 'validatePredicatePathRefs'); @@ -354,6 +470,66 @@ describe('#7010 corpus — shipped METADATA_FORM_REGISTRY', () => { ).toEqual([]); }); + // #7659's own corpus measurement, on the same population and through the same + // production entry point. The rule above is asserted at zero for its two ids; + // this one asserts zero for the third at BOTH severities, which is what a new + // `error` finding costs before it may land. Measured on `origin/main@5823d59`: + // 0 and 0. A non-zero `error` count would have been a STOP. + it('reports NOTHING on the RHS rule over the shipped forms, at either severity', () => { + const rhsFindings = validatePredicatePathRefs(shippedStack) + .filter((f) => f.rule === PREDICATE_RHS_PATH_SHAPED); + expect(rhsFindings.filter((f) => f.severity === 'error').map((f) => f.path)).toEqual([]); + expect(rhsFindings.filter((f) => f.severity === 'warning').map((f) => f.path)).toEqual([]); + }); + + it('sees the shipped corpus (reverse verification for the RHS rule)', () => { + // The anti-vacuity direction, and the reason it is written against the + // SHIPPED predicates rather than a fixture: rewriting each real + // `== 'literal'` comparison into `== data.__rhs__` turns it into exactly the + // defect, so the assertion is an EQUALITY on the number of rewritten + // comparisons rather than a floor any subset would satisfy. Zero here would + // mean the measurement above was a green gate over nothing (#4984). + // + // The rewrite is anchored on the OPERATOR, not on "any quoted string": a + // literal inside `data.type in ['a','b']` belongs to `in`, whose array parse + // is objectui#4266 and deliberately not this rule's, so rewriting those too + // would have inflated the expected count past what this rule answers for. + const corrupted = structuredClone(shippedStack) as { views: unknown[] }; + let comparisons = 0; + const rewrite = (node: unknown): void => { + if (Array.isArray(node)) { + for (const child of node) rewrite(child); + return; + } + if (!node || typeof node !== 'object') return; + const rec = node as Record; + for (const key of ['visibleWhen', 'visibleOn']) { + const value = rec[key]; + const source = typeof value === 'string' ? value + : value && typeof value === 'object' + && typeof (value as Record).source === 'string' + ? ((value as Record).source as string) + : undefined; + if (source === undefined) continue; + const swapped = source.replace(/(==|!=)(\s*)'[^']*'/g, (_m, op, gap) => { + comparisons++; + return `${op}${gap}data.__rhs__`; + }); + if (swapped === source) continue; + if (typeof value === 'string') rec[key] = swapped; + else (value as Record).source = swapped; + } + for (const value of Object.values(rec)) rewrite(value); + }; + rewrite(corrupted.views); + expect(comparisons, 'no shipped predicate carries an `==`/`!=` literal comparison').toBe(45); + + const rhsFindings = validatePredicatePathRefs(corrupted) + .filter((f) => f.rule === PREDICATE_RHS_PATH_SHAPED); + expect(rhsFindings).toHaveLength(comparisons); + expect(new Set(rhsFindings.map((f) => f.severity))).toEqual(new Set(['error'])); + }); + it('catches the pre-#6254 bare spellings when they are restored (reverse verification)', () => { // The reverse direction is RED-on-restore: #6254 rewrote 16 predicates in // `object.form.ts` from `type ...` to `data.type ...`. Restoring the bare diff --git a/packages/lint/src/validate-predicate-path-refs.ts b/packages/lint/src/validate-predicate-path-refs.ts index 21400b5335..c83a00140e 100644 --- a/packages/lint/src/validate-predicate-path-refs.ts +++ b/packages/lint/src/validate-predicate-path-refs.ts @@ -115,6 +115,88 @@ * both halves: the root is still spelled `data` at every depth, and the object it * binds is the ROW. This rule descends with the same rebinding, which is what * makes the shipped corpus read 0 instead of 16 false positives. + * + * ## The right-hand side is a different question entirely (#7659) + * + * Both rules above ask whether a path RESOLVES. `data.a == data.b` answers yes + * twice and is still broken, because the right-hand **position** does not + * evaluate paths at all. objectui's metadata-admin evaluator + * (`packages/app-shell/src/views/metadata-admin/predicate.ts`) resolves the LEFT + * side through `resolveValue` and hands the RIGHT side to `parseLiteral`, whose + * tail returns anything it does not recognise as a literal VERBATIM — so + * `data.a == data.b` compares `data.a`'s value 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 declared subset says so + * (`path == 'literal'`, never `path == path`), but until objectui#4049 the + * boundary was enforced by silence, and #7010's resolution check cannot reach + * it: both paths resolve, so there is nothing for it to report. + * + * ### One grammar, two enforcement points + * + * {@link PATH_SHAPED_RHS} is quoted verbatim from the consumer's + * `PATH_SHAPED_LITERAL` (objectui PR #4264, verified against + * `objectstack-ai/objectui@37cd8e4` rather than against the issue body — a regex + * relayed through two issue texts is exactly the thing that drifts by a + * character). Producer and renderer must refuse and warn about the same set, or + * an author fixes one door and trips the other. + * + * The token comes from the canonical AST, not from re-splitting the source: a + * `==` / `!=` node whose right operand is a plain identifier or a `.`-chain of + * identifiers. That agrees with the consumer on every shape it can reach — + * `foo(1)`, `data.b[0]` and `-3` all fail the consumer's regex on their text and + * all fail `memberChain` here — and it costs exactly one case: an identifier + * containing `$`, which the grammar admits but CEL's own identifier syntax does + * not, so `data.a == $b` never parses and is `visibility-predicate-syntax`'s + * (#6253) verdict rather than a missed catch here. `true` / `false` / `null` / + * numbers / quoted strings arrive as `value` nodes, never identifiers, so the + * negative controls are structural rather than a hand-maintained deny-list — + * the same early returns `parseLiteral` makes before its tail. + * + * ### Two severities under one id, and why that is not a hedge + * + * The shape is one question with one fix; the CONSEQUENCE splits, and the + * family's own bar is written on consequence — `error` where "there is no + * reading of the metadata under which it was going to work" + * (`visibility-bare-identifier`), `warning` where the predicate is merely + * advisory-wrong (`visibility-root-mislayered`). + * + * - **A dotted chain** (`data.a == data.b`, or `'x' == data.a` with the sides + * swapped) — `error`. Nobody writes a dotted identifier chain meaning the + * literal text of it, so there is no reading under which this worked; the + * verdict does not depend on the right-hand path at all, and for `==` it is + * false whatever the two sides hold, which HIDES the element. That is the + * same silent-wrong-verdict consequence + * `predicate-path-unresolved` already gates on one function up, so gating + * here is the file's existing bar, not a new one. + * - **A bare single word** (`status == active`) — `warning`. This one WORKS + * today, and the consumer's ruling says so in as many words: resolving the + * right side was rejected precisely because "it would flip + * `data.type == text`, the unquoted-string spelling that works today by + * accident, into a fail-open `true`". An author who meant the text gets the + * text. It is still outside the declared subset and still stops working when + * ROADMAP M9 swaps this evaluator for `@objectstack/formula` (bare `active` + * becomes an undeclared reference), so it must be reported — but refusing a + * `view` write at the runtime publish door over metadata that renders + * correctly is a false build error in the one direction a gate may not fail + * in. + * + * ### What this rule deliberately does NOT do + * + * - **Suppress the resolution limbs.** `data.a == data.tpye` reports twice — + * once because `tpye` is not a key, once because the position is a literal. + * Both statements are true and their fixes differ, and #7214's behaviour is + * not this rule's to narrow. + * - **Judge `in`'s array parse.** The same `parseLiteral` tail is reachable + * through `x in [...]` (objectui#4266), which is a distinct defect in the + * consumer and deliberately not folded in here. + * - **Descend into a comprehension macro body.** `data.tags.all(t, t == x)` + * binds `t` inside the body; the interim evaluator supports no macros at all, + * so a comparison in there is not a statement about this subset. The receiver + * is still walked. + * - **Reach a form whose `schemaId` resolves to no schema.** This rule needs no + * oracle, but it shares the walk with the two that do, and that walk stops at + * an unresolvable `schemaId`. A missed catch in the safe direction, stated + * here rather than silently inherited. */ import { parseCelToAst } from '@objectstack/formula'; @@ -126,8 +208,18 @@ import { formViewSites } from './view-walk.js'; export const PREDICATE_PATH_UNRESOLVED = 'predicate-path-unresolved'; export const PREDICATE_PATH_UNROOTED = 'predicate-path-unrooted'; +/** + * A `==` / `!=` RIGHT-hand side that is path-shaped — an unquoted identifier + * chain (#7659). See the module note's §The right-hand side for the two + * severities this one id carries and why they are not the same defect. + */ +export const PREDICATE_RHS_PATH_SHAPED = 'predicate-rhs-path-shaped'; -/** Both rules GATE — see the module note for why each is safe at `error`. */ +/** + * The two path-RESOLUTION rules always GATE; {@link PREDICATE_RHS_PATH_SHAPED} + * gates on a dotted chain and is advisory on a bare word — see the module note + * for why the same shape earns two answers. + */ export type PredicatePathSeverity = 'error' | 'warning'; export interface PredicatePathFinding { @@ -383,6 +475,57 @@ function rootedPaths(node: unknown, out: string[][]): void { rootedPaths(node.args, out); } +/** + * The identifier grammar a path-shaped right-hand side matches — quoted + * verbatim from objectui's `PATH_SHAPED_LITERAL` + * (`packages/app-shell/src/views/metadata-admin/predicate.ts`, objectui#4049 / + * PR #4264). One grammar, two enforcement points: the renderer warns on exactly + * this set in dev, and this rule refuses it at the publish door. + * + * Every chain {@link memberChain} can build already satisfies it (CEL's + * identifier syntax is a strict subset — no `$`), so today it accepts + * everything it is handed. It stays as the DEFINITION rather than as a + * comment: it is the line a reviewer diffs against the consumer, and it is the + * boundary if either side ever widens. + */ +const PATH_SHAPED_RHS = /^[A-Za-z_$][A-Za-z0-9_$]*(?:\.[A-Za-z_$][A-Za-z0-9_$]*)*$/; + +/** The comparison operators whose right side the renderer parses as a literal. */ +const EQUALITY_OPS = new Set(['==', '!=']); + +/** One `==` / `!=` site: the operator as written, and its right operand. */ +interface EqualitySite { + op: string; + right: unknown; +} + +/** + * Every `==` / `!=` comparison in the AST, with its RIGHT operand. + * + * A comprehension macro's BODY is skipped (its receiver is not) — see the + * module note. Written as an explicit early return rather than a filter on the + * results, so the boundary is visible where the walk makes it. + */ +function equalitySites(node: unknown, out: EqualitySite[]): void { + if (Array.isArray(node)) { + for (const child of node) equalitySites(child, out); + return; + } + if (!isNode(node)) return; + const args = node.args; + if ( + node.op === 'rcall' && Array.isArray(args) && typeof args[0] === 'string' + && COMPREHENSION_MACROS.has(args[0]) + ) { + equalitySites(args[1], out); + return; + } + if (typeof node.op === 'string' && EQUALITY_OPS.has(node.op) && Array.isArray(args) && args.length === 2) { + out.push({ op: node.op, right: args[1] }); + } + equalitySites(args, out); +} + /** * Split the AST's identifiers into the ones used as a NAMESPACE (`a.b`, `a?.b`, * `a['b']`, `a.exists(…)`) or BOUND by a comprehension macro, and the plain @@ -522,6 +665,49 @@ function checkPredicate( + `still spelled \`${ROOT}\` (there is no implicit row scope).`, }); } + + // ── `predicate-rhs-path-shaped` (#7659) ── + // + // Deliberately independent of `scope`: this asks about the POSITION a token + // sits in, never about what it resolves to. It therefore runs even where the + // two limbs above went opaque, and it does not suppress them — see the module + // note's §The right-hand side. + const sites: EqualitySite[] = []; + equalitySites(ast, sites); + for (const { op, right } of sites) { + const chain = memberChain(right); + if (!chain) continue; + const text = chain.join('.'); + if (!PATH_SHAPED_RHS.test(text)) continue; + const dotted = chain.length > 1; + findings.push({ + severity: dotted ? 'error' : 'warning', + rule: PREDICATE_RHS_PATH_SHAPED, + where, + path, + message: dotted + ? `predicate compares against \`${text}\` on the RIGHT of \`${op}\`, which is a path but is ` + + `not evaluated as one. A metadata-editing form resolves paths on the LEFT of \`${op}\` ` + + `only; the right-hand side goes to the literal parser, so \`${text}\` is compared as the ` + + `literal string "${text}". The verdict therefore does not depend on the right-hand path ` + + `at all: \`a == ${text}\` is FALSE even when both sides hold the same value, and ` + + `\`a != ${text}\` is correspondingly TRUE. An \`==\` written this way hides the element ` + + `on every row, and nothing in the console says why (objectui#4049).` + : `predicate compares against the unquoted word \`${text}\` on the RIGHT of \`${op}\`. The ` + + `right-hand side of \`${op}\` is a literal, never a reference, so this is read as the ` + + `literal string "${text}" — which is probably what you meant, and is why it appears to ` + + `work. It is outside the declared subset all the same (\`path == 'literal'\`), it is ` + + `indistinguishable from a dropped \`${ROOT}.\` root, and it stops working when this ` + + `surface moves to the real CEL evaluator, where a bare \`${text}\` resolves to nothing ` + + `(objectui#4049).`, + hint: + `Two sanctioned spellings. (1) If you meant the TEXT, quote it: \`${op} '${text}'\`. ` + + `(2) If you meant the PATH, restructure so the path is on the LEFT and a literal is on ` + + `the right — comparing one path against another is outside the subset this surface ` + + `renders, which is \`path == 'literal'\` / \`path != 'literal'\` and nothing wider. There ` + + `is no third spelling that compares two paths here.`, + }); + } } function walkFields( @@ -567,11 +753,18 @@ function walkFields( * resolves, is skipped: see the module note for why the `record.*` layer is out * of scope rather than merely unimplemented. * - * Both rules emit `error` and the caller is expected to fail the build on them. - * The corpus measurement behind that severity is on the PR for #7010: over the - * shipped `METADATA_FORM_REGISTRY` (17 forms, 46 predicates) the count is **0** - * for both, and 16 for `predicate-path-unrooted` once #6254's pre-fix - * `object.form.ts` is restored. + * Both path-resolution rules emit `error` and the caller is expected to fail the + * build on them. The corpus measurement behind that severity is on the PR for + * #7010: over the shipped `METADATA_FORM_REGISTRY` (17 forms, 46 predicates) the + * count is **0** for both, and 16 for `predicate-path-unrooted` once #6254's + * pre-fix `object.form.ts` is restored. + * + * The third rule, `predicate-rhs-path-shaped` (#7659), asks a different question + * about the same predicates — whether a `==` / `!=` RIGHT-hand side is + * path-shaped, which the renderer parses as a literal — and carries two + * severities: `error` on a dotted chain, `warning` on a bare word. Its corpus + * count over the same shipped forms is **0** at both severities. See the module + * note. * * Returns findings (empty = clean). */