feat(app-shell): warn at typing time on a record-scope data.* predicate

.changeset/8972-cel-authoring-data-root-advisory.md

@@ -0,0 +1,19 @@
+---
+'@object-ui/app-shell': minor
+---
+
+Warn at typing time when a record-scope CEL predicate is rooted on `data`.
+
+`@objectstack/formula`'s scope vocabulary still accepts `data`, so a
+`visibleWhen` / `readonlyWhen` / `requiredWhen`, a formula `expression` or a
+conditional-formatting `condition` written as `data.status == 'x'` used to lint
+green and then fault at runtime with `Unknown variable: data`, because the row
+is bound as `record.*` and nothing else (objectui#5741, objectui#8166). The
+metadata editors now surface `@object-ui/core`'s `detectNonCanonicalRowSpelling`
+as an inline warning naming `record` as the fix.
+
+The accepted set is unchanged: this is advisory only, so it does not block save
+and does not mark the field invalid. It applies to `scope: 'record'` authoring
+sites only — flattened RLS `USING` / `CHECK` predicates, where a bare field
+reference is the correct spelling, are untouched, as is the metadata-editing
+layer where `data` is canonical.

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

@@ -177,23 +177,33 @@ describe('ConditionalFormattingEditor · CEL authoring scope (#2571 follow-up)',
     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.
+  it('a `data.*` condition is ACCEPTED but no longer SILENT — the author gets a warning (objectui#8972)', async () => {
+    // This pin used to assert an unbroken silence, and said it would redden
+    // "when the acceptance is fixed". Read that literally: the acceptance is
+    // NOT fixed here. `@objectstack/formula`'s `SCOPE_ROOTS` still lists
+    // `data`, so the engine still waves this through with zero findings, and
+    // narrowing that set remains the producer-side half (objectui#8166's
+    // ruling). What objectui#8972 changed is the other half of the defect —
+    // the ABSENCE of any diagnostic — by wiring `@object-ui/core`'s
+    // `detectNonCanonicalRowSpelling` into `celAuthoring` as a WARNING.
     //
-    // 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.
+    // So the two halves are now asserted separately, and the split is the
+    // point: the accept set is untouched (no error, `aria-invalid` unset, the
+    // editor's own error count unchanged, save open), while the author is told
+    // at typing time instead of at misbehaviour time. The runtime half is
+    // pinned in the contract suite below, where the same predicate against the
+    // same host bag evaluates to FALSE.
     render(<Harness initial={[{ condition: "data.status == 'overdue'", style: {} }]} />);
-    expect(await screen.findByText('perm.cel.valid', {}, { timeout: 3000 })).toBeTruthy();
+    expect(
+      await screen.findByText(/Re-root the reference on/, {}, { timeout: 3000 }),
+    ).toBeTruthy();
+    // ACCEPT SET UNCHANGED — the falsifiable half. Promoting the advisory to
+    // an error reddens both of these.
     const ta = document.getElementById('cf-condition-0') as HTMLTextAreaElement;
     expect(ta.getAttribute('aria-invalid')).not.toBe('true');
+    // `border-destructive` is applied from the same `errors.length > 0` the
+    // Save gate counts, so its absence is the editor's own "no blocking issue".
+    expect(ta.className).not.toMatch(/border-destructive/);
   });
 
   it('ALIGNED (objectui#8155) — `app` is neither advertised nor bound, and the lint refuses it', async () => {

packages/app-shell/src/views/metadata-admin/celAuthoring.test.ts

@@ -83,6 +83,67 @@ describe('celAuthoring · lintCelPredicate in record scope (field conditional ru
   });
 });
 
+/**
+ * objectui#8972 — the wrong-layer `data.*` advisory.
+ *
+ * The engine's own `SCOPE_ROOTS` carries `data`, so every assertion in here
+ * that a finding EXISTS is an assertion about this module, not about
+ * `@objectstack/formula`: measured on `@objectstack/formula@17.4.0`,
+ * `validateExpression('predicate', "data.status == 'x'", { scope: 'record' })`
+ * answers `{ ok: true, errors: [], warnings: [] }`.
+ *
+ * Three of the five pins below are LIVE CONTROLS rather than true-positive
+ * pins, and they are the reason this is a WARNING and not an error. Each one
+ * describes a world the advisory must NOT create, so each one stays green
+ * across both legs of the ablation.
+ */
+describe('celAuthoring · the wrong-layer `data.*` advisory (objectui#8972)', () => {
+  const RULE_HINT = { ...HINT, scope: 'record' as const };
+
+  it('TRUE POSITIVE — a record-scope `data.*` predicate now warns, naming `record` as the fix', async () => {
+    const issues = await lintCelPredicate("data.status == 'x'", RULE_HINT);
+    const advisory = issues.filter((i) => /\bdata\b/.test(i.message) && /Re-root/.test(i.message));
+    expect(advisory).toHaveLength(1);
+    expect(advisory[0].severity).toBe('warning');
+    expect(advisory[0].message).toMatch(/`record`/);
+  });
+
+  it('LIVE CONTROL — the ACCEPT SET is not narrowed: the same predicate raises no error', async () => {
+    // Every save gate on this tier counts `severity === 'error'` and nothing
+    // else, so "zero errors" IS "still accepted". Deliberately asserts ONLY
+    // that: it is green with the advisory and green without it, and reddens on
+    // exactly one change — promoting the advisory to `error`. That is the
+    // falsifiable form of WARN-not-REFUSE, and mixing the presence of the
+    // warning into it would turn the control into a second true-positive pin.
+    const issues = await lintCelPredicate("data.status == 'x'", RULE_HINT);
+    expect(issues.filter((i) => i.severity === 'error')).toEqual([]);
+  });
+
+  it('LIVE CONTROL — the canonical spelling stays completely clean', async () => {
+    // `rowPredicateCanon.test.ts` pins the detector returning null for this;
+    // this pins that the wiring does not turn it into a finding anyway.
+    expect(await lintCelPredicate("record.status == 'x'", RULE_HINT)).toEqual([]);
+  });
+
+  it('LIVE CONTROL — a FLATTENED (RLS) predicate is untouched, bare identifiers included', async () => {
+    // The detector's other arm would fire on all three genuine RLS predicates
+    // in this repo. The advisory is gated on `scope: 'record'` and passes
+    // `row = null`, so neither arm can reach this tier.
+    expect(await lintCelPredicate('organization_id == current_user.organization_id', HINT)).toEqual([]);
+    expect(await lintCelPredicate("data.status == 'x'", HINT)).toEqual([]);
+  });
+
+  it('adds nothing on top of a parse error, and stands down on a non-CEL dialect', async () => {
+    const broken = await lintCelPredicate('data.status ==', RULE_HINT);
+    expect(broken.some((i) => i.severity === 'error')).toBe(true);
+    expect(broken.some((i) => /Re-root/.test(i.message))).toBe(false);
+    // A legacy `${…}` string is not CEL; the detector stands down and so must
+    // this — classifying it belongs to its own dialect's rules.
+    const legacy = await lintCelPredicate('${data.status}', RULE_HINT);
+    expect(legacy.some((i) => /Re-root/.test(i.message))).toBe(false);
+  });
+});
+
 describe('celAuthoring · lintCelPredicate role "value" (formula expressions, #1582 follow-up)', () => {
   const FORMULA_HINT = { ...HINT, scope: 'record' as const, role: 'value' as const };
 

packages/app-shell/src/views/metadata-admin/celAuthoring.ts

@@ -181,6 +181,110 @@ const PUSHDOWN_FIELD_ROOTS = ['record', ''] as const;
 /** Roots resolved as scope VALUES for pushdown analysis. */
 const PUSHDOWN_VARIABLE_ROOTS = ['current_user', 'user'] as const;
 
+/* ── Lazy row-canon detector (objectui#8972) ─────────────────────────── */
+
+/** The shape of `@object-ui/core`'s offline row-spelling instrument. */
+interface RowCanonModule {
+  detectNonCanonicalRowSpelling?: (
+    source: string,
+    row: Record<string, unknown> | null | undefined,
+    dataNamesRow: boolean,
+  ) => { kind: string; identifier: string; canonical: string } | null;
+}
+
+let rowCanonCached: Promise<RowCanonModule | null> | null = null;
+
+/**
+ * Load `@object-ui/core`'s row-spelling detector the same way the engine is
+ * loaded: lazily, feature-detected, swallowing every failure. The detector
+ * imports `@objectstack/formula` at module scope, so a STATIC import here
+ * would drag the CEL parser into whatever chunk holds this module and undo the
+ * bundle split the header describes.
+ */
+function loadRowCanon(): Promise<RowCanonModule | null> {
+  if (!rowCanonCached) {
+    rowCanonCached = import('@object-ui/core')
+      .then((m) => m as unknown as RowCanonModule)
+      .catch(() => null);
+  }
+  return rowCanonCached;
+}
+
+/**
+ * The wrong-layer `data.*` advisory — why it is wired HERE, and why it is only
+ * an advisory (objectui#8972).
+ *
+ * ## What it closes
+ *
+ * `@objectstack/formula`'s `SCOPE_ROOTS` carries `data`, so at `scope: 'record'`
+ * the engine lint ACCEPTS `data.status == 'x'` with zero findings (measured on
+ * `@objectstack/formula@17.4.0`). objectui#5741 retired that spelling on runtime
+ * record surfaces and objectui#8166 stopped this tier binding an ambient `data`,
+ * so the predicate now faults at runtime with `Unknown variable: data` — but the
+ * author still gets a GREEN lint while typing it. That gap is the whole card:
+ * the diagnostic arrives at misbehaviour time instead of at typing time.
+ *
+ * ## Why this is not a re-run of the warning objectui#5741 deleted
+ *
+ * That ruling removed the Phase-1 warning from the runtime hot path and kept
+ * the export, in its own words, "as the offline instrument". `listConditional.ts`
+ * records the same split: the detector "stays exported for OFFLINE sweeps of
+ * authored metadata, not for this hot path". This call site is neither the hot
+ * path nor a render — it is the editor bridge, classifying authored text as the
+ * author types it, which is the sweep case one document at a time. Nothing is
+ * added back to `evalRowPredicate` / `listConditional.ts`.
+ *
+ * ## Why WARNING and never ERROR
+ *
+ * An `error` here would narrow the accepted set: every save gate on this tier
+ * counts `severity === 'error'` (`ConditionalFormattingEditor`,
+ * `ObjectFieldInspector`, `ConditionBuilder`, `PermissionAdvancedFacets`,
+ * `clientValidation.validateObjectFieldRules`), so promoting this would refuse
+ * predicates already stored in customer metadata. A `warning` leaves
+ * `aria-invalid` unset and every gate open. Narrowing the ACCEPT SET is the
+ * producer-side half and lives in `@objectstack/formula` (objectui#8166's
+ * ruling); adding an advisory the engine never had is not that change.
+ *
+ * ## Why only ONE of the detector's two arms is consulted
+ *
+ * The detector also reports the bare shorthand, and that arm is deliberately
+ * disabled here by passing `row = null` (the arm requires an own key of the
+ * row). Measured, both directions:
+ *
+ *  - at `scope: 'record'` the engine ALREADY errors on a bare identifier and
+ *    names the `record.<field>` fix, so the arm can only duplicate it;
+ *  - at `scope: 'flattened'` — RLS `USING` / `CHECK` — a bare identifier is the
+ *    CORRECT spelling (`organization_id == current_user.organization_id` is the
+ *    editor's own placeholder). All three genuine RLS predicates in this repo
+ *    fire that arm, i.e. it is a 100% false positive rate on that tier.
+ *
+ * `dataNamesRow` is passed `true` because what that guard actually decides — in
+ * its own words, "a surface whose `data` is the host scope's own … is a
+ * legitimate `data.*` site and is left alone" — is whether `data` is legitimate
+ * here. On a record-scope authoring site it is not: `ROW_PREDICATE_ROOTS`,
+ * `FIELD_RULE_ROOTS` and `FORMULA_ROOTS` all exclude it and `buildExpressionScope`
+ * binds nothing under it. The metadata-editing layer where `data` IS canonical
+ * (ADR-0089 D3) is `views/metadata-admin/SchemaForm.tsx`, which evaluates through
+ * `views/metadata-admin/predicate.ts` and never reaches this function — which is
+ * why the gate below is `scope === 'record'` and not a source pattern.
+ */
+function rowCanonAdvisory(finding: {
+  kind: string;
+  identifier: string;
+  canonical: string;
+}): CelLintIssue | null {
+  if (finding.kind !== 'metadata-layer-root') return null;
+  return {
+    severity: 'warning',
+    message:
+      `\`${finding.identifier}\` is not the row on this surface: a row predicate binds the ` +
+      `record as \`${finding.canonical}\` and nothing else (objectui#5741). The CEL scope ` +
+      `vocabulary still accepts \`${finding.identifier}\`, so nothing here blocks the save, but ` +
+      `at runtime the expression faults with \`Unknown variable: ${finding.identifier}\` and the ` +
+      `rule never fires. Re-root the reference on \`${finding.canonical}\`.`,
+  };
+}
+
 /* ── 1. Lint ─────────────────────────────────────────────────────────── */
 
 /**
@@ -198,6 +302,10 @@ const PUSHDOWN_VARIABLE_ROOTS = ['current_user', 'user'] as const;
  * expression (usually paired with `scope: 'record'`, where a bare field ref IS
  * a hard error — it silently evaluates to null at runtime).
  *
+ * At `scope: 'record'` one finding comes from outside the engine: the
+ * wrong-layer `data.*` advisory described on {@link rowCanonAdvisory}. It is
+ * always a `warning`, so it never narrows what this surface accepts.
+ *
  * Empty input is always clean.
  */
 export async function lintCelPredicate(
@@ -244,6 +352,18 @@ export async function lintCelPredicate(
         /* advisory only — never let it break the lint */
       }
     }
+    // Wrong-layer `data.*` advisory (objectui#8972) — see `rowCanonAdvisory`.
+    // Only in `record` scope, only once the predicate parses, only a WARNING.
+    if (issues.every((i) => i.severity !== 'error') && hint.scope === 'record') {
+      try {
+        const canon = await loadRowCanon();
+        const finding = canon?.detectNonCanonicalRowSpelling?.(source, null, true);
+        const advisory = finding ? rowCanonAdvisory(finding) : null;
+        if (advisory) issues.push(advisory);
+      } catch {
+        /* advisory only — never let it break the lint */
+      }
+    }
     return issues;
   } catch {
     return [];