diff --git a/.changeset/action-runner-declared-disabled-gate-3848.md b/.changeset/action-runner-declared-disabled-gate-3848.md new file mode 100644 index 0000000000..3690491dd6 --- /dev/null +++ b/.changeset/action-runner-declared-disabled-gate-3848.md @@ -0,0 +1,54 @@ +--- +"@object-ui/core": patch +--- + +An empty `disabled` predicate no longer refuses to run the action (objectui#3848) + +`ActionRunner.execute` — the engine's public execution entry, shared by every +action surface — gated on `action.disabled != null && action.disabled !== false` +and handed the value straight to `evaluateCondition`. That function documents one +default for "there is no condition here": return `true`, meaning +*visible/enabled*. On `disabled`, `true` means BLOCKED. So every empty predicate +was read as "disabled": the handler was never invoked and the caller got +`{ success: false, error: 'Action is disabled' }` — a state the metadata never +declared. Measured with a call-counting handler: + +- `disabled: ''` — handler ran: false +- `disabled: ' '` (whitespace only) — handler ran: false +- `disabled: { dialect: 'cel', source: '' }` (the empty envelope `objectstack build` can emit) — handler ran: false + +After objectui#3842 / objectui#3849 fixed the renderer halves, this was live +user-visible behaviour: the button became clickable and clicking it returned +`Action is disabled` — the renderer and the execution entry disagreeing about one +predicate value, the shape objectui#3314 already paid for once. + +The gate now asks whether a `disabled` gate is DECLARED — whether there is a +condition to reach a verdict on — before evaluating. "Nothing to evaluate" is +read from core's single predicate normalizer (`toPredicateInput`, which maps +`''`, `null`, an empty-`source` envelope and non-predicate values to +`undefined`), plus the whitespace-only string, which the normalizer wraps rather +than collapses and which `evaluateCondition` itself calls "no condition" +(`evalRowPredicate` applies the same blank-source rule). + +**Behaviour change surface, deliberately one-directional.** Only values with +nothing to evaluate change, and only from blocked to allowed: `''`, +whitespace-only, an empty-`source` envelope, and non-predicate junk (`0`, `{}`, +which previously coerced to "disabled"). `disabled: true`, a truthy expression +and a truthy CEL envelope still block; `disabled: false` and an absent `disabled` +still run; no expression- or envelope-valued predicate changes verdict. The +existing `catch { isDisabled = false }` fail-open posture is untouched, and this +change can only stop blocking things, never start. + +The value handed to the evaluator is deliberately left RAW rather than normalized +first. `evaluateCondition(toPredicateInput(x))` is not interchangeable with +`evaluateCondition(x)` for a string that is already a `${…}` template: +`toPredicateInput` assumes a bare expression and wraps unconditionally, so +`'${x}'` becomes `'${${x}}'`, fails to parse, returns verbatim, and coerces to a +constant `true` — a template-spelled predicate evaluated that way is ALWAYS +"disabled", whatever it says. That normalizer defect is filed as objectui#3871 +(it is live at the action renderers and `ActionEngine`, while `SchemaRenderer` and +`page:header` evaluate the raw value and pin the correct verdict); a tripwire next +to the new pins goes red the day it is fixed. Two rows therefore still differ +between the execution and renderer paths, each recorded with its owning issue: +the empty envelope (objectui#3850 owns the renderer half's scope ruling) and the +`${…}` spelling (objectui#3871). diff --git a/packages/core/src/actions/ActionRunner.ts b/packages/core/src/actions/ActionRunner.ts index d0d6bd329a..676206c779 100644 --- a/packages/core/src/actions/ActionRunner.ts +++ b/packages/core/src/actions/ActionRunner.ts @@ -24,6 +24,7 @@ import type { RunnableActionType } from '@object-ui/types'; import type { ActionInput as SpecActionInput } from '@objectstack/spec/ui'; import { ExpressionEvaluator } from '../evaluator/ExpressionEvaluator'; +import { toPredicateInput } from '../evaluator/predicateInput'; import { globalUndoManager, type UndoableOperation } from './UndoManager'; import { warnOnUnknownActionKeys } from './actionKeys'; @@ -526,6 +527,67 @@ function withIdentityAlias(context: ActionContext): ActionContext { return { ...context, os: { ...(os && typeof os === 'object' ? os : {}), user } }; } +/** + * Did this action declare a `disabled` gate at all — i.e. is there a CONDITION + * for the evaluator to reach a verdict on? (objectui#3848) + * + * `ExpressionEvaluator.evaluateCondition` documents and implements one default + * for "there is no condition here": it returns `true`, meaning + * *visible/enabled*. That default is correct on `visible` / `enabled`, and + * INVERTED on `disabled`, where `true` means "blocked". So the execution gate + * must decide "is there a condition?" itself, BEFORE evaluating — asking + * `!= null && !== false` (what it used to ask) hands every empty predicate to a + * function whose answer for "nothing to evaluate" is the strongest possible + * "yes, disabled". Measured consequence: `disabled: ''` returned + * `{ success: false, error: 'Action is disabled' }` and the handler never ran. + * + * The shapes `evaluateCondition` itself calls "no condition" are `null` / + * `undefined` / `''` / a whitespace-only string (`if (!trimmed) return true`) / + * an envelope whose `source` is blank. This gate excludes exactly those: + * + * - `toPredicateInput` is core's single predicate normalizer and already maps + * `''`, `null`, `undefined`, an empty-`source` envelope, and any + * non-predicate junk (`0`, `{}`) to `undefined` = "nothing to evaluate". + * - the whitespace-only string is the one shape it does NOT collapse (it + * wraps it as `'${ }'`), so it is named here. This is the same blank-source + * rule core's other predicate entry already applies — + * `evalRowPredicate` (`evaluator/listConditional.ts`) returns its fallback + * for `!source.trim()`. + * + * ## Why the gate normalizes but the VERDICT still reads the raw value + * + * `evaluateCondition(toPredicateInput(raw))` is NOT interchangeable with + * `evaluateCondition(raw)` for a string that is ALREADY a `${…}` template: + * `toPredicateInput` assumes a bare expression and wraps unconditionally, so + * `'${x}'` becomes `'${${x}}'`, which no longer matches the single-template + * fast path, fails to parse, and comes back as the original NON-EMPTY STRING — + * `Boolean(…)` = `true`. A `${…}`-spelled `disabled` predicate evaluated that + * way is therefore ALWAYS "disabled", whatever it says. Measured on this tree: + * `disabled: '${user.role === "guest"}'` (false, admin context) → blocked. + * + * So the value handed to the evaluator is left exactly as it was. Every + * non-empty shape — boolean, bare CEL, `${…}` template, `{ dialect, source }` + * envelope — keeps the verdict it reaches today, and this change can only stop + * blocking things, never start. The double-wrap itself is a defect of + * `toPredicateInput`, live at the action renderers and `ActionEngine` (which do + * compose it that way) and filed separately; `SchemaRenderer` and `page:header` + * both evaluate the raw value and both pin the correct verdict for that + * spelling, which is the behaviour preserved here. + * + * Scope note: `hasDeclaredVisibilityGate` + * (`components/renderers/action/visibility-gate.ts`, `!= null && !== ''`) is + * the renderer-side spelling of this question. It is deliberately NOT reused or + * moved: it lives in a package that depends on this one, it does not cover the + * empty-`source` envelope (objectui#3850) or the whitespace string, and + * objectui#3850 is the queued ruling on unifying the scope of "empty + * predicate" across the sites. Kept module-private until that lands — one gate, + * one site, no new exported dialect of the same question. + */ +function hasDisabledCondition(value: unknown): boolean { + if (typeof value === 'string' && value.trim() === '') return false; + return toPredicateInput(value) !== undefined; +} + export class ActionRunner { private handlers = new Map(); private scripts = new Map(); @@ -663,7 +725,15 @@ export class ActionRunner { } } - if (action.disabled != null && action.disabled !== false) { + // Ask "is a `disabled` gate DECLARED?" — not "is the key present and not + // `false`?" (objectui#3848). The old test let every EMPTY predicate reach + // `evaluateCondition`, whose answer for "no condition" is `true`, which on + // this key means BLOCKED: `disabled: ''` / `' '` / + // `{ dialect: 'cel', source: '' }` all returned `Action is disabled` with + // the handler never invoked. See `hasDisabledCondition` above for why the + // gate normalizes to decide but the verdict below still reads the RAW + // value (`toPredicateInput` double-wraps an already-`${…}` string). + if (hasDisabledCondition(action.disabled)) { // `disabled` may be a boolean, a CEL string, or the normalized envelope // `{ dialect, source }` (what `objectstack build` emits). The previous // code only evaluated the STRING form and treated any object as truthy, diff --git a/packages/core/src/actions/__tests__/ActionRunner.disabledGate.test.ts b/packages/core/src/actions/__tests__/ActionRunner.disabledGate.test.ts new file mode 100644 index 0000000000..4a25162e89 --- /dev/null +++ b/packages/core/src/actions/__tests__/ActionRunner.disabledGate.test.ts @@ -0,0 +1,266 @@ +/** + * 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#3848 — `ActionRunner.execute`'s declared-`disabled` gate. The + * EXECUTION half of the objectui#3492 family whose renderer halves are + * objectui#3842 (PR #3851) and objectui#3849 (PR #3861). + * + * The gate asked `action.disabled != null && action.disabled !== false`, so every + * EMPTY predicate got past it and was handed to `evaluateCondition` — which + * documents exactly one default for "there is no condition here": return `true`, + * meaning visible/enabled. On `disabled` that `true` means BLOCKED. Measured on + * `origin/main` @ f0a625aa7 before this change, with a `probe` handler that + * counts its own calls: + * + * disabled '' | handler ran: false | {"success":false,"error":"Action is disabled"} + * disabled ' ' | handler ran: false | {"success":false,"error":"Action is disabled"} + * disabled {dialect:'cel',source:''} | handler ran: false | {"success":false,"error":"Action is disabled"} + * disabled absent | handler ran: true | {"success":true} + * disabled false | handler ran: true | {"success":true} + * + * So this was not "the click did nothing" — execution was refused and the caller + * got an error naming a state the metadata never declared. + * + * ## What each row detects + * + * • the three EMPTY shapes (`''`, whitespace-only, empty-`source` envelope) → + * the handler must run. THE defect; each is an independent mutation + * detector, since the three arrive at "nothing to evaluate" by three + * different routes (string identity, `trim()`, envelope `source`). + * • `true` / a truthy expression / a truthy CEL envelope → still blocked. + * Anti-mutation guards: "never block anything" satisfies most of this table + * on its own, and these are what refuse it. + * • `false` / absent → still runs (ungated stays ungated), and `false` proves + * the gate did not start treating a declared-and-false gate as absent. + * • the non-predicate junk rows (`0`, `{}`) → now run. Behaviour change, and + * the fail-open direction the file already commits to (`catch { isDisabled + * = false }`): a value that is not a predicate at all must not decide that + * an action is disabled. + * + * ## The parity claim, and the two rows it deliberately does NOT make + * + * `rendererDisabled` is transcribed from objectui#3848's divergence table (the + * action face as it stands after #3842 + #3849), NOT recomputed here: + * `@object-ui/core` is the dependency of the renderer packages, so it cannot + * import them, and the live renderer-side pins are in those two PRs' tests. The + * parity test below therefore asserts that the verdicts THIS suite pins equal + * the verdicts recorded from the renderers — the #3314 invariant (one predicate + * value, one answer, whichever entry reads it) — for every row where that claim + * is honest. Two rows are excluded, each with its owning issue: + * + * • `{ dialect: 'cel', source: '' }` — the renderer still greys this out + * (`hasDeclaredVisibilityGate` is `!= null && !== ''`, which an envelope + * passes). That residual is objectui#3850, the queued ruling on how wide + * "empty predicate" is across the sites; the execution side pins the + * normalized semantics (an envelope with no source is nothing to evaluate). + * Not fixed here — objectui#3850 owns the renderer half. + * • a `${…}`-spelled predicate — the action-face renderers compose + * `evaluateCondition(toPredicateInput(x))`, which double-wraps an + * already-templated string into `'${${x}}'` and comes back TRUE whatever the + * expression says. That is objectui#3871. The execution path keeps reading + * the RAW value and therefore keeps the correct verdict — the same verdict + * `SchemaRenderer` and `page:header` already pin for that spelling. + * + * ## Reverse verification (direction predicted before running) + * + * Restoring the old gate (`action.disabled != null && action.disabled !== false`) + * while leaving evaluation untouched must turn exactly the five + * nothing-to-evaluate rows RED — `''`, `' '`, the empty envelope, `0`, `{}` — + * each naming its own shape, and leave `absent` / `false` / `true` / both + * expression rows / both non-empty envelope rows GREEN. That is the whole diff: + * this change can only stop blocking things, never start. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ActionRunner, type ActionContext, type ActionDef } from '../ActionRunner'; +import { ExpressionEvaluator } from '../../evaluator/ExpressionEvaluator'; +import { toPredicateInput } from '../../evaluator/predicateInput'; + +const CONTEXT: ActionContext = { + data: { id: 1 }, + record: { id: 1, status: 'active' }, + user: { id: 'u1', role: 'admin' }, +}; + +interface Shape { + label: string; + /** Omit the key entirely (the "undeclared" row). */ + absent?: boolean; + disabled?: unknown; + /** Execution-side expectation: does the gate refuse to run the handler? */ + blocked: boolean; + /** + * The action-face renderer's verdict for the SAME value, transcribed from + * objectui#3848's table. `null` = parity deliberately not claimed for this + * row; `divergence` names the issue that owns the difference. + */ + rendererDisabled: boolean | null; + divergence?: string; +} + +const SHAPES: Shape[] = [ + // ── the defect: nothing to evaluate must not block ─────────────────────── + { label: "disabled: '' (empty predicate)", disabled: '', blocked: false, rendererDisabled: false }, + { label: "disabled: ' ' (whitespace-only predicate)", disabled: ' ', blocked: false, rendererDisabled: false }, + { + label: "disabled: { dialect: 'cel', source: '' } (empty envelope)", + disabled: { dialect: 'cel', source: '' }, + blocked: false, + rendererDisabled: null, + divergence: 'objectui#3850 — the renderer still reads an empty-source envelope as a declared gate', + }, + // ── unchanged: ungated stays ungated ──────────────────────────────────── + { label: 'disabled absent (undeclared)', absent: true, blocked: false, rendererDisabled: false }, + { label: 'disabled: false (declared, verdict false)', disabled: false, blocked: false, rendererDisabled: false }, + // ── unchanged: a real gate still blocks ───────────────────────────────── + { label: 'disabled: true', disabled: true, blocked: true, rendererDisabled: true }, + { + label: 'disabled: bare CEL that is true', + disabled: 'user.role == "admin"', + blocked: true, + rendererDisabled: true, + }, + { + label: 'disabled: bare CEL that is false', + disabled: 'user.role == "guest"', + blocked: false, + rendererDisabled: false, + }, + { + label: "disabled: { dialect: 'cel', source: 'true' }", + disabled: { dialect: 'cel', source: 'true' }, + blocked: true, + rendererDisabled: true, + }, + { + label: "disabled: { dialect: 'cel', source: 'false' }", + disabled: { dialect: 'cel', source: 'false' }, + blocked: false, + rendererDisabled: false, + }, + { + label: 'disabled: legacy template that is true', + disabled: '${user.role === "admin"}', + blocked: true, + rendererDisabled: null, + divergence: 'objectui#3871 — the renderers double-wrap a template-spelled predicate into a constant true', + }, + { + label: 'disabled: legacy template that is false', + disabled: '${user.role === "guest"}', + blocked: false, + rendererDisabled: null, + divergence: 'objectui#3871 — same double-wrap; here it is the row where the constant true is WRONG', + }, + // ── behaviour change: non-predicate junk fails open ───────────────────── + { label: 'disabled: 0 (not a predicate)', disabled: 0, blocked: false, rendererDisabled: null, divergence: 'objectui#3871 family — the renderers coerce junk through the same compose' }, + { label: 'disabled: {} (not a predicate)', disabled: {}, blocked: false, rendererDisabled: null, divergence: 'objectui#3871 family — the renderers coerce junk through the same compose' }, +]; + +/** Execute one shape and report whether the handler ran. */ +async function runShape(shape: Shape) { + const runner = new ActionRunner(CONTEXT); + const onClick = vi.fn(); + const action: Record = { onClick }; + if (!shape.absent) action.disabled = shape.disabled; + const result = await runner.execute(action as unknown as ActionDef); + return { result, ran: onClick.mock.calls.length > 0 }; +} + +describe('ActionRunner.execute — declared `disabled` gate (objectui#3848)', () => { + it.each(SHAPES)('$label', async (shape) => { + const { result, ran } = await runShape(shape); + if (shape.blocked) { + expect(ran).toBe(false); + expect(result).toEqual({ success: false, error: 'Action is disabled' }); + } else { + // The handler RUNNING is the assertion that matters: objectui#3848 was + // measured by the handler never being invoked, not by a missing toast. + expect(ran).toBe(true); + expect(result.success).toBe(true); + expect(result.error).toBeUndefined(); + } + }); + + it('an empty predicate does not even reach the evaluator (the gate decides, not the verdict)', async () => { + const runner = new ActionRunner(CONTEXT); + const spy = vi.spyOn( + runner.getEvaluator() as unknown as { evaluateCondition: (c: unknown) => boolean }, + 'evaluateCondition', + ); + const onClick = vi.fn(); + await runner.execute({ disabled: '', onClick } as unknown as ActionDef); + expect(onClick).toHaveBeenCalledOnce(); + expect(spy).not.toHaveBeenCalled(); + spy.mockRestore(); + }); + + it('the execution verdict equals the renderer verdict for every shape where parity is claimed', () => { + const claimed = SHAPES.filter(s => s.rendererDisabled !== null); + // Guard the guard: if a future edit nulls out the whole column, this test + // would pass by asserting nothing. + expect(claimed.length).toBeGreaterThanOrEqual(8); + for (const shape of claimed) { + expect( + shape.blocked, + `${shape.label}: execution and the action-face renderer must agree (objectui#3314)`, + ).toBe(shape.rendererDisabled); + } + }); + + it('records which shapes are knowingly still divergent, and who owns each', () => { + const divergent = SHAPES.filter(s => s.rendererDisabled === null); + for (const shape of divergent) { + expect(shape.divergence, `${shape.label} must name the issue that owns it`).toMatch(/objectui#\d+/); + } + expect(divergent.map(s => s.label)).toEqual([ + "disabled: { dialect: 'cel', source: '' } (empty envelope)", + 'disabled: legacy template that is true', + 'disabled: legacy template that is false', + 'disabled: 0 (not a predicate)', + 'disabled: {} (not a predicate)', + ]); + }); +}); + +describe('why the gate cannot delegate "is there a condition?" to the verdict (objectui#3848)', () => { + it('evaluateCondition answers `true` for "no condition" — which on `disabled` means blocked', () => { + const ev = new ExpressionEvaluator(CONTEXT); + // The inverted default, in one line. This is the whole mechanism: correct on + // `visible`/`enabled`, backwards on `disabled`. + expect(ev.evaluateCondition(undefined)).toBe(true); + expect(ev.evaluateCondition('')).toBe(true); + expect(ev.evaluateCondition(' ')).toBe(true); + expect(ev.evaluateCondition({ dialect: 'cel', source: '' })).toBe(true); + }); + + it('toPredicateInput collapses two of the three empty shapes, and wraps the third', () => { + // Why the gate is not simply `toPredicateInput(x) !== undefined`: the + // whitespace string survives normalization as an evaluable-looking template, + // so the gate names it explicitly (same blank-source rule `evalRowPredicate` + // applies in `evaluator/listConditional.ts`). + expect(toPredicateInput('')).toBeUndefined(); + expect(toPredicateInput({ dialect: 'cel', source: '' })).toBeUndefined(); + expect(toPredicateInput(' ')).toBe('${ }'); + }); + + it('TRIPWIRE (objectui#3871): toPredicateInput double-wraps an already-`${…}` string into a constant true', () => { + const ev = new ExpressionEvaluator(CONTEXT); + const falsePredicate = '${user.role === "guest"}'; + // Raw — the correct verdict, and what this gate evaluates. + expect(ev.evaluateCondition(falsePredicate)).toBe(false); + // Normalized — wrapped a second time, unparseable, returned verbatim, truthy. + expect(toPredicateInput(falsePredicate)).toBe('${${user.role === "guest"}}'); + expect(ev.evaluateCondition(toPredicateInput(falsePredicate) as never)).toBe(true); + // When objectui#3871 is fixed this case goes RED on the last two + // expectations. That is the signal to delete this tripwire and (only then) + // let the gate evaluate the normalized value, which is what objectui#3848's + // dispatch originally proposed. + }); +});