From 1d77a75074f1da72f25a743af27a17a69b9b722b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 15:31:26 +0000 Subject: [PATCH] fix(lint): one prescription for a bare unquoted word on the right of `==` (#7696) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three publish-time rules fired on one token — a bare word on the right of `==` / `!=` in a metadata-editing form's visibility predicate — and prescribed opposite fixes. `visibility-bare-identifier` (`error`) and, when the word is also a schema key, `predicate-path-unrooted` (`error`) read it as a dropped binding root and asked for `.active`; `predicate-rhs-path-shaped` (`warning`) read it as a literal missing its quotes and asked for `'active'`. The `error`s blocked, and the spelling they asked for is one this same gate refuses: `data.type == data.active` is a path on the RIGHT, which is `predicate-rhs-path-shaped`'s own `error` arm. Obeying the loud finding landed the author on a louder one, over metadata that renders correctly today (objectui#4049). The two root-prescribing rules now stand down for identifiers occurring ONLY as a bare right operand of `==` / `!=` on a schema-bound metadata form (`predicate-rhs-position.ts`), and the surviving advisory names both readings — the quoted spelling and the move-the-path-left spelling — and states that adding the root in place is not a third option. Which reading the author meant is still not decided; inventing an answer is what the contradicting messages were doing. Narrow in all three directions: per identifier (`status == active` keeps its refusal over `status`), per surface (a runtime view goes to real CEL and is untouched), and never a silence (the condition is the same `schemaIdOf` test the replacement rule walks on). A form whose `schemaId` resolves to no schema is now walked with no scope rather than skipped, so the oracle-free right-hand check still runs there — 0 new findings over the shipped `METADATA_FORM_REGISTRY` corpus and both example apps. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VazgUHoW88FzFkGykVzJLM --- .../predicate-bare-word-rhs-reconciled.md | 57 ++++++ packages/lint/src/predicate-rhs-position.ts | 170 +++++++++++++++++ packages/lint/src/runtime-gate.test.ts | 56 +++++- .../src/validate-predicate-path-refs.test.ts | 74 ++++++++ .../lint/src/validate-predicate-path-refs.ts | 174 +++++++++++------- .../validate-visibility-predicates.test.ts | 41 +++++ .../src/validate-visibility-predicates.ts | 80 +++++++- 7 files changed, 577 insertions(+), 75 deletions(-) create mode 100644 .changeset/predicate-bare-word-rhs-reconciled.md create mode 100644 packages/lint/src/predicate-rhs-position.ts diff --git a/.changeset/predicate-bare-word-rhs-reconciled.md b/.changeset/predicate-bare-word-rhs-reconciled.md new file mode 100644 index 0000000000..a55f84c470 --- /dev/null +++ b/.changeset/predicate-bare-word-rhs-reconciled.md @@ -0,0 +1,57 @@ +--- +"@objectstack/lint": patch +--- + +fix(lint): one prescription for a bare unquoted word on the right of `==` (#7696) + +For a single token — a bare word on the right of `==` / `!=` in a +metadata-editing form's visibility predicate, e.g. `data.type == active` — +three publish-time rules fired and prescribed **opposite** fixes: + +| rule | severity | read it as | prescribed | +|---|---|---|---| +| `visibility-bare-identifier` | `error` | a dropped binding root | `.active` | +| `predicate-path-unrooted` | `error` (when the word is also a schema key) | a dropped binding root | `data.active` | +| `predicate-rhs-path-shaped` | `warning` | a literal missing its quotes | `'active'` | + +The `error`s were the ones that blocked the write, and they asked for a spelling +**this same gate refuses**: `data.type == data.active` is a path on the RIGHT, +which is `predicate-rhs-path-shaped`'s `error` arm. An author who obeyed the +loud finding landed on a louder one, and "fixing" that by making the path +resolve reached `predicate-path-unresolved` — a three-corner walk over metadata +that renders correctly today (objectui#4049). + +**The bare-word right-hand position now produces exactly one finding.** The two +root-prescribing rules stand down for identifiers that occur *only* as a bare +right operand of `==` / `!=` on a schema-bound metadata form, and +`predicate-rhs-path-shaped`'s advisory carries both readings: it names the +quoted spelling (`== 'active'`) and the field spelling (move the path to the +LEFT, `data.active == 'yes'`), and says in as many words that adding the root in +place is not a third option. ⛔ Which reading the author meant is still not +decided — that is the thing no linter can know, and inventing an answer is what +the contradicting messages were doing. + +**What this changes for you.** A `view` write whose only defect is a bare word +on the right of `==` on a metadata-editing form is no longer refused; it comes +back as an advisory on the 2xx response, at the severity #7659 already argued +for a spelling that renders correctly. Nothing else moves: + +- a bare word on the LEFT (`status == active`) is still an `error`; +- a word that also occurs outside a right-hand slot is still an `error`; +- a dotted chain on the right (`data.a == data.b`) is still an `error`; +- a runtime `*.view.ts` / `*.page.ts` predicate is untouched — that surface goes + to real CEL, where a path on the right is legal and a bare word there really + is a dropped root, so `record.status == active` keeps its refusal and its + `record.active` hint; +- the #7659 severity split (`error` on a dotted chain, `warning` on a bare word) + is unchanged. + +**One coverage increase, in the safe direction.** A form whose `schemaId` +resolves to no schema this package can see used to be skipped wholesale, taking +the right-hand check with it even though that check needs no schema oracle. Such +sites are now walked with no scope: the two path-resolution rules stay silent +(no oracle, no verdict) and the right-hand position is judged. Without it, the +stand-down above would have been a silence on that shape rather than a +reconciliation. Measured at 0 new findings over the shipped +`METADATA_FORM_REGISTRY` corpus and over `examples/app-showcase` / +`examples/app-crm`. diff --git a/packages/lint/src/predicate-rhs-position.ts b/packages/lint/src/predicate-rhs-position.ts new file mode 100644 index 0000000000..067488b7a6 --- /dev/null +++ b/packages/lint/src/predicate-rhs-position.ts @@ -0,0 +1,170 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * **The right-hand-of-`==` POSITION, shared by every rule that judges it** + * (#7696). + * + * On a metadata-editing form the console's evaluator + * (`packages/app-shell/src/views/metadata-admin/predicate.ts`, objectui#4049) + * resolves the LEFT side of `==` / `!=` through `resolveValue` and hands the + * RIGHT side to `parseLiteral`. So the right-hand slot is a LITERAL slot: a + * token sitting there is never resolved as a reference, whatever it is spelled + * like. That single fact is read by three rules across two files, and #7696 was + * filed because they read it differently and prescribed opposite fixes for one + * token: + * + * - `visibility-bare-identifier` (`validate-visibility-predicates.ts`, #6128) + * read a bare `active` as a dropped binding root and said, at `error`, write + * `.active`. + * - `predicate-path-unrooted` (`validate-predicate-path-refs.ts`, #7010) said + * the same thing at `error` whenever the word also happens to be a schema + * key. + * - `predicate-rhs-path-shaped` (`validate-predicate-path-refs.ts`, #7659) read + * it as a literal missing its quotes and said, at `warning`, write + * `'active'`. + * + * The first two prescribe a spelling this gate itself REFUSES: `data.type == + * data.active` is a dotted chain on the right, which is `predicate-rhs-path- + * shaped`'s `error` arm. An author who complies with the loud finding lands on + * a louder one. That is the walk #7696 is about, and it is why the rooted + * prescription is not "a different opinion" here — on this surface it is not a + * spelling at all. + * + * {@link bareRhsOnlyIdentifiers} is the mechanism that stands the two + * root-prescribing rules down for exactly this position, leaving + * `predicate-rhs-path-shaped` as the single voice. It answers a narrow + * question: which identifiers occur ONLY as the bare right operand of a + * `==` / `!=`? + * + * ## Why "only", and why the set is deliberately small + * + * `status == active` must still be REFUSED — `status` is a genuine dropped root + * on the left, and the fact that `active` is a literal says nothing about it. + * So the suppression is per-identifier and per-occurrence: a name that appears + * anywhere other than a bare right operand (`active == data.x`, + * `data.a == active && active`) keeps its finding. The two callers add this set + * to the names they treat as resolvable, which is the same conservative + * direction `namespaceRoots` already takes in the sibling file — every name it + * adds can only remove a finding, never create one. + * + * ## Where it deliberately stands nothing down + * + * Suppression is only safe where the replacement finding actually fires, so the + * walk refuses to suppress anywhere `predicate-rhs-path-shaped` does not reach: + * + * - **Inside a comprehension-macro body.** `equalitySites` in + * `validate-predicate-path-refs.ts` skips macro bodies (the interim + * evaluator supports no macros at all, so a comparison in there is not a + * statement about this subset), so an `==` in there produces no replacement. + * This walk descends into the body with suppression turned OFF, which files + * every identifier it finds there as "occurs elsewhere". + * - **A dotted chain on the right.** `data.a == data.b` is already the `error` + * arm and needs no reconciliation; only a BARE `id` node is collected. + * - **A surface the metadata-admin evaluator does not render.** That is the + * caller's condition, not this walk's — {@link schemaIdOf} is the test, and + * a runtime `*.view.ts` predicate (real CEL, where a path on the right is + * perfectly legal) never reaches it. + */ + +type AnyRec = Record; +type AstNode = { op?: string; args?: unknown }; + +function isNode(v: unknown): v is AstNode { + return !!v && typeof v === 'object' && typeof (v as AstNode).op === 'string'; +} + +/** The comparison operators whose right side the metadata-admin evaluator parses as a literal. */ +export const EQUALITY_OPS = new Set(['==', '!=']); + +/** + * CEL comprehension macros: the receiver-call forms that BIND their first + * argument as a loop variable. + */ +export const COMPREHENSION_MACROS = new Set(['all', 'exists', 'exists_one', 'map', 'filter']); + +/** The bare identifier name a node spells, or `null` when it is anything else. */ +function bareId(node: unknown): string | null { + if (!isNode(node)) return null; + return node.op === 'id' && typeof node.args === 'string' ? node.args : null; +} + +/** + * Identifiers that occur ONLY as the bare right operand of a `==` / `!=`. + * + * `suppressible` is threaded rather than checked at the top because the answer + * changes with DEPTH: inside a comprehension-macro body no equality produces a + * `predicate-rhs-path-shaped` finding, so an identifier found there must count + * as an ordinary occurrence even though it sits in a right-hand slot. + */ +export function bareRhsOnlyIdentifiers(ast: unknown): Set { + const rhs = new Set(); + const elsewhere = new Set(); + + const walk = (node: unknown, suppressible: boolean): void => { + if (Array.isArray(node)) { + for (const child of node) walk(child, suppressible); + return; + } + if (!isNode(node)) return; + const args = node.args; + + // A comprehension binds a loop variable and its body is out of the + // replacement rule's reach — descend with suppression off. The RECEIVER is + // an ordinary sub-expression and keeps the caller's mode. + if ( + node.op === 'rcall' && Array.isArray(args) && typeof args[0] === 'string' + && COMPREHENSION_MACROS.has(args[0]) + ) { + walk(args[1], suppressible); + walk(args[2], false); + return; + } + + if ( + typeof node.op === 'string' && EQUALITY_OPS.has(node.op) + && Array.isArray(args) && args.length === 2 + ) { + const right = suppressible ? bareId(args[1]) : null; + walk(args[0], suppressible); + if (right !== null) { + rhs.add(right); + return; // the right operand IS the suppressible occurrence — not "elsewhere" + } + walk(args[1], suppressible); + return; + } + + const name = bareId(node); + if (name !== null) { + elsewhere.add(name); + return; + } + walk(args, suppressible); + }; + + walk(ast, true); + for (const name of elsewhere) rhs.delete(name); + return rhs; +} + +function isRec(v: unknown): v is AnyRec { + return !!v && typeof v === 'object' && !Array.isArray(v); +} + +/** + * The `schemaId` a form view resolves its row shape from, or `undefined` when + * the view is not schema-bound. Read off `ViewDataSourceSchema`'s `schema` + * member (`view.zod.ts:151-161`) — the shape `defineForm` writes. + * + * Shared rather than duplicated: it is the test for "the metadata-admin + * evaluator renders this", which is the precondition BOTH the rule that + * reports the right-hand position and the rules that stand down for it must + * agree on. Two copies of it is exactly how one side starts suppressing on a + * surface the other side has gone quiet on. + */ +export function schemaIdOf(view: AnyRec): string | undefined { + const data = view.data; + if (!isRec(data)) return undefined; + if (data.provider !== 'schema') return undefined; + return typeof data.schemaId === 'string' ? data.schemaId : undefined; +} diff --git a/packages/lint/src/runtime-gate.test.ts b/packages/lint/src/runtime-gate.test.ts index 0cb037edab..70e92773fd 100644 --- a/packages/lint/src/runtime-gate.test.ts +++ b/packages/lint/src/runtime-gate.test.ts @@ -341,17 +341,60 @@ describe('the views[] visibility-predicate family at the runtime publish gate (# // 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. + // #7696: it is now the ONLY thing the author hears about that token. This + // pin used to record the opposite — `visibility-bare-identifier` refusing + // the same write and prescribing `.active` — and the pair it pinned + // was a contradiction, not a division of labour: the rooted spelling it + // asked for is a path on the RIGHT, which this same gate refuses at `error` + // (the test below measures it). So the root-prescribing rules stand down + // for this position and the advisory carries both readings. 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'); + expect( + result.errors.map((e) => e.rule), + 'one token, one prescription — the write is no longer blocked by a rule asking for a ' + + 'spelling this gate refuses (#7696)', + ).toEqual([]); + // The merged message must state BOTH readings and must not prescribe the + // in-place rooted spelling, which is the thing the next test refuses. + expect(f!.hint).toMatch(/'active'/); + expect(f!.hint).toMatch(/data\.active == /); + expect(f!.hint).toMatch(/Do NOT simply add the root in place/); + }); + + it('does not prescribe a fix it would itself refuse (#7696)', () => { + // The accept bar the old pair failed. `visibility-bare-identifier` told the + // author to root the word in place; both rootings of that exact predicate + // are a dotted chain on the RIGHT, and the gate refuses them at `error`. + for (const root of ['data', 'record']) { + const { errors } = gateView(schemaBoundForm(`data.type == ${root}.active`)); + expect( + errors.map((e) => e.rule), + `\`== ${root}.active\` is the spelling the old error asked for`, + ).toContain('predicate-rhs-path-shaped'); + } + }); + + it('still REFUSES the same bare word on the LEFT, and on a runtime surface', () => { + // The scanner proves it can see before any silence is believed. The + // stand-down is per-IDENTIFIER and per-SURFACE, so three neighbours of the + // reconciled case keep their refusal unchanged. + expect( + gateView(schemaBoundForm('status == active')).errors.map((e) => e.rule), + 'a bare word on the LEFT is a genuine dropped root, whatever sits on the right', + ).toContain('visibility-bare-identifier'); + expect( + gateView(schemaBoundForm('data.name == active && active')).errors.map((e) => e.rule), + 'the word also occurs outside a right-hand slot — not a literal position', + ).toContain('visibility-bare-identifier'); + expect( + gateView(runtimeView('record.status == active')).errors.map((e) => e.rule), + 'a runtime surface goes to real CEL, where a path on the right is legal and a bare word ' + + 'there really is a dropped root — nothing about it was reconciled', + ).toContain('visibility-bare-identifier'); }); it('reports a MISLAYERED root through the advisory channel, not a refusal', () => { @@ -418,6 +461,7 @@ describe('the views[] visibility-predicate family at the runtime publish gate (# schemaBoundForm("data.tpye == 'text'"), // unresolvable path schemaBoundForm("type == 'text'"), // unrooted schema key schemaBoundForm("data.type == 'text'"), // resolvable path + schemaBoundForm('data.type == active'), // bare word on the RIGHT (#7696) ]; const fingerprints = (fs: readonly AuthoringFinding[]) => diff --git a/packages/lint/src/validate-predicate-path-refs.test.ts b/packages/lint/src/validate-predicate-path-refs.test.ts index 1a5bbc1133..f72419d038 100644 --- a/packages/lint/src/validate-predicate-path-refs.test.ts +++ b/packages/lint/src/validate-predicate-path-refs.test.ts @@ -377,6 +377,80 @@ describe('validatePredicatePathRefs — path-shaped right-hand side (#7659)', () }); }); +describe('validatePredicatePathRefs — one position, one prescription (#7696)', () => { + const rhs = (source: string) => + run(form([{ label: 'S', fields: [{ field: 'name', visibleWhen: source }] }])); + + it('does not ALSO call a right-hand bare word a dropped root', () => { + // `type` is a declared key of DemoSchema, so before #7696 this drew + // `predicate-path-unrooted` at `error` ("write `data.type`") on top of the + // right-hand `warning` ("write `'type'`") — two findings, opposite fixes, + // and the blocking one asking for a spelling this same rule refuses. + const findings = rhs('data.name == type'); + expect(findings.map((f) => f.rule)).toEqual([PREDICATE_RHS_PATH_SHAPED]); + expect(findings[0].severity).toBe('warning'); + }); + + it('proves that silence is the stand-down, not a walk that stopped seeing keys', () => { + // Same schema key, LEFT of the comparison: the unrooted limb must still + // fire, or the assertion above is measuring a dead limb. + expect(rhs("type == 'formula'").map((f) => f.rule)).toEqual([PREDICATE_PATH_UNROOTED]); + // And in a right-hand slot it stays reported whenever it ALSO occurs + // somewhere that is not one. + expect(rhs('data.name == type && type').map((f) => f.rule).sort()) + .toEqual([PREDICATE_PATH_UNROOTED, PREDICATE_RHS_PATH_SHAPED]); + }); + + it('stands down for `!=` as well, and not for a dotted chain', () => { + expect(rhs('data.name != type').map((f) => f.rule)).toEqual([PREDICATE_RHS_PATH_SHAPED]); + // A dotted chain is the `error` arm and was never part of the contradiction. + expect(rhs('data.name == data.type')[0].severity).toBe('error'); + }); + + it('never suppresses inside a comprehension-macro body, where nothing replaces it', () => { + // `equalitySites` skips macro bodies, so an `==` in there produces no + // right-hand finding. Suppressing the unrooted verdict there would be a + // silence rather than a reconciliation. + expect(rhs('data.tags.all(t, t == type)').map((f) => f.rule)).toEqual([PREDICATE_PATH_UNROOTED]); + }); + + it('names both readings and refuses to prescribe the in-place rooting', () => { + const f = rhs('data.name == type')[0]; + expect(f.hint).toMatch(/quote it: `== 'type'`/); + expect(f.hint).toMatch(/data\.type == 'yes'/); + expect(f.hint).toMatch(/Do NOT simply add the root in place/); + // The bar: BOTH spellings the message recommends must be clean… + expect(rhs("data.type == 'yes'")).toEqual([]); + expect(rhs("data.name == 'type'")).toEqual([]); + // …and the spelling it warns AGAINST must be reported, or the warning is + // noise. This is the exact edit the old `error` used to demand. + expect(rhs('data.name == data.type').map((x) => x.severity)).toEqual(['error']); + }); + + it('the LEFT-side spelling it recommends is judged on its own merits, not circular', () => { + // If the word names no field at all, moving it left is answered by #7214 — + // "`active` is not a key" — which is a NEW statement about a different + // mistake, not the old ring-around. The right-hand rule has nothing further + // to say once the token is out of the literal slot. + expect(rhs("data.active == 'yes'").map((x) => x.rule)).toEqual([PREDICATE_PATH_UNRESOLVED]); + }); + + it('judges the right-hand position on a form whose schemaId resolves to nothing', () => { + // The two resolution limbs need an oracle and correctly go quiet; the + // right-hand limb never did. Walking past the whole site made the + // stand-down above a silence on this shape, so the site is now walked with + // no scope instead of skipped. + const unknown = (source: string) => + run(form([{ label: 'S', fields: [{ field: 'name', visibleWhen: source }] }], 'no_such_type')); + expect(unknown('data.a == data.b').map((f) => f.rule)).toEqual([PREDICATE_RHS_PATH_SHAPED]); + expect(unknown('data.a == active').map((f) => f.severity)).toEqual(['warning']); + // The resolution limbs stay silent there — no oracle, no verdict. + expect(unknown("data.tpye == 'x'")).toEqual([]); + // Control: the same predicate against a schema that DOES resolve reports. + expect(rhs("data.tpye == 'x'").map((f) => f.rule)).toEqual([PREDICATE_PATH_UNRESOLVED]); + }); +}); + 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'); diff --git a/packages/lint/src/validate-predicate-path-refs.ts b/packages/lint/src/validate-predicate-path-refs.ts index c83a00140e..b3b154b215 100644 --- a/packages/lint/src/validate-predicate-path-refs.ts +++ b/packages/lint/src/validate-predicate-path-refs.ts @@ -103,7 +103,10 @@ * unjudged. * - **A `schemaId` that resolves to no schema.** A stack may name a schema this * package cannot see (a custom one, or a type served only at runtime). No - * oracle, no verdict. + * oracle, no verdict — for the two RESOLUTION limbs. The site is still + * walked so the oracle-free right-hand limb runs (#7696); before that it was + * skipped wholesale, which is the shape a shared walk quietly imposes on a + * rule that never needed the thing being shared. * * ## Repeater rows rebind `data`, and the rule follows (#6254) * @@ -180,12 +183,47 @@ * correctly is a false build error in the one direction a gate may not fail * in. * + * ### One position, one prescription (#7696) + * + * A BARE word on the right — `data.type == active` — used to draw three + * findings across two files, and they prescribed opposite fixes: + * `visibility-bare-identifier` (`error`) and `predicate-path-unrooted` + * (`error`, when the word is also a schema key) both read it as a dropped + * binding root and said write `.active`; this rule read it as a literal + * missing its quotes and said write `'active'`. The `error`s were the ones that + * blocked the write. + * + * They are not two defensible opinions. The rooted spelling they prescribe — + * `data.type == data.active` — is a path on the RIGHT, which is this rule's own + * `error` arm one paragraph up: an author who obeys the loud finding lands on a + * louder one, and if they then "fix" that by making the path resolve, #7214's + * `predicate-path-unresolved` is the third corner. So the root reading is not a + * competing fix on this surface; it is a spelling the surface does not have, + * and the `error`'s own bar ("there is no reading of the metadata under which it + * was going to work") is FALSE here — the literal reading works today, which is + * why the bare arm is `warning` in the first place. + * + * Both root-prescribing rules therefore stand down for identifiers that occur + * ONLY as a bare right operand of `==` / `!=` on a metadata-editing form + * ({@link bareRhsOnlyIdentifiers}), and this rule's bare-arm finding absorbs + * what they were trying to say: it names BOTH readings, gives the sanctioned + * spelling for each, and says in as many words that adding the root in place is + * not one of them. ⛔ Which reading the author meant is still not decided — that + * is the thing no linter can know, and inventing an answer is what the three + * contradicting messages were doing. + * + * The stand-down is per-IDENTIFIER, never per-predicate: `status == active` + * keeps its refusal, because `status` on the left is a genuine dropped root and + * nothing about the right-hand word says otherwise. + * * ### 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. + * not this rule's to narrow. The one exception is the bare-word position + * above, where the two verdicts were not merely different but mutually + * unsatisfiable. * - **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. @@ -204,6 +242,12 @@ import { getMetadataTypeSchema } from '@objectstack/spec/kernel'; import { findClosestMatches, formatSuggestion } from '@objectstack/spec'; import { collectionEntries } from './collection-entries.js'; +import { + COMPREHENSION_MACROS, + EQUALITY_OPS, + bareRhsOnlyIdentifiers, + schemaIdOf, +} from './predicate-rhs-position.js'; import { formViewSites } from './view-walk.js'; export const PREDICATE_PATH_UNRESOLVED = 'predicate-path-unresolved'; @@ -268,13 +312,6 @@ const PREDICATE_KEYS = ['visibleWhen', 'visibleOn'] as const; /** The binding root this rule resolves. Metadata-editing forms only — see the module note. */ const ROOT = 'data'; -/** - * CEL comprehension macros: the receiver-call forms that BIND their first - * argument as a loop variable. A bare identifier that is one of those is not a - * dropped root, so it is declared before the unrooted limb runs. - */ -const COMPREHENSION_MACROS = new Set(['all', 'exists', 'exists_one', 'map', 'filter']); - // ── Zod introspection ─────────────────────────────────────────────── // // Reads `.def` directly rather than importing zod's internals, and tolerates @@ -490,9 +527,6 @@ function rootedPaths(node: unknown, out: string[][]): void { */ 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; @@ -582,18 +616,6 @@ function isRec(v: unknown): v is AnyRec { return !!v && typeof v === 'object' && !Array.isArray(v); } -/** - * The `schemaId` a form view resolves its row shape from, or `undefined` when - * the view is not schema-bound. Read off `ViewDataSourceSchema`'s `schema` - * member (`view.zod.ts:151-161`) — the shape `defineForm` writes. - */ -function schemaIdOf(view: AnyRec): string | undefined { - const data = view.data; - if (!isRec(data)) return undefined; - if (data.provider !== 'schema') return undefined; - return typeof data.schemaId === 'string' ? data.schemaId : undefined; -} - function checkPredicate( source: string, scope: unknown, @@ -641,37 +663,51 @@ function checkPredicate( } // ── `predicate-path-unrooted` ── + // + // Skipped entirely when the scope is not key-bearing (no oracle, no verdict). + // Written as a guarded BLOCK rather than an early return so the right-hand + // limb below still runs: that one asks about a POSITION and needs no oracle + // at all, and gating it behind a resolvable schema is how it went silent on a + // form whose `schemaId` this package cannot see. const declaredHere = keysOf(scope); - if (!declaredHere) return; - const values = new Set(); - const excluded = new Set(); - classifyIdentifiers(ast, values, excluded); - for (const id of values) { - if (excluded.has(id) || !declaredHere.includes(id)) continue; - findings.push({ - severity: 'error', - rule: PREDICATE_PATH_UNROOTED, - where, - path, - message: - `predicate references \`${id}\` as a bare identifier, but \`${id}\` is a key of the schema ` - + `this form edits — the binding root was dropped. Values are bound under \`${ROOT}\` and are ` - + `never flattened to top level, so \`${id}\` resolves to nothing, the predicate can never ` - + `evaluate and the console falls OPEN: the element renders unconditionally and looks exactly ` - + `like one carrying no predicate at all (#5149, #6254).`, - hint: - `Write \`${ROOT}.${id}\` instead of \`${id}\`. A metadata-editing form binds the row under ` - + `edit as \`${ROOT}\` at every depth — inside a repeater \`${ROOT}\` is the ROW, but it is ` - + `still spelled \`${ROOT}\` (there is no implicit row scope).`, - }); + // #7696 — an identifier that occurs ONLY as the bare right operand of + // `==` / `!=` is in a LITERAL slot, so it is not a dropped root. Suppressed + // here rather than re-diagnosed: the right-hand limb below reports the same + // token, once, with the prescription that surface actually accepts. + const rhsOnly = bareRhsOnlyIdentifiers(ast); + if (declaredHere) { + const values = new Set(); + const excluded = new Set(); + classifyIdentifiers(ast, values, excluded); + for (const id of values) { + if (excluded.has(id) || rhsOnly.has(id) || !declaredHere.includes(id)) continue; + findings.push({ + severity: 'error', + rule: PREDICATE_PATH_UNROOTED, + where, + path, + message: + `predicate references \`${id}\` as a bare identifier, but \`${id}\` is a key of the schema ` + + `this form edits — the binding root was dropped. Values are bound under \`${ROOT}\` and are ` + + `never flattened to top level, so \`${id}\` resolves to nothing, the predicate can never ` + + `evaluate and the console falls OPEN: the element renders unconditionally and looks exactly ` + + `like one carrying no predicate at all (#5149, #6254).`, + hint: + `Write \`${ROOT}.${id}\` instead of \`${id}\`. A metadata-editing form binds the row under ` + + `edit as \`${ROOT}\` at every depth — inside a repeater \`${ROOT}\` is the ROW, but it is ` + + `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. + // two limbs above went opaque, and it does not suppress them where they have + // something of their own to say — see the module note's §The right-hand side. + // For the ONE position where they contradicted it, they now stand down and + // this is the single finding (#7696, §One position, one prescription). const sites: EqualitySite[] = []; equalitySites(ast, sites); for (const { op, right } of sites) { @@ -696,16 +732,25 @@ function checkPredicate( : `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.`, + + `work. It is outside the declared subset all the same (\`path == 'literal'\`), and it ` + + `stops working when this surface moves to the real CEL evaluator, where a bare ` + + `\`${text}\` resolves to nothing (objectui#4049). The token also reads as a \`${ROOT}.\` ` + + `root someone dropped, so this one finding carries BOTH readings: which one you meant ` + + `is the thing no linter can know, and it changes the fix (#7696).`, + hint: dotted + ? `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.` + : `Two sanctioned spellings, and you must pick — they are not the same predicate. ` + + `(1) If you meant the TEXT \`${text}\`, quote it: \`${op} '${text}'\`. That is what this ` + + `renders as today, so it changes no behaviour and is the fix unless you know otherwise. ` + + `(2) If you meant the FIELD \`${ROOT}.${text}\`, move it to the LEFT and put a literal on ` + + `the right, e.g. \`${ROOT}.${text} == 'yes'\`. ⛔ Do NOT simply add the root in place: ` + + `\`${op} ${ROOT}.${text}\` is a path on the RIGHT, which this surface parses as the ` + + `literal string "${ROOT}.${text}" — it is refused by this same rule at \`error\`, and it ` + + `is FALSE on every row. The subset here is \`path == 'literal'\` and nothing wider.`, }); } } @@ -784,16 +829,19 @@ export function validatePredicatePathRefs( for (const site of formViewSites(view, viewPath)) { const schemaId = schemaIdOf(site.view); if (!schemaId) continue; + // An id no schema resolves leaves the two RESOLUTION limbs without an + // oracle, and they go quiet on their own (`keysOf(undefined)` is `null`, + // and every `stepInto` is `opaque`). The site is still walked, because + // the right-hand-position limb needs no oracle — and since #7696 the + // sibling rules STAND DOWN on this surface, walking past it would turn a + // reconciliation into a silence. A resolver that throws on an unknown id + // is the same case and must not take the build down with it. let root: unknown; try { root = resolveSchema(schemaId); } catch { - // A resolver that throws on an unknown id must not take the build down - // with it — no oracle, no verdict, same as an id that resolves to - // `undefined`. - continue; + root = undefined; } - if (!root) continue; const where = site.surface ? `view "${viewName}" · ${site.surface} (schema "${schemaId}")` diff --git a/packages/lint/src/validate-visibility-predicates.test.ts b/packages/lint/src/validate-visibility-predicates.test.ts index e02de72892..01e299f843 100644 --- a/packages/lint/src/validate-visibility-predicates.test.ts +++ b/packages/lint/src/validate-visibility-predicates.test.ts @@ -452,6 +452,47 @@ describe('visibility-bare-identifier (#6128 / #5149 requirement 3)', () => { }); }); + describe('the ONE position this rule stands down on (#7696)', () => { + /** The same one-field form, but SCHEMA-BOUND — a metadata-editing form. */ + const metaForm = (predicate: string) => ({ + views: [{ + name: 'field_editor', + data: { provider: 'schema', schemaId: 'field' }, + sections: [{ fields: [{ field: 'notes', visibleWhen: predicate }] }], + }], + }); + + it('a bare word on the RIGHT of `==` is a literal slot, not a dropped root', () => { + // The console's metadata-admin evaluator hands the right side to + // `parseLiteral` and never resolves it (objectui#4049), so this rule's + // prescription — `.active` — is not a fix but a second defect, and + // `predicate-rhs-path-shaped` refuses that spelling at `error`. One + // position, one prescription: this rule is silent and that one speaks. + expect(bareFindings(metaForm('data.type == active'))).toEqual([]); + expect(bareFindings(metaForm('data.type != active'))).toEqual([]); + }); + + it('proves the scanner still sees — the stand-down is per IDENTIFIER', () => { + // Every one of these is the same schema-bound form, so a walk that had + // gone blind would report nothing here either. + expect(bareFindings(metaForm('status == active')).map((f) => f.hint)) + .toEqual([expect.stringContaining('`record.status`')]); + expect(bareFindings(metaForm('active == data.type'))).toHaveLength(1); + expect(bareFindings(metaForm('data.type == active && active'))).toHaveLength(1); + // A macro body produces no replacement finding, so nothing stands down. + expect(bareFindings(metaForm('data.tags.all(t, t == active)'))).toHaveLength(1); + }); + + it('is scoped to the metadata-editing surface — a runtime view is untouched', () => { + // A runtime `*.view.ts` predicate goes to real CEL, where a path on the + // right is perfectly legal and a bare word there IS a dropped root. No + // replacement rule walks this shape, so standing down would be silence. + const findings = bareFindings(formStack('record.status == active')); + expect(findings).toHaveLength(1); + expect(findings[0].hint).toContain('`record.active`'); + }); + }); + describe('the layer decides the prescribed root (ADR-0089 D3)', () => { it('a metadata-editing form is told to write `data.`, not `record.`', () => { const findings = bareFindings(formStack("layout == 'grid'"), { layer: 'metadata' }); diff --git a/packages/lint/src/validate-visibility-predicates.ts b/packages/lint/src/validate-visibility-predicates.ts index 8920a914a9..43ae45d491 100644 --- a/packages/lint/src/validate-visibility-predicates.ts +++ b/packages/lint/src/validate-visibility-predicates.ts @@ -68,7 +68,9 @@ * - `visibility-bare-identifier` (**error**, #6128 / #5149 requirement 3) — a * predicate referencing a top-level identifier that no binding root can * resolve (`status == 'active'` instead of `record.status == 'active'`). See - * the §Bare identifiers block below for the mechanism and the boundaries. + * the §Bare identifiers block below for the mechanism and the boundaries, and + * §The one position this rule does not judge for the metadata-form right-hand + * slot it stands down on (#7696). * - `visibility-root-mislayered` — a visibility predicate whose binding root does * not match its layer (ADR-0089 D3, §Context). The check is **bidirectional**: * - **runtime** view/page surfaces (`*.view.ts` / `*.page.ts`) bind @@ -253,6 +255,34 @@ * `type(record.x) == string` is legitimate CEL over the very same names. A * measured blind spot in the safe direction — a missed catch, never a false * build error — pinned by a test so it reads as a decision. + * + * ### The one position this rule does not judge (#7696) + * + * A bare word on the RIGHT of `==` / `!=`, on a **metadata-editing form** + * (`{ provider: 'schema', schemaId }`), is not judged here — see + * `predicate-rhs-position.ts` for the mechanism and + * `validate-predicate-path-refs.ts`'s §One position, one prescription for the + * argument. The short form: on that surface the console's evaluator hands the + * right side to its literal parser and never resolves it (objectui#4049), so + * this rule's prescription — write `.` — is not a fix but a second + * defect, and the SAME publish gate refuses it at `error` under + * `predicate-rhs-path-shaped`. Telling an author to write a spelling the gate + * refuses is the harm #7696 was filed about, and it is loudest here because + * this is the finding that BLOCKS. + * + * The stand-down is narrow in all three directions that matter: + * + * - **Per identifier, not per predicate.** `status == active` is still refused + * over `status`; only names occurring *solely* in a right-hand slot are let + * through. + * - **Only where the replacement fires.** A runtime `*.view.ts` / `*.page.ts` + * predicate goes to real CEL, where a path on the right is legal and a bare + * word there really is a dropped root — `record.status == active` keeps its + * `error`, with the same hint it always had. + * - **Never a silence.** The condition is the same `schemaIdOf` test + * `validatePredicatePathRefs` walks on, so every token this rule stops + * reporting is reported by that one — as a `warning`, which is the severity + * #7659 already argued for a spelling that renders correctly today. */ import { @@ -265,6 +295,7 @@ import type { CelAstNode, CelBoundsOverrun } from '@objectstack/formula'; import { collectionEntries } from './collection-entries.js'; import { walkPageComponents } from './page-walk.js'; +import { bareRhsOnlyIdentifiers, schemaIdOf } from './predicate-rhs-position.js'; import { formViewSites } from './view-walk.js'; export const VISIBILITY_ROOT_MISLAYERED = 'visibility-root-mislayered'; @@ -568,15 +599,31 @@ function namespaceRoots(node: unknown, out: Set): void { * when every reference is rooted. See the module note for why this is two * oracles (the canonical AST for namespace roots, the shared strict-environment * checker for the verdict) and for the shapes it deliberately leaves alone. + * + * `literalRhs` is set for a METADATA-EDITING form, where the console's + * evaluator hands the right of `==` / `!=` to its literal parser and never + * resolves it (objectui#4049). A bare word there is not a dropped root — it is + * a literal, possibly missing its quotes — and `predicate-rhs-path-shaped` + * (#7659) is the rule that says so, with the two spellings this surface + * actually accepts. Standing down here is what makes that ONE finding instead + * of two that prescribe opposite fixes (#7696). It is fed through the DECLARED + * list rather than post-filtering the verdict, which is the same conservative + * direction {@link namespaceRoots} takes: a name added there can only remove a + * finding, never invent one. */ -function firstBareIdentifier(source: string): string | null { +function firstBareIdentifier(source: string, literalRhs: boolean): string | null { const ast = parseCelToAst(source); // Not parseable through the canonical front end (syntax fault, or over // DEFAULT_LIMITS) — not this rule's verdict to give. if (!ast) return null; const rooted = new Set(); namespaceRoots(ast, rooted); - return firstUndeclaredReference(source, [...VIEW_PAGE_EXTRA_ROOTS, ...rooted]); + const literalSlot = literalRhs ? bareRhsOnlyIdentifiers(ast) : []; + return firstUndeclaredReference(source, [ + ...VIEW_PAGE_EXTRA_ROOTS, + ...rooted, + ...literalSlot, + ]); } /** @@ -638,6 +685,7 @@ function checkElement( path: string, layer: VisibilityLayer, findings: VisibilityFinding[], + literalRhs = false, ): void { // (1) mis-layered binding root — check the effective predicate (canonical wins) // against the root expected for this layer. @@ -733,8 +781,18 @@ function checkElement( // rather than relying on `firstBareIdentifier`'s own null-AST guard, so the // one-finding-per-broken-predicate property is visible at the call site // instead of depending on a callee's internals. + // + // #7696 — and skipped for the ONE position where this rule's prescription is + // not available: a bare word on the right of `==` / `!=` on a + // metadata-editing form. There the console parses the right side as a + // literal, so `.` is not a fix but a second defect (a path on the + // right, which `predicate-rhs-path-shaped` refuses at `error`), and this + // rule's own bar — "there is no reading under which it was going to work" — + // is false, because the literal reading works today. `literalRhs` is set only + // where that replacement rule actually fires, so the token is never silenced, + // only diagnosed once. if (source && !refusal) { - const bare = firstBareIdentifier(source); + const bare = firstBareIdentifier(source, literalRhs); if (bare) { const root = CANONICAL_ROOT_BY_LAYER[layer]; findings.push({ @@ -801,6 +859,16 @@ export function validateVisibilityPredicates( for (const site of formViewSites(view, viewPath)) { const where = site.surface ? `view "${viewName}" · ${site.surface}` : `view "${viewName}"`; + // A `{ provider: 'schema', schemaId }` data source is a METADATA-EDITING + // form — the surface the console renders through its own predicate + // evaluator, whose right-hand slot is a literal slot (objectui#4049). + // That is exactly the set `validatePredicatePathRefs` judges, so it is + // exactly the set where standing the bare-identifier verdict down for a + // right-hand word leaves a finding behind rather than a silence (#7696). + // A runtime `*.view.ts` predicate goes to real CEL, where a path on the + // right is perfectly legal and a bare word there really is a dropped + // root — so it is not in this set and its refusal is untouched. + const literalRhs = schemaIdOf(site.view) !== undefined; // `sections` (canonical) and `groups` (legacy alias → sections) both hold // FormSection objects with an optional visibility predicate + `fields`. for (const bucket of ['sections', 'groups'] as const) { @@ -809,13 +877,13 @@ export function validateVisibilityPredicates( const sec = sections[s]; if (!sec || typeof sec !== 'object') continue; const secPath = `${site.path}.${bucket}[${s}]`; - checkElement(sec as AnyRec, where, secPath, layer, findings); + checkElement(sec as AnyRec, where, secPath, layer, findings, literalRhs); const secFields = Array.isArray((sec as AnyRec).fields) ? ((sec as AnyRec).fields as unknown[]) : []; for (let f = 0; f < secFields.length; f++) { const entry = secFields[f]; if (isFieldObject(entry)) { - checkElement(entry, where, `${secPath}.fields[${f}]`, layer, findings); + checkElement(entry, where, `${secPath}.fields[${f}]`, layer, findings, literalRhs); } } }