diff --git a/.changeset/8972-cel-authoring-data-root-advisory.md b/.changeset/8972-cel-authoring-data-root-advisory.md new file mode 100644 index 0000000000..0516466ad9 --- /dev/null +++ b/.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. diff --git a/packages/app-shell/src/views/metadata-admin/ConditionalFormattingEditor.test.tsx b/packages/app-shell/src/views/metadata-admin/ConditionalFormattingEditor.test.tsx index a16e83f6ba..da636f36e4 100644 --- a/packages/app-shell/src/views/metadata-admin/ConditionalFormattingEditor.test.tsx +++ b/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(); - 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 () => { diff --git a/packages/app-shell/src/views/metadata-admin/celAuthoring.test.ts b/packages/app-shell/src/views/metadata-admin/celAuthoring.test.ts index 78b64db4b4..dbfc210822 100644 --- a/packages/app-shell/src/views/metadata-admin/celAuthoring.test.ts +++ b/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 }; diff --git a/packages/app-shell/src/views/metadata-admin/celAuthoring.ts b/packages/app-shell/src/views/metadata-admin/celAuthoring.ts index 1df376e775..94d8e4d6ac 100644 --- a/packages/app-shell/src/views/metadata-admin/celAuthoring.ts +++ b/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 | null | undefined, + dataNamesRow: boolean, + ) => { kind: string; identifier: string; canonical: string } | null; +} + +let rowCanonCached: Promise | 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 { + 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.` 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 [];