diff --git a/src/actions/catalog.actions.ts b/src/actions/catalog.actions.ts index 0b5ccd5..b6826e9 100644 --- a/src/actions/catalog.actions.ts +++ b/src/actions/catalog.actions.ts @@ -4,6 +4,8 @@ import { defineAction } from '@objectstack/spec'; import { CATALOG_APPLY_ACTION, + CATALOG_APPLY_TO_PEOPLE_ACTION, + CATALOG_ITEM_OBJECT, CATALOG_SYNC_ACTION, } from './catalog.handlers.js'; @@ -31,6 +33,18 @@ import { * button. Declaring a location a renderer does not serve would be the * ADR-0078 declares-renders-does-nothing shape. * + * ── And why apply ALSO has an object-bound twin ─────────────────────────── + * Honest is not the same as reachable. Onboarding — "apply this customer's + * existing catalog to these people" — is the adoption path the product lives + * or dies on, and headless left it with no button at all. The answer is not to + * make the global action render somewhere it cannot: it is + * {@link CatalogApplyToPeopleAction}, a SECOND DECLARATION bound to + * `duly_catalog_item` and placed on that list's toolbar, wired to the SAME + * handler function. Two placements of one action, never two implementations. + * `duly_catalog_sync` gets no twin — it rewrites authored cadence on duties + * people are already working to, org-wide by default, and a one-click button + * beside the catalog is the wrong affordance for that. + * * ── Why `type: 'script'` with a `target` and no `body` ──────────────────── * The cadence maths and the idempotency probe are real code with real tests, * not a sandboxed L1/L2 snippet. `target` names the handler registered from @@ -87,6 +101,85 @@ export const CatalogApplyAction = defineAction({ ], }); +/** + * `duly_catalog_apply_to_people` — the SAME action, on a button. + * + * ── The gap this closes ─────────────────────────────────────────────────── + * `duly_catalog_apply` above is object-less, and in protocol 17 an object-less + * action has no UI home: `global_nav` was retired and every surviving location + * is object-bound. So the product's single biggest adoption path — "apply the + * catalog this customer already has to these people" — was reachable only over + * `POST /api/v1/actions/global/duly_catalog_apply` or MCP. A pilot whose first + * step is writing curl does not happen. + * + * The catalog list IS where an admin is standing when they want this, so the + * action is bound to `duly_catalog_item` and placed on its `list_toolbar`. + * + * ── Spread from the global, deliberately ────────────────────────────────── + * Everything except the four keys placement actually changes — `params`, + * `requiredPermissions`, `description`, `icon`, `variant`, `type` — is spread + * from {@link CatalogApplyAction} rather than restated. Two hand-written copies + * of a param contract drift, and the drift is silent in the direction that + * matters: the dispatcher validates the params of the action you CALLED + * (ADR-0104 D2), so a twin that fell behind would 400 on a dialog the global + * route accepts, or — worse — quietly stop requiring `users`. `defineAction` + * deep-copies on parse, so the two declarations share no mutable state. + * + * `requiredPermissions` rides that spread on purpose. An object-bound action + * that skipped the capability its global twin requires would not be a + * convenience, it would be a bypass: the same 403 gate on the platform action + * route, and the same hide on the toolbar (objectui's `action:bar` filters its + * own set through the shared capability gate before placement). + * + * ── What this is NOT ────────────────────────────────────────────────────── + * Not `ai: { exposed: true }`. Arming an agent to bulk-create duties for + * arbitrary people is a decision to take deliberately, and it belongs to the + * capability work, not to a card about where a button goes. + * + * Not a replacement for the global. `duly_catalog_apply` stays registered and + * headless: it is what the REST route and MCP use, and nothing about giving + * the flow a button makes those paths less true. + * + * ── Why the input is one dialog and not two steps ───────────────────────── + * Measured before it was written, because the alternative (select catalog rows + * in the list, then a modal for the people) is a different action with a + * different handler contract. A `list_toolbar` action CAN carry this input: + * + * - The spec couples `locations` to nothing — `ActionSchema` has no + * refinement relating a location to `params`, and this exact declaration + * parses clean. + * - Param collection is location-blind: objectui's `ActionRunner.execute` + * opens the param dialog on `Array.isArray(action.params) && length > 0`, + * before dispatch, with no location gate + * (`packages/core/src/actions/ActionRunner.ts`). + * - `type: 'user'` + `multiple` really is a multi-person picker on that path: + * `resolveActionParams` carries `multiple` through the inline branch, + * `paramToField` maps `user` onto the user widget with it, and `UserField` + * delegates to `LookupField`, whose multi-select is the picker itself. + * - And the bag the dialog submits — `{ position_code, users: [...] }` — + * passes the dispatcher's own `validateActionParams`, which refuses a + * scalar in `users`. The contract is enforced, not merely declared. + * + * So the one-step form is what ships. The two-step fallback would have needed + * the handler to read `_selectedIds` instead of `position_code` — a second + * implementation of the thing this action already does. + */ +export const CatalogApplyToPeopleAction = defineAction({ + ...CatalogApplyAction, + name: CATALOG_APPLY_TO_PEOPLE_ACTION, + objectName: CATALOG_ITEM_OBJECT, + // `target` names the registered handler and must move with the name: the + // engine key is `:`, so this one resolves to + // `duly_catalog_item:duly_catalog_apply_to_people` — registered in + // `catalog.handlers.ts` to the very same `applyCatalogHandler` function. + target: CATALOG_APPLY_TO_PEOPLE_ACTION, + // Contextual, and the reason the label is not simply inherited: standing on + // the Role catalog, "Apply role catalog" asks the reader to apply the thing + // they are already looking at. The choice being made here is WHO. + label: 'Apply to people', + locations: ['list_toolbar'], +}); + /** * `duly_catalog_sync` — replay catalog cadence edits onto instantiated duties. * diff --git a/src/actions/catalog.handlers.ts b/src/actions/catalog.handlers.ts index 74e296b..4f27697 100644 --- a/src/actions/catalog.handlers.ts +++ b/src/actions/catalog.handlers.ts @@ -19,6 +19,27 @@ import type { HandlerRegistrationContext } from './register-handlers.js'; export const CATALOG_APPLY_ACTION = 'duly_catalog_apply'; export const CATALOG_SYNC_ACTION = 'duly_catalog_sync'; +/** + * The OBJECT-BOUND twin of `duly_catalog_apply` — the same action, given a + * place to be clicked. See `catalog.actions.ts` for why it exists and what it + * is deliberately not. + * + * A DISTINCT name, not a second declaration of `duly_catalog_apply`, and that + * is measured rather than stylistic: `defineStack` accepts two actions sharing + * one `name` without a word — both survive into `stack.actions`, and it does + * so even for two GLOBAL actions, where the `:` handler map then + * has one silently shadow the other. Reported upstream rather than relied on. + */ +export const CATALOG_APPLY_TO_PEOPLE_ACTION = 'duly_catalog_apply_to_people'; + +/** + * The object the twin binds to — and therefore the engine key its handler + * registers under. `executeAction` is an exact-string `Map` lookup on + * `:` and tries the action's OWN object before `global`, so an + * object-bound action's handler filed under `global` is unreachable. + */ +export const CATALOG_ITEM_OBJECT = 'duly_catalog_item'; + /** * The engine object key an object-less action registers under. * @@ -447,10 +468,19 @@ export const syncCatalogHandler: ActionHandler = async (ctx) * Register both catalog handlers on the engine. * * Called from `registerDulyActionHandlers` in `register-handlers.ts`, which - * `objectstack.config.ts` invokes from `onEnable`. Both register under - * {@link GLOBAL_ACTION_OBJECT} because both actions are object-less. + * `objectstack.config.ts` invokes from `onEnable`. The two OBJECT-LESS actions + * register under {@link GLOBAL_ACTION_OBJECT}; the object-bound twin registers + * under {@link CATALOG_ITEM_OBJECT}, which is the only key its dispatch can + * reach. + * + * THREE registrations, TWO handler functions. The twin passes the very same + * `applyCatalogHandler` REFERENCE the global one does — not a copy, not a + * wrapper. A second key on one function is the whole cost of giving the action + * a button; a second function would be a second implementation to keep in step, + * which is the thing this placement was explicitly not allowed to buy. */ export function registerCatalogActionHandlers(ql: HandlerRegistrationContext): void { ql.registerAction(GLOBAL_ACTION_OBJECT, CATALOG_APPLY_ACTION, applyCatalogHandler); ql.registerAction(GLOBAL_ACTION_OBJECT, CATALOG_SYNC_ACTION, syncCatalogHandler); + ql.registerAction(CATALOG_ITEM_OBJECT, CATALOG_APPLY_TO_PEOPLE_ACTION, applyCatalogHandler); } diff --git a/src/actions/index.ts b/src/actions/index.ts index 445184c..693cf97 100644 --- a/src/actions/index.ts +++ b/src/actions/index.ts @@ -13,15 +13,26 @@ // makes `name` optional and fails the assignment. A named array is `never[]` // while empty and infers correctly the moment something is pushed into it. -import { CatalogApplyAction, CatalogSyncAction } from './catalog.actions.js'; +import { + CatalogApplyAction, + CatalogApplyToPeopleAction, + CatalogSyncAction, +} from './catalog.actions.js'; import { TaskCompleteAction, TaskSkipAction, TaskUndoAction } from './task.actions.js'; -export { CatalogApplyAction, CatalogSyncAction }; +export { CatalogApplyAction, CatalogApplyToPeopleAction, CatalogSyncAction }; export { TaskCompleteAction, TaskSkipAction, TaskUndoAction }; export const dulyActions = [ CatalogApplyAction, CatalogSyncAction, + // The object-bound twin of `duly_catalog_apply` — same handler, given a + // place to be clicked. `objectName: 'duly_catalog_item'`, so defineStack() + // merges it into that object's actions and its `list_toolbar` button + // renders on the Role catalog list. Missing from this array it would be + // dead metadata: it type-checks, it reads as wired, and no toolbar ever + // sees it. + CatalogApplyToPeopleAction, // Object-bound (`objectName: 'duly_task'`), so defineStack() merges them // into duly_task.actions and the dispatcher can find their declaration. // An action reachable from a row still needs its handler registered in diff --git a/test/catalog-action-placement.test.ts b/test/catalog-action-placement.test.ts new file mode 100644 index 0000000..a793674 --- /dev/null +++ b/test/catalog-action-placement.test.ts @@ -0,0 +1,195 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, expect, it } from 'vitest'; + +import { validateActionParams } from '@objectstack/spec/ui'; + +import { dulyActions } from '../src/actions/index.js'; +import { registerDulyActionHandlers } from '../src/actions/register-handlers.js'; +import type { HandlerRegistrationContext } from '../src/actions/register-handlers.js'; +import { + CATALOG_APPLY_ACTION, + CATALOG_APPLY_TO_PEOPLE_ACTION, + CATALOG_ITEM_OBJECT, + CATALOG_SYNC_ACTION, + GLOBAL_ACTION_OBJECT, + applyCatalogHandler, +} from '../src/actions/catalog.handlers.js'; + +/** + * Where `duly_catalog_apply` can be CLICKED — and the wiring that makes the + * click do something. + * + * `test/catalog-instantiate.test.ts` owns what the action DOES. This file owns + * its placement: that the object-bound twin exists, that it is the same action + * rather than a second one, that its capability gate is not weaker than the + * global's, and — the part with no author-time gate at all — that its handler + * is registered under the one engine key its dispatch can reach. + * + * That last one is the failure this suite exists for. An action whose handler + * is not registered RENDERS, is clickable, and fails at call time with + * `Action '' on object '' not found`. `pnpm validate` parses the + * declaration and knows nothing about the registry, so it passes green either + * way. The ablation is in the PR body: deleting the twin's `registerAction` + * line turns "wired under the key its dispatch reaches" red and leaves + * `pnpm validate` at exit 0. + */ + +function registered(): Array<{ object: string; action: string; handler: unknown }> { + const calls: Array<{ object: string; action: string; handler: unknown }> = []; + const ql: HandlerRegistrationContext = { + registerAction: (...args: unknown[]) => { + calls.push({ object: String(args[0]), action: String(args[1]), handler: args[2] }); + }, + // This suite is about the action-handler registry, so the engine methods + // `bindDispatchEngine(ql)` needs are unused no-ops rather than a real one. + find: async () => [], + insert: async () => ({}), + update: async () => undefined, + }; + registerDulyActionHandlers(ql); + return calls; +} + +const twin = () => dulyActions.find((a) => a.name === CATALOG_APPLY_TO_PEOPLE_ACTION); +const global = () => dulyActions.find((a) => a.name === CATALOG_APPLY_ACTION); + +describe('the catalog-apply twin is reachable from the UI', () => { + it('is in the barrel — an action missing from it is dead metadata that type-checks', () => { + expect(twin()).toBeDefined(); + }); + + it('binds to duly_catalog_item and declares the one location a renderer serves', () => { + // `global_nav` was retired in protocol 17 and every surviving location is + // object-bound, so `objectName` is what buys the placement: defineStack() + // merges the action into that object's `actions`, which is the array the + // list toolbar filters by location. + expect(twin()?.objectName).toBe(CATALOG_ITEM_OBJECT); + expect(twin()?.locations).toEqual(['list_toolbar']); + }); + + it('leaves the global action headless and registered — REST and MCP still use it', () => { + // The twin adds a placement; it does not replace the object-less action. + expect(global()?.objectName).toBeUndefined(); + expect(global()?.locations).toEqual([]); + const wired = registered().map((c) => `${c.object}:${c.action}`); + expect(wired).toContain(`${GLOBAL_ACTION_OBJECT}:${CATALOG_APPLY_ACTION}`); + expect(wired).toContain(`${GLOBAL_ACTION_OBJECT}:${CATALOG_SYNC_ACTION}`); + }); +}); + +describe('the twin is the same action, not a second implementation', () => { + it('is wired to the SAME handler function reference as the global action', () => { + // Identity, not equivalence. Two functions that behave the same today are + // two functions to keep in step tomorrow, and this card was explicitly not + // allowed to buy that. + const calls = registered(); + const globalCall = calls.find( + (c) => c.object === GLOBAL_ACTION_OBJECT && c.action === CATALOG_APPLY_ACTION, + ); + const twinCall = calls.find( + (c) => c.object === CATALOG_ITEM_OBJECT && c.action === CATALOG_APPLY_TO_PEOPLE_ACTION, + ); + expect(twinCall?.handler).toBe(applyCatalogHandler); + expect(twinCall?.handler).toBe(globalCall?.handler); + }); + + it('is wired under the key its dispatch reaches, and nowhere else', () => { + // THE UNGATED FAILURE. `executeAction` is an exact-string Map lookup on + // `:` and tries the action's own object before `global`, so + // an object-bound action registered under `global` is a button that 404s. + // Nothing at author time says so — this assertion is the whole guard. + const wired = new Set(registered().map((c) => `${c.object}:${c.action}`)); + expect(wired).toContain(`${CATALOG_ITEM_OBJECT}:${CATALOG_APPLY_TO_PEOPLE_ACTION}`); + expect(wired).not.toContain(`${GLOBAL_ACTION_OBJECT}:${CATALOG_APPLY_TO_PEOPLE_ACTION}`); + }); + + it('names its own target, so the declaration cannot point at a key nobody registered', () => { + expect(twin()?.target).toBe(CATALOG_APPLY_TO_PEOPLE_ACTION); + }); + + it('carries the same param contract as the global, key for key', () => { + // The dispatcher validates the params of the action you CALLED (ADR-0104 + // D2). Two hand-maintained copies drift, and the drift is silent in the + // direction that matters — a twin that stopped requiring `users` would + // accept a dialog the global route refuses. + const shape = (action: ReturnType) => + (action?.params ?? []).map((p) => ({ + name: p.name, + type: p.type, + required: p.required, + multiple: p.multiple, + })); + expect(shape(twin())).toEqual(shape(global())); + // Non-vacuous: the comparison above would also pass if both were empty. + expect(shape(global())).toHaveLength(2); + }); + + it('does not weaken the capability gate — an object-bound bypass is not a convenience', () => { + // Same 403 on the platform action route, same hide on the toolbar. A twin + // that dropped `duly.catalog.apply` would hand anyone who can reach the + // Role catalog the power to mint duties for any user id they typed. + expect(twin()?.requiredPermissions).toEqual(['duly.catalog.apply']); + expect(twin()?.requiredPermissions).toEqual(global()?.requiredPermissions); + }); + + it('is not exposed to agents — bulk-creating duties for arbitrary people is a deliberate decision', () => { + expect(twin()?.ai?.exposed).toBeFalsy(); + expect(global()?.ai?.exposed).toBeFalsy(); + }); +}); + +describe('the input a list_toolbar action can actually collect', () => { + // Measured before the twin was written, because the alternative — select + // catalog rows, then a modal for the people — is a different handler + // contract (`_selectedIds` in place of `position_code`). It can, so the + // one-step dialog is what ships. + + it('declares position_code plus a multi-person picker', () => { + const params = twin()?.params ?? []; + const position = params.find((p) => p.name === 'position_code'); + expect(position?.type).toBe('text'); + expect(position?.required).toBe(true); + + const users = params.find((p) => p.name === 'users'); + expect(users?.type).toBe('user'); + expect(users?.multiple).toBe(true); + expect(users?.required).toBe(true); + }); + + it('the bag that dialog submits passes the dispatcher\'s own param contract', () => { + // Not a restatement of the declaration: this runs the spec's + // `validateActionParams` — the same ADR-0104 D2 check the REST and MCP + // dispatch paths run before the handler — over the values the multi-person + // picker produces. + const resolved = (twin()?.params ?? []).map((p) => ({ + name: String(p.name), + type: p.type, + required: p.required, + multiple: p.multiple, + })); + expect( + validateActionParams(resolved, { + position_code: 'plant_compliance_officer', + users: ['user_a', 'user_b', 'user_c'], + }), + ).toEqual([]); + }); + + it('and that contract is enforced, not merely declared — a scalar in `users` is refused', () => { + // The negative leg. Without it the assertion above would pass just as + // happily against a param whose value shape was left open. + const resolved = (twin()?.params ?? []).map((p) => ({ + name: String(p.name), + type: p.type, + required: p.required, + multiple: p.multiple, + })); + const issues = validateActionParams(resolved, { + position_code: 'plant_compliance_officer', + users: 'user_a', + }); + expect(issues.map((i) => i.param)).toContain('users'); + expect(issues.find((i) => i.param === 'users')?.code).toBe('invalid_shape'); + }); +});