From 35e49ace1b3cac3a91add3765eebf0f89d656778 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 21:25:17 +0000 Subject: [PATCH 1/3] fix(app-shell): lint conditional-formatting conditions in the record scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The conditional-formatting editor authored its CEL in the `flattened` scope, where any bare identifier is legal, and advertised `data` in its autocomplete roots. Phase 2 of the row-predicate canon retired both spellings on runtime record surfaces: `evalRowPredicate` binds the row as `record.*` and nothing else, so `status == 'overdue'` and `data.status == 'overdue'` fault at runtime while the editor linted them green. - `CelPredicateField` authors in `scope="record"`, the scope the field conditional rules already use, so a bare field ref is an ERROR carrying the `record.` fix. - `ROW_PREDICATE_ROOTS` drops `'data'`. - The docblock and the inline comment describing the old three-way binding are rewritten to the one binding that survives. The shared `hint.scope ?? 'flattened'` default is untouched: RLS predicates and flow conditions are not row surfaces. Tests: the pin that asserted "a bare field lints clean" is turned to assert the `record.` diagnostic — its own comment predicted this edit. The roots-to-runtime pin is repaired: it looped every advertised root asserting `size() >= 0` against a host scope that itself carried `data: {}`, so for `data` the probe hit the host's own empty object and could not fail. Each root is now checked against the binder that is supposed to supply it, in both directions, and `data` and `os` get their own pins against a scope that does carry them. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S --- ...727-conditional-formatting-record-scope.md | 38 +++++ .../ConditionalFormattingEditor.test.tsx | 143 ++++++++++++++++-- .../ConditionalFormattingEditor.tsx | 47 ++++-- 3 files changed, 202 insertions(+), 26 deletions(-) create mode 100644 .changeset/7727-conditional-formatting-record-scope.md diff --git a/.changeset/7727-conditional-formatting-record-scope.md b/.changeset/7727-conditional-formatting-record-scope.md new file mode 100644 index 0000000000..73b2ea2060 --- /dev/null +++ b/.changeset/7727-conditional-formatting-record-scope.md @@ -0,0 +1,38 @@ +--- +'@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. + +The editor was the last place still 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; +- the exported `ROW_PREDICATE_ROOTS` loses `'data'`, which Phase 2 retired but + autocomplete was still recommending; +- the docblock and inline comment that described the old three-way binding are + rewritten to the one binding that survives. + +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. + +**Known gap this makes reachable:** `app.*` is bound at runtime by the app-shell +predicate scope and advertised by this editor, but `@objectstack/formula`'s +`SCOPE_ROOTS` has no `app`, so under `scope="record"` the lint refuses it. Measured, +pinned as a characterization test, and filed as objectui#8155. 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 6177c8c833..95eaa090fd 100644 --- a/packages/app-shell/src/views/metadata-admin/ConditionalFormattingEditor.test.tsx +++ b/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 { @@ -133,15 +133,58 @@ 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. fix — the row binds only record.*', async () => { render(); - // 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. condition clean', async () => { + render(); + // 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 a host-scope root (current_user / features) clean in the record scope', async () => { + // The advertised host roots must survive the narrowing — a row predicate + // legitimately reads the global predicate scope (#1583/ADR-0068). + render(); + expect(await screen.findByText('perm.cel.valid', {}, { timeout: 3000 })).toBeTruthy(); + }); + + 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. + render(); + 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. with did-you-mean', async () => { render(); expect(await screen.findByText(/did you mean/i, {}, { timeout: 3000 })).toBeTruthy(); @@ -176,36 +219,108 @@ 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' }; + /** + * The app-shell global predicate scope (`buildExpressionScope` in + * `providers/ExpressionProvider.tsx`, #1583/ADR-0068) that hosts hand to the + * shared row-predicate evaluator — MODELLED here, and deliberately WITHOUT + * `data` or `os`. + * + * Why the model is load-bearing (objectui#7727). This block used to carry + * `data: {}` and probe every advertised root with `size() >= 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. + * Every assertion below now names WHICH binder is supposed to supply the + * root and checks the other direction too, so each one can fail for the + * reason it is written for. The two roots a host legitimately carries + * (`data`, `os`) get their own pins against a scope that does carry them. + */ const hostScope = { current_user: u, user: u, ctx: { user: u }, app: { name: 'crm' }, - data: {}, features: { beta: true }, }; + const row = { id: 'r1', status: 'overdue' }; - it('every advertised root is bound when a row predicate evaluates', () => { + /** Advertised roots the HOST binds — the row contributes nothing to them. */ + const HOST_BOUND_ROOTS = ['current_user', 'user', 'ctx', 'app', 'features']; + + 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 OTHER advertised root is bound by the HOST — and unbound without it', () => { for (const root of ROW_PREDICATE_ROOTS) { - // `size() >= 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. + 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. That is what made the old probe useless... + const ambient = { ...hostScope, data: {} }; + 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); }); 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', () => { + // `buildExpressionScope` binds `os: { user }`, so a probe run against the + // real host scope resolves it. 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(ROW_PREDICATE_ROOTS).not.toContain('os'); + expect( + evalRowPredicate('size(os) >= 0', row, { fallback: false, scope: { ...hostScope, os: { user: u } } }), + ).toBe(true); + }); }); diff --git a/packages/app-shell/src/views/metadata-admin/ConditionalFormattingEditor.tsx b/packages/app-shell/src/views/metadata-admin/ConditionalFormattingEditor.tsx index 4e744f5691..5caa9119b8 100644 --- a/packages/app-shell/src/views/metadata-admin/ConditionalFormattingEditor.tsx +++ b/packages/app-shell/src/views/metadata-admin/ConditionalFormattingEditor.tsx @@ -35,12 +35,32 @@ import type { CelLintIssue } from './celAuthoring.js'; * * A formatting `condition` is evaluated by `@object-ui/core`'s * `evalRowPredicate` (ADR-0058 — list rows, grid rows, kanban cards), which - * binds the row's fields BARE, under `record.*`, and under `data.*`, plus the - * host shell's global predicate scope (`ExpressionProvider`, #1583/ADR-0068: - * `current_user` / `user` / `ctx` / `app` / `features`). The engine's default - * advertisement adds `previous` / `input` / `os` / `vars`, which are NOT bound - * for row predicates — suggesting those would author a condition that silently - * never matches, so this override pins the truthful catalog (#2571 follow-up). + * binds the row ONE way — as the `record` namespace — plus the host shell's + * global predicate scope (`ExpressionProvider`, #1583/ADR-0068: + * `current_user` / `user` / `ctx` / `app` / `features`). + * + * ## What changed, and why this list lost a member (objectui#7727) + * + * It used to bind the row THREE ways: bare fields, `record.*` and `data.*`. + * Phase 2 of the objectui#5330 canon (objectui#5741, ruled 2026-09-02, amended + * 2026-09-05) RETIRED the other two — see `@object-ui/core`'s + * `evaluator/rowPredicateCanon.ts`. Neither `status` nor `data.status` names + * this row any more; both fault as unknown variables, exactly as they always + * did on the server. + * + * `data` is therefore off this list. The subtlety worth keeping: a host scope + * may legitimately carry its OWN ambient `data` (app-shell's + * `buildExpressionScope` does), so `data.*` still RESOLVES — against the + * host's object rather than the row. That is the constant-false signature + * `rowPredicateCanon.ts` describes, and it is why "does `data` resolve?" is + * not a test of whether `data` names the row. + * + * The engine's default advertisement adds `previous` / `input` / `os` / + * `vars`. `previous` / `input` / `vars` are NOT bound for row predicates at + * all; `os` IS bound by the app-shell host scope but is deliberately not + * advertised here. Suggesting an unbound root would author a condition that + * silently never matches, so this override pins the truthful catalog + * (#2571 follow-up). */ export const ROW_PREDICATE_ROOTS = [ 'record', @@ -48,7 +68,6 @@ export const ROW_PREDICATE_ROOTS = [ 'user', 'features', 'app', - 'data', 'ctx', ]; @@ -326,11 +345,15 @@ export function ConditionalFormattingEditor({ placeholder="record.status == 'overdue'" objectName={objectName} fieldNames={fieldNames} - // Row predicates bind the row's fields BARE at runtime - // (`status == 'overdue'` works — evalRowPredicate spreads the - // row), so lint stays in the flattened scope; only the advertised - // roots change to the runtime-bound set. - scope="flattened" + // Row predicates bind the row as `record.*` and nothing else at + // runtime — objectui#5741 (Phase 2 of the objectui#5330 canon) + // retired the bare shorthand and `data.*`. So this lints in the + // RECORD scope, the same one the field conditional rules + // `visibleWhen` / `readonlyWhen` / `requiredWhen` use: a bare + // `status` is an ERROR carrying the `record.status` fix instead of + // linting clean and authoring a rule that never matches + // (objectui#7727). The advertised roots stay the runtime-bound set. + scope="record" roots={ROW_PREDICATE_ROOTS} onChange={(v) => setRule(i, { condition: v })} onLintChange={(issues) => reportCel(i, issues)} From 7a1e2723e029e8650eae014d1943d34cbfb0cde3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 22:25:44 +0000 Subject: [PATCH 2/3] test(app-shell): read the host predicate scope from its producer, and pin the data half Contract-review follow-up on the conditional-formatting scope flip. No implementation logic changes; this is coverage and text. - The roots-to-runtime suite no longer writes the app-shell predicate bag out by hand. It calls `buildExpressionScope`, derives the advertised-root expectation from `Object.keys(...)` minus an explicit curated-exclusion list, and runs the `os` and `data` pins against that same bag. A literal cannot disagree with its producer, so it silently absorbs drift -- and it already had: the literal omitted `os`, which the producer really does bind, so the old `os` assertion "proved" it unbound. The previous `os` pin also handed `os` in by hand, which showed only that `evalRowPredicate` forwards `scope`; it now reads the producer and carries an unbound-root control. - New characterization pin: a `data.*` condition still lints CLEAN. Dropping `data` from the advertised roots stops recommending it, not accepting it -- the engine's `SCOPE_ROOTS` lists `data`, so the record-scope lint waves it through while the runtime pin one suite lower asserts it is false. Green here plus false there is the defect, and the pair is the referent. - The `app` pin's comment now says which of its card's two candidate fixes it is a tripwire for; the closure assertion covers the other. - The host-roots test no longer says "advertised host roots must survive" while sitting above a test proving one of them does not. The changeset drops the false "last place" claim, states that this closes the bare-field half of the retirement and not the `data.*` half, spells out that a saved view with a legacy condition becomes unsavable in the designer until it is rewritten, and records the bare-position autocomplete change. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S --- ...727-conditional-formatting-record-scope.md | 57 ++++++-- .../ConditionalFormattingEditor.test.tsx | 131 +++++++++++++----- 2 files changed, 144 insertions(+), 44 deletions(-) diff --git a/.changeset/7727-conditional-formatting-record-scope.md b/.changeset/7727-conditional-formatting-record-scope.md index 73b2ea2060..3b22feb059 100644 --- a/.changeset/7727-conditional-formatting-record-scope.md +++ b/.changeset/7727-conditional-formatting-record-scope.md @@ -10,14 +10,23 @@ Lint conditional-formatting conditions in the `record` scope, and stop advertisi Studio's conditional-formatting editor and now raises a blocking error carrying the `record.status` fix. -The editor was the last place still 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. +**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`: @@ -28,11 +37,35 @@ Three changes, all on `ConditionalFormattingEditor`: - 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. -**Known gap this makes reachable:** `app.*` is bound at runtime by the app-shell -predicate scope and advertised by this editor, but `@objectstack/formula`'s -`SCOPE_ROOTS` has no `app`, so under `scope="record"` the lint refuses it. Measured, -pinned as a characterization test, and filed as objectui#8155. +**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. 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 95eaa090fd..55a110bc50 100644 --- a/packages/app-shell/src/views/metadata-admin/ConditionalFormattingEditor.test.tsx +++ b/packages/app-shell/src/views/metadata-admin/ConditionalFormattingEditor.test.tsx @@ -12,6 +12,7 @@ import { type ConditionalFormattingRuleDraft, } from './ConditionalFormattingEditor'; import { __setCelFormulaLoader } from './celAuthoring'; +import { buildExpressionScope } from '../../providers/ExpressionProvider.js'; afterEach(() => { cleanup(); @@ -158,13 +159,42 @@ describe('ConditionalFormattingEditor · CEL authoring scope (#2571 follow-up)', expect(ta.getAttribute('aria-invalid')).not.toBe('true'); }); - it('lints a host-scope root (current_user / features) clean in the record scope', async () => { - // The advertised host roots must survive the narrowing — a row predicate - // legitimately reads the global predicate scope (#1583/ADR-0068). - render(); + 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( + , + ); 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(); + 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 @@ -179,6 +209,13 @@ describe('ConditionalFormattingEditor · CEL authoring scope (#2571 follow-up)', // 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(); const ta = document.getElementById('cf-condition-0') as HTMLTextAreaElement; await waitFor(() => expect(ta.getAttribute('aria-invalid')).toBe('true'), { timeout: 3000 }); @@ -221,33 +258,49 @@ describe('ConditionalFormattingEditor · CEL authoring scope (#2571 follow-up)', describe('ROW_PREDICATE_ROOTS ↔ evalRowPredicate runtime contract', () => { const u = { id: 'u1' }; /** - * The app-shell global predicate scope (`buildExpressionScope` in - * `providers/ExpressionProvider.tsx`, #1583/ADR-0068) that hosts hand to the - * shared row-predicate evaluator — MODELLED here, and deliberately WITHOUT - * `data` or `os`. + * 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() >= 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. * - * Why the model is load-bearing (objectui#7727). This block used to carry - * `data: {}` and probe every advertised root with `size() >= 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. - * Every assertion below now names WHICH binder is supposed to supply the - * root and checks the other direction too, so each one can fail for the - * reason it is written for. The two roots a host legitimately carries - * (`data`, `os`) get their own pins against a scope that does carry them. + * 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 hostScope = { - current_user: u, + 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 — the row contributes nothing to them. */ - const HOST_BOUND_ROOTS = ['current_user', 'user', 'ctx', 'app', 'features']; + /** 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`. @@ -279,6 +332,10 @@ describe('ROW_PREDICATE_ROOTS ↔ evalRowPredicate runtime contract', () => { ).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()); }); @@ -291,13 +348,17 @@ describe('ROW_PREDICATE_ROOTS ↔ evalRowPredicate runtime contract', () => { 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. That is what made the old probe useless... - const ambient = { ...hostScope, data: {} }; + // `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', () => { @@ -314,13 +375,19 @@ describe('ROW_PREDICATE_ROOTS ↔ evalRowPredicate runtime contract', () => { }); it('`os` is unadvertised by CURATION, not because it is unbound', () => { - // `buildExpressionScope` binds `os: { user }`, so a probe run against the - // real host scope resolves it. Held apart from the extras above so that - // list keeps meaning "not bound". Whether `os` SHOULD be advertised is + // 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: { ...hostScope, os: { user: u } } }), - ).toBe(true); + 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); }); }); From 86be827b2086cbb87d88a772044a0f1b6c736382 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 23:29:54 +0000 Subject: [PATCH 3/3] docs(changeset): stop implying the roots const is on the published face `ROW_PREDICATE_ROOTS` is an `export const`, which the release note read as an API change. Measured: `packages/app-shell/src/index.ts` has 0 `export *` lines and names neither the const nor `ConditionalFormattingEditor`, and the package `exports` map is `"."` plus `./styles.css` with no deep subpath -- so nothing outside the package can import it. The behavioural narrowing is real and is what the note is about; the published surface is unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S --- .changeset/7727-conditional-formatting-record-scope.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/.changeset/7727-conditional-formatting-record-scope.md b/.changeset/7727-conditional-formatting-record-scope.md index 3b22feb059..e28c7f8f2b 100644 --- a/.changeset/7727-conditional-formatting-record-scope.md +++ b/.changeset/7727-conditional-formatting-record-scope.md @@ -32,8 +32,12 @@ Three changes, all on `ConditionalFormattingEditor`: - its `CelPredicateField` authors in `scope="record"`, the scope the field conditional rules `visibleWhen` / `readonlyWhen` / `requiredWhen` already use; -- the exported `ROW_PREDICATE_ROOTS` loses `'data'`, which Phase 2 retired but - autocomplete was still recommending; +- `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.