fix(app-shell): lint conditional-formatting conditions in the record scope

.changeset/7727-conditional-formatting-record-scope.md

@@ -0,0 +1,75 @@
+---
+'@object-ui/app-shell': minor
+---
+
+Lint conditional-formatting conditions in the `record` scope, and stop advertising
+`data` (objectui#7727).
+
+**Breaking for authors, deliberately.** A bare field reference in a list/grid/kanban
+`conditionalFormatting` condition — `status == 'overdue'` — used to lint clean in
+Studio's conditional-formatting editor and now raises a blocking error carrying the
+`record.status` fix.
+
+**Read this before upgrading.** The error is a *blocking* one: it bubbles through
+`onBlockingIssuesChange` (objectui#4527), which the inspector aggregates and the host
+that owns Save reads. So an already-saved view whose `conditionalFormatting` carries a
+legacy bare condition becomes **unsavable in the designer until that condition is
+rewritten** — including when you opened the view to change something unrelated. Nothing
+is migrated automatically and nothing at runtime changes: those conditions were already
+dead (see below), the editor just stops hiding it. Rewrite `status == 'overdue'` as
+`record.status == 'overdue'`.
+
+The editor was teaching a spelling the runtime had already retired. objectui#5741
+(Phase 2 of the objectui#5330 canon, ruled 2026-09-02 and amended 2026-09-05) unbound
+the bare shorthand and `data.*` on runtime record surfaces: `evalRowPredicate` binds the
+row as `record.*` and nothing else, so `status == 'overdue'` faults with
+`Unknown variable: status` and the authored rule never matches. The editor nevertheless
+linted it green, because it authored in the `flattened` scope — where any bare
+identifier is legal. That is declared-but-unenforced in the direction that costs an
+author a silently dead formatting rule.
+
+Three changes, all on `ConditionalFormattingEditor`:
+
+- its `CelPredicateField` authors in `scope="record"`, the scope the field conditional
+  rules `visibleWhen` / `readonlyWhen` / `requiredWhen` already use;
+- `ROW_PREDICATE_ROOTS` loses `'data'`, which Phase 2 retired but autocomplete was
+  still recommending. It is an `export const`, but **not** on this package's
+  published face: `@object-ui/app-shell`'s `index.ts` has no `export *` lines and
+  re-exports neither the const nor this editor, and the package `exports` map is
+  `"."` plus `./styles.css` with no deep subpath — so no consumer outside the
+  package can import it, and nothing you depend on changes shape;
+- the docblock and inline comment that described the old three-way binding are
+  rewritten to the one binding that survives.
+
+**Autocomplete moves with the scope.** Under `scope="record"`, `CelPredicateField`
+builds its bare-position catalog with `fields: []`, so typing `sta` at the start of a
+condition no longer offers `status`; fields are offered as member completion after
+`record.` instead. That is the correct affordance for the new scope — the bare form it
+used to complete is now an error — and the member-completion list itself is unchanged:
+the engine's `introspectScope` returns byte-identical `fields` for `record` and
+`flattened` (measured against `@objectstack/formula@17.2.0`; it echoes the caller's
+`fields` hint rather than deriving one per scope).
+
+The `flattened` default at the shared authoring seam is **untouched**: RLS predicates
+and flow conditions are not row surfaces (objectui#5738 stand-down 3) and stay
+flattened.
+
+**What this does NOT close — two halves are left open, both filed.**
+
+- **The `data.*` half.** Dropping `'data'` from `ROW_PREDICATE_ROOTS` stops
+  *recommending* it; it does not stop the lint *accepting* it.
+  `@objectstack/formula`'s `SCOPE_ROOTS` lists `data`, so `data.status == 'x'` still
+  lints clean at `scope:'record'` while resolving against the host's ambient `data`
+  rather than the row — constant-false, silently. Pinned here as a characterization
+  test, tracked as objectui#8166. This changeset closes the **bare-field** half of the
+  retirement only.
+- **The `app` root.** `app` is bound at runtime by app-shell's predicate scope and
+  advertised by this editor, but `SCOPE_ROOTS` has no `app`, so under `scope="record"`
+  the lint now refuses it. Measured, pinned, and filed as objectui#8155.
+
+⛔ And this editor is **not** the last authoring site still on the flattened default —
+`ConditionBuilder` reaches it by passing no `scope` at all, which is why a grep for the
+explicit spelling missed it. An action's `visible` / `disabled` guard is a row predicate
+by the canon's own words and still lints bare refs clean. Filed as objectui#8167; ⛔ not
+fixed here, because three of `ConditionBuilder`'s six callers need a per-surface tier
+verdict first.

packages/app-shell/src/views/metadata-admin/ConditionalFormattingEditor.test.tsx

@@ -2,7 +2,7 @@
 
 import * as React from 'react';
 import { describe, it, expect, afterEach } from 'vitest';
-import { render, screen, cleanup, fireEvent } from '@testing-library/react';
+import { render, screen, cleanup, fireEvent, waitFor } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 import { evalRowPredicate } from '@object-ui/core';
 import {
@@ -12,6 +12,7 @@ import {
   type ConditionalFormattingRuleDraft,
 } from './ConditionalFormattingEditor';
 import { __setCelFormulaLoader } from './celAuthoring';
+import { buildExpressionScope } from '../../providers/ExpressionProvider.js';
 
 afterEach(() => {
   cleanup();
@@ -133,15 +134,94 @@ describe('ConditionalFormattingEditor', () => {
 });
 
 describe('ConditionalFormattingEditor · CEL authoring scope (#2571 follow-up)', () => {
-  it('lints a BARE field condition clean — row predicates bind fields bare at runtime', async () => {
+  it('flags a BARE field condition with the record.<field> fix — the row binds only record.*', async () => {
     render(<Harness initial={[{ condition: "status == 'overdue'", style: {} }]} />);
-    // The real engine must accept the bare form (evalRowPredicate spreads the
-    // row); flipping this editor to scope="record" would break this test.
+    // TURNED, deliberately (objectui#7727). This pin used to assert the
+    // opposite — "the real engine must accept the bare form (evalRowPredicate
+    // spreads the row)" — and its own comment predicted this edit: "flipping
+    // this editor to scope=\"record\" would break this test". objectui#5741
+    // (Phase 2 of the objectui#5330 canon) retired the bare shorthand on
+    // runtime record surfaces, so `evalRowPredicate` no longer spreads the row
+    // and `status == 'overdue'` faults with `Unknown variable: status`. The
+    // editor must say so at authoring time rather than lint it clean; the
+    // runtime half of this claim is pinned in the contract suite below.
+    expect(await screen.findByText(/record\.status/, {}, { timeout: 3000 })).toBeTruthy();
+    const ta = document.getElementById('cf-condition-0') as HTMLTextAreaElement;
+    await waitFor(() => expect(ta.getAttribute('aria-invalid')).toBe('true'), { timeout: 3000 });
+  });
+
+  it('still lints a canonical record.<field> condition clean', async () => {
+    render(<Harness initial={[{ condition: "record.status == 'overdue'", style: {} }]} />);
+    // The other half of the narrowing: the scope flip must reject the retired
+    // spelling WITHOUT rejecting the canonical one.
     expect(await screen.findByText('perm.cel.valid', {}, { timeout: 3000 })).toBeTruthy();
     const ta = document.getElementById('cf-condition-0') as HTMLTextAreaElement;
     expect(ta.getAttribute('aria-invalid')).not.toBe('true');
   });
 
+  it('lints the host roots the ENGINE KNOWS clean in the record scope', async () => {
+    // Four of the five advertised host roots survive the narrowing. Deliberately
+    // NOT "all advertised host roots": the fifth, `app`, does not — see the
+    // known-gap pin below. Saying "advertised" here while the next test proves
+    // `app` is refused would make this comment contradict its own neighbour.
+    // What these four have in common is not that this editor advertises them,
+    // it is that `@objectstack/formula`'s `SCOPE_ROOTS` lists them.
+    render(
+      <Harness
+        initial={[
+          { condition: "features.beta && current_user.id != '' && user.id != '' && ctx.user.id != ''", style: {} },
+        ]}
+      />,
+    );
+    expect(await screen.findByText('perm.cel.valid', {}, { timeout: 3000 })).toBeTruthy();
+  });
+
+  it('KNOWN GAP — a `data.*` condition still lints CLEAN although the row is not bound under it', async () => {
+    // NOT desired behaviour, and it is the half of the retirement this card
+    // does NOT close. Dropping `'data'` from ROW_PREDICATE_ROOTS stops
+    // RECOMMENDING it; it does not stop the lint ACCEPTING it, because
+    // `@objectstack/formula`'s `SCOPE_ROOTS` lists `data` and so the
+    // record-scope bare-reference check waves it through. `rowPredicateCanon.ts`
+    // already records exactly this for the server oracle: `data.status` is
+    // "⚠️ silently accepted" while the runtime faults on it.
+    //
+    // The runtime half is pinned in the contract suite below, where the same
+    // predicate against the same host bag evaluates to FALSE. Green here plus
+    // false there IS the defect. This test REDDENS when the acceptance is
+    // fixed, at which point objectui#8166 can be closed.
+    render(<Harness initial={[{ condition: "data.status == 'overdue'", style: {} }]} />);
+    expect(await screen.findByText('perm.cel.valid', {}, { timeout: 3000 })).toBeTruthy();
+    const ta = document.getElementById('cf-condition-0') as HTMLTextAreaElement;
+    expect(ta.getAttribute('aria-invalid')).not.toBe('true');
+  });
+
+  it('KNOWN GAP — an `app.*` condition is advertised yet the record-scope lint refuses it', async () => {
+    // NOT desired behaviour. Pinned so the one regression the scope flip
+    // introduces cannot go silent, and so this test REDDENS the day it is
+    // fixed and objectui#8155 can be closed.
+    //
+    // `app` IS bound at runtime: app-shell's `buildExpressionScope`
+    // (ExpressionProvider, #1583/ADR-0068) puts it in the predicate scope that
+    // `ObjectGrid` / `ListView` hand to `resolveConditionalFormatting`, and
+    // ROW_PREDICATE_ROOTS advertises it for that reason. But
+    // `@objectstack/formula`'s `SCOPE_ROOTS` (17.2.0) has no `app`, so under
+    // `scope="record"` the engine reads it as a bare field reference and
+    // errors with the nonsense fix `record.app`. Under the previous
+    // `scope="flattened"` it was clean, because flattened accepts ANY bare
+    // identifier. Full measurement and the two candidate fixes: objectui#8155.
+    //
+    // ⚠️ Reads on objectui#8155 OPTION A only — adding `app` to the engine's
+    // `SCOPE_ROOTS`. Under option B (app stops being bound and leaves
+    // ROW_PREDICATE_ROOTS) this test would still pass, so it is not a complete
+    // tripwire for that card; the closure assertion in the contract suite
+    // below is what catches option B, because it reads the advertised list
+    // against `buildExpressionScope` itself.
+    render(<Harness initial={[{ condition: "app.name == 'crm'", style: {} }]} />);
+    const ta = document.getElementById('cf-condition-0') as HTMLTextAreaElement;
+    await waitFor(() => expect(ta.getAttribute('aria-invalid')).toBe('true'), { timeout: 3000 });
+    expect(screen.getByText(/bare reference/)).toBeTruthy();
+  });
+
   it('still flags an unknown record.<field> with did-you-mean', async () => {
     render(<Harness initial={[{ condition: "record.statu == 'x'", style: {} }]} />);
     expect(await screen.findByText(/did you mean/i, {}, { timeout: 3000 })).toBeTruthy();
@@ -176,36 +256,138 @@ describe('ConditionalFormattingEditor · CEL authoring scope (#2571 follow-up)',
 });
 
 describe('ROW_PREDICATE_ROOTS ↔ evalRowPredicate runtime contract', () => {
-  // Shaped like the app-shell global predicate scope (ExpressionProvider,
-  // #1583/ADR-0068) that hosts pass into the shared row-predicate evaluator.
   const u = { id: 'u1' };
-  const hostScope = {
-    current_user: u,
+  /**
+   * The app-shell global predicate scope that hosts hand to the shared
+   * row-predicate evaluator — READ FROM ITS PRODUCER, not modelled here.
+   *
+   * Why it is read rather than written out (objectui#7727). This block used to
+   * carry a hand-written literal including `data: {}`, and probed every
+   * advertised root with `size(<root>) >= 0`. For `data` that probe hit the
+   * HOST's own empty object and never the row, so it was green whether or not
+   * `data` named the row: a reading that could not fail, and therefore
+   * indistinguishable from one that passed — the exact trap
+   * `rowPredicateCanon.ts` documents for `data.*` on a record surface.
+   *
+   * Writing the bag out by hand is the same defect one level up: a literal
+   * cannot disagree with the producer, so it silently absorbs any drift. It had
+   * already drifted — the literal omitted `os`, which
+   * `buildExpressionScope` really does bind, and an assertion below therefore
+   * "proved" `os` unbound. Calling the producer is what makes these readings
+   * able to fail: if `buildExpressionScope` gains or loses a root, the closure
+   * assertion says so instead of quietly agreeing with itself.
+   */
+  const fullHostScope = buildExpressionScope({
     user: u,
-    ctx: { user: u },
     app: { name: 'crm' },
     data: {},
     features: { beta: true },
-  };
+  });
+  /**
+   * Roots the host binds that this editor deliberately does NOT advertise.
+   * `data` is retired on row surfaces (objectui#5741); `os` is an alias bag
+   * withheld by curation (objectui#8156). Both get their own pins below,
+   * against `fullHostScope`, which does carry them.
+   */
+  const CURATED_EXCLUSIONS = ['os', 'data'];
+  /**
+   * The same bag with those two removed. Probes for the ADVERTISED roots run
+   * against this one, so no probe can pass off a host binding as a row binding.
+   */
+  const hostScope = Object.fromEntries(
+    Object.entries(fullHostScope).filter(([k]) => !CURATED_EXCLUSIONS.includes(k)),
+  );
+  const row = { id: 'r1', status: 'overdue' };
+
+  /** Advertised roots the HOST binds — derived, never typed out. */
+  const HOST_BOUND_ROOTS = Object.keys(fullHostScope).filter((k) => !CURATED_EXCLUSIONS.includes(k));
+
+  it('binds the row as `record`, and it is the ROW rather than a host `record`', () => {
+    // No host scope at all: only the row can be supplying `record`.
+    expect(evalRowPredicate("record.status == 'overdue'", row, { fallback: false })).toBe(true);
+    // And the row still wins over a host scope carrying its own `record`
+    // (listConditional.ts pins `record` AFTER the spread).
+    expect(
+      evalRowPredicate("record.status == 'overdue'", row, {
+        fallback: false,
+        scope: { ...hostScope, record: { status: 'paid' } },
+      }),
+    ).toBe(true);
+  });
 
-  it('every advertised root is bound when a row predicate evaluates', () => {
+  it('every OTHER advertised root is bound by the HOST — and unbound without it', () => {
     for (const root of ROW_PREDICATE_ROOTS) {
-      // `size(<root>) >= 0` is true iff the root resolves to a bound map —
-      // an unbound root faults and falls back to `false`.
+      if (root === 'record') continue;
+      expect(HOST_BOUND_ROOTS, `advertised root "${root}" is unaccounted for`).toContain(root);
       expect(
-        evalRowPredicate(`size(${root}) >= 0`, { id: 'r1' }, { fallback: false, scope: hostScope }),
-        `root "${root}" should be bound at runtime`,
+        evalRowPredicate(`size(${root}) >= 0`, row, { fallback: false, scope: hostScope }),
+        `root "${root}" should be bound by the host scope`,
       ).toBe(true);
+      // The half that makes the line above a reading: drop the host scope and
+      // the root must go unbound. Without this, a root bound by nothing in
+      // particular would still pass.
+      expect(
+        evalRowPredicate(`size(${root}) >= 0`, row, { fallback: false }),
+        `root "${root}" must come from the HOST scope, not from thin air`,
+      ).toBe(false);
     }
+    // ...and no member escapes the two assertions above by not being checked.
+    // Both sides are derived: the left from the editor, the right from
+    // `buildExpressionScope` minus the curated exclusions. Drift on either
+    // side — a root added to the host bag, a root added to or dropped from the
+    // advertised list — reddens here.
+    expect([...ROW_PREDICATE_ROOTS].sort()).toEqual([...HOST_BOUND_ROOTS, 'record'].sort());
+  });
+
+  it('a BARE field ref no longer names the row — the editor ERROR matches the runtime', () => {
+    // The runtime half of the flipped authoring pin above (objectui#5741).
+    expect(evalRowPredicate("record.status == 'overdue'", row, { fallback: false, scope: hostScope })).toBe(true);
+    expect(evalRowPredicate("status == 'overdue'", row, { fallback: false, scope: hostScope })).toBe(false);
+  });
+
+  it('`data` is RETIRED: unadvertised, and an ambient host `data` never names the row', () => {
+    expect(ROW_PREDICATE_ROOTS).not.toContain('data');
+    // A host may still legitimately carry its own ambient `data` — app-shell's
+    // `buildExpressionScope` does, and this is that bag rather than a model of
+    // it. That is what made the old probe useless...
+    const ambient = fullHostScope;
+    expect(evalRowPredicate('size(data) >= 0', row, { fallback: false, scope: ambient })).toBe(true);
+    // ...while the ROW is not reachable through it at all. Canonical spelling
+    // against the same scope, so the two differ only in the spelling.
+    expect(evalRowPredicate("record.status == 'overdue'", row, { fallback: false, scope: ambient })).toBe(true);
+    expect(evalRowPredicate("data.status == 'overdue'", row, { fallback: false, scope: ambient })).toBe(false);
+    // ⚠️ The line above is FALSE at runtime while the authoring pin above
+    // ("a `data.*` condition still lints CLEAN") is green. That pair is the
+    // half of the retirement this card does not close — objectui#8166.
   });
 
   it('the engine-default extras stay unadvertised because they are NOT bound', () => {
-    for (const root of ['previous', 'input', 'os', 'vars']) {
+    // `os` is NOT in this list any more: it is unadvertised but genuinely
+    // bound, so asserting it here would be the same hand-model artefact as the
+    // old `data` probe, in the opposite direction. See the pin below.
+    for (const root of ['previous', 'input', 'vars']) {
       expect(ROW_PREDICATE_ROOTS).not.toContain(root);
       expect(
-        evalRowPredicate(`size(${root}) >= 0`, { id: 'r1' }, { fallback: false, scope: hostScope }),
+        evalRowPredicate(`size(${root}) >= 0`, row, { fallback: false, scope: hostScope }),
         `root "${root}" should NOT be bound at runtime`,
       ).toBe(false);
     }
   });
+
+  it('`os` is unadvertised by CURATION, not because it is unbound', () => {
+    // The bag here is `buildExpressionScope`'s own output, NOT a scope with
+    // `os` handed in by this test: injecting it would only have proved that
+    // `evalRowPredicate` forwards `scope`, which `size(zzz) >= 0` with `zzz`
+    // injected proves just as well. Reading the producer is what makes this a
+    // statement about app-shell. Held apart from the extras above so that list
+    // keeps meaning "not bound". Whether `os` SHOULD be advertised is
+    // objectui#8156, not this card.
+    expect(CURATED_EXCLUSIONS).toContain('os');
+    expect(Object.keys(fullHostScope)).toContain('os');
+    expect(ROW_PREDICATE_ROOTS).not.toContain('os');
+    expect(evalRowPredicate('size(os) >= 0', row, { fallback: false, scope: fullHostScope })).toBe(true);
+    // The control that makes the line above a reading: a root the host bag does
+    // NOT carry is unbound against the very same scope.
+    expect(evalRowPredicate('size(zzz) >= 0', row, { fallback: false, scope: fullHostScope })).toBe(false);
+  });
 });