From 76d46b68a7628343217aac5529a45332c2a98899 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 09:17:45 +0000 Subject: [PATCH 1/2] fix(lint): judge field-rule roots against the bound-somewhere vocabulary (#13935) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `fieldRuleRootIssue` filtered candidate roots through `@objectstack/formula`'s `SCOPE_ROOTS`, which answers "is this declared platform-wide" rather than the question this rule asks, "is this bound at some evaluation site". The two sets agreed for 27 roots and disagreed for `app` — bound by objectui's `ExpressionProvider`, absent from the baseline — so a field-level `*When` reading `app` fell through to the generic bare-reference check and was told to write `record.app`, which then earns `unknown field `app``. Assemble the judged vocabulary in this package as SCOPE_ROOTS plus the ambient roots the spec records in ui/page.zod, leaving the published baseline untouched, and give ambient roots a prescription tier that is true of them. Keep the two partitions disjoint by suppressing the bare-reference verdict for a root this rule has claimed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5 --- .changeset/lint-field-rule-ambient-roots.md | 39 ++++ .../lint/src/validate-expressions.test.ts | 141 +++++++++++++- packages/lint/src/validate-expressions.ts | 183 ++++++++++++++++-- 3 files changed, 338 insertions(+), 25 deletions(-) create mode 100644 .changeset/lint-field-rule-ambient-roots.md diff --git a/.changeset/lint-field-rule-ambient-roots.md b/.changeset/lint-field-rule-ambient-roots.md new file mode 100644 index 0000000000..97c363366d --- /dev/null +++ b/.changeset/lint-field-rule-ambient-roots.md @@ -0,0 +1,39 @@ +--- +"@objectstack/lint": patch +--- + +fix(lint): a field-level `*When` reading `app` gets the scope diagnostic, not the false `record.app` prescription (#13935) + +`fieldRuleRootIssue` judged field-rule roots against `@objectstack/formula`'s +`SCOPE_ROOTS`, which answers "is this root declared **platform-wide**". The +question this rule needs answered is "is this root bound at **some** evaluation +site". The two agreed for all 27 baseline roots and disagreed for exactly one: +`app`, which objectui's `ExpressionProvider` binds on the form-view surface an +author migrates a field rule *down* from. + +Falling outside the membership test sent `app` to the generic bare-reference +check, whose prescription is ``Write `record.app` `` — and following that +advice earns ``unknown field `app` on `invoice` `` from the field-existence +pass. A first diagnostic that asserts something false about where the root +binds, plus a wasted correction cycle. `current_user`, `user`, `ctx`, `os`, +`features` and `data` all got the correct message; `app` alone did not. + +Authoring a field-level `visibleWhen` / `readonlyWhen` / `requiredWhen` on +`app` now earns the same scope diagnostic every other unbound root gets — +"a field-level conditional rule binds only `record` (plus `previous`, and +`parent` on a master-detail line item)" — with a prescription tier of its own +that says what is actually true of an ambient root: it is *not* declared +platform-wide, it is mounted only by the renderer, and `record.app` is +explicitly refused rather than merely omitted, because that is the advice the +author just followed out of the old diagnostic. + +**No accept set moves.** `SCOPE_ROOTS` is `@objectstack/formula`'s published +strict-lint baseline — adding `app` there would stop *every* surface that +judges bare identifiers from faulting it, to fix one surface's wording. The +widened vocabulary is assembled in `@objectstack/lint` instead, where the +per-surface question is asked, and both diagnostics involved were already +`severity: 'error'`, so this changes which message an author reads and nothing +about what lints clean. + +`FIELD_RULE_AMBIENT_ROOTS` and `FIELD_RULE_JUDGED_ROOTS` are exported beside +the existing `FIELD_RULE_BOUND_ROOTS`. diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index 1a5c4291d4..8711cc9ebc 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -10,7 +10,12 @@ import { ExpressionInputSchema, ObjectStackSchema } from '@objectstack/spec'; import { FieldSchema, ObjectSchema, SelectOptionSchema } from '@objectstack/spec/data'; import { SharingRuleSchema } from '@objectstack/spec/security'; -import { validateStackExpressions, FIELD_RULE_BOUND_ROOTS } from './validate-expressions.js'; +import { + validateStackExpressions, + FIELD_RULE_BOUND_ROOTS, + FIELD_RULE_AMBIENT_ROOTS, + FIELD_RULE_JUDGED_ROOTS, +} from './validate-expressions.js'; import type { ExprIssue } from './validate-expressions.js'; // [#8405] Cross-site pin only — see the describe block at the bottom of this // file. Not otherwise used here; validate-semantic-roles.test.ts owns the @@ -956,15 +961,19 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { * resolves in the strict env, so the bare-reference check never fired on it * either, and the denylist did not know it. * - * These tests are written against the IMPORTED `SCOPE_ROOTS`, not a copy of + * These tests are written against the IMPORTED vocabulary, not a copy of * it, because "a future root is covered for free" is the whole argument for * the allowlist and a hand-copied list in the test would assert the opposite - * of what it claims — it would go green on a root the rule never saw. + * of what it claims — it would go green on a root the rule never saw. Since + * #13935 the imported thing is `FIELD_RULE_JUDGED_ROOTS` rather than + * `SCOPE_ROOTS`: the judged vocabulary is now the WIDER "bound at some + * evaluation site" set, and generating from the baseline would have left + * exactly the ambient roots #13935 added out of the table. */ - describe('field-level `*When` roots are an ALLOWLIST over SCOPE_ROOTS (#6713)', () => { + describe('field-level `*When` roots are an ALLOWLIST over the judged vocabulary (#6713/#13935)', () => { /** The three the surface really binds. Everything else must be rejected. */ const BOUND = ['record', 'previous', 'parent'] as const; - const RESIDUAL = SCOPE_ROOTS.filter((r) => !(BOUND as readonly string[]).includes(r)); + const RESIDUAL = FIELD_RULE_JUDGED_ROOTS.filter((r) => !(BOUND as readonly string[]).includes(r)); const fieldIssues = (predicate: string, slot = 'visibleWhen') => validateStackExpressions({ @@ -1019,6 +1028,128 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { expect(hit[0]!.message).toContain('`visibleWhen` reads `data`'); }); + /** + * ── The AMBIENT roots (#13935) ──────────────────────────────────────── + * + * `SCOPE_ROOTS` answers "is this declared platform-wide"; this rule needs + * "is this bound at SOME evaluation site". They agreed for 27 roots and + * disagreed for `app`, which objectui's `ExpressionProvider` binds on the + * very surface an author migrates a rule DOWN from. Falling outside the + * membership test sent `app` to the bare-reference check, which + * prescribed `record.app` — and following THAT earns `unknown field + * \`app\``. The defect is WHICH diagnostic fires, so every assertion here + * names the specific diagnostic rather than counting that "something + * fired". + */ + describe('ambient roots — bound somewhere, absent from SCOPE_ROOTS (#13935)', () => { + /** + * The ruling, pinned as a boundary test rather than restated in prose: + * the fix widens the vocabulary THIS package assembles and leaves + * `@objectstack/formula`'s published accept baseline alone. A future + * edit that "simplifies" this by adding `app` to `SCOPE_ROOTS` widens a + * published accept set — every surface judging bare identifiers stops + * faulting it — and goes red right here. + */ + it('does NOT widen `SCOPE_ROOTS` — the judged set is a strict superset assembled locally', () => { + expect([...FIELD_RULE_AMBIENT_ROOTS]).toEqual(['app']); + // The baseline is untouched: `app` is still not declared platform-wide. + expect(SCOPE_ROOTS).not.toContain('app'); + // …and the judged vocabulary contains all of it, plus the ambient set. + expect(FIELD_RULE_JUDGED_ROOTS).toEqual([...SCOPE_ROOTS, ...FIELD_RULE_AMBIENT_ROOTS]); + expect(FIELD_RULE_JUDGED_ROOTS.length).toBe(SCOPE_ROOTS.length + 1); + }); + + it('gives `app` the SCOPE diagnostic — not the bare-reference prescription', () => { + const hit = fieldIssues("app.locale == 'en'"); + // One verdict, not two: the bare-reference check no longer also fires. + // ⛔ Do not soften this to `toBeGreaterThan(0)` — the length IS the + // pin that catches the suppression silently missing. + expect(hit).toHaveLength(1); + expect(hit[0]!.severity).toBe('error'); + expect(hit[0]!.message).toContain('`visibleWhen` reads `app`'); + expect(hit[0]!.message).toContain('binds only `record`'); + // The wording the card was filed about, in both halves: the generic + // diagnostic's identity, and the prescription that is actively false. + expect(hit[0]!.message).not.toContain('bare reference'); + expect(hit[0]!.message).not.toContain('Write `record.app`'); + }); + + it('tells `app` the truth about where it binds — ambient, renderer-only', () => { + const hit = fieldIssues("app.locale == 'en'"); + expect(hit[0]!.message).toContain('AMBIENT root'); + expect(hit[0]!.message).toContain('⛔ Do NOT write `record.app`'); + // ⛔ NOT the general tier's claim, which is false for an ambient root + // in both of its clauses. + expect(hit[0]!.message).not.toContain('is declared platform-wide'); + }); + + /** + * Why `record.app` had to be refused IN the message rather than merely + * left out: it is exactly what the pre-#13935 diagnostic told this + * author to write, and it does not work. + */ + it('pins that the OLD prescription was false — `record.app` earns `unknown field`', () => { + const hit = fieldIssues('record.app == 1'); + expect(hit).toHaveLength(1); + expect(hit[0]!.message).toContain('unknown field `app`'); + }); + + /** + * The discriminator. `current_user` is the positive control that passed + * before this card and must keep passing UNCHANGED — same tier, same + * prescription. A repair that gave every rejected root the new ambient + * wording would satisfy the `app` assertions above and be wrong. + */ + it('leaves the `current_user` control on the USER tier, not the ambient one', () => { + const hit = fieldIssues("current_user.id == 'U1'"); + expect(hit).toHaveLength(1); + expect(hit[0]!.message).toContain('`visibleWhen` reads `current_user`'); + expect(hit[0]!.message).toContain('move the predicate to the option\'s own'); + expect(hit[0]!.message).not.toContain('AMBIENT root'); + }); + + /** + * Tie-break no-regression. `SCOPE_ROOTS` is spliced in FIRST, so a + * predicate reading both a baseline root and an ambient one reports the + * baseline root — the same root, and the same message, it reported + * before #13935 widened the vocabulary. + */ + it('keeps the pre-#13935 tie-break — a baseline root still wins over an ambient one', () => { + const hit = fieldIssues("ctx.locale == 'en' && app.locale == 'en'"); + expect(hit).toHaveLength(1); + expect(hit[0]!.message).toContain('`visibleWhen` reads `ctx`'); + }); + + /** + * Root-vs-MEMBER, at ambient width: `record.app_id` is an ordinary + * field name that merely starts like the new root. + */ + it('does NOT trip on a `record` member merely spelled like an ambient root', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'showcase_deal', + fields: { + app_id: { type: 'text' }, + gate: { type: 'text', visibleWhen: "record.app_id != ''" }, + }, + }], + }).filter((i) => i.where.includes("field 'gate' visibleWhen")); + expect(issues).toHaveLength(0); + }); + + /** + * The suppression is scoped to the root this rule CLAIMED — an + * unrelated bare field reference in the same predicate keeps its own + * verdict, which is the one that names the author's other mistake. + */ + it('suppresses only the claimed root — a second bare reference still reports', () => { + const hit = fieldIssues("app.locale == 'en' && nope == 1"); + expect(hit.map((i) => i.message).join('\n')).toContain('bare reference `nope`'); + expect(hit.some((i) => i.message.includes('`visibleWhen` reads `app`'))).toBe(true); + expect(hit.some((i) => i.message.includes('bare reference `app`'))).toBe(false); + }); + }); + /** * The partition, both halves. The rule judges `SCOPE_ROOTS` membership, * NOT strict-env declaredness — and that is a measured distinction, not a diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index 5546233821..8b0af1c12f 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -463,19 +463,29 @@ function rulePredicates(rule: AnyRec, path: string): Array<{ label: string; raw: * maintenance burden onto the three roots that are pinned by three anchors and * change only when the evaluators do. * - * The membership test is `SCOPE_ROOTS` minus the allowlist, taken from - * `@objectstack/formula` rather than restated here (#6713 published it for - * this consumer) — one list, one definition, no drift. It deliberately is NOT - * `firstUndeclaredReference`, the declaredness oracle the sibling visibility - * rule uses, and the difference is a measured false positive rather than a - * preference: the strict env also declares CEL's TYPE names, so - * `type(record.x) == string` reports `string` as a root that "resolves". - * Judging by declaredness would reject that legitimate predicate; judging by - * `SCOPE_ROOTS` membership does not. Everything the oracle owns and this list + * The membership test is {@link FIELD_RULE_JUDGED_ROOTS} minus the allowlist. + * Its bulk is `SCOPE_ROOTS`, taken from `@objectstack/formula` rather than + * restated here (#6713 published it for this consumer) — one list, one + * definition, no drift — plus the ambient roots #13935 measured outside that + * baseline; see "Why the judged vocabulary is WIDER than `SCOPE_ROOTS`" below. + * It deliberately is NOT `firstUndeclaredReference`, the declaredness oracle + * the sibling visibility rule uses, and the difference is a measured false + * positive rather than a preference: the strict env also declares CEL's TYPE + * names, so `type(record.x) == string` reports `string` as a root that + * "resolves". Judging by declaredness would reject that legitimate predicate; + * judging by membership does not. Everything the oracle owns and this list * does not — bare field references, comprehension-macro variables — keeps * falling to the bare-reference check, which has the right prescription for * it. The two partitions are disjoint and there is no gap between them. * + * ⚠️ That disjointness used to hold for FREE and no longer does. While every + * judged root was a `SCOPE_ROOTS` member it was declared in the strict env, so + * the bare-reference check could not fire on it whatever this rule decided. + * An AMBIENT root is undeclared there, so both checks see it — the walk + * suppresses the bare-reference verdict for a root this rule has claimed + * (`claimedRoot` on the `check` closure) to keep the invariant true by + * construction instead of by coincidence. + * * ## Why it is an error and not a warning * * Every fault direction available here is silent, and two of the three are @@ -617,14 +627,76 @@ function rulePredicates(rule: AnyRec, path: string): Array<{ label: string; raw: * - **`data`** gets the metadata-form-vs-runtime-form explanation, because * that is what the mistake IS — the same key name, the other form kind's * root; + * - **`app` and the other AMBIENT roots** get the renderer-mounted + * explanation and an explicit refusal of `record.` (#13935, below); * - **everything else** gets the general rewrite, phrased without claiming * which other surface the author copied it from. + * + * ## Why the judged vocabulary is WIDER than `SCOPE_ROOTS` (#13935) + * + * `SCOPE_ROOTS` was the membership test until #13935, and it is the wrong + * question by one word: it answers "is this root declared PLATFORM-WIDE", + * while the rule needs "is this root bound at SOME evaluation site". The two + * agreed for 27 roots and then disagreed for `app` — bound by objectui's + * `ExpressionProvider` on the very surface an author migrates a rule DOWN + * from, absent from the baseline. Falling outside the membership test sent it + * to the bare-reference check, whose prescription is "Write `record.app`" — + * and following that earns ``unknown field `app` `` from the field-existence + * pass one line up. A first diagnostic that is actively false about where the + * root binds, and a wasted correction cycle. + * + * `SCOPE_ROOTS`' own docblock made this measurable rather than a matter of + * taste: its `current_user` entry claims to be "the last one this list was + * missing (#6290)". `app` is that sentence's second counterexample — the same + * mechanism (#6713's point: a hand-maintained list doing a per-surface job + * drifts), a second sighting, not an analogy to the first. + * + * ⛔ The repair deliberately does NOT add `app` to `SCOPE_ROOTS`. That list is + * the published strict-lint accept baseline in `@objectstack/formula`, so + * adding a root there stops EVERY surface that judges bare identifiers from + * faulting it — a widened public accept set, to fix one surface's diagnostic. + * The judged vocabulary is assembled HERE, where the per-surface question is + * asked, and `SCOPE_ROOTS` is left as the proper subset it already is. */ /** * The roots a field-level `*When` predicate binds. Everything else in - * `SCOPE_ROOTS` is rejected — see the allowlist section above. + * {@link FIELD_RULE_JUDGED_ROOTS} is rejected — see the allowlist section above. */ export const FIELD_RULE_BOUND_ROOTS = ['record', 'previous', 'parent'] as const; +/** + * Roots bound at some evaluation site that `SCOPE_ROOTS` does not declare + * (#13935) — the difference between "declared platform-wide" and "bound + * somewhere", which is the question this rule actually asks. + * + * The in-repo source is `packages/spec/src/ui/page.zod` — the `visibleWhen` + * docblock's **"Ambient roots — renderer behaviour, NOT contract-guaranteed"** + * section, which names `app`, `features` and `os.user` as mounted by + * app-shell's `ExpressionProvider`, measured at a pinned objectui sha. Only + * `app` lands here: `features` and `os` are already `SCOPE_ROOTS` members, so + * the intersection of "ambient" and "not in the baseline" is this one root. + * That spec section is deliberately the anchor rather than objectui's list — + * a lint package reaching across repos for a vocabulary is how the drift this + * constant exists to stop gets one repo wider. + * + * ⚠️ Membership here says only that SOMETHING binds the root, which is exactly + * what earns the scope diagnostic instead of the bare-reference one. It is not + * a claim that the FIELD level binds it — that is {@link FIELD_RULE_BOUND_ROOTS}, + * and it is unchanged. + */ +export const FIELD_RULE_AMBIENT_ROOTS = ['app'] as const; +/** + * "Roots bound at some evaluation site" — the vocabulary this rule judges + * against, of which `SCOPE_ROOTS` is a proper subset (#13935). + * + * `SCOPE_ROOTS` comes FIRST so the tie-break in {@link fieldRuleRootIssue} — + * "anything else falls back to `SCOPE_ROOTS` order" — keeps its exact + * pre-#13935 precedence: a predicate reading both a baseline root and an + * ambient one reports the baseline root, the same root it reported before. + */ +export const FIELD_RULE_JUDGED_ROOTS: readonly string[] = [ + ...SCOPE_ROOTS, + ...FIELD_RULE_AMBIENT_ROOTS, +]; /** * ADR-0068 D1's four user spellings, in the order the message's tie-break * prefers them (canonical first — the #6585 ordering, with `os` appended so @@ -698,10 +770,10 @@ export function fieldRuleRootIssue( ): { root: string; message: string } | null { const roots = collectCelRootIdentifiers(source); if (!roots.ok) return null; - // Filtered through SCOPE_ROOTS, in SCOPE_ROOTS order — so a bare field + // Filtered through the judged vocabulary, in its order — so a bare field // reference (owned by the bare-reference check one line up) and a CEL type // name (`type(record.x) == string`) can never land here. - const kept = SCOPE_ROOTS.filter( + const kept = FIELD_RULE_JUDGED_ROOTS.filter( (r) => !(FIELD_RULE_BOUND_ROOTS as readonly string[]).includes(r) && roots.roots.includes(r), ); if (kept.length === 0) return null; @@ -729,7 +801,32 @@ export function fieldRuleRootIssue( `being edited); ` + `this is an OBJECT field, whose runtime form binds the row as \`record\` — one key name, ` + `two form kinds, two roots. Rewrite \`data.\` as \`record.\`.` - : `\`${root}\` is declared platform-wide and bound at OTHER evaluation sites (flow, ` + + : (FIELD_RULE_AMBIENT_ROOTS as readonly string[]).includes(root) + // #13935 — the AMBIENT tier. Everything the general clause below says + // is false about these roots: they are NOT declared platform-wide, and + // the sites that bind them are renderer-side, not flow/automation. The + // `record.` rewrite is refused IN THE MESSAGE rather than merely + // omitted, because that is the advice this author just followed out of + // the bare-reference check, and the second diagnostic it earns + // (`unknown field`) names a different problem than the one they have. + // + // `page.zod` / `ExpressionProvider` are spelled WITHOUT their + // extensions for the same reason `sectionFields` and `*.form` are + // above: this is a STRING literal, and #5017's receiver scan strips + // comments but not strings. + ? `\`${root}\` is NOT declared platform-wide — it is an AMBIENT root, mounted only by ` + + `the renderer (objectui app-shell's \`ExpressionProvider\` binds it beside ` + + `\`current_user\` / \`user\` / \`ctx\` / \`os\` / \`data\` / \`features\`; \`page.zod\` ` + + `records the ambient set as renderer behaviour and explicitly NOT ` + + `contract-guaranteed). So it resolves in a form VIEW's own field predicate and on no ` + + `server path at all, while a field-level object rule is server-enforced. ` + + `⛔ Do NOT write \`record.${root}\`: \`${root}\` is not a field on this object, so that ` + + `spelling only trades this diagnostic for an \`unknown field\` error on \`${root}\`. ` + + `Rewrite the ` + + `predicate against \`record\` (plus \`previous\`, and \`parent\` on a master-detail line ` + + `item), or leave the \`${root}\`-dependent decision on the view's own field predicate ` + + `where \`${root}\` IS bound — renderer-only, enforcing nothing server-side.` + : `\`${root}\` is declared platform-wide and bound at OTHER evaluation sites (flow, ` + `automation, screen and action predicates), never at the field level. Rewrite the ` + `predicate against \`record\` (plus \`previous\`, and \`parent\` on a master-detail line ` + `item), or move the decision to a surface that binds \`${root}\`.`; @@ -744,6 +841,22 @@ export function fieldRuleRootIssue( }; } +/** + * [#13935] Does this `@objectstack/formula` error carry the bare-reference + * verdict for `root`? + * + * Matched on the message's opening clause because that diagnostic carries no + * code to filter on — `validateExpression` pushes `{ source, message }` and + * nothing else. The fragility that buys is bounded and made LOUD rather than + * left silent: a reword upstream makes the suppression miss, an ambient root + * then earns two diagnostics instead of one, and the `toHaveLength(1)` + * assertions in the residual-root table go red. ⛔ Do not soften those to + * `toBeGreaterThan(0)` — the length is the pin. + */ +function isBareReferenceTo(message: string, root: string): boolean { + return message.startsWith(`bare reference \`${root}\``); +} + /** * Validate every predicate in the stack. Returns the list of issues (empty = * clean). Caller decides how to surface / whether to fail the build. @@ -864,6 +977,17 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { raw: unknown, objectName?: string, scope: 'record' | 'flattened' = 'flattened', + /** + * [#13935] A root the field-rule check has already claimed at this site. + * Its bare-reference error is dropped here so the two partitions stay + * DISJOINT — the invariant this rule's docblock has always asserted, which + * used to hold for free (every judged root was a `SCOPE_ROOTS` member, and + * a declared root never trips the bare-reference check) and stops holding + * for free the moment the judged vocabulary is wider than the baseline. + * Without it an ambient root earns BOTH verdicts, one of which is the + * false `record.` prescription this card exists to remove. + */ + claimedRoot?: string, ): void => { if (raw == null) return; const fields = objectName ? fieldIndex.get(objectName) : undefined; @@ -872,7 +996,10 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { const fieldTypes = objectName ? fieldTypeIndex.get(objectName) : undefined; const res = validateExpression('predicate', raw as string | { dialect?: string; source?: string }, objectName ? { objectName, fields, fieldTypes, scope } : { scope }); - for (const e of res.errors) issues.push({ where, message: e.message, source: e.source, severity: 'error' }); + for (const e of res.errors) { + if (claimedRoot && isBareReferenceTo(e.message, claimedRoot)) continue; + issues.push({ where, message: e.message, source: e.source, severity: 'error' }); + } for (const w of res.warnings) issues.push({ where, message: w.message, source: w.source, severity: 'warning' }); // [#8116] Provenance rides every object-bound predicate this helper // validates, whichever scope: the `record`/`previous` roots are explicit @@ -884,13 +1011,21 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { /** * The metadata-walk half: locate the source, then defer to the shared * {@link fieldRuleRootIssue} for the verdict and the message. + * + * [#13935] Split into a COMPUTE half and a PUSH half. The compute half runs + * before {@link check} so the claimed root can be handed to it, while the + * push half still runs after, keeping the emitted order of the two + * diagnostics exactly as it was — a slot can carry a field-rule verdict AND + * an unrelated field-existence error, and tests index into that order. */ - const checkFieldRuleRoot = (where: string, slot: string, raw: unknown): void => { + const fieldRuleRootVerdict = ( + slot: string, + raw: unknown, + ): { root: string; message: string; source: string } | null => { const source = celSourceOf(raw); - if (!source) return; + if (!source) return null; const issue = fieldRuleRootIssue(slot, source); - if (!issue) return; - issues.push({ where, message: issue.message, source, severity: 'error' }); + return issue ? { ...issue, source } : null; }; /** @@ -1084,8 +1219,16 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { // record-scoped — a bare ref silently fails the rule (required/readonly // not enforced = data-integrity hole). #1928 class, same as actions. for (const key of ['requiredWhen', 'readonlyWhen', 'conditionalRequired', 'visibleWhen'] as const) { - check(`object '${objectName}' · field '${fname}' ${key}`, (f as AnyRec)[key], objectName, 'record'); - checkFieldRuleRoot(`object '${objectName}' · field '${fname}' ${key}`, key, (f as AnyRec)[key]); + const where = `object '${objectName}' · field '${fname}' ${key}`; + const raw = (f as AnyRec)[key]; + // [#13935] Verdict FIRST, emitted second. `check` needs to know which + // root this rule has claimed so the two partitions stay disjoint; the + // push order below is the pre-#13935 one. + const verdict = fieldRuleRootVerdict(key, raw); + check(where, raw, objectName, 'record', verdict?.root); + if (verdict) { + issues.push({ where, message: verdict.message, source: verdict.source, severity: 'error' }); + } } // [#6290] Per-OPTION `visibleWhen` — a `select`/`multiselect`/`radio` // option's own predicate (`SelectOptionSchema.visibleWhen`, From 80f50dd5f759e98555d7cde991fa9f5092c793a5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 09:27:03 +0000 Subject: [PATCH 2/2] fix(lint): scope the bare-reference suppression to ambient roots, dodge the #5017 receiver scan (#13935) Three findings from the first full run, all of them real: - A predicate reading a baseline root AND an ambient one kept the bare reference for the ambient one, re-emitting the exact false `record.app` prescription this card removes. Suppression is now gated on a verdict having been issued and covers the ambient roots, not only the root the tie-break named. - #5017's receiver scan reads `page.zod` and `record.${root}` inside a STRING literal as reads off `page` / `record` receivers, exactly as the file's existing `sectionFields` and `*.form` comments warn. Name the spec module in prose and assemble the `record.` spelling with `+`. - The two new locals are named `verdict` / `diagnostic` rather than `message` so excusing them cannot mask a genuine validations[].message read. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5 --- .../lint/src/validate-expressions.test.ts | 68 +++++++++++++-- packages/lint/src/validate-expressions.ts | 82 ++++++++++++------- 2 files changed, 113 insertions(+), 37 deletions(-) diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index 8711cc9ebc..f74e4eb669 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -1113,11 +1113,29 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { * predicate reading both a baseline root and an ambient one reports the * baseline root — the same root, and the same message, it reported * before #13935 widened the vocabulary. + * + * The LENGTH is the second half of this pin and it is the half that + * moved: before #13935 this predicate earned two issues — the `ctx` + * verdict plus a bare reference to `app` prescribing `record.app`, the + * exact false advice this card removes. The rule emits one verdict per + * slot, so `app` waits its turn rather than being told something untrue. */ it('keeps the pre-#13935 tie-break — a baseline root still wins over an ambient one', () => { const hit = fieldIssues("ctx.locale == 'en' && app.locale == 'en'"); expect(hit).toHaveLength(1); expect(hit[0]!.message).toContain('`visibleWhen` reads `ctx`'); + expect(hit[0]!.message).not.toContain('Write `record.app`'); + }); + + /** + * …and the second root is not LOST, only deferred: fixing `ctx` earns + * `app` its own correct verdict on the next run. Without this the pin + * above would be satisfied by a repair that simply dropped the root. + */ + it('reports the ambient root on the next pass, once the baseline root is fixed', () => { + const hit = fieldIssues("record.amount > 0 && app.locale == 'en'"); + expect(hit).toHaveLength(1); + expect(hit[0]!.message).toContain('`visibleWhen` reads `app`'); }); /** @@ -1138,15 +1156,40 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { }); /** - * The suppression is scoped to the root this rule CLAIMED — an - * unrelated bare field reference in the same predicate keeps its own - * verdict, which is the one that names the author's other mistake. + * BLAST RADIUS. The suppression is gated on a field-rule verdict, so it + * reaches the field level and nothing else. A per-OPTION `visibleWhen` + * is deliberately NOT passed through this rule (options resolve against + * the host's predicate scope — see the #6290 note in the field walk), + * so `app` there still meets the bare-reference check exactly as it did + * before this card. Pinned because "suppress the bare-reference verdict" + * is the half of this repair that could quietly go wide. + */ + it('does not reach the per-OPTION surface — `app` there keeps the bare-reference verdict', () => { + const hit = validateStackExpressions({ + objects: [{ + name: 'showcase_deal', + fields: { + gate: { + type: 'select', + options: [{ value: 'a', visibleWhen: "app.locale == 'en'" }], + }, + }, + }], + }).filter((i) => i.where.includes('option')); + expect(hit).toHaveLength(1); + expect(hit[0]!.message).toContain('bare reference `app`'); + }); + + /** + * …and a bare FIELD reference on a field-rule slot is untouched: no + * ambient root is read, so no verdict fires and nothing is suppressed. + * Guards the gate itself — a suppression keyed on the wrong condition + * would swallow this and leave the author with silence. */ - it('suppresses only the claimed root — a second bare reference still reports', () => { - const hit = fieldIssues("app.locale == 'en' && nope == 1"); - expect(hit.map((i) => i.message).join('\n')).toContain('bare reference `nope`'); - expect(hit.some((i) => i.message.includes('`visibleWhen` reads `app`'))).toBe(true); - expect(hit.some((i) => i.message.includes('bare reference `app`'))).toBe(false); + it('leaves a plain bare field reference on a field-rule slot alone', () => { + const hit = fieldIssues("nope == 1"); + expect(hit).toHaveLength(1); + expect(hit[0]!.message).toContain('bare reference `nope`'); }); }); @@ -2395,6 +2438,15 @@ describe('validateStackExpressions — reads only keys the spec declares (meta-t // receiver above) and the provenance index (`unprovisionedIndex` / // `anchors`), whose keys are Map/Set methods, never metadata keys. 'pending', 'celNode', 'celRecv', 'anchors', 'unprovisionedIndex', + // [#13935] The field-rule verdict, split into a compute half and a push + // half so the walk can tell `check` a verdict was issued. `verdict`'s + // keys are this helper's own `{ root, message, source }`, never metadata + // keys; `diagnostic` is a formula error STRING and its one "key" is + // `String.prototype.startsWith`. Both are named to stay clear of the + // `message` / `source` metadata receivers — a local called `message` + // here would have been excused into masking a genuine + // `validations[].message` read. + 'verdict', 'diagnostic', ]); expect(receivers.filter((r) => !tabled.has(r) && !PLUMBING.has(r))).toEqual([]); }); diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index 8b0af1c12f..56f78d28bb 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -810,19 +810,26 @@ export function fieldRuleRootIssue( // the bare-reference check, and the second diagnostic it earns // (`unknown field`) names a different problem than the one they have. // - // `page.zod` / `ExpressionProvider` are spelled WITHOUT their - // extensions for the same reason `sectionFields` and `*.form` are - // above: this is a STRING literal, and #5017's receiver scan strips - // comments but not strings. + // The spec module is named in PROSE ("the page-component schema") + // rather than as a path, and `ExpressionProvider` carries no extension, + // for the same reason `sectionFields` and `*.form` above do not: this + // is a STRING literal, and #5017's receiver scan strips comments but + // not strings, so a `page.zod` inside the message registers `page` as a + // read receiver of this rule. Measured — it went red on the first run, + // exactly as the two siblings did. + // + // `record.` + the root is assembled with `+` for the same scan: written + // as one template literal, `record.${'$'}{root}` reads as a member + // access off a `record` receiver, because `$` is an identifier char. ? `\`${root}\` is NOT declared platform-wide — it is an AMBIENT root, mounted only by ` + `the renderer (objectui app-shell's \`ExpressionProvider\` binds it beside ` + - `\`current_user\` / \`user\` / \`ctx\` / \`os\` / \`data\` / \`features\`; \`page.zod\` ` + - `records the ambient set as renderer behaviour and explicitly NOT ` + + `\`current_user\` / \`user\` / \`ctx\` / \`os\` / \`data\` / \`features\`, and the spec's ` + + `page-component schema records that ambient set as renderer behaviour, explicitly NOT ` + `contract-guaranteed). So it resolves in a form VIEW's own field predicate and on no ` + `server path at all, while a field-level object rule is server-enforced. ` + - `⛔ Do NOT write \`record.${root}\`: \`${root}\` is not a field on this object, so that ` + - `spelling only trades this diagnostic for an \`unknown field\` error on \`${root}\`. ` + - `Rewrite the ` + + `⛔ Do NOT write \`` + 'record.' + `${root}\`: \`${root}\` is not a field on this ` + + `object, so that spelling only trades this diagnostic for an \`unknown field\` error ` + + `on \`${root}\`. Rewrite the ` + `predicate against \`record\` (plus \`previous\`, and \`parent\` on a master-detail line ` + `item), or leave the \`${root}\`-dependent decision on the view's own field predicate ` + `where \`${root}\` IS bound — renderer-only, enforcing nothing server-side.` @@ -843,18 +850,23 @@ export function fieldRuleRootIssue( /** * [#13935] Does this `@objectstack/formula` error carry the bare-reference - * verdict for `root`? + * verdict for one of `roots`? * - * Matched on the message's opening clause because that diagnostic carries no - * code to filter on — `validateExpression` pushes `{ source, message }` and - * nothing else. The fragility that buys is bounded and made LOUD rather than - * left silent: a reword upstream makes the suppression miss, an ambient root - * then earns two diagnostics instead of one, and the `toHaveLength(1)` - * assertions in the residual-root table go red. ⛔ Do not soften those to + * Matched on the diagnostic's opening clause because it carries no code to + * filter on — `validateExpression` pushes `{ source, message }` and nothing + * else. The fragility that buys is bounded and made LOUD rather than left + * silent: a reword upstream makes the suppression miss, an ambient root then + * earns two diagnostics instead of one, and the `toHaveLength(1)` assertions + * in the residual-root table go red. ⛔ Do not soften those to * `toBeGreaterThan(0)` — the length is the pin. + * + * The parameter is `diagnostic` rather than `message` so #5017's receiver scan + * reads the `.startsWith` below as this file's own plumbing instead of as a + * read off a `message` metadata receiver, which is a real key elsewhere in the + * spec (`validations[].message`). */ -function isBareReferenceTo(message: string, root: string): boolean { - return message.startsWith(`bare reference \`${root}\``); +function isBareReferenceToAny(diagnostic: string, roots: readonly string[]): boolean { + return roots.some((root) => diagnostic.startsWith(`bare reference \`${root}\``)); } /** @@ -978,16 +990,28 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { objectName?: string, scope: 'record' | 'flattened' = 'flattened', /** - * [#13935] A root the field-rule check has already claimed at this site. - * Its bare-reference error is dropped here so the two partitions stay - * DISJOINT — the invariant this rule's docblock has always asserted, which - * used to hold for free (every judged root was a `SCOPE_ROOTS` member, and - * a declared root never trips the bare-reference check) and stops holding - * for free the moment the judged vocabulary is wider than the baseline. - * Without it an ambient root earns BOTH verdicts, one of which is the - * false `record.` prescription this card exists to remove. + * [#13935] Set when the field-rule check has issued a verdict for this + * site. Bare-reference errors naming an AMBIENT root are dropped so the + * two partitions stay DISJOINT — the invariant this rule's docblock has + * always asserted, which used to hold for free (every judged root was a + * `SCOPE_ROOTS` member, and a declared root never trips the bare-reference + * check) and stops holding for free the moment the judged vocabulary is + * wider than the baseline. Without it an ambient root earns BOTH verdicts, + * one of which is the false `record.` prescription this card exists + * to remove. + * + * The suppressed set is the ambient roots rather than only the root the + * verdict NAMED, and the difference shows up when one predicate reaches + * for two rejected roots. This rule emits one verdict per slot, so with + * `ctx.locale == 'en' && app.locale == 'en'` the tie-break names `ctx` and + * `app` would otherwise keep its bare-reference — re-emitting the exact + * false prescription, on the exact root, that this card removes. Suppressed, + * the author fixes `ctx`, re-runs, and `app` earns its own correct verdict: + * the same one-root-at-a-time iteration this rule already does for two + * baseline roots. Baseline roots need no entry — being declared in the + * strict env, they never trip the bare-reference check at all. */ - claimedRoot?: string, + fieldRuleVerdictIssued?: boolean, ): void => { if (raw == null) return; const fields = objectName ? fieldIndex.get(objectName) : undefined; @@ -997,7 +1021,7 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { const res = validateExpression('predicate', raw as string | { dialect?: string; source?: string }, objectName ? { objectName, fields, fieldTypes, scope } : { scope }); for (const e of res.errors) { - if (claimedRoot && isBareReferenceTo(e.message, claimedRoot)) continue; + if (fieldRuleVerdictIssued && isBareReferenceToAny(e.message, FIELD_RULE_AMBIENT_ROOTS)) continue; issues.push({ where, message: e.message, source: e.source, severity: 'error' }); } for (const w of res.warnings) issues.push({ where, message: w.message, source: w.source, severity: 'warning' }); @@ -1225,7 +1249,7 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { // root this rule has claimed so the two partitions stay disjoint; the // push order below is the pre-#13935 one. const verdict = fieldRuleRootVerdict(key, raw); - check(where, raw, objectName, 'record', verdict?.root); + check(where, raw, objectName, 'record', verdict !== null); if (verdict) { issues.push({ where, message: verdict.message, source: verdict.source, severity: 'error' }); }