diff --git a/.changeset/6443-nav-visible-fault-diagnostic.md b/.changeset/6443-nav-visible-fault-diagnostic.md new file mode 100644 index 0000000000..516875ab35 --- /dev/null +++ b/.changeset/6443-nav-visible-fault-diagnostic.md @@ -0,0 +1,41 @@ +--- +'@object-ui/app-shell': patch +--- + +A nav / area / field `visible` predicate that FAULTS now says so, in both builds, once per +distinct predicate source (objectui#6443). Observability only — no verdict moves. + +`ExpressionProvider.evaluateVisibility` is the gate behind a navigation item's `visible`, +an area's derived visibility, and the field list `RecordFormPage` renders. It is fail-open: +a predicate that cannot be evaluated returns `true`, so a menu entry whose role gate has +stopped working renders **for everyone — including the role it was written to exclude** and +looks exactly like an entry the author meant to show. + +That fault was swallowed one layer down. `evaluateCondition` is fail-soft: it answers an +unevaluable predicate with `true` from its own `catch` and does not throw, so this site's +`try/catch` never saw a predicate fault at all. Measured per dialect at this site before the +fix — the bare-string dialect, the one a live gate was measured breaking on, printed +**nothing at all**: + +| dialect | console at this site, before | after | +|---|---|---| +| bare string | nothing | one named line | +| `{ dialect: 'cel' }` envelope | one generic line | one named line (the generic one is *replaced*, not added to) | +| `${…}` template | one generic line **per evaluation** | one named line, deduped | + +The fix wires `EvaluationOptions.onFault` (the seam objectui#6038 landed) to +`reportUnresolvableVisibilityPredicate`, exported from `@object-ui/react` — the same +reporter, message, severity, dedupe `Set` and rate limit the node gate and `page:tabs` +already use, so one authored predicate is entitled to one line rather than one line per +package. It costs no extra engine call: the evaluator hands back the reason at the point it +already knows the predicate faulted, with no `throwOnError` double evaluation. + +A nav item is not a schema node, so the reporter's `type` slot — which, with the gate key +and the predicate source, is the dedupe key — is the constant `app-shell:visible`. The rate +limit is therefore **one line per distinct authored predicate source**, not one per menu +entry: a broken role gate copy-pasted across eight entries is one authoring mistake, in one +string, fixed in one edit. + +**Fail-open is unchanged.** The item still renders for everyone on a fault. Flipping that to +fail-closed is a permission-boundary change, not a diagnostic, and is not this change's to +make; the change makes the silence stop and nothing else. diff --git a/content/docs/guide/metadata-diagnostics.md b/content/docs/guide/metadata-diagnostics.md index 27ab97bd36..145f674984 100644 --- a/content/docs/guide/metadata-diagnostics.md +++ b/content/docs/guide/metadata-diagnostics.md @@ -192,6 +192,27 @@ The verdict is unchanged in every case: this is a diagnostic about a predicate, not a change to what the gate decides. A node gate that fails open renders exactly as it always did; the difference is that it now says so. +The **app shell's own `visible` gate** joins the same reporter and the same +rate limit: a `visible` predicate on a navigation item, on an area's +navigation, or on an object field rendered by the record form page. This one +was silent in **both** builds before — including the bare-string dialect, +which printed nothing at all — so a menu entry whose role gate had stopped +working rendered for everyone, silently, with nothing to grep for. It now +reports under the surface label `app-shell:visible`: + +```text +[ObjectUI] A visibility predicate could not be evaluated - node "app-shell:visible" + visible: "'org_admin' in current_user.postions" + Reason: ... +``` + +The dedupe key is the predicate **source**, not the menu entry — one broken +role gate copy-pasted across eight entries is one authoring mistake and prints +one line, while a second, differently-broken predicate still gets its own. +Fail-open is unchanged here too: the item still renders for everyone, +including the role the predicate was written to exclude. That is what the line +exists to tell you. + ### 4. Governance overview page `/apps//metadata/_diagnostics` — a single sortable table of every diff --git a/packages/app-shell/src/providers/ExpressionProvider.tsx b/packages/app-shell/src/providers/ExpressionProvider.tsx index 7ce2f99618..1c18358ca4 100644 --- a/packages/app-shell/src/providers/ExpressionProvider.tsx +++ b/packages/app-shell/src/providers/ExpressionProvider.tsx @@ -16,7 +16,7 @@ import React, { createContext, useContext, useMemo } from 'react'; import { ExpressionEvaluator } from '@object-ui/core'; -import { PredicateScopeProvider } from '@object-ui/react'; +import { PredicateScopeProvider, reportUnresolvableVisibilityPredicate } from '@object-ui/react'; export interface ExpressionContextValue { /** Current authenticated user */ @@ -93,6 +93,62 @@ export function useExpressionContext(): ExpressionContextValue { return ctx; } +/** + * The `type` slot of the shared visibility-fault reporter, for THIS surface + * (objectui#6443). + * + * ## Why the slot needed a decision at all + * + * `reportUnresolvableVisibilityPredicate` (@object-ui/react, objectui#6038) + * dedupes on `(type, key, predicate source)` — `id` rides along in the printed + * line but is deliberately NOT in the key. So whatever goes in `type` sets this + * site's RATE LIMIT, and a nav item is not a schema node: there is no + * `schema.type` to hand over. + * + * ## Why a CONSTANT, and not the item's identity + * + * The choice is between a constant (one line per distinct authored predicate + * SOURCE) and something per-item (one line per menu entry). Measured against + * the three cases that can actually occur: + * + * - two entries, two different broken predicates -> BOTH keys already differ + * on `source`, so both report either way. No difference. + * - two entries sharing ONE broken predicate (the copy-pasted role gate, the + * common shape) -> a constant reports ONCE, which is correct: it is one + * authoring mistake, in one string, fixed in one edit. A per-item key would + * report once per entry and add no information — same text, same reason. + * - one entry, evaluated many times -> both dedupe. This site is re-entered + * more than a node gate is: `hasVisibleNavigationItems` re-runs every item + * predicate to DERIVE area visibility (objectui#3311) before + * `NavigationRenderer` runs them again to render, and both re-run on every + * sidebar re-render. + * + * A per-item key is therefore never better and sometimes much worse. It is also + * unreachable without widening `VisibilityEvaluator` (@object-ui/layout), whose + * whole signature is `(expression) => boolean` — a cross-package public type + * change this diagnostics-only card does not get to make. The predicate SOURCE + * is the locator instead, and it is in both the key and the printed line: it is + * the string an author greps their metadata for, and it is unique to the bug in + * a way an item id is not. + * + * ## Why this spelling + * + * Namespaced with a colon, like `page:tabs` (the other non-node surface wired to + * this reporter). It names the app-shell `visible` GATE, not a component key — + * `app-shell` is deliberately NOT a registry key (objectui#4841), and the colon + * keeps this label out of that bare namespace so a diagnostic can never be read + * as claiming one. + * + * It is deliberately NOT `nav:item`, even though this card is written about nav + * and area items: `evaluateVisibility` is also the gate `RecordFormPage` runs + * over an object's field `visible` predicates, and labelling those "nav" would + * be false. The consequence is stated rather than hidden — a nav item and a + * form field carrying the IDENTICAL broken predicate text share one dedupe + * entry and produce one line. That is still one typo in one string; the line + * names the string, which is what both sites are grepped by. + */ +const APP_SHELL_VISIBLE_SURFACE = 'app-shell:visible'; + /** * Evaluate a visibility expression. * Supports: @@ -122,9 +178,38 @@ export function evaluateVisibility( if (expression === true || expression === 'true') return true; if (expression === false || expression === 'false') return false; + // objectui#6443 — the fault is REPORTED now, and the verdict is untouched. + // + // `evaluateCondition` is fail-soft: it answers an unevaluable predicate with + // `true` from its OWN catch and does not throw, so the `catch` below never + // saw a predicate fault and this site swallowed every one of them in + // silence — in production AND in development, which is what made it worse + // than the node gate objectui#6038 fixed. `onFault` is the only channel that + // reaches that swallowed fault, and it costs nothing: no `throwOnError` + // second evaluation, same single engine call as before. + // + // FAIL-OPEN IS UNCHANGED, deliberately. A predicate that cannot be evaluated + // still returns `true`, so the nav item, area or field still renders for + // everyone — including the role the predicate was written to exclude. That is + // the shipped permission-boundary semantics; flipping it to fail-closed is a + // behaviour change, not a diagnostic, and it is not this card's to make. This + // card makes the silence stop, nothing else. + const report = (reason: unknown): void => + reportUnresolvableVisibilityPredicate( + APP_SHELL_VISIBLE_SURFACE, + undefined, + 'visible', + expression, + reason, + ); + try { - return evaluator.evaluateCondition(expression); - } catch { + return evaluator.evaluateCondition(expression, { onFault: report }); + } catch (err) { + // Defensive, and now loud too: `evaluateCondition` handles its own faults, + // so reaching here means the evaluator itself threw. That path returned the + // same fail-open `true` in silence; it no longer does. + report(err); return true; // Default to visible on error } } diff --git a/packages/app-shell/src/providers/ExpressionProvider.visibleFaultDiagnostic.test.ts b/packages/app-shell/src/providers/ExpressionProvider.visibleFaultDiagnostic.test.ts new file mode 100644 index 0000000000..4d23915182 --- /dev/null +++ b/packages/app-shell/src/providers/ExpressionProvider.visibleFaultDiagnostic.test.ts @@ -0,0 +1,316 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#6443 — a nav / area / field `visible` predicate that FAULTS is + * reported, through the same reporter and the same rate limit as the node gate + * (objectui#6038) and the `page:tabs` item gate. + * + * ## The defect, and why a runtime suite could not see it + * + * `evaluateCondition` is FAIL-SOFT: it answers an unevaluable predicate with + * `true` from its own `catch` and does not throw. So `evaluateVisibility`'s + * `try/catch` never saw a predicate fault at all — the whole class was + * swallowed one layer down, in silence, in BOTH builds. Measured on this base + * before the fix, the neighbouring suite + * (`ExpressionProvider.evaluateVisibility.test.ts`) passes green against the + * defect and always did: its `fails open (visible) on an unevaluable predicate` + * case asserts the exact verdict the broken site returns. That is the argument + * for this file's existence — the observable difference is on the console, and + * nothing was looking there. + * + * The consequence is a permission boundary that stops biting: a menu entry + * whose `visible` was written to exclude a role renders for that role, looking + * exactly like an entry the author meant to show. + * + * ## What is pinned, split into cells that DISAGREE and cells that must not + * + * DISCRIMINATING (red against `origin/main`, green after) — every dialect from + * the card's measured table, the message content that carries this site's + * dedupe-key decision, both halves of the rate limit, and the defensive throw + * path. + * + * CONTROLS (green BOTH ways, deliberately, each named at its case): + * - the positive/degenerate console-capture pair, without which every + * `toHaveLength(0)` here is equally green on a spy that observes nothing; + * - FAIL-OPEN IS UNCHANGED. This card is diagnostics only. A faulting + * predicate must still leave the item visible — including for the role it + * was written to exclude. The cell guards the future wrong shape where a + * diagnostic change quietly also flips fail-open to fail-closed, which + * would be a permission-boundary change wearing an observability costume. + * - a HEALTHY predicate stays silent on BOTH verdicts, which guards the + * future wrong shape where the reporter fires on a genuine `false`. + * + * ## Reverse verification — direction predicted BEFORE the run + * + * Restoring `ExpressionProvider.tsx` from `origin/main` (i.e. deleting the + * `onFault` option and the `report` call in the `catch`) turns RED exactly the + * discriminating cells above — `reports()` goes to 0 in each — and leaves the + * four control cells GREEN. The `${…}` cell moves in a second, independent + * direction: its TOTAL console line count goes 1 -> 3, because that dialect's + * built-in warning fires once per evaluation and is not deduped (the separate + * defect objectui#6444 tracks, which this card does not fix and does not need + * to: supplying `onFault` transfers reporting away from it at this site). + * + * ## Predicate sources are unique per case, on purpose + * + * The `unit` project runs with `isolate: false`, and the built-in CEL warning + * in `@object-ui/core`'s `evalFieldPredicate` dedupes in module state this file + * cannot reset. A source string shared with any other test file would let that + * dedupe decide this file's line counts. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { ExpressionEvaluator } from '@object-ui/core'; +import { + UNRESOLVABLE_VISIBILITY_PREFIX, + __resetVisibilityPredicateWarnings, +} from '@object-ui/react'; +import { hasVisibleNavigationItems } from '@object-ui/layout'; +import { evaluateVisibility } from './ExpressionProvider'; + +/** The label this site puts in the reporter's `type` slot — the dedupe-key decision. */ +const SURFACE = 'app-shell:visible'; + +function makeEvaluator(user: Record = { id: 'u1', positions: ['worker'] }) { + const context = { current_user: user, user, ctx: { user }, os: { user }, app: {}, data: {}, features: {} }; + return new ExpressionEvaluator(context as any); +} + +type WarnSpy = { mock: { calls: unknown[][] } }; +const spyWarn = () => vi.spyOn(console, 'warn').mockImplementation(() => {}); +const reports = (warn: WarnSpy): string[] => + warn.mock.calls.map((c) => String(c[0])).filter((m) => m.includes(UNRESOLVABLE_VISIBILITY_PREFIX)); +const allWarnings = (warn: WarnSpy): string[] => warn.mock.calls.map((c) => String(c[0])); + +beforeEach(() => { + __resetVisibilityPredicateWarnings(); +}); +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe('objectui#6443 — controls', () => { + it('POSITIVE CONTROL: the spy observes a line carrying the prefix', () => { + // Green both ways. Without it every `toHaveLength(0)` below is equally + // green on a capture that observes nothing at all. + const warn = spyWarn(); + console.warn(`${UNRESOLVABLE_VISIBILITY_PREFIX} - synthetic control line`); + expect(reports(warn)).toHaveLength(1); + }); + + it('DEGENERATE CONTROL: unrelated console output does not satisfy the pin', () => { + // Green both ways. `reports()` must be a filter, not a call counter. + const warn = spyWarn(); + console.warn('[object-ui] an entirely unrelated warning'); + expect(allWarnings(warn)).toHaveLength(1); + expect(reports(warn)).toHaveLength(0); + }); + + it('CONTROL — FAIL-OPEN IS UNCHANGED: the excluded role still sees the item', () => { + // GREEN BOTH WAYS, and that is the point: this card is diagnostics only. + // The cell guards the future wrong shape where a diagnostic change also + // flips fail-open to fail-closed — a permission-boundary change wearing an + // observability costume. It carries NO assertion about the console, so it + // cannot go red for the reason the discriminating cells do. + // + // Asserted on the OBSERVABLE OUTCOME — whether the nav item survives the + // real guard `AppSidebar` runs — not on the helper's return value alone, + // because "a fault was reported" is also true of a site that stopped + // rendering the item entirely. + spyWarn(); // keep the fault line off the suite's own output + const evaluator = makeEvaluator({ id: 'u1', positions: ['worker'] }); + const fault = "'org_admin' in nosuchroot6443ctl.positions"; + + // The role the predicate was written to exclude is a `worker`. + expect(evaluateVisibility(fault, evaluator)).toBe(true); + expect( + hasVisibleNavigationItems( + [{ type: 'link', id: 'admin', label: 'Admin', href: '/admin', visible: fault }] as any, + { evaluateVisibility: (e) => evaluateVisibility(e as any, evaluator) }, + ), + ).toBe(true); + }); + + it('CONTROL — a HEALTHY predicate stays silent, on BOTH verdicts', () => { + // Green both ways. The half that makes the loud half mean something: a + // predicate that says NO is a verdict, not a fault. + const warn = spyWarn(); + const orgAdmin = makeEvaluator({ id: 'u2', positions: ['org_admin'] }); + const worker = makeEvaluator({ id: 'u1', positions: ['worker'] }); + const visible = { dialect: 'cel', source: "'org_admin' in current_user.positions" }; + + expect(evaluateVisibility(visible, orgAdmin)).toBe(true); + expect(evaluateVisibility(visible, worker)).toBe(false); + expect(evaluateVisibility("${user.role === 'admin'}", makeEvaluator({ role: 'guest' }))).toBe(false); + + expect(allWarnings(warn)).toHaveLength(0); + }); +}); + +describe('objectui#6443 — every dialect from the card`s measured table now reports', () => { + it('BARE STRING — the dialect that printed NOTHING, and the one a live gate broke on', () => { + // Pairs the log with the OBSERVABLE OUTCOME in one cell, so this cannot go + // green on a site that reported a fault by refusing to render the item. + const warn = spyWarn(); + const evaluator = makeEvaluator(); + const fault = 'nosuchroot6443bare.x > 1'; + + expect(evaluateVisibility(fault, evaluator)).toBe(true); // verdict unchanged + expect( + hasVisibleNavigationItems( + [{ type: 'link', id: 'reports', label: 'Reports', href: '/r', visible: fault }] as any, + { evaluateVisibility: (e) => evaluateVisibility(e as any, evaluator) }, + ), + ).toBe(true); // still visible to the excluded role — fail-open, unchanged + + expect(reports(warn)).toHaveLength(1); + expect(allWarnings(warn)).toHaveLength(1); // and nothing else was added + }); + + it('CEL ENVELOPE — the generic line is REPLACED by the named one, not added to it', () => { + // `onFault` transfers reporting to this caller (`warn: false` reaches + // `evalFieldPredicate`), so the total stays ONE line while its content + // becomes the line that names the surface, the key and the source. + const warn = spyWarn(); + const evaluator = makeEvaluator(); + const fault = { dialect: 'cel', source: 'nosuchroot6443cel.x > 1 &&&' }; + + expect(evaluateVisibility(fault, evaluator)).toBe(true); + expect(reports(warn)).toHaveLength(1); + expect(allWarnings(warn)).toHaveLength(1); + }); + + it('${…} TEMPLATE — one line for three evaluations, where the built-in warns once per evaluation', () => { + // Two independent directions in one cell: `reports()` 0 -> 1, and the TOTAL + // 3 -> 1. The second is the un-deduped built-in (objectui#6444) being + // transferred away from at this site. + const warn = spyWarn(); + const evaluator = makeEvaluator(); + const fault = '${nosuchroot6443tpl.x > 1}'; + + expect(evaluateVisibility(fault, evaluator)).toBe(true); + expect(evaluateVisibility(fault, evaluator)).toBe(true); + expect(evaluateVisibility(fault, evaluator)).toBe(true); + + // TOTAL first, on purpose: it is the assertion that MEASURES the built-in + // flood when this cell is run against the pre-fix site (it reads 3 there, + // one per evaluation). Asserting `reports()` first would short-circuit + // before the number this cell exists to show. + expect(allWarnings(warn)).toHaveLength(1); + expect(reports(warn)).toHaveLength(1); + }); + + it('the line NAMES the surface, the gate key, the predicate source and the engine reason', () => { + const warn = spyWarn(); + const evaluator = makeEvaluator(); + const fault = 'nosuchroot6443msg.x > 1'; + + evaluateVisibility(fault, evaluator); + const [line] = reports(warn); + expect(line).toContain(SURFACE); + expect(line).toContain('visible:'); + expect(line).toContain(fault); + expect(line).toContain('Reason:'); + }); + + it('the DEFENSIVE catch is loud too: an evaluator that THROWS still fails open, and says so', () => { + // `evaluateCondition` handles its own faults, so this path needs the + // evaluator itself to throw. It returned the same fail-open `true` in + // silence before; it no longer does. This is the cell that keeps the + // `catch` honest if a future caller ever passes `throwOnError` here. + const warn = spyWarn(); + const thrower = { + evaluateCondition() { + throw new Error('engine exploded 6443'); + }, + } as unknown as ExpressionEvaluator; + + expect(evaluateVisibility('nosuchroot6443throw.x > 1', thrower)).toBe(true); + const [line] = reports(warn); + expect(line).toContain('engine exploded 6443'); + }); +}); + +describe('objectui#6443 — the rate limit, measured in both directions', () => { + it('SAME source, many evaluations across the real area-election pass: ONE line', () => { + // Not a synthetic loop. This is the composition `AppSidebar` runs: + // `areas.filter(a => hasVisibleNavigationItems(a.navigation, …))` derives + // area visibility (objectui#3311) by re-running every item predicate, + // before the navigation renders them again — and both re-run on every + // sidebar re-render. The measured re-entry count is asserted, so this cell + // cannot pass on a site that is entered once. + const warn = spyWarn(); + const evaluator = makeEvaluator(); + const fault = "'org_admin' in nosuchroot6443dedupe.positions"; + + let faultEvaluations = 0; + const evalVis = (expr: unknown): boolean => { + if (expr === fault) faultEvaluations += 1; + return evaluateVisibility(expr as any, evaluator); + }; + + // One faulting entry per area. `type: 'action'` with no action handler is + // evaluated and then skipped, exactly as `AppSidebar` wires it, so the pass + // does not short-circuit on the first item. + const navigation = [ + { type: 'action', id: 'run', label: 'Run report', visible: fault }, + { type: 'link', id: 'home', label: 'Home', href: '/home' }, + ]; + const areas = [ + { name: 'sales', navigation }, + { name: 'ops', navigation }, + { name: 'admin', navigation }, + ]; + + // Two passes = the derivation plus one re-render. + for (let pass = 0; pass < 2; pass += 1) { + const visibleAreas = areas.filter((area) => + hasVisibleNavigationItems(area.navigation as any, { + evaluateVisibility: evalVis, + hasActionHandler: false, + }), + ); + expect(visibleAreas).toHaveLength(3); + } + + expect(faultEvaluations).toBe(6); + expect(reports(warn)).toHaveLength(1); + expect(allWarnings(warn)).toHaveLength(1); + }); + + it('TWO DIFFERENT sources: TWO lines — a dedupe that suppressed everything would look identical', () => { + const warn = spyWarn(); + const evaluator = makeEvaluator(); + const a = 'nosuchroot6443two_a.x > 1'; + const b = 'nosuchroot6443two_b.y == 3'; + + evaluateVisibility(a, evaluator); + evaluateVisibility(b, evaluator); + evaluateVisibility(a, evaluator); + evaluateVisibility(b, evaluator); + + const lines = reports(warn); + expect(lines).toHaveLength(2); + expect(lines.some((l) => l.includes(a))).toBe(true); + expect(lines.some((l) => l.includes(b))).toBe(true); + }); + + it('ONE rate limit, shared with @object-ui/react: a second evaluator does not buy a second line', () => { + // The property the shared export exists for. The dedupe `Set` lives in + // `@object-ui/react`; a local copy in this package would entitle one + // authored predicate to one line PER PACKAGE. + const warn = spyWarn(); + const fault = 'nosuchroot6443shared.x > 1'; + + evaluateVisibility(fault, makeEvaluator({ id: 'u1' })); + expect(reports(warn)).toHaveLength(1); + evaluateVisibility(fault, makeEvaluator({ id: 'u2' })); + expect(reports(warn)).toHaveLength(1); + }); +});