diff --git a/.changeset/action-visible-disabled-unified-condition.md b/.changeset/action-visible-disabled-unified-condition.md new file mode 100644 index 0000000000..78f79f4ef0 --- /dev/null +++ b/.changeset/action-visible-disabled-unified-condition.md @@ -0,0 +1,57 @@ +--- +"@objectstack/spec": minor +--- + +feat(spec): `ActionSchema.visible` / `disabled` speak one shape — `boolean | string(CEL) | {dialect, source}` (#5970) + +An action's two condition keys accepted different vocabularies. `disabled` took +all three arms; `visible` had no **boolean** arm, so `visible: true` — the most +obvious thing an author can write, and a shape already present in stored +metadata — was a parse error on the spec side while objectui's `ActionDef` +accepted it and pinned it with tests. + +Both keys now accept the same three arms, cheapest first: + +| arm | example | meaning | +|:---|:---|:---| +| `boolean` | `visible: false` | the degenerate literal — settled at authoring time | +| `string` | `disabled: "record.status == 'closed'"` | CEL shorthand, normalized to the envelope at parse time | +| `{ dialect, source }` | `{ dialect: 'cel', source: '…', meta: { rationale } }` | the full envelope, for authorship metadata or a non-default dialect | + +**Purely additive** — every shape that parsed before parses the same way, and +every shape that was rejected is still rejected (an empty CEL string, a number, +`null`, an envelope missing `dialect`, an envelope with neither `source` nor +`ast`, an unknown dialect). No migration, no ADR-0087 disposition: nothing an +author can write was removed or renamed. + +The boolean arm is deliberately **not** normalized into +`{dialect: 'cel', source: 'true'}`. A literal survives as a literal so a +renderer can branch on it without standing up an evaluator, and `false` stays +statically greppable. + +**Why unify rather than leave it.** An asymmetry between two keys that mean the +same *kind* of thing is a dialect nursery: it teaches every consumer to carry +its own widening, and each of those is a second de-facto contract (Prime +Directive #12). Console's `DeclaredActionsBar` was carrying exactly that as an +`(action as any).disabled` cast. This change is what lets #4075 step 3 derive +objectui's `ActionDef` from the spec schema and delete the casts. + +**One new rejection, at the interaction with `requiresFeature`.** The +declarative feature-gate sugar lowers into `visible`, so it now meets two +literals it never could before, and boolean algebra decides them in opposite +directions: + +- `visible: true` + `requiresFeature: 'x'` → the gate alone. `true && ` IS + ``, so spelling the default out explicitly lowers exactly like omitting + the key. +- `visible: false` + `requiresFeature: 'x'` → **parse error**. `false && ` + is `false` whatever the flag says, so the gate could never take effect and the + declaration is inert on arrival — the parses-clean-changes-nothing shape + ADR-0078 exists to reject. The message names both exits: drop + `requiresFeature` to keep it hidden, or drop `visible: false` to let the flag + decide. This combination was unwritable before (the boolean arm did not + exist), so no stored metadata can carry it. + +`bulkActions[].visible` is unchanged and keeps the two predicate arms only — a +per-record eligibility predicate has nothing to say as a constant. Its +description no longer claims shape-identity with `action.visible`. diff --git a/content/docs/references/ui/action.mdx b/content/docs/references/ui/action.mdx index aea4421b6d..691ce611b4 100644 --- a/content/docs/references/ui/action.mdx +++ b/content/docs/references/ui/action.mdx @@ -80,9 +80,9 @@ const result = ActionSchema.parse(data); | **refreshAfter** | `boolean` | optional | Refresh view after execution | | **undoable** | `boolean` | optional | Offer an Undo affordance after this single-record update action succeeds. | | **resultDialog** | `{ title?: string; description?: string; acknowledge?: string; format?: Enum<'qrcode' \| 'code-list' \| 'secret' \| 'text' \| 'json'>; … }` | optional | Render API response in a one-shot reveal dialog (suppresses successMessage when set). | -| **visible** | `string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }` | optional | Visibility predicate (CEL). | +| **visible** | `boolean \| string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }` | optional | Visibility predicate — `true`/`false` literal, CEL string, or `{dialect, source}` envelope. The action is offered when it evaluates TRUE. Omit = always visible. | | **requiresFeature** | `Enum<'twoFactor' \| 'passkeys' \| 'magicLink' \| 'organization' \| 'multiOrgEnabled' \| 'degradedTenancy' \| 'oidcProvider' \| 'sso' \| 'ssoEnforced' \| 'deviceAuthorization' \| … +3 more>` | optional | Public auth feature flag gating this action; lowered into `visible` at parse time. | -| **disabled** | `boolean \| string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }` | optional | Boolean or predicate (CEL) — action is disabled when TRUE. | +| **disabled** | `boolean \| string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }` | optional | Disabled predicate — `true`/`false` literal, CEL string, or `{dialect, source}` envelope. The action is shown but refused when it evaluates TRUE. Omit = never disabled. | | **requiredPermissions** | `string[]` | optional | [ADR-0066 D4] Capabilities required to invoke this action. Enforced with 403 on the platform action route (script/flow/modal + MCP) and mirrored as a UI hide; a `type: api` action pointed at a custom endpoint must re-check it there. | | **shortcut** | `never` | optional | [REMOVED] `action.shortcut` was removed in @objectstack/spec 17.0.0 (#3896 audit close-out) — it never triggered anything: no keydown listener feeds ActionEngine.getShortcuts(), and objectui's keyboard stack (useKeyboardShortcuts) is hand-registered and never consults action metadata. Delete the key. For a real shortcut, register the key in the Console keyboard stack and have its handler invoke the action by name. | | **bulkEnabled** | `never` | optional | [REMOVED] `action.bulkEnabled` was removed in @objectstack/spec 17.0.0 (#3896 audit close-out) — the multi-select toolbar is driven by the LIST VIEW's `bulkActions` / `bulkActionDefs`, never by this flag, so setting it changed nothing. Delete the key and declare the action in the view's `bulkActions` instead. | diff --git a/content/docs/references/ui/bulk-action.mdx b/content/docs/references/ui/bulk-action.mdx index 8804bcb365..4181f0de2a 100644 --- a/content/docs/references/ui/bulk-action.mdx +++ b/content/docs/references/ui/bulk-action.mdx @@ -49,7 +49,7 @@ const result = BulkActionDefSchema.parse(data); | **params** | `({ name: string; label?: string; help?: string; type: Enum<'text' \| 'textarea' \| 'email' \| 'url' \| 'phone' \| 'password' \| 'secret' \| … +42 more>; … } & Record)[]` | optional | Inputs collected once before the run. Omit to skip the params step and go straight to confirm. | | **confirmText** | `string` | optional | Confirmation text shown above the affected-record summary. | | **confirmLabel** | `string` | optional | Custom Confirm button label (default: "Run"). | -| **visible** | `string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }` | optional | Eligibility predicate (CEL), same shape as `action.visible`. Evaluated once PER SELECTED RECORD with that record bound: the button is offered when at least one passes, the run covers only those, and the rest are reported as skipped. A record-free predicate (`features.x`, `current_user.y`) therefore behaves as a plain button-level gate. Fail-closed — a predicate that faults excludes the record. | +| **visible** | `string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }` | optional | Eligibility predicate (CEL) — a string or a `{dialect, source}` envelope, i.e. `action.visible` without its boolean-literal arm (#5970): a per-record predicate has nothing to say as a constant. Evaluated once PER SELECTED RECORD with that record bound: the button is offered when at least one passes, the run covers only those, and the rest are reported as skipped. A record-free predicate (`features.x`, `current_user.y`) therefore behaves as a plain button-level gate. Fail-closed — a predicate that faults excludes the record. | | **requiredPermissions** | `string[]` | optional | [ADR-0066 D4] Capability gate on the button, `action.requiredPermissions` semantics verbatim: absent or empty always passes, several are AND-ed, and a client that cannot resolve the caller's capabilities fails OPEN (the server stays the authority). This key exists for INLINE defs — notably the `update`/`delete` data-plane forms, which dispatch no action and so have nothing to inherit a gate from; a def promoted from `bulkActions: ['']` (or an aggregate def naming a declared action) inherits the action's own declaration instead. On a data-plane def the gate governs visibility only — the write itself is still authorized by the data API's object permissions and server hooks. | | **maxRecords** | `integer` | optional | Selection size above which the run is blocked. Set it on defs whose server work is expensive — an aggregate def carries every selected id in one request. | | **batchSize** | `integer` | optional | Records per executor batch (default 200). Data-plane operations only — an aggregate run is a single call by definition. | diff --git a/packages/spec/src/kernel/public-auth-features.ts b/packages/spec/src/kernel/public-auth-features.ts index b541888fed..4261e05645 100644 --- a/packages/spec/src/kernel/public-auth-features.ts +++ b/packages/spec/src/kernel/public-auth-features.ts @@ -281,8 +281,12 @@ export function featureGatePredicate(name: PublicAuthFeatureName): string { /** Object shape the lowering transform operates on (post field-level parse). */ type WithRequiresFeature = { requiresFeature?: PublicAuthFeatureName; - /** Already normalized by ExpressionInputSchema to the `{dialect, source}` envelope. */ - visible?: { dialect?: unknown; source?: unknown } & Record; + /** + * Already normalized by ExpressionInputSchema to the `{dialect, source}` + * envelope — except for the literal arm, which surfaces here verbatim on the + * surfaces that declare one (`ActionSchema.visible`, #5970). + */ + visible?: boolean | ({ dialect?: unknown; source?: unknown } & Record); }; /** @@ -296,6 +300,14 @@ type WithRequiresFeature = { * - Existing CEL `visible` with a `source` → composed as * `() && ` (existing predicate first, gate last — the * hand-written convention). + * - Existing `visible: true` → the gate alone. `true && ` IS ``, so + * an author who spelled the default out explicitly gets the same lowering as + * one who omitted the key (the literal arm arrived with #5970). + * - Existing `visible: false` → loud parse error. Here the boolean algebra runs + * the other way: `false && ` is `false` whatever the flag says, so the + * gate could never take effect and the declaration is inert on arrival — + * precisely the parses-clean-changes-nothing key ADR-0078 exists to reject. + * Drop one of the two rather than shipping a gate that reads as load-bearing. * - Existing `visible` that is non-CEL or AST-only → loud parse error * (ADR-0078 no-silently-inert); write the combined predicate by hand. * @@ -310,10 +322,27 @@ export function lowerRequiresFeature( if (requiresFeature === undefined) return rest as Omit; const gate = featureGatePredicate(requiresFeature); - const existing = rest.visible; - if (existing === undefined) { + // Annotated rather than inferred: `rest` is a generic `Omit`, so + // `rest.visible` is a deferred indexed access that control flow cannot narrow + // — the `=== true` / `=== false` guards below would not strip the boolean arm + // off it, and the envelope spread at the end would not compile. + const existing: WithRequiresFeature['visible'] = rest.visible; + // `true` is the explicit spelling of "no gate of my own" — same lowering as an + // absent key. `false` can never be gated into visibility, so it is refused. + if (existing === undefined || existing === true) { return { ...rest, visible: { dialect: 'cel', source: gate } } as Omit; } + if (existing === false) { + ctx.addIssue({ + code: 'custom', + path: ['requiresFeature'], + message: + '`requiresFeature` cannot compose with `visible: false` — the literal already hides this ' + + 'unconditionally, so the feature gate can never take effect. Drop `requiresFeature` to keep it ' + + 'hidden, or drop `visible: false` to let the flag decide.', + }); + return rest as Omit; + } if (existing.dialect !== 'cel' || typeof existing.source !== 'string') { ctx.addIssue({ code: 'custom', diff --git a/packages/spec/src/ui/action.test.ts b/packages/spec/src/ui/action.test.ts index 51fb6460d1..5743573de3 100644 --- a/packages/spec/src/ui/action.test.ts +++ b/packages/spec/src/ui/action.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect } from 'vitest'; +import { z } from 'zod'; import { ActionSchema, ActionParamSchema, Action, type Action as ActionType, ACTION_LOCATIONS, ActionLocationSchema, type ActionLocation } from './action.zod'; import { getMetadataTypeSchema } from '../kernel/metadata-type-schemas'; import { ObjectSchema } from '../data/object.zod'; @@ -252,6 +253,126 @@ describe('requiresFeature lowering', () => { expect(bare.visible).toBeUndefined(); expect(bare).not.toHaveProperty('requiresFeature'); }); + + // #5970 gave `visible` a boolean arm, so the lowering now meets two literals + // it never could before. Boolean algebra decides both, in opposite directions. + it('treats `visible: true` as the explicit default — lowers to the gate alone', () => { + const result = ActionSchema.parse({ + name: 'invite_user', + label: 'Invite', + type: 'api', + target: '/api/v1/auth/organization/invite-member', + visible: true, + requiresFeature: 'organization', + } satisfies z.input); + // Identical to the sugar-only case above: `true && ` IS ``. + expect(result.visible).toEqual({ dialect: 'cel', source: 'features.organization != false' }); + expect(result).not.toHaveProperty('requiresFeature'); + }); + + it('rejects composition with `visible: false` loudly — the gate could never fire (ADR-0078)', () => { + const result = ActionSchema.safeParse({ + name: 'invite_user', + label: 'Invite', + type: 'api', + target: '/api/v1/auth/organization/invite-member', + visible: false, + requiresFeature: 'organization', + } satisfies z.input); + expect(result.success).toBe(false); + if (!result.success) { + const issue = result.error.issues.find((i) => i.path.includes('requiresFeature')); + expect(issue).toBeDefined(); + // The message must name BOTH exits, not just the diagnosis. + expect(issue!.message).toContain('`visible: false`'); + expect(issue!.message).toContain('Drop `requiresFeature`'); + } + }); +}); + +// ── #5970 — `visible` / `disabled` speak ONE shape ──────────────────────────── +// Ruled 2026-08-06: both keys are `boolean | string(CEL) | {dialect, source}`. +// Before this, `visible` had no boolean arm while `disabled` did, so the very +// common `visible: true` was a spec-side parse error that objectui's `ActionDef` +// accepted anyway — see the `ActionConditionInputSchema` comment in +// `action.zod.ts` for why an asymmetry between two keys of the same kind is a +// dialect nursery rather than a cosmetic gap. +describe('ActionSchema — visible/disabled unified condition shape', () => { + const base = { + name: 'transfer_ownership', + label: 'Transfer Ownership', + type: 'api', + target: '/api/v1/auth/organization/update-member-role', + } as const satisfies Partial>; + + const parse = (key: 'visible' | 'disabled', value: unknown) => + ActionSchema.safeParse({ ...base, [key]: value }); + + for (const key of ['visible', 'disabled'] as const) { + describe(`\`${key}\``, () => { + it('accepts the boolean arm and keeps the literal a literal', () => { + // NOT normalized into `{dialect:'cel', source:'true'}`: a renderer must + // be able to branch on the constant without standing up an evaluator. + for (const literal of [true, false]) { + const result = parse(key, literal); + expect(result.success, `${key}: ${literal}`).toBe(true); + if (result.success) expect(result.data[key]).toBe(literal); + } + }); + + it('accepts the CEL string arm and normalizes it to the envelope', () => { + const result = parse(key, "record.status == 'open'"); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data[key]).toEqual({ dialect: 'cel', source: "record.status == 'open'" }); + } + }); + + it('accepts the full envelope arm verbatim, authorship metadata included', () => { + const envelope = { + dialect: 'cel' as const, + source: "record.status == 'open'", + meta: { rationale: 'only open records can transfer', generatedBy: 'agent:spec' }, + }; + const result = parse(key, envelope); + expect(result.success).toBe(true); + if (result.success) expect(result.data[key]).toEqual(envelope); + }); + + // The widening must not shrink the rejection surface by one shape. Each + // of these was rejected before #5970 and is asserted to still be. + it.each([ + ['an empty CEL string', ''], + ['a number', 1], + ['null', null], + ['an envelope with neither `source` nor `ast`', { dialect: 'cel' }], + ['an envelope missing `dialect`', { source: "record.status == 'open'" }], + ['an envelope with an unknown dialect', { dialect: 'sql', source: 'SELECT 1' }], + ['a bare empty object', {}], + ['an array of predicates', ["record.a == 1", "record.b == 2"]], + ])('still rejects %s', (_label, value) => { + expect(parse(key, value).success).toBe(false); + }); + }); + } + + it('accepts both keys on one action, each on a different arm', () => { + const result = ActionSchema.parse({ + ...base, + visible: true, + disabled: "record.status == 'closed'", + } satisfies z.input); + expect(result.visible).toBe(true); + expect(result.disabled).toEqual({ dialect: 'cel', source: "record.status == 'closed'" }); + }); + + it('reaches the same shape through the registered `action` metadata schema', () => { + // The authoring door the Studio form and `GET /api/v1/meta` go through — + // a widening that stopped at the bare export would not reach an author. + const schema = getMetadataTypeSchema('action'); + expect(schema, "the 'action' metadata type must resolve to a schema").toBeDefined(); + expect(schema!.safeParse({ ...base, visible: true, disabled: false }).success).toBe(true); + }); }); describe('ActionSchema', () => { diff --git a/packages/spec/src/ui/action.zod.ts b/packages/spec/src/ui/action.zod.ts index 87240e3272..2cd4cdba9e 100644 --- a/packages/spec/src/ui/action.zod.ts +++ b/packages/spec/src/ui/action.zod.ts @@ -612,6 +612,33 @@ export type ActionAi = z.input; /** Post-parse shape of {@link ActionAi} — defaults applied, transforms run (ADR-0122). */ export type ActionAiParsed = z.infer; +/** + * The shape both action-level condition keys speak — `visible` and `disabled`. + * + * Three arms for one meaning, cheapest first: + * + * | arm | example | meaning | + * |:---|:---|:---| + * | `boolean` | `visible: false` | the degenerate literal — a condition that is settled at authoring time | + * | `string` | `disabled: "record.status == 'closed'"` | CEL shorthand, normalized to the envelope at parse time | + * | `{ dialect, source }` | `{ dialect: 'cel', source: '…', meta: { rationale } }` | the full envelope, for authorship metadata or a non-default dialect | + * + * The two keys were asymmetric until #5970 — `visible` had no `boolean` arm, so + * the very common `visible: true` was a parse error on the spec side while + * objectui's `ActionDef` accepted it and stored metadata was already written + * that way. An asymmetry between two keys that mean the same *kind* of thing is + * a dialect nursery: it teaches each consumer to keep its own widening (the + * `(action as any).disabled` cast in console's `DeclaredActionsBar` was exactly + * that), and every one of those is a second de-facto contract (Prime Directive + * #12). Unifying here is what lets #4075 step 3 derive `ActionDef` from this + * schema and delete the casts. + * + * The boolean arm is deliberately NOT normalized into `{dialect:'cel', + * source:'true'}`: a literal survives as a literal, so a renderer can branch on + * it without standing up an evaluator, and `false` stays statically greppable. + */ +const ActionConditionInputSchema = z.union([z.boolean(), ExpressionInputSchema]); + /** * The object half of {@link ActionSchema}, before its refinements. * @@ -894,7 +921,16 @@ const actionObject = () => strictObject({ }).optional().describe('Render API response in a one-shot reveal dialog (suppresses successMessage when set).'), /** Access */ - visible: ExpressionInputSchema.optional().describe('Visibility predicate (CEL).'), + /** + * Whether the action is offered at all. Three arms, one meaning — see + * {@link ActionConditionInputSchema}: `false` parks the action, `true` is the + * explicit default, and a predicate gates it per record/user/app/features. + * + * ⚠️ Client-side hiding is UX, not authorization — the button is gone, the + * route is not. An action gated for access-control reasons must also be + * refused server-side (`requiredPermissions`, or the action's own body). + */ + visible: ActionConditionInputSchema.optional().describe('Visibility predicate — `true`/`false` literal, CEL string, or `{dialect, source}` envelope. The action is offered when it evaluates TRUE. Omit = always visible.'), /** * Declarative capability gate (#2874) — action-level twin of the param * `requiresFeature`. Lowered at parse time into `visible` (`== true` for @@ -903,7 +939,13 @@ const actionObject = () => strictObject({ * `@objectstack/spec/kernel`. */ requiresFeature: z.enum(PUBLIC_AUTH_FEATURE_NAMES).optional().describe('Public auth feature flag gating this action; lowered into `visible` at parse time.'), - disabled: z.union([z.boolean(), ExpressionInputSchema]).optional().describe('Boolean or predicate (CEL) — action is disabled when TRUE.'), + /** + * Whether the action is offered but refused. Same three arms as `visible` + * ({@link ActionConditionInputSchema}) — a disabled action stays on screen + * (usually greyed, with the reason in a tooltip) where a non-visible one is + * gone entirely. + */ + disabled: ActionConditionInputSchema.optional().describe('Disabled predicate — `true`/`false` literal, CEL string, or `{dialect, source}` envelope. The action is shown but refused when it evaluates TRUE. Omit = never disabled.'), /** * [ADR-0066 D4] System capabilities required to INVOKE this action — a diff --git a/packages/spec/src/ui/bulk-action.zod.ts b/packages/spec/src/ui/bulk-action.zod.ts index 98658b50e8..4772f5aa2a 100644 --- a/packages/spec/src/ui/bulk-action.zod.ts +++ b/packages/spec/src/ui/bulk-action.zod.ts @@ -214,7 +214,7 @@ export const BulkActionDefSchema = lazySchema(() => z.object({ params: z.array(BulkActionParamSchema).optional().describe('Inputs collected once before the run. Omit to skip the params step and go straight to confirm.'), confirmText: z.string().optional().describe('Confirmation text shown above the affected-record summary.'), confirmLabel: z.string().optional().describe('Custom Confirm button label (default: "Run").'), - visible: ExpressionInputSchema.optional().describe('Eligibility predicate (CEL), same shape as `action.visible`. Evaluated once PER SELECTED RECORD with that record bound: the button is offered when at least one passes, the run covers only those, and the rest are reported as skipped. A record-free predicate (`features.x`, `current_user.y`) therefore behaves as a plain button-level gate. Fail-closed — a predicate that faults excludes the record.'), + visible: ExpressionInputSchema.optional().describe('Eligibility predicate (CEL) — a string or a `{dialect, source}` envelope, i.e. `action.visible` without its boolean-literal arm (#5970): a per-record predicate has nothing to say as a constant. Evaluated once PER SELECTED RECORD with that record bound: the button is offered when at least one passes, the run covers only those, and the rest are reported as skipped. A record-free predicate (`features.x`, `current_user.y`) therefore behaves as a plain button-level gate. Fail-closed — a predicate that faults excludes the record.'), requiredPermissions: z.array(z.string()).optional().describe("[ADR-0066 D4] Capability gate on the button, `action.requiredPermissions` semantics verbatim: absent or empty always passes, several are AND-ed, and a client that cannot resolve the caller's capabilities fails OPEN (the server stays the authority). This key exists for INLINE defs — notably the `update`/`delete` data-plane forms, which dispatch no action and so have nothing to inherit a gate from; a def promoted from `bulkActions: ['']` (or an aggregate def naming a declared action) inherits the action's own declaration instead. On a data-plane def the gate governs visibility only — the write itself is still authorized by the data API's object permissions and server hooks."), maxRecords: z.number().int().positive().optional().describe('Selection size above which the run is blocked. Set it on defs whose server work is expensive — an aggregate def carries every selected id in one request.'), batchSize: z.number().int().positive().optional().describe('Records per executor batch (default 200). Data-plane operations only — an aggregate run is a single call by definition.'),