From 34206a0629d15bb865e30e4799e315a68c590134 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 05:22:50 +0000 Subject: [PATCH 1/3] test(runtime): drive the nullish-id delete arm before removing its guard (#17620) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The driven reading this card is accepted on, written first and against the UNCHANGED arm so its red is a measurement rather than a claim: an untyped host passing a nullish element into `ActionEngineFacade.delete` is silently skipped today and the call resolves as though the deletion happened. The double opens with the producer's own `assertEngineDeleteDispatch`, so the refusal the next commit makes reachable is `ObjectQL.delete`'s and not a second copy of it, and the pin is the exported message constant compared exactly — a bare `toThrow()` would stay green against any unnamed `Error`. Claude-Session: https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c Co-authored-by: Claude --- .../action-engine-facade-nullish-id.test.ts | 154 ++++++++++++++++++ 1 file changed, 154 insertions(+) create mode 100644 packages/runtime/src/action-engine-facade-nullish-id.test.ts diff --git a/packages/runtime/src/action-engine-facade-nullish-id.test.ts b/packages/runtime/src/action-engine-facade-nullish-id.test.ts new file mode 100644 index 0000000000..f0b6bc0874 --- /dev/null +++ b/packages/runtime/src/action-engine-facade-nullish-id.test.ts @@ -0,0 +1,154 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#17620] `ActionEngineFacade.delete` no longer swallows a NULLISH element. + * + * ## What was here, and who could reach it + * + * The `delete` arm of {@link buildActionEngineFacade} normalises its argument + * to a list and issues one `ql.delete` per id. It used to open that loop with + * `if (id != null)`, so a nullish element was **silently skipped**: nothing + * refused it, nothing warned, and the call resolved as though the deletion had + * happened — a silent no-op on a destructive verb, which is the one failure a + * caller cannot detect. + * + * The declared type is `string | string[]` (#15117), so **no typed caller ever + * reached the guard** — the population is UNTYPED hosts: a JS host, or a + * `registerAction` handler whose slot is still `(ctx: any)`. That is also why + * this file is not a correction of #15117 / PR #17608: that card's contract + * sentence is true, and this arm's declared behaviour is unchanged by the + * removal. + * + * ## Why removing the guard is enough to make it loud + * + * Every id now reaches `ql.delete(object, { where: { id }, context })` as + * written, and the engine's own dispatch predicate answers that call: a + * `where.id` that is not a TRUTHY SCALAR is neither `by-id` nor (absent + * `multi`) a bulk intent, so `ObjectQL.delete` throws + * {@link ENGINE_DELETE_REJECT_MESSAGE}. The refusal is the producer's, not a + * second copy of it — which is why the double below opens with + * {@link assertEngineDeleteDispatch} rather than a hand-rolled id check: a + * double looser than the engine would keep this file green against a facade + * that still swallowed the value. + * + * ⚠️ The pin is the exported MESSAGE CONSTANT, compared exactly. This refusal + * is a plain `Error` — it carries no ADR-0112 `code`/`status` — so a bare + * `toThrow()` here would stay green against any unnamed `Error` at all, which + * is precisely what an unfixed arm would have to produce to be believed. + * + * @see packages/runtime/src/action-execution.ts — `buildActionEngineFacade`. + * @see packages/spec/src/ui/action-params.test.ts — the declaration's own pin, + * whose `@ts-expect-error` reads «"delete nothing" is the EMPTY ARRAY, + * never a null id». + */ + +import { describe, it, expect } from 'vitest'; +import { ENGINE_DELETE_REJECT_MESSAGE, assertEngineDeleteDispatch } from '@objectstack/objectql'; +import { buildActionEngineFacade } from './action-execution.js'; + +const deps: any = { resolveService: () => undefined, getObjectQL: async () => undefined }; + +/** + * An engine double whose `delete` is bound to the REAL engine's dispatch + * contract: one call to the producer's own predicate, never a mirrored `if`. + * Everything it accepts, a running server accepts; everything it refuses, a + * running server refuses (`scripts/check-engine-double-contract.mjs`). + */ +function makeEngine() { + const deleted: Array<{ object: string; id: unknown; context: unknown }> = []; + const ql: any = { + deleted, + async insert(_object: string, data: Record) { + return { id: (data as Record)?.id ?? 'rec_new' }; + }, + async find(_object: string, _options?: Record) { + return []; + }, + async count(_object: string, _options?: Record) { + return 0; + }, + async delete(object: string, options?: Record) { + assertEngineDeleteDispatch(options); + const where = (options as { where?: Record } | undefined)?.where; + deleted.push({ object, id: where?.id, context: (options as { context?: unknown } | undefined)?.context }); + return { ok: true }; + }, + }; + return ql; +} + +/** Drive the arm and hand back whatever it rejected with, or `undefined`. */ +async function rejection(run: Promise): Promise { + return run.then(() => undefined, (e: unknown) => e); +} + +describe('#17620 — ActionEngineFacade.delete refuses a nullish id', () => { + it('refuses a nullish ELEMENT of the array form instead of skipping it', async () => { + const ql = makeEngine(); + const engine = buildActionEngineFacade(deps, ql, { userId: 'u1' }); + + const err = await rejection(engine.delete('crm_case', [null])); + + expect(err).toBeInstanceOf(Error); + expect((err as Error).message).toBe(ENGINE_DELETE_REJECT_MESSAGE); + // …and it is loud INSTEAD of deleting, not as well as: nothing landed. + expect(ql.deleted).toEqual([]); + }); + + it('refuses a nullish SINGLE id (the non-array spelling) the same way', async () => { + const ql = makeEngine(); + const engine = buildActionEngineFacade(deps, ql, { userId: 'u1' }); + + for (const nullish of [null, undefined]) { + const err = await rejection(engine.delete('crm_case', nullish)); + expect(err).toBeInstanceOf(Error); + expect((err as Error).message).toBe(ENGINE_DELETE_REJECT_MESSAGE); + } + expect(ql.deleted).toEqual([]); + }); + + it('stops AT the nullish element — ids before it are deleted, ids after it untouched', async () => { + const ql = makeEngine(); + const engine = buildActionEngineFacade(deps, ql, { userId: 'u1' }); + + const err = await rejection(engine.delete('crm_case', ['case_1', null, 'case_3'])); + + expect((err as Error).message).toBe(ENGINE_DELETE_REJECT_MESSAGE); + // The declared partial-progress shape, unchanged: "a failure part-way + // through leaves the ids before it deleted and the ids after it + // untouched" (`ActionEngineFacade.delete`'s member doc). + expect(ql.deleted.map((d: { id: unknown }) => d.id)).toEqual(['case_1']); + }); +}); + +describe('#17620 — controls: the declared contract is untouched', () => { + it('a well-formed single id still deletes', async () => { + const ql = makeEngine(); + const engine = buildActionEngineFacade(deps, ql, { userId: 'u1', tenantId: 'org_acme' }); + + await expect(engine.delete('crm_case', 'case_1')).resolves.toBeUndefined(); + + expect(ql.deleted).toHaveLength(1); + expect(ql.deleted[0]).toMatchObject({ object: 'crm_case', id: 'case_1' }); + // the elevated caller envelope still rides every call (#3914) + expect(ql.deleted[0].context).toMatchObject({ isSystem: true, userId: 'u1', tenantId: 'org_acme' }); + }); + + it('the declared ARRAY form still deletes every id, in order, one call each', async () => { + const ql = makeEngine(); + const engine = buildActionEngineFacade(deps, ql, { userId: 'u1' }); + + await expect(engine.delete('crm_case', ['case_1', 'case_2', 'case_3'])).resolves.toBeUndefined(); + + expect(ql.deleted.map((d: { id: unknown }) => d.id)).toEqual(['case_1', 'case_2', 'case_3']); + }); + + it('an empty array still deletes nothing and resolves', async () => { + const ql = makeEngine(); + const engine = buildActionEngineFacade(deps, ql, { userId: 'u1' }); + + await expect(engine.delete('crm_case', [])).resolves.toBeUndefined(); + + expect(ql.deleted).toEqual([]); + }); +}); From 54e2fedf5f17cdbbecce7c755d7bd1542f2422bb Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 05:31:24 +0000 Subject: [PATCH 2/3] fix(runtime): ActionEngineFacade.delete refuses a nullish id instead of skipping it (#17620) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `delete` arm's loop opened with `if (id != null)`, so a nullish element was silently dropped and the call resolved as though the row had been deleted — a silent no-op on a destructive verb, which is the failure a caller cannot detect. No typed caller could reach it: the declared slot is `string | string[]` (#15117), which excludes nullish. The population is UNTYPED hosts — a JS host, or a `registerAction` handler whose slot is still `(ctx: any)` — and they are precisely the callers with nothing to tell them the delete did not happen. Removing the guard declares nothing new. Every id now reaches `ql.delete` as written, and the engine's own dispatch predicate refuses a `where.id` that is not a truthy scalar, so the arm answers `ENGINE_DELETE_REJECT_MESSAGE` where it used to answer silence. That is the runtime agreeing with three statements already on the record: the declared type, the member doc, and the `never a null id` pin in `packages/spec/src/ui/action-params.test.ts`. The declared array form is untouched — one `ql.delete` per id, in order, an empty array still resolving — and pinned as a control that can fail. Claude-Session: https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c Co-authored-by: Claude --- .../src/action-engine-facade-nullish-id.test.ts | 2 +- packages/runtime/src/action-execution.ts | 17 ++++++++++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/packages/runtime/src/action-engine-facade-nullish-id.test.ts b/packages/runtime/src/action-engine-facade-nullish-id.test.ts index f0b6bc0874..dce5d2cf8a 100644 --- a/packages/runtime/src/action-engine-facade-nullish-id.test.ts +++ b/packages/runtime/src/action-engine-facade-nullish-id.test.ts @@ -43,7 +43,7 @@ */ import { describe, it, expect } from 'vitest'; -import { ENGINE_DELETE_REJECT_MESSAGE, assertEngineDeleteDispatch } from '@objectstack/objectql'; +import { ENGINE_DELETE_REJECT_MESSAGE, assertEngineDeleteDispatch } from '@objectstack/metadata-core'; import { buildActionEngineFacade } from './action-execution.js'; const deps: any = { resolveService: () => undefined, getObjectQL: async () => undefined }; diff --git a/packages/runtime/src/action-execution.ts b/packages/runtime/src/action-execution.ts index f812c06158..1bd873a79a 100644 --- a/packages/runtime/src/action-execution.ts +++ b/packages/runtime/src/action-execution.ts @@ -1468,10 +1468,25 @@ export function buildActionEngineFacade(_deps: ActionExecutionDeps, ql: any, ec? }, // Both spellings are DECLARED contract (#15117), not a tolerance: the // spec's `ActionEngineFacade.delete` takes `string | string[]`. + // + // [#17620] And there is no third, undeclared one. This loop used to + // open with `if (id != null)`, so a NULLISH element was silently + // skipped and the call resolved as though the deletion had happened — + // a silent no-op on a destructive verb. The declared type excludes + // nullish, so no typed caller ever reached it; the population was + // UNTYPED hosts (a JS host, a `registerAction` handler whose slot is + // still `(ctx: any)`), which is exactly the population that cannot see + // the loss. Every id now goes to `ql.delete` as written, where the + // engine's own dispatch predicate refuses a `where.id` that is not a + // truthy scalar (`ENGINE_DELETE_REJECT_MESSAGE`) — the loud answer the + // declaration already implied. Removing the guard declares nothing new: + // it pulls the runtime back onto the contract that is already on the + // record, here, in the spec member doc, and in the `never a null id` + // pin at `packages/spec/src/ui/action-params.test.ts`. async delete(object: string, idOrIds: string | string[]): Promise { const ids = Array.isArray(idOrIds) ? idOrIds : [idOrIds]; for (const id of ids) { - if (id != null) await ql.delete(object, { where: { id }, context }); + await ql.delete(object, { where: { id }, context }); } }, async find(object: string, query: Record): Promise>> { From 3d2026622a0027209e7ac8e53dbf2899e2230b21 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 05:45:04 +0000 Subject: [PATCH 3/3] chore(changeset,gates): patch changeset and the pinned-double ledger row (#17620) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The changeset is written for the population the removal is observable to — untyped hosts — and says in its own words that it is NOT a correction of the `string | string[]` widening: that declaration is accurate, and no typed caller could reach the skipped branch under it. `engine-double-contract.pinned.json` gains one row for the new test's double, which opens its `delete` with the producer's own `assertEngineDeleteDispatch`. Regenerated with `--write`; the run reports 1 added or grown, 0 lost. Claude-Session: https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c Co-authored-by: Claude --- .../17620-action-engine-delete-nullish-id.md | 39 +++++++++++++++++++ scripts/engine-double-contract.pinned.json | 5 +++ 2 files changed, 44 insertions(+) create mode 100644 .changeset/17620-action-engine-delete-nullish-id.md diff --git a/.changeset/17620-action-engine-delete-nullish-id.md b/.changeset/17620-action-engine-delete-nullish-id.md new file mode 100644 index 0000000000..f02e242349 --- /dev/null +++ b/.changeset/17620-action-engine-delete-nullish-id.md @@ -0,0 +1,39 @@ +--- +'@objectstack/runtime': patch +--- + +`ActionEngineFacade.delete` refuses a nullish id instead of silently skipping it + +**Who this is for: untyped hosts.** A JS host, or a `registerAction` handler +whose context slot is still `(ctx: any)`, can hand `ctx.engine.delete()` a +nullish id — `delete('todo_task', null)`, or an array with a hole in it. Until +now the arm dropped that element on the floor: nothing refused it, nothing +warned, and the call **resolved as though the row had been deleted**. A silent +no-op on a destructive verb is the one failure an untyped caller has no way to +detect, which is why it is worth a line in your changelog rather than a shrug. + +**What changes.** Every id now reaches the engine as written, and the engine's +own delete-dispatch predicate refuses a `where.id` that is not a truthy scalar: +the call rejects with `Delete requires an ID or options.multi=true` where it +used to resolve in silence. In the array form the refusal stops the loop where +the declared member doc already said a failure stops it — ids before the +nullish element are deleted, ids after it are untouched. + +**If a host was leaning on the old behaviour**, filter before you call: + +```js +const ids = candidates.filter((id) => id != null); +if (ids.length > 0) await ctx.engine.delete('todo_task', ids); +// `delete nothing` is the EMPTY ARRAY (it resolves, deleting nothing) — +// never a null id. An empty array is contract; a nullish id never was. +``` + +⛔ **No declaration moves, and this is not a correction of the `string | string[]` +widening that shipped just before it.** That declaration is accurate: it takes a +single id or an array of them, and under it **no typed caller could ever reach +the skipped branch** — the accept set it publishes has never admitted nullish. +The array form, its per-row semantics, its ordering and its empty-array case are +all unchanged and pinned as controls. What moves is only the runtime's +undeclared tolerance for a value three separate statements already excluded: the +published type, the member's own doc comment, and the spec-side pin that reads +«"delete nothing" is the EMPTY ARRAY, never a null id». diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index b70be27081..b888922a21 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -3356,6 +3356,11 @@ "verb": "update", "pinned": 1 }, + { + "file": "packages/runtime/src/action-engine-facade-nullish-id.test.ts", + "verb": "delete", + "pinned": 1 + }, { "file": "packages/runtime/src/action-execution-calldata-batch-retired.test.ts", "verb": "findOne",