fix(spec): ActionEngineFacade.delete declares the id array the runtime already accepts, and says which convention is the contract

.changeset/15117-action-engine-delete-id-array.md

@@ -0,0 +1,32 @@
+---
+'@objectstack/spec': minor
+---
+
+fix(spec): `ActionEngineFacade.delete` declares the id ARRAY the runtime has always accepted, and says which convention is the contract (#15117)
+
+`delete(object, id: string)` declared one id. The runtime facade
+(`buildActionEngineFacade` in `packages/runtime`) has accepted `string | string[]`
+all along — normalising the argument and issuing one `ql.delete` per id — and
+described that in a comment as a tolerance two handler suites happened to cause.
+The declaration was simply behind the behaviour, and the one first-party suite on
+the array form could only reach it by hand-rolling a private copy of the
+interface (a copy that had already drifted on `find`).
+
+The slot is now `delete(object: string, idOrIds: string | string[])`, and the
+member's doc comment states the contract instead of leaving it to be inferred
+from a runtime comment two packages away:
+
+- **Both spellings are contract.** One row is `delete(object, id)`; a set is
+  `delete(object, ids)` — a handler holding a list does not have to unroll it
+  into a loop to stay on the contract.
+- **The array form is a convenience over the same per-row path** — not a bulk or
+  atomic delete. There is no transaction around the set: a failure part-way
+  leaves the ids before it deleted. An empty array deletes nothing and resolves.
+
+Nothing is removed and nothing narrows: every existing single-id call still
+type-checks, and no runtime behaviour changes — this release makes the published
+type describe what was already being served. That makes it non-breaking, not a
+patch: widening a published parameter is a purely additive widening of a public
+surface, which takes at least `minor` whatever the commit type says. Handler authors who copied the
+facade into a local context type to reach the array form can delete the copy and
+annotate with `ActionHandlerContext` / `ActionHandler` from `@objectstack/spec/ui`.

content/docs/ui/actions.mdx

@@ -137,15 +137,39 @@ For logic that belongs in real source files, point `target` at a handler name
 and register it in your config's `onEnable` lifecycle hook:
 
 ```typescript title="src/actions/task.handlers.ts"
-export async function completeTask(ctx: ActionContext): Promise<void> {
-  const { record, engine } = ctx;   // ctx = { record, user, engine, params }
+import type { ActionHandlerContext } from '@objectstack/spec/ui';
+
+export async function completeTask(ctx: ActionHandlerContext): Promise<void> {
+  const { record, engine } = ctx;   // ctx = { record, params, user, session, engine }
   await engine.update('todo_task', record.id as string, {
     status: 'completed',
-    completed_date: new Date().toISOString(),
   });
 }
 ```
 
+Annotate `ctx` with the **published** `ActionHandlerContext`, never a local copy
+of it: a hand-rolled context interface drifts from the contract the moment either
+side moves, and the drift is silent because the copy is what your handler is
+checked against. (The contract's own docblock asks for `ActionHandler`; that is a
+function type, so it fits an arrow-function handler assigned to a `const`, while
+a function *declaration* like the one above annotates its parameter instead.)
+
+Write only the fields the action owns — and read that literally, because a
+handler's `ctx.engine` runs **elevated** (`isSystem`). The read-only strip that
+protects a server-owned column from an ordinary caller is gated on *not* being
+a system write, so it does not run on a handler's write at all. **What a
+handler names, lands.** A server-owned field is therefore more dangerous here
+than in a form PUT, not less.
+
+`completed_date` is the example. It is `readonly` on `todo_task` — the server
+owns it, and the object's `beforeUpdate` hook stamps it on the completion
+**transition**. Adding `completed_date: new Date().toISOString()` to the write
+above looks harmless, and on that transition it is: the hook overwrites it. But
+a completion write that is *not* a transition — re-completing a task that is
+already `completed` — is not stamped, so nothing overwrites the handler's value
+and nothing strips it either, and the action silently replaces the real
+completion timestamp with "now". That is why the snippet sends `status` alone.
+
 <Callout type="warn">
 **`ctx.engine.find(object, filter)` takes a filter, not a query.** The second
 argument is the `where` half only — `{ status: 'completed' }`, operators
@@ -161,6 +185,24 @@ refuses a primitive or a mistyped `$and` / `$or` / `$not` but still admits
 double must honour it too.
 </Callout>
 
+<Callout type="info">
+**`ctx.engine.delete(object, id)` takes one id — or an array of them.** Both
+spellings are contract: `ActionEngineFacade` in `@objectstack/spec/ui` declares
+the parameter `string | string[]`, so a handler that already holds a list does
+not have to unroll it into a loop to stay on the contract.
+
+```typescript
+await ctx.engine.delete('todo_task', record.id as string);        // one row
+await ctx.engine.delete('todo_task', ['tsk_0001', 'tsk_0002']);   // a set
+```
+
+The array form is a **convenience over the same per-row path** — not a bulk and
+not an atomic delete. The runtime issues one delete per id, in order, with no
+transaction around the set: a failure part-way through leaves the ids before it
+deleted and the ids after it untouched, and the rejection you see is the one that
+stopped it. An empty array deletes nothing and resolves.
+</Callout>
+
 ```typescript title="objectstack.config.ts"
 export const onEnable = async (ctx: { ql: { registerAction: (...args: unknown[]) => void } }) => {
   ctx.ql.registerAction('todo_task', 'completeTask', completeTask);

examples/app-todo/src/actions/task.handlers.ts

@@ -14,22 +14,30 @@
  * ```
  */
 
-// ─── Handler Context (simplified for example purposes) ──────────────
-interface ActionContext {
-  /** The record being acted upon */
-  record: Record<string, unknown>;
-  /** Current authenticated user */
-  user: { id: string; name: string };
-  /** Data engine for CRUD operations */
-  engine: {
-    update(object: string, id: string, data: Record<string, unknown>): Promise<void>;
-    insert(object: string, data: Record<string, unknown>): Promise<{ id: string }>;
-    find(object: string, query: Record<string, unknown>): Promise<Array<Record<string, unknown>>>;
-    delete(object: string, ids: string[]): Promise<void>;
-  };
-  /** Action parameters (from user input / params) */
-  params?: Record<string, unknown>;
-}
+import type { ActionHandlerContext } from '@objectstack/spec/ui';
+
+// ─── Handler Context ─────────────────────────────────────
+//
+// `ActionHandlerContext` (`@objectstack/spec/ui`) is the PUBLISHED contract for
+// what an action body reads as `ctx` — `record`, `params`, `user`, `session` and
+// the trusted `engine` facade.
+//
+// What the contract asks for in its own words is `ActionHandler`. That is a
+// FUNCTION type, and the handlers below are function DECLARATIONS, which cannot
+// carry one; rebinding them as `const cloneTask: ActionHandler = ...` would also
+// erase their return types, because `ActionHandler` returns `unknown`. Annotating
+// the ctx parameter with `ActionHandlerContext` — the type `ActionHandler` is
+// defined in terms of — is the same contract, reached the way this file is
+// written.
+//
+// This file used to declare a local simplified copy of that context instead,
+// and the copy drifted: its `find` still took an ObjectQL-shaped `query` bag
+// long after the contract had settled on a FILTER (#14175). It existed because
+// `ActionEngineFacade.delete` was declared as a single id while the runtime had
+// always accepted an id array too, so `deleteCompletedTasks` below could not be
+// written against the published type at all. The declaration now says
+// `string | string[]` (#15117), so the copy is gone and this example
+// type-checks against exactly the types a real app gets.
 
 /**
  * Mark a single task as complete.
@@ -41,23 +49,23 @@ interface ActionContext {
  * `beforeUpdate` leg of `src/objects/task.hook.ts`, which runs on the
  * transition and whose write the strip lets through.
  */
-export async function completeTask(ctx: ActionContext): Promise<void> {
+export async function completeTask(ctx: ActionHandlerContext): Promise<void> {
   const { record, engine } = ctx;
   await engine.update('todo_task', record.id as string, {
     status: 'completed',
   });
 }
 
 /** Mark a task as in-progress */
-export async function startTask(ctx: ActionContext): Promise<void> {
+export async function startTask(ctx: ActionHandlerContext): Promise<void> {
   const { record, engine } = ctx;
   await engine.update('todo_task', record.id as string, {
     status: 'in_progress',
   });
 }
 
 /** Clone a task (duplicate with reset status) */
-export async function cloneTask(ctx: ActionContext): Promise<{ id: string }> {
+export async function cloneTask(ctx: ActionHandlerContext): Promise<{ id: string }> {
   const { record, engine } = ctx;
   const { id, created_at, updated_at, completed_date, ...fields } = record as Record<string, unknown>;
   return engine.insert('todo_task', {
@@ -68,7 +76,7 @@ export async function cloneTask(ctx: ActionContext): Promise<{ id: string }> {
 }
 
 /** Mark all selected tasks as complete (bulk) — same `status`-only rule as {@link completeTask} (#7036) */
-export async function massCompleteTasks(ctx: ActionContext): Promise<void> {
+export async function massCompleteTasks(ctx: ActionHandlerContext): Promise<void> {
   const { params, engine } = ctx;
   const ids = (params?.selectedIds ?? []) as string[];
   for (const id of ids) {
@@ -79,7 +87,7 @@ export async function massCompleteTasks(ctx: ActionContext): Promise<void> {
 }
 
 /** Delete all completed tasks */
-export async function deleteCompletedTasks(ctx: ActionContext): Promise<void> {
+export async function deleteCompletedTasks(ctx: ActionHandlerContext): Promise<void> {
   const { engine } = ctx;
   const completed = await engine.find('todo_task', { status: 'completed' });
   const ids = completed.map((r) => r.id as string);
@@ -89,7 +97,7 @@ export async function deleteCompletedTasks(ctx: ActionContext): Promise<void> {
 }
 
 /** Defer a task by updating its due date (params collected by the action dialog) */
-export async function deferTask(ctx: ActionContext): Promise<void> {
+export async function deferTask(ctx: ActionHandlerContext): Promise<void> {
   const { record, engine, params } = ctx;
   await engine.update('todo_task', record.id as string, {
     due_date: params?.new_due_date ? String(params.new_due_date) : null,
@@ -99,7 +107,7 @@ export async function deferTask(ctx: ActionContext): Promise<void> {
 }
 
 /** Set a reminder on a task (params collected by the action dialog) */
-export async function setReminder(ctx: ActionContext): Promise<void> {
+export async function setReminder(ctx: ActionHandlerContext): Promise<void> {
   const { record, engine, params } = ctx;
   await engine.update('todo_task', record.id as string, {
     reminder_date: params?.reminder_date ? String(params.reminder_date) : null,
@@ -108,7 +116,7 @@ export async function setReminder(ctx: ActionContext): Promise<void> {
 }
 
 /** Export tasks to CSV format */
-export async function exportTasksToCSV(ctx: ActionContext): Promise<string> {
+export async function exportTasksToCSV(ctx: ActionHandlerContext): Promise<string> {
   const { engine } = ctx;
   const tasks = await engine.find('todo_task', {});
   const header = 'subject,status,priority,category,due_date';

packages/runtime/src/action-execution.ts

@@ -1466,8 +1466,8 @@ export function buildActionEngineFacade(_deps: ActionExecutionDeps, ql: any, ec?
         async update(object: string, id: string, data: Record<string, unknown>): Promise<void> {
             await ql.update(object, data, { where: { id }, context });
         },
-        // Tolerant of both the single-id and array conventions handler suites
-        // use (CRM handlers pass one id; todo handlers pass an id array).
+        // Both spellings are DECLARED contract (#15117), not a tolerance: the
+        // spec's `ActionEngineFacade.delete` takes `string | string[]`.
         async delete(object: string, idOrIds: string | string[]): Promise<void> {
             const ids = Array.isArray(idOrIds) ? idOrIds : [idOrIds];
             for (const id of ids) {

packages/spec/src/ui/action-params.test.ts

@@ -468,3 +468,58 @@ describe('#14175 — ActionEngineFacade.find takes a FILTER (the `where` half),
     expect('where' in envelope && 'where' in nested).toBe(true);
   });
 });
+
+// ---------------------------------------------------------------------------
+// #15117 — `ActionEngineFacade.delete` takes ONE id, or an ARRAY of ids
+// ---------------------------------------------------------------------------
+
+// The declared slot, read off the interface — not a retyped copy of it, so a
+// re-narrowing back to the single-id `string` this card retired fails HERE,
+// rather than in the first handler that hands the facade a list.
+type DeleteIds = Parameters<ActionEngineFacade['delete']>[1];
+
+// The type-level pin (the tsc channel, `tsc -p tsconfig.test.json`). `Eq` is
+// the strict mutual-assignability test, so the pre-#15117 declaration
+// (`string` alone) does not satisfy it — neither does a slot widened all the
+// way to `unknown`. Exported for the same reason the sibling pins are.
+export type DeleteIdsAcceptsOneOrMany = Assert< Eq< DeleteIds, string | string[] > >;
+
+describe('#15117 — ActionEngineFacade.delete accepts one id or an array, both as contract', () => {
+  it('types the second parameter as `string | string[]` (the tsc channel)', () => {
+    // The value-level half of `DeleteIdsAcceptsOneOrMany` above: literals
+    // annotated with the slot type, so the runtime run exercises the same
+    // declaration the type pin reads.
+    const one: DeleteIds = 'tsk_0001';
+    const many: DeleteIds = ['tsk_0001', 'tsk_0002'];
+
+    expect([typeof one, Array.isArray(many)]).toEqual(['string', true]);
+  });
+
+  it('positive control — both handler conventions compile, and so does the empty set', () => {
+    // The single-id convention (the CRM handler suites).
+    const singleId: DeleteIds = 'tsk_0001';
+    // The array convention (`examples/app-todo`'s `deleteCompletedTasks`),
+    // which before this card was reachable only through a hand-rolled copy of
+    // `ActionEngineFacade` — the workaround the widening retired.
+    const idArray: DeleteIds = ['tsk_0001', 'tsk_0002', 'tsk_0003'];
+    // An empty list deletes nothing and resolves; the member doc says so.
+    const empty: DeleteIds = [];
+
+    expect([singleId, idArray, empty].length).toBe(3);
+  });
+
+  it('refuses at compile time what neither convention admits', () => {
+    // Each `@ts-expect-error` is itself checked: if the slot ever ADMITS one
+    // of these, the directive goes unused and `tsc -p tsconfig.test.json` reds.
+    // @ts-expect-error — an id is a string; a number is not an id under either convention.
+    const numericId: DeleteIds = 42;
+    // @ts-expect-error — the array form is an array of ids, not of numbers.
+    const numericIds: DeleteIds = [1, 2];
+    // @ts-expect-error — one level of array, never a nested one.
+    const nestedIds: DeleteIds = [['tsk_0001']];
+    // @ts-expect-error — "delete nothing" is the EMPTY ARRAY, never a null id.
+    const nullId: DeleteIds = null;
+
+    expect([numericId, numericIds, nestedIds, nullId]).toHaveLength(4);
+  });
+});

packages/spec/src/ui/action-params.zod.ts

@@ -231,15 +231,49 @@ export function validateActionParams(
  * context-less, RLS/FLS-bypassing by design (#2849); the boundary is enforced
  * at invoke time (`ai.exposed` + the ADR-0066 D4 capability gate), not here.
  *
- * `find` is the one member whose argument shape the signature alone never
- * settled: it takes a bare FILTER — the `where` half of a query — and never
- * an ObjectQL query envelope; read its doc comment before writing a handler
- * or a test double against it (#14175).
+ * Two members carry an argument contract the signature alone does not settle,
+ * and both state it on the member: `find` takes a bare FILTER — the `where`
+ * half of a query — and never an ObjectQL query envelope (#14175); `delete`
+ * accepts a single id OR an array of them, both as declared contract, served
+ * one row at a time (#15117). Read those doc comments before writing a handler
+ * or a test double against either.
  */
 export interface ActionEngineFacade {
   insert(object: string, data: Record<string, unknown>): Promise<{ id: string }>;
   update(object: string, id: string, data: Record<string, unknown>): Promise<void>;
-  delete(object: string, id: string): Promise<void>;
+  /**
+   * Delete rows of `object` by id.
+   *
+   * ## Which convention is the contract: BOTH — one id, or an array of them
+   *
+   * `idOrIds` takes a SINGLE id or an ARRAY of ids, and both spellings are
+   * contract, not a runtime tolerance a handler author has to discover by
+   * reading `packages/runtime`. Deleting one row is `delete(object, id)`;
+   * deleting a set is `delete(object, ids)` — a handler that already holds a
+   * list does NOT have to unroll it into a loop to stay on the contract.
+   *
+   * What the array form is, exactly: a convenience over the SAME per-row path,
+   * never a bulk or atomic delete. The `delete` arm of
+   * `packages/runtime/src/action-execution.ts#buildActionEngineFacade`
+   * normalises the argument to a list and issues one `ql.delete` per id, in
+   * order, under the caller's execution context. There is no transaction
+   * around the set: a failure part-way through leaves the ids before it
+   * deleted and the ids after it untouched, and the rejection a caller sees is
+   * the one that stopped it. An empty array deletes nothing and resolves.
+   *
+   * Until #15117 this slot declared `id: string` while the runtime had been
+   * accepting both forms all along, described in a comment there as an
+   * accident of the two handler suites that happened to use them. It was
+   * neither: it is this declaration. The one first-party suite on the array
+   * form (`examples/app-todo/src/actions/task.handlers.ts`) could only reach
+   * it by hand-rolling a copy of this interface — the same hand-written-fake
+   * pattern #14175 found on `find`, one member over — and that copy drifted,
+   * exactly as a copy does. Widening the declaration is what retired it.
+   *
+   * Both accepted forms, and the argument types that stay refused, are pinned
+   * in `action-params.test.ts`.
+   */
+  delete(object: string, idOrIds: string | string[]): Promise<void>;
   /**
    * Read the rows of `object` that match `filter`.
    *