diff --git a/.changeset/engine-verb-result-declarations.md b/.changeset/engine-verb-result-declarations.md new file mode 100644 index 0000000000..300876eb9b --- /dev/null +++ b/.changeset/engine-verb-result-declarations.md @@ -0,0 +1,35 @@ +--- +"@objectstack/spec": minor +"@objectstack/objectql": minor +"@objectstack/metadata": minor +"@objectstack/metadata-protocol": minor +"@objectstack/plugin-auth": minor +--- + +feat(engine)!: `findOne`, `update` and `delete` declare what they answer, and their hook seams are guarded (#16231) + + + +**BREAKING** on three published `.d.ts` surfaces. `ObjectQL.findOne`, `ObjectQL.update` and `ObjectQL.delete` — and the `IDataEngine` / `IScopedObjectRepository` contracts they implement — declared `Promise` and now declare the answers they have always given: + +- `findOne` → `Promise | null>` +- `update` → `Promise | number | null>` +- `delete` → `Promise` + +`any` is assignable to everything and admits every property read, so TypeScript consumers of these three methods can stop compiling — most often on the null check the declaration now demands. Shipped as `minor` under the repo's launch-window convention, in which `major` is refused by `check-changeset-no-major` and breaking-ness is carried by this banner plus the ADR-0087 disposition rather than by the level. The governing text is the **WHICH LEVEL** maintainer ruling of 2026-09-04 (decision batch #35, on #15294) recorded at `.github/workflows/pr-automation.yml`; `AGENTS.md`'s "a bug fix in a released package takes a patch changeset — never none" is the floor against `none` and was rejected as the ceiling here, because this PR also widens `@objectstack/objectql`'s index with new exported symbols, which that ruling puts at `minor` on its own. + +**Why.** `engine.ts` has four `return hookContext.result` sites, one per hook-bearing verb. #15823 closed the `find()` one — an `afterFind` handler that replaced the array made a method declared `Promise` resolve to an envelope, silently — and recorded that it could close only that one: the other three declared `Promise` and so carried no declaration a handler could break. A guard cannot exist before a declaration worth guarding does. The maintainer ruled the gap shut (option A, 2026-09-07, director seat summon #17, decision batch #2; option B "declare only, no enforcement" and option C "record `any` as intended" were refused). + +The shapes are read off the driver contract each engine exit delegates to, not invented: `driver.findOne` and the by-id `driver.update` declare `Record | null`, `driver.delete` declares `boolean`, and the predicate exits `driver.updateMany` / `driver.deleteMany` declare the affected-row `number` a bulk write resolves (#4639). Row FIELD values stay erased (`Record`), which is #15823's precedent extended exactly rather than softened: `find()` declares `Promise`, so the CONTAINER is the contract and the rows inside it are `any`. It is also the only spelling that can state "record or null" at all, since `any | null` collapses to `any`. + +**What is enforced now.** Each seam re-checks `hookContext.result` against its declaration immediately after the `after*` dispatch and ahead of the consumers that already assume the shape, and refuses a value outside it with a registered ADR-0112 envelope — `FIND_ONE_HOOK_RESULT_NOT_RECORD`, `UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE`, `DELETE_HOOK_RESULT_NOT_WRITE_SHAPE`, all `500`, all branchable on `error.code`. Shaping stays legal exactly as it does on `find()`: a handler may mutate what it is handed, drop keys, or assign a different value of a declared shape. The falsy answers are legal and deliberately so — `null` from `findOne`, `null` or a count from `update`, and `false` or `0` from `delete`, the two most ordinary answers that verb gives. + +**Who has to change something, on the TYPE axis.** A TypeScript consumer that reads a field off `findOne`'s result without a null check, or off `update`'s result without separating the by-id record from the predicate count. In this repository that was measured before anything moved, at the maintainer's instruction: 18 files and 92 compile errors, all repaired here. + +**What changes at RUNTIME, per door.** TWO things can put an off-declaration value at a seam, and every refusal's `developerMessage` names both: an `after*` handler that assigned one, and a DRIVER whose own exit answered off `IDataDriver`. Each door goes from returning that value silently to refusing it — one door, one registered code, all `500`: + +- `findOne` — FROM: whatever the `afterFind` dispatch left in `ctx.result`, or whatever `driver.findOne` answered off its declared `Promise | null>`, returned to the caller as-is and walked first by `maskSecretFields` / `stripSearchCompanionFromRead`. TO: `500 FIND_ONE_HOOK_RESULT_NOT_RECORD`, raised at the seam when that value is neither a record nor `null`. +- `update` — FROM: whatever the `afterUpdate` dispatch left in the batch `ctx.result`, or whatever `driver.update` / `driver.updateMany` answered off their declared `Promise | null>` / `Promise`, returned as-is and read first by `stripSearchCompanion` and the realtime publish. TO: `500 UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE`, raised when that value is outside record-or-count-or-`null`. +- `delete` — FROM: whatever the `afterDelete` dispatch left in `ctx.result`, or whatever `driver.delete` / `driver.deleteMany` answered off their declared `Promise` / `Promise`, returned as-is to a caller such as `metadata-protocol`'s `deleteData`, which turns `false` into a 404. TO: `500 DELETE_HOOK_RESULT_NOT_WRITE_SHAPE`, raised when that value is neither a boolean nor a number — never on `false` or `0`, which are declared answers. + +The driver half of each line is not hypothetical: the seven off-contract test doubles this PR repairs are exactly that source, and they are why the refusal sentence names the SEAM instead of accusing the handler. diff --git a/content/docs/references/api/contract.mdx b/content/docs/references/api/contract.mdx index 9e24b29b2a..79112e1085 100644 --- a/content/docs/references/api/contract.mdx +++ b/content/docs/references/api/contract.mdx @@ -27,7 +27,7 @@ const result = ApiErrorSchema.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **code** | `Enum<'VALIDATION_ERROR' \| 'INVALID_FIELD' \| 'MISSING_REQUIRED_FIELD' \| 'INVALID_FORMAT' \| 'VALUE_TOO_LONG' \| 'VALUE_TOO_SHORT' \| 'VALUE_OUT_OF_RANGE' \| … +322 more>` | ✅ | Error code (e.g. VALIDATION_ERROR; StandardErrorCode ∪ the ledger the serving side registers — ERROR_CODE_LEDGER for framework packages) | +| **code** | `Enum<'VALIDATION_ERROR' \| 'INVALID_FIELD' \| 'MISSING_REQUIRED_FIELD' \| 'INVALID_FORMAT' \| 'VALUE_TOO_LONG' \| 'VALUE_TOO_SHORT' \| 'VALUE_OUT_OF_RANGE' \| … +325 more>` | ✅ | Error code (e.g. VALIDATION_ERROR; StandardErrorCode ∪ the ledger the serving side registers — ERROR_CODE_LEDGER for framework packages) | | **declaredCode** | `string` | optional | The producer-declared code, verbatim, when it is not a member of the closed `code` vocabulary — the open, author-authored channel (app-specific spellings; ADR-0112) | | **message** | `string` | ✅ | Readable error message | | **userMessage** | `string` | optional | Producer-marked user-facing refusal text, verbatim. Present exactly when the producer opted in at throw time; consumers render it to end users and keep their generic substitution for anything unmarked. Status-agnostic; never replaces `message`. | @@ -134,6 +134,7 @@ const result = ApiErrorSchema.parse(data); * `DATASET_INVALID` * `DATASOURCE_ADMIN_ERROR` * `DELEGABLE_SCOPE_FAILED` +* `DELETE_HOOK_RESULT_NOT_WRITE_SHAPE` * `DELIVERY_NEVER_SENT` * `DELIVERY_NOT_ELIGIBLE` * `DESTRUCTIVE_CHANGE` @@ -185,6 +186,7 @@ const result = ApiErrorSchema.parse(data); * `FILTER_TOKEN_UNKNOWN` * `FILTER_TOKEN_UNRESOLVED` * `FIND_HOOK_RESULT_NOT_ARRAY` +* `FIND_ONE_HOOK_RESULT_NOT_RECORD` * `FLOW_CONVERSION_CONFLICT` * `FLOW_DISABLED` * `FLOW_FAILED` @@ -356,6 +358,7 @@ const result = ApiErrorSchema.parse(data); * `UNSUPPORTED` * `UNSUPPORTED_QUERY_PARAM` * `UNSUPPORTED_TRANSFORM` +* `UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE` * `UPDATE_ID_MISMATCH` * `UPLOAD_SESSION_EXPIRED` * `UPLOAD_SESSION_NOT_FOUND` diff --git a/content/docs/references/api/error-code-ledger.mdx b/content/docs/references/api/error-code-ledger.mdx index b72c1098eb..b079fc5b03 100644 --- a/content/docs/references/api/error-code-ledger.mdx +++ b/content/docs/references/api/error-code-ledger.mdx @@ -293,6 +293,7 @@ const result = ErrorCode.parse(data); * `DATASET_INVALID` * `DATASOURCE_ADMIN_ERROR` * `DELEGABLE_SCOPE_FAILED` +* `DELETE_HOOK_RESULT_NOT_WRITE_SHAPE` * `DELIVERY_NEVER_SENT` * `DELIVERY_NOT_ELIGIBLE` * `DESTRUCTIVE_CHANGE` @@ -344,6 +345,7 @@ const result = ErrorCode.parse(data); * `FILTER_TOKEN_UNKNOWN` * `FILTER_TOKEN_UNRESOLVED` * `FIND_HOOK_RESULT_NOT_ARRAY` +* `FIND_ONE_HOOK_RESULT_NOT_RECORD` * `FLOW_CONVERSION_CONFLICT` * `FLOW_DISABLED` * `FLOW_FAILED` @@ -515,6 +517,7 @@ const result = ErrorCode.parse(data); * `UNSUPPORTED` * `UNSUPPORTED_QUERY_PARAM` * `UNSUPPORTED_TRANSFORM` +* `UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE` * `UPDATE_ID_MISMATCH` * `UPLOAD_SESSION_EXPIRED` * `UPLOAD_SESSION_NOT_FOUND` diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 6824397145..5b194ac438 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -869,6 +869,42 @@ export function zodIssuesToMetadataIssues(issues: unknown): MetadataIssueEntry[] */ export { recordNotFoundError }; +/** + * The RECORD limb of `IDataEngine.update`'s declared result, for the four + * ingresses in this file whose call is BY-ID by construction (#16231). + * + * `update()` used to declare `Promise` and now declares the union its two + * dispatch paths actually answer: a record (or `null`) from the by-id exit + * (`driver.update`), or the affected-row COUNT from the predicate exit + * (`driver.updateMany`, #4639). Every call site below hands the engine a + * `where` naming exactly one primary key — or folds the row's id into the + * payload — with no `multi`, which `resolveEngineUpdateDispatch` resolves + * `by-id`. So the count limb is unreachable from here, and it is REFUSED + * loudly rather than cast away: a dispatch change that started routing these + * calls through `updateMany` would otherwise drop an affected count into a + * per-row receipt's `record` / `data` slot, where every consumer reads it as a + * row. + * + * The `null` limb is NOT refused and NOT narrowed away — it is passed through + * exactly as it was while the declaration said `any`. `update()`'s by-id exit + * answers `null` when the post-write readback leaves the caller's row scope + * (see `updateData`'s own note on that), while `DataProtocol`'s row receipts + * declare `record` / `data` non-null. That disagreement pre-dates this change, + * and this file preserves it rather than widening a shipped response shape as + * a rider; the assertion below is the one place it is written down. + */ +function byIdUpdateRecord(result: Record | number | null): Record { + if (typeof result === 'number') { + throw new Error( + `A by-id update resolved an affected-row count (${result}) instead of a record. ` + + `This ingress addresses exactly one row, so the engine's predicate dispatch ` + + `(driver.updateMany) is not reachable from it — the dispatch ladder or this ` + + `call site changed.`, + ); + } + return result as Record; +} + /** * A 400 for a `$filter` ARRAY that looks like a filter AST but is not one. * @@ -11096,7 +11132,7 @@ export class ObjectStackProtocolImplementation implements ) ? { ...(request.data as Record), id: request.id } : request.data; - const result = await this.engine.update(request.object, writeData, opts); + const result = byIdUpdateRecord(await this.engine.update(request.object, writeData, opts)); // [#7823] The PATCH 200 body is the surface #7728's fourth measurement // caught: a client revoking a `sys_api_key` (apiMethods keeps `update` // open, #7727) got the stored hash back in this response. That closure @@ -12132,7 +12168,7 @@ export class ObjectStackProtocolImplementation implements await this.assertRecordExists(object, record.id); // [#3455] Collect the engine's LEGAL write strips per row. const dropped: DroppedFieldsEvent[] = []; - const updated = await this.engine.update(object, record.data || {}, { where: { id: record.id }, onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); }, ...ctxOpt } as any); + const updated = byIdUpdateRecord(await this.engine.update(object, record.data || {}, { where: { id: record.id }, onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); }, ...ctxOpt } as any)); omitInternalFieldsFromWriteResponse(batchSchema, updated); // [#7823] results.push({ id: record.id, success: true, data: updated, index, ...(dropped.length > 0 ? { droppedFields: dropped } : {}) }); succeeded++; @@ -12164,7 +12200,7 @@ export class ObjectStackProtocolImplementation implements const existing = await this.probeRecord(object, record.id); if (existing) { const dropped: DroppedFieldsEvent[] = []; - const updated = await this.engine.update(object, record.data || {}, { where: { id: record.id }, onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); }, ...ctxOpt } as any); + const updated = byIdUpdateRecord(await this.engine.update(object, record.data || {}, { where: { id: record.id }, onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); }, ...ctxOpt } as any)); omitInternalFieldsFromWriteResponse(batchSchema, updated); // [#7823] results.push({ id: record.id, success: true, data: updated, index, ...(dropped.length > 0 ? { droppedFields: dropped } : {}) }); } else { @@ -12547,7 +12583,7 @@ export class ObjectStackProtocolImplementation implements const dropped: DroppedFieldsEvent[] = []; const opts: any = { where: { id: record.id }, onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); } }; if (context !== undefined) opts.context = context; - const updated = await this.engine.update(object, record.data || {}, opts); + const updated = byIdUpdateRecord(await this.engine.update(object, record.data || {}, opts)); omitInternalFieldsFromWriteResponse(updateManySchema, updated); // [#7823] results.push({ id: record.id, success: true, data: updated, index, ...(dropped.length > 0 ? { droppedFields: dropped } : {}) }); succeeded++; diff --git a/packages/metadata/src/loaders/database-loader.ts b/packages/metadata/src/loaders/database-loader.ts index a907523dab..57ee5739bd 100644 --- a/packages/metadata/src/loaders/database-loader.ts +++ b/packages/metadata/src/loaders/database-loader.ts @@ -430,7 +430,16 @@ export class DatabaseLoader implements MetadataLoader { // nothing to compare against: a `data.id` spread over the id parameter // would retarget the write to a row no caller resolved. The separate // `id` parameter is the row address — do not let a payload outrank it. - return this.engine.update(table, { ...data, id }); + // [#16231] `IDataEngine.update` now declares its dispatch union + // (`record | affected-count | null`). This call passes NO `where`, so + // the payload id is the only address the dispatch ladder sees and it + // resolves `by-id` — the limb that answers a record or `null`. The + // `number` limb is the predicate path (`driver.updateMany`'s affected + // count, #4639), which this call cannot reach; it is narrowed away here + // rather than cast, so a future dispatch change surfaces at THIS line + // instead of as a wrong-shaped row at the caller. + const updated = await this.engine.update(table, { ...data, id }); + return typeof updated === 'number' ? null : updated; } return this.driver!.update(table, id, data); } diff --git a/packages/objectql/src/batch-row-authoring-feedback.test.ts b/packages/objectql/src/batch-row-authoring-feedback.test.ts index 0810e34667..b88a79c237 100644 --- a/packages/objectql/src/batch-row-authoring-feedback.test.ts +++ b/packages/objectql/src/batch-row-authoring-feedback.test.ts @@ -140,7 +140,13 @@ describe('[#8502] a REAL validation refusal keeps its sentence on a batch row', expect(res.results[0].errors[0].message).toContain('Reason'); expect(res.results[0].errors[0].message).not.toContain('The reason is in the server log'); // The stored row is untouched: the refusal happened before the write. - expect((await engine.findOne('bf_leave_request', { where: { id: 'lr1' } })).reason).toBe('ok'); + // [#16231] `findOne` declares record-or-null now, so the read is + // narrowed before the field is asserted — the row's presence IS half of + // what this case measures ("the stored row is untouched"), and reading + // it through `?.` would have let a vanished row pass as `undefined`. + const untouched = await engine.findOne('bf_leave_request', { where: { id: 'lr1' } }); + expect(untouched).not.toBeNull(); + expect(untouched!.reason).toBe('ok'); }); it('the refusal carries no `status`, so a status-only rule WOULD have blanked it', async () => { diff --git a/packages/objectql/src/engine-autonumber-runtime-owned.test.ts b/packages/objectql/src/engine-autonumber-runtime-owned.test.ts index 9dce2cb645..b93e8b6633 100644 --- a/packages/objectql/src/engine-autonumber-runtime-owned.test.ts +++ b/packages/objectql/src/engine-autonumber-runtime-owned.test.ts @@ -35,6 +35,33 @@ import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protoco import { ObjectQL } from './engine.js'; import type { DroppedFieldsEvent } from '@objectstack/spec/data'; +/** + * [#16231] `findOne` and `update` declare what they answer now, so a pin that + * reads a field off either has to say WHICH limb it means. Said once here + * rather than scattered through the cases as `!` and casts: + * + * - every read below is a by-id `findOne`, so `null` means the row this case + * just wrote is gone — a failure of the case, not a shape to tolerate; + * - every write below is a by-id `update` (payload id plus a `where` naming + * the one row), so the affected-row COUNT limb is unreachable and a count + * arriving here would mean the dispatch ladder changed under the pin. + * + * Both refuse loudly, so the case fails where the wrong shape appeared instead + * of several lines later on an `undefined` field. + */ +function readRow(row: Record | null): Record { + if (row === null) throw new Error('expected findOne to answer the row this case wrote'); + return row; +} + +function writtenRow(result: Record | number | null): Record { + if (result === null) throw new Error('expected a by-id update to answer the written record, got null'); + if (typeof result === 'number') { + throw new Error(`expected a by-id update to answer the written record, got an affected count (${result})`); + } + return result; +} + const ACCOUNT = { name: 'an_account', label: 'Account', @@ -395,7 +422,7 @@ describe('#5503 x #5126 — strictReadonlyWrites covers runtime-owned fields', ( expect(err.fields).toEqual(['account_number']); expect([...err.drops].map((d: DroppedFieldsEvent) => d.reason)).toEqual(['readonly']); // Nothing was written — not even the legitimate rename. - const readback = await rig.engine.findOne('an_account', { where: { id } }); + const readback = readRow(await rig.engine.findOne('an_account', { where: { id } })); expect(readback.name).toBe('Acme'); expect(readback.account_number).toBe('ACC-0001'); }); @@ -413,7 +440,7 @@ describe('#5503 x #5126 — strictReadonlyWrites covers runtime-owned fields', ( ); expect(events.flatMap((e) => e.fields)).toContain('account_number'); - const readback = await rig.engine.findOne('an_account', { where: { id } }); + const readback = readRow(await rig.engine.findOne('an_account', { where: { id } })); expect(readback.name).toBe('renamed'); // the legitimate half landed expect(readback.account_number).toBe('ACC-0001'); // the forged half did not }); @@ -422,11 +449,11 @@ describe('#5503 x #5126 — strictReadonlyWrites covers runtime-owned fields', ( const rig = await makeEngine(); const created = await rig.protocol.createData({ object: 'an_account', data: { name: 'Acme' } }); const id = created.id as string; - const res = await rig.engine.update( + const res = writtenRow(await rig.engine.update( 'an_account', { id, account_number: 'LEGACY-0007' }, { where: { id }, strictReadonlyWrites: true, context: { preserveAudit: true } }, - ); + )); expect(res.account_number).toBe('LEGACY-0007'); }); @@ -467,7 +494,7 @@ describe('#5503 — autonumber is runtime-owned: UPDATE', () => { data: { account_number: 'ACC-888888' }, }); expect(res.record.account_number).toBe('ACC-0001'); - const readback = await rig.engine.findOne('an_account', { where: { id } }); + const readback = readRow(await rig.engine.findOne('an_account', { where: { id } })); expect(readback.account_number).toBe('ACC-0001'); }); @@ -512,20 +539,20 @@ describe('#5503 — autonumber is runtime-owned: UPDATE', () => { }); it('keeps an explicit value for a system write', async () => { - const res = await rig.engine.update( + const res = writtenRow(await rig.engine.update( 'an_account', { id, account_number: 'ACC-000042' }, { where: { id }, context: { isSystem: true } } as any, - ); + )); expect(res.account_number).toBe('ACC-000042'); }); it('keeps an explicit value for a `preserveAudit` historical import / undo (#3493)', async () => { - const res = await rig.engine.update( + const res = writtenRow(await rig.engine.update( 'an_account', { id, account_number: 'LEGACY-0007' }, { where: { id }, context: { preserveAudit: true } } as any, - ); + )); expect(res.account_number).toBe('LEGACY-0007'); }); @@ -533,7 +560,7 @@ describe('#5503 — autonumber is runtime-owned: UPDATE', () => { rig.engine.registerHook('beforeUpdate', async (ctx: any) => { ctx.input.data.account_number = 'HOOK-0002'; }, { object: 'an_account' }); - const res = await rig.engine.update('an_account', { id, name: 'x' }, { where: { id } } as any); + const res = writtenRow(await rig.engine.update('an_account', { id, name: 'x' }, { where: { id } } as any)); expect(res.account_number).toBe('HOOK-0002'); }); }); diff --git a/packages/objectql/src/engine-filter-alias.test.ts b/packages/objectql/src/engine-filter-alias.test.ts index d0dbcd448e..4d1846d9c4 100644 --- a/packages/objectql/src/engine-filter-alias.test.ts +++ b/packages/objectql/src/engine-filter-alias.test.ts @@ -119,8 +119,13 @@ describe('filter → where folds on every engine method (#4346)', () => { it('findOne({filter}) returns a MATCHING row, not the first row of the table', async () => { const row = await engine.findOne('task', { filter: { status: 'done' } } as any); - expect(row.status).toBe('done'); - expect(row.id).not.toBe(a.id); + // [#16231] The `null` is part of the declaration now, and it is part of + // what this case measures: an unfolded `filter` used to match ALL rows, + // so "a row came back at all" and "it is the MATCHING one" are two + // different assertions and both have to be made. + expect(row).not.toBeNull(); + expect(row!.status).toBe('done'); + expect(row!.id).not.toBe(a.id); }); it('count({filter}) counts the matching rows, not the whole table', async () => { @@ -201,7 +206,8 @@ describe('filter → where folds on every engine method (#4346)', () => { it('findOne({top}) stays single-row — the forced limit: 1 wins over the folded alias', async () => { const row = await engine.findOne('task', { top: 5, filter: { status: 'done' } } as any); - expect(row.status).toBe('done'); + expect(row).not.toBeNull(); + expect(row!.status).toBe('done'); }); // ── the documented hook call path (ScopedContext / ObjectRepository) ─ diff --git a/packages/objectql/src/engine-filter-tokens.test.ts b/packages/objectql/src/engine-filter-tokens.test.ts index f6d5480766..98f33ec09b 100644 --- a/packages/objectql/src/engine-filter-tokens.test.ts +++ b/packages/objectql/src/engine-filter-tokens.test.ts @@ -61,7 +61,11 @@ function makeDriver() { update: vi.fn(async (_o: string, id: any, d: any) => { seen.updateId = id; return { id, ...d }; }), updateMany: vi.fn(async (_o: string, ast: any) => { seen.updateManyAst = ast; return { modified: 0 }; }), delete: vi.fn(async (_o: string, id: any) => { seen.deleteId = id; return true; }), - deleteMany: vi.fn(async (_o: string, ast: any) => { seen.deleteManyAst = ast; return { deleted: 0 }; }), + // [#16231] `IDataDriver.deleteMany` declares `Promise` — the + // affected-row count. This double answered an invented `{ deleted: n }` + // envelope that no driver produces; `Promise` on the engine door + // admitted it all the way out to the caller. + deleteMany: vi.fn(async (_o: string, ast: any) => { seen.deleteManyAst = ast; return 0; }), }; return { driver, seen }; } diff --git a/packages/objectql/src/engine-verb-hook-result-shape.test.ts b/packages/objectql/src/engine-verb-hook-result-shape.test.ts new file mode 100644 index 0000000000..b0a1d1baab --- /dev/null +++ b/packages/objectql/src/engine-verb-hook-result-shape.test.ts @@ -0,0 +1,426 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #16231 — `findOne`, `update` and `delete` DECLARE what they answer, and the +// `after*` seam that could break each declaration is closed. +// +// ## What this suite is for +// +// `engine.ts` has four `return hookContext.result` sites. #15823 closed the +// `find()` one and recorded why it could close only that one: `find` declared +// `Promise`, a concrete container to violate, while these three declared +// `Promise` and so carried nothing an `after*` handler could break. +// +// The maintainer ruled that gap shut (option A, 2026-09-07, director seat +// summon #17, decision batch #2; options B "declare only" and C "record `any` +// as intended" were refused). The declarations moved onto what each verb +// actually answers — read off the driver contract each exit delegates to, not +// invented — and each seam is guarded as `find()`'s is: +// +// findOne → `Record | null` (driver.findOne) +// update → `Record | number | null` (driver.update | driver.updateMany) +// delete → `boolean | number` (driver.delete | driver.updateMany's twin) +// +// ## The three things this suite has to hold apart +// +// (a) the declared limbs are ANSWERABLE — including the ones a lenient guard +// would eat: `null` from `findOne`, `null` and a count from `update`, +// and — the load-bearing pair — `false` and `0` from `delete`; +// (b) SHAPING STAYS LEGAL — a handler may mutate what it is handed, or assign +// a different value of a declared shape. This is the half that keeps the +// refusal from being a behaviour regression, and it is written against +// the SHAPE and never against identity; +// (c) a value outside the declaration is REFUSED, with the registered +// ADR-0112 envelope — asserted by `code` AND `status`, never a bare +// `toThrow()`: an unfixed engine throws nothing at all here, so a bare +// `toThrow()` would be satisfied by any unrelated failure. +// +// ⚠️ (a) is where this suite differs most from its `find()` elder. There, every +// non-array was refusable and `[]` was the only "nothing" answer. Here each +// verb has MORE than one legal answer and two of them are falsy, so a guard +// written as a truthiness check passes `find()`'s suite and destroys this one. + +import { describe, it, expect } from 'vitest'; +// [check:test-source-alias] Module-top, not `await import(...)` inside a case: +// objectql resolves this specifier through `dist/`, so a first load paid inside +// a test body transforms that whole module graph while `testTimeout` runs. +import { ErrorCode } from '@objectstack/spec/api'; +import { ObjectQL } from './engine.js'; +import { + FIND_ONE_HOOK_RESULT_NOT_RECORD_CODE, + FIND_ONE_HOOK_RESULT_NOT_RECORD_STATUS, + FindOneHookResultNotRecordError, + UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE_CODE, + UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE_STATUS, + UpdateHookResultNotWriteShapeError, + DELETE_HOOK_RESULT_NOT_WRITE_SHAPE_CODE, + DELETE_HOOK_RESULT_NOT_WRITE_SHAPE_STATUS, + DeleteHookResultNotWriteShapeError, +} from './verb-hook-result-shape.js'; + +function silentLogger() { + const logger: any = { + trace() {}, debug() {}, info() {}, warn() {}, error() {}, fatal() {}, + child() { return logger; }, + }; + return logger; +} + +const ROW = { id: 't1', name: 'first', done: false }; + +/** + * A driver that answers each exit the way `IDataDriver` declares it, so the + * engine's own limbs are exercised rather than simulated: + * `findOne` → record-or-null, `update` → record-or-null, `updateMany` → count, + * `delete` → boolean, `deleteMany` → count. + * + * `missing` flips the reads to their empty answer, which is how the `null` and + * `false` limbs below are reached through the REAL engine path rather than by + * a handler assigning them. + */ +function makeDriver(opts: { missing?: boolean } = {}) { + const driver: any = { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find() { return opts.missing ? [] : [{ ...ROW }]; }, + async findOne() { return opts.missing ? null : { ...ROW }; }, + async create(_o: string, data: any) { return data; }, + async update(_o: string, id: string, data: any) { + return opts.missing ? null : { ...ROW, ...data, id }; + }, + async updateMany() { return 2; }, + async delete() { return !opts.missing; }, + async deleteMany() { return 2; }, + async count() { return opts.missing ? 0 : 1; }, + async bulkCreate(_o: string, rows: any[]) { return rows; }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + }; + return driver; +} + +async function makeEngine(opts: { missing?: boolean } = {}) { + const engine = new ObjectQL({ logger: silentLogger() }); + engine.registerDriver(makeDriver(opts), true); + await engine.init(); + engine.registry.registerObject({ + name: 'task', + fields: { + id: { name: 'id', type: 'text', primaryKey: true, readonly: true }, + name: { name: 'name', type: 'text' }, + done: { name: 'done', type: 'boolean' }, + }, + } as any, 'test'); + return engine; +} + +/** Run and return whatever came out — a value or the thrown error. */ +async function outcomeOf(run: () => Promise): Promise<{ value?: unknown; error?: any }> { + try { + return { value: await run() }; + } catch (error) { + return { error }; + } +} + +// --------------------------------------------------------------------------- +// findOne +// --------------------------------------------------------------------------- + +describe('#16231 findOne — the declared limbs are answerable', () => { + it('answers the record on the no-hook path', async () => { + const engine = await makeEngine(); + const row: any = await engine.findOne('task', { where: { id: 't1' } }); + expect(row).not.toBeNull(); + expect(row.id).toBe('t1'); + }); + + it('answers `null` when the query selects nothing — through the ENGINE, not a handler', async () => { + // The limb the declaration exists to write down, reached the way a caller + // reaches it. A guard that refused nullish would break this. + const engine = await makeEngine({ missing: true }); + expect(await engine.findOne('task', { where: { id: 'nope' } })).toBeNull(); + }); +}); + +describe('#16231 findOne — shaping stays legal', () => { + it('an afterFind that mutates the record IN PLACE still answers the record', async () => { + const engine = await makeEngine(); + engine.registerHook('afterFind', (ctx: any) => { delete ctx.result.name; }, { object: 'task' } as any); + + const row: any = await engine.findOne('task', { where: { id: 't1' } }); + expect(row.id).toBe('t1'); + expect('name' in row).toBe(false); + }); + + it('an afterFind that assigns a DIFFERENT record still answers the record', async () => { + // The identity half of the predicate: the container object is replaced and + // that is legal, because what replaced it is still a record. + const engine = await makeEngine(); + engine.registerHook('afterFind', (ctx: any) => { ctx.result = { id: ctx.result.id }; }, { object: 'task' } as any); + + expect(await engine.findOne('task', { where: { id: 't1' } })).toEqual({ id: 't1' }); + }); + + it('an afterFind that assigns `null` is LEGAL — it is a declared limb', async () => { + // ⚠️ The sharpest difference from `find()`'s guard, where nullish is + // refused because `[]` is the only way to answer "nothing". Here `null` IS + // the way to answer "no record", so refusing it would refuse the + // documented spelling. + const engine = await makeEngine(); + engine.registerHook('afterFind', (ctx: any) => { ctx.result = null; }, { object: 'task' } as any); + + expect(await engine.findOne('task', { where: { id: 't1' } })).toBeNull(); + }); +}); + +describe('#16231 findOne — a value outside the declaration is refused', () => { + it('an envelope is refused with FIND_ONE_HOOK_RESULT_NOT_RECORD', async () => { + const engine = await makeEngine(); + engine.registerHook('afterFind', (ctx: any) => { + ctx.result = [{ id: 'ENVELOPE' }]; + }, { object: 'task' } as any); + + const { value, error } = await outcomeOf(() => engine.findOne('task', { where: { id: 't1' } })); + + expect(value, 'findOne() answered instead of refusing').toBeUndefined(); + expect(error, 'findOne() did not refuse').toBeInstanceOf(FindOneHookResultNotRecordError); + // ADR-0112 envelope: the code AND the status, never a bare `toThrow()`. + expect(error.code).toBe(FIND_ONE_HOOK_RESULT_NOT_RECORD_CODE); + expect(error.code).toBe('FIND_ONE_HOOK_RESULT_NOT_RECORD'); + expect(error.status).toBe(FIND_ONE_HOOK_RESULT_NOT_RECORD_STATUS); + expect(error.status).toBe(500); + expect(error.event).toBe('afterFind'); + expect(error.object).toBe('task'); + expect(error.observed).toBe('array'); + // The remedy half is addressed to the handler's author and names both + // supported spellings — `null` for no record, a throw to refuse the read. + expect(error.developerMessage).toContain("assign 'null'"); + expect(error.developerMessage).toContain('throw from the handler'); + // ⚠️ …and it names BOTH sources, because the seam sees a driver's answer as + // well as a handler's. The user-facing sentence therefore states what is + // AT the seam and accuses nobody: a `driver.update` double resolving + // `undefined` is the measured case this wording exists for, and a sentence + // reading "your handler replaced it" would have sent four repairs in this + // repository to the wrong file. + expect(error.developerMessage).toContain('off its own contract'); + expect(error.message).toContain("after the 'afterFind' dispatch"); + expect(error.message).not.toContain('handler replaced'); + }); + + it('`undefined` is refused — decided here, and it is NOT the same as `null`', async () => { + const engine = await makeEngine(); + engine.registerHook('afterFind', (ctx: any) => { ctx.result = undefined; }, { object: 'task' } as any); + + const { error } = await outcomeOf(() => engine.findOne('task', { where: { id: 't1' } })); + expect(error?.code).toBe(FIND_ONE_HOOK_RESULT_NOT_RECORD_CODE); + expect(error.observed).toBe('undefined'); + }); + + it('a string is refused — the predicate is a shape test, not `typeof`', async () => { + const engine = await makeEngine(); + engine.registerHook('afterFind', (ctx: any) => { ctx.result = 'row'; }, { object: 'task' } as any); + + const { error } = await outcomeOf(() => engine.findOne('task', { where: { id: 't1' } })); + expect(error?.code).toBe(FIND_ONE_HOOK_RESULT_NOT_RECORD_CODE); + expect(error.observed).toBe('string'); + }); + + it('the refusal fires BEFORE maskSecretFields / stripSearchCompanionFromRead see it', async () => { + // Driven, not asserted about source, exactly as #15823 drives the same + // placement claim on `find()`: both consumers run on `hookContext.result` + // between the dispatch and the return, so a replaced value they walked + // first would be diagnosed from the wrong place. + const engine = await makeEngine(); + let poisonRead = 0; + engine.registerHook('afterFind', (ctx: any) => { + ctx.result = new Proxy([], { + get(target, prop, recv) { poisonRead += 1; return Reflect.get(target, prop, recv); }, + }); + }, { object: 'task' } as any); + + const { error } = await outcomeOf(() => engine.findOne('task', { where: { id: 't1' } })); + expect(error?.code).toBe(FIND_ONE_HOOK_RESULT_NOT_RECORD_CODE); + expect(poisonRead, 'a consumer walked the replaced value before the refusal').toBe(0); + }); +}); + +// --------------------------------------------------------------------------- +// update +// --------------------------------------------------------------------------- + +describe('#16231 update — the declared limbs are answerable', () => { + it('a BY-ID write answers the record', async () => { + const engine = await makeEngine(); + const out: any = await engine.update('task', { id: 't1', name: 'renamed' }); + expect(typeof out).toBe('object'); + expect(out.name).toBe('renamed'); + }); + + it('a PREDICATE write answers the affected-row COUNT, and the guard admits it', async () => { + // #4639: a predicate write names no row. This is the limb that made + // `update`'s declaration a UNION rather than a record type, and the + // measurement that a record-only guard would have refused. + const engine = await makeEngine(); + const out = await engine.update('task', { done: true }, { multi: true, where: { done: false } } as any); + expect(out).toBe(2); + }); +}); + +describe('#16231 update — shaping stays legal', () => { + it('an afterUpdate that mutates the record IN PLACE still answers the record', async () => { + const engine = await makeEngine(); + engine.registerHook('afterUpdate', (ctx: any) => { ctx.result.stamped = true; }, { object: 'task' } as any); + + const out: any = await engine.update('task', { id: 't1', name: 'renamed' }); + expect(out.stamped).toBe(true); + }); + + it('an afterUpdate may assign a different RECORD, `null`, or a count', async () => { + for (const [label, assigned, expected] of [ + ['a different record', { id: 't1' }, { id: 't1' }], + ['null', null, null], + ['a count', 7, 7], + ['a zero count', 0, 0], + ] as const) { + const engine = await makeEngine(); + engine.registerHook('afterUpdate', (ctx: any) => { ctx.result = assigned; }, { object: 'task' } as any); + expect(await engine.update('task', { id: 't1', name: 'x' }), label).toEqual(expected); + } + }); +}); + +describe('#16231 update — a value outside the declaration is refused', () => { + for (const [label, assigned, observed] of [ + ['a string', 'done', 'string'], + ['an array', [{ id: 't1' }], 'array'], + ['a boolean', true, 'boolean'], + ['undefined', undefined, 'undefined'], + ] as const) { + it(`${label} is refused with UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE`, async () => { + const engine = await makeEngine(); + engine.registerHook('afterUpdate', (ctx: any) => { ctx.result = assigned; }, { object: 'task' } as any); + + const { value, error } = await outcomeOf(() => engine.update('task', { id: 't1', name: 'x' })); + expect(value, 'update() answered instead of refusing').toBeUndefined(); + expect(error, 'update() did not refuse').toBeInstanceOf(UpdateHookResultNotWriteShapeError); + expect(error.code).toBe(UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE_CODE); + expect(error.code).toBe('UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE'); + expect(error.status).toBe(UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE_STATUS); + expect(error.status).toBe(500); + expect(error.event).toBe('afterUpdate'); + expect(error.object).toBe('task'); + expect(error.observed).toBe(observed); + }); + } + + it("the message does NOT begin with a SQL verb — `sanitizeRowError` would blank it", async () => { + // ⛔ Load-bearing here in a way it was not on `find()`: this refusal is + // ABOUT `update`, so the obvious first word is the one `@objectstack/rest`'s + // importer replaces with generic text. The constraint is pinned rather than + // trusted to a comment. + const engine = await makeEngine(); + engine.registerHook('afterUpdate', (ctx: any) => { ctx.result = 'done'; }, { object: 'task' } as any); + + const { error } = await outcomeOf(() => engine.update('task', { id: 't1', name: 'x' })); + expect(error.message.startsWith('Refusing')).toBe(true); + for (const verb of ['insert', 'update', 'delete']) { + expect(error.message.toLowerCase().startsWith(verb), `message starts with '${verb}'`).toBe(false); + } + }); +}); + +// --------------------------------------------------------------------------- +// delete +// --------------------------------------------------------------------------- + +describe('#16231 delete — the declared limbs are answerable, INCLUDING the falsy ones', () => { + it('a BY-ID delete answers `true` when the row was there', async () => { + const engine = await makeEngine(); + expect(await engine.delete('task', { where: { id: 't1' } })).toBe(true); + }); + + it('a PREDICATE delete answers the affected-row COUNT', async () => { + const engine = await makeEngine(); + expect(await engine.delete('task', { multi: true, where: { done: false } } as any)).toBe(2); + }); + + it('an afterDelete may assign `false` or `0` — a truthiness guard would refuse both', async () => { + // ⭐ The case that separates this guard from a lenient one. `false` is what + // `@objectstack/metadata-protocol`'s `deleteData` turns into its 404, and + // `0` is "the predicate matched nothing". Both are ordinary answers. + for (const falsy of [false, 0] as const) { + const engine = await makeEngine(); + engine.registerHook('afterDelete', (ctx: any) => { ctx.result = falsy; }, { object: 'task' } as any); + expect(await engine.delete('task', { where: { id: 't1' } }), String(falsy)).toBe(falsy); + } + }); +}); + +describe('#16231 delete — a value outside the declaration is refused', () => { + for (const [label, assigned, observed] of [ + ['a record', { deleted: 1 }, 'object'], + ['an array', [], 'array'], + ['null', null, 'null'], + ['undefined', undefined, 'undefined'], + ['a string', 'ok', 'string'], + ] as const) { + it(`${label} is refused with DELETE_HOOK_RESULT_NOT_WRITE_SHAPE`, async () => { + const engine = await makeEngine(); + engine.registerHook('afterDelete', (ctx: any) => { ctx.result = assigned; }, { object: 'task' } as any); + + const { value, error } = await outcomeOf(() => engine.delete('task', { where: { id: 't1' } })); + expect(value, 'delete() answered instead of refusing').toBeUndefined(); + expect(error, 'delete() did not refuse').toBeInstanceOf(DeleteHookResultNotWriteShapeError); + expect(error.code).toBe(DELETE_HOOK_RESULT_NOT_WRITE_SHAPE_CODE); + expect(error.code).toBe('DELETE_HOOK_RESULT_NOT_WRITE_SHAPE'); + expect(error.status).toBe(DELETE_HOOK_RESULT_NOT_WRITE_SHAPE_STATUS); + expect(error.status).toBe(500); + expect(error.event).toBe('afterDelete'); + expect(error.object).toBe('task'); + expect(error.observed).toBe(observed); + }); + } + + it('`{ deleted: 1 }` — the invented envelope two test doubles carried — is refused', async () => { + // Not hypothetical: `packages/spec/src/contracts/data-engine.test.ts` and + // `packages/runtime/src/seed-loader.test.ts` both modelled `delete` as + // answering this shape, which no driver and no engine has ever produced. + // `Promise` admitted it. Both were repaired with this card, and this + // case is what stops the shape coming back. + const engine = await makeEngine(); + engine.registerHook('afterDelete', (ctx: any) => { ctx.result = { deleted: 1 }; }, { object: 'task' } as any); + + const { error } = await outcomeOf(() => engine.delete('task', { where: { id: 't1' } })); + expect(error?.code).toBe(DELETE_HOOK_RESULT_NOT_WRITE_SHAPE_CODE); + }); +}); + +// --------------------------------------------------------------------------- +// The vocabulary +// --------------------------------------------------------------------------- + +describe('#16231 — all three refusals are registered ADR-0112 vocabulary', () => { + it('each code is a member of the generated ErrorCode union', () => { + for (const code of [ + FIND_ONE_HOOK_RESULT_NOT_RECORD_CODE, + UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE_CODE, + DELETE_HOOK_RESULT_NOT_WRITE_SHAPE_CODE, + ]) { + expect(ErrorCode.safeParse(code).success, code).toBe(true); + } + // Control: the union really does reject an unregistered spelling, so the + // assertions above are a reading rather than a schema that accepts + // anything. + expect(ErrorCode.safeParse('FIND_ONE_HOOK_RESULT_NOT_A_RECORD').success).toBe(false); + }); + + it('the three codes are distinct — one per declaration, not one shared refusal', () => { + const codes = new Set([ + FIND_ONE_HOOK_RESULT_NOT_RECORD_CODE, + UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE_CODE, + DELETE_HOOK_RESULT_NOT_WRITE_SHAPE_CODE, + ]); + expect(codes.size).toBe(3); + }); +}); diff --git a/packages/objectql/src/engine.test.ts b/packages/objectql/src/engine.test.ts index fba0c3f585..abe62bb642 100644 --- a/packages/objectql/src/engine.test.ts +++ b/packages/objectql/src/engine.test.ts @@ -83,8 +83,14 @@ describe('ObjectQL Engine', () => { find: vi.fn().mockResolvedValue([{ id: '1', name: 'Test Record' }]), findOne: vi.fn(), create: vi.fn().mockResolvedValue({ id: '1', success: true }), - update: vi.fn(), - delete: vi.fn(), + // [#16231] `IDataDriver.update` declares `Promise | null>` + // and `IDataDriver.delete` declares `Promise`. A bare `vi.fn()` + // resolves `undefined`, which is neither — and the engine used to hand that + // straight out under `Promise`. The engine's seam guard refuses it now, + // so the doubles answer the shapes their own contract declares. Individual + // cases still override these with `mockResolvedValue`. + update: vi.fn().mockResolvedValue({ id: '1' }), + delete: vi.fn().mockResolvedValue(true), count: vi.fn(), capabilities: {} as any // Simplified } as unknown as IDataDriver; @@ -96,8 +102,8 @@ describe('ObjectQL Engine', () => { find: vi.fn().mockResolvedValue([{ id: '2', name: 'Mongo Record' }]), findOne: vi.fn(), create: vi.fn().mockResolvedValue({ id: '2', success: true }), - update: vi.fn(), - delete: vi.fn(), + update: vi.fn().mockResolvedValue({ id: '2' }), + delete: vi.fn().mockResolvedValue(true), count: vi.fn(), capabilities: {} as any } as unknown as IDataDriver; @@ -2398,7 +2404,8 @@ describe('ObjectQL Engine', () => { expand: { assignee: { object: 'assignee' } }, }); - expect(result.assignee).toEqual({ id: 'u1', name: 'Alice' }); + expect(result).not.toBeNull(); + expect(result!.assignee).toEqual({ id: 'u1', name: 'Alice' }); }); it('should handle already-expanded objects (skip re-expansion)', async () => { diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 1cead9fce9..d2d5f8251a 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -93,6 +93,14 @@ import { import { ReadonlyFieldRejectedError } from './readonly-strict-errors.js'; import { HookTargetRebindError } from './hook-target-rebind-errors.js'; import { FindHookResultNotArrayError } from './find-hook-result-shape.js'; +import { + isFindOneResultShape, + isUpdateResultShape, + isDeleteResultShape, + FindOneHookResultNotRecordError, + UpdateHookResultNotWriteShapeError, + DeleteHookResultNotWriteShapeError, +} from './verb-hook-result-shape.js'; import { DriverConnectError, DatasourceUnavailableError, @@ -9702,7 +9710,7 @@ export class ObjectQL implements IObjectQLEngine { * * Fires the same `beforeFind`/`afterFind` hooks as `find` (#3195). */ - async findOne(objectName: string, query?: EngineQueryOptions, options?: EngineReadOptions): Promise { + async findOne(objectName: string, query?: EngineQueryOptions, options?: EngineReadOptions): Promise | null> { objectName = this.resolveObjectName(objectName); // Same alias fold as find() (#4346). Without it, `findOne({ filter })` // matched the first row of the WHOLE table rather than the predicate. @@ -9808,6 +9816,27 @@ export class ObjectQL implements IObjectQLEngine { hookContext.result = result; await this.triggerHooks('afterFind', hookContext); + // [#16231] `findOne()` now DECLARES what it answers — the one record the + // query selects, or `null` — so the seam that can break the declaration + // is closed here, on the terms #15823 set for `find()`. An `afterFind` + // handler may SHAPE the record (mutate it, drop keys, assign a different + // RECORD built from it); replacing it with something that is neither a + // record nor `null` is a hook-contract violation, refused loudly rather + // than returned as a value the caller's type says cannot occur. + // + // ⛔ Placement is load-bearing and not the `return`, exactly as on + // `find()`: `maskSecretFields` and `stripSearchCompanionFromRead` below + // both run on `hookContext.result`, so the check has to precede them — + // otherwise the first consumer to walk a replaced value is the one that + // reports the problem, from the wrong place. + if (!isFindOneResultShape(hookContext.result)) { + throw new FindOneHookResultNotRecordError({ + object: objectName, + event: 'afterFind', + result: hookContext.result, + }); + } + // Mask secret fields — plaintext never leaves through the read path. this.maskSecretFields(objectName, hookContext.result); // [#7642] Hidden `__search` companion — same door, same rule as `find`. @@ -10868,7 +10897,7 @@ export class ObjectQL implements IObjectQLEngine { * `catch` also sees the `afterUpdate` dispatch and the roll-up recompute, and * a violation raised by a nested driver call in there is not this object's. */ - async update(object: string, data: any, options?: EngineUpdateOptions & WriteObservabilityOptions): Promise { + async update(object: string, data: any, options?: EngineUpdateOptions & WriteObservabilityOptions): Promise | number | null> { object = this.resolveObjectName(object); this.logger.debug('Update operation starting', { object }); this.assertWriteAllowed(object, 'update'); @@ -12073,6 +12102,28 @@ export class ObjectQL implements IObjectQLEngine { await this.triggerHooks('afterUpdate', hookContext); } + // [#16231] The `update()` twin of `find()`'s #15823 refusal, now + // that this verb declares the union its two dispatch paths answer: + // a post-write record (or `null`), or the affected-row COUNT a + // predicate write resolves. An `afterUpdate` handler may SHAPE what + // it is handed; replacing it with a shape outside the declaration + // is refused. + // + // ⛔ Ahead of `stripSearchCompanion` and the realtime publish for + // the same reason `find()`'s guard precedes its consumers: both + // read this value already assuming it is a record or a non-object + // they may skip, so a replaced container would be diagnosed from + // whichever of them tripped over it first. AFTER the per-row + // `afterUpdate` fan-out (#5038), because the batch context is the + // one this call returns and a per-row handler can reassign it. + if (!isUpdateResultShape(hookContext.result)) { + throw new UpdateHookResultNotWriteShapeError({ + object, + event: 'afterUpdate', + result: hookContext.result, + }); + } + // Roll-up: recompute parent summaries; pass priorRecord too so a child // that moved to a different parent updates BOTH old and new parent. const summaryFailures = await this.recomputeSummaries(object, result, priorRecord, opCtx.context); @@ -13086,7 +13137,7 @@ export class ObjectQL implements IObjectQLEngine { } } - async delete(object: string, options?: EngineDeleteOptions): Promise { + async delete(object: string, options?: EngineDeleteOptions): Promise { object = this.resolveObjectName(object); this.logger.debug('Delete operation starting', { object }); this.assertWriteAllowed(object, 'delete'); @@ -13531,6 +13582,21 @@ export class ObjectQL implements IObjectQLEngine { await this.triggerHooks('afterDelete', hookContext); } + // [#16231] The `delete()` twin, on its own declaration: whether the + // by-id row was there (`boolean`), or how many rows a predicate + // delete removed (`number`). ⚠️ `false` and `0` are the two most + // ordinary answers this verb gives, so `isDeleteResultShape` is a + // pair of `typeof` tests and never a truthiness check — a lenient + // guard here would refuse exactly the answers + // `metadata-protocol`'s `deleteData` turns into its 404. + if (!isDeleteResultShape(hookContext.result)) { + throw new DeleteHookResultNotWriteShapeError({ + object, + event: 'afterDelete', + result: hookContext.result, + }); + } + // Roll-up: recompute the parent summary now that the child is gone, // from the row's FK values captured BEFORE deletion. [#5272] That // capture is now the same single pre-image read `previous` rides on diff --git a/packages/objectql/src/find-hook-result-shape.ts b/packages/objectql/src/find-hook-result-shape.ts index ede0117732..89c7f11fcc 100644 --- a/packages/objectql/src/find-hook-result-shape.ts +++ b/packages/objectql/src/find-hook-result-shape.ts @@ -15,11 +15,13 @@ * diagnostic, no log. Measured on a real engine over a real SQL driver — * `ARRAY(len=1)` with no hooks, `OBJECT{records}` with the handler. * - * Of the four `return hookContext.result` sites in `engine.ts` this is the only - * one with a concrete declared shape to violate; `findOne`, `update` and - * `delete` all declare `Promise` and carry no enforceable declaration at - * all. That is a separate question about those declarations and is deliberately - * NOT answered here. + * Of the four `return hookContext.result` sites in `engine.ts` this was, when + * #15823 landed, the only one with a concrete declared shape to violate: + * `findOne`, `update` and `delete` all declared `Promise` and carried no + * enforceable declaration at all. #15823 fenced that out as a separate question + * about those declarations; #16231 answered it (maintainer ruling, option A, + * 2026-09-07). All four verbs declare now, and the other three seams are closed + * on these terms in `verb-hook-result-shape.ts`. * * ## Why a refusal rather than a wider declaration * diff --git a/packages/objectql/src/index.ts b/packages/objectql/src/index.ts index 13f463116b..f5646c28d1 100644 --- a/packages/objectql/src/index.ts +++ b/packages/objectql/src/index.ts @@ -200,6 +200,27 @@ export { FIND_HOOK_RESULT_NOT_ARRAY_STATUS, describeFindHookResult, } from './find-hook-result-shape.js'; +// [#16231] The other three seams, closed on the same terms once their verbs +// had declarations worth guarding (maintainer ruling, option A, 2026-09-07). +// Exported for the same reason: the remedy belongs to the HOOK'S AUTHOR, who +// branches on the code. The three shape predicates ride along because a host +// that installs `after*` handlers can use them to check its own answer before +// the engine does — which is cheaper than reading the refusal. +export { + FindOneHookResultNotRecordError, + FIND_ONE_HOOK_RESULT_NOT_RECORD_CODE, + FIND_ONE_HOOK_RESULT_NOT_RECORD_STATUS, + isFindOneResultShape, + UpdateHookResultNotWriteShapeError, + UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE_CODE, + UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE_STATUS, + isUpdateResultShape, + DeleteHookResultNotWriteShapeError, + DELETE_HOOK_RESULT_NOT_WRITE_SHAPE_CODE, + DELETE_HOOK_RESULT_NOT_WRITE_SHAPE_STATUS, + isDeleteResultShape, + describeHookResult, +} from './verb-hook-result-shape.js'; // [#14010] `Hook.runAs` — the declared execution identity of a hook's `ctx.api` // data operations. The refusal a `runAs: 'user'` hook raises when its trigger // resolved no user (ADR-0112 code + status), the api that raises it, and the diff --git a/packages/objectql/src/internal-fields.test.ts b/packages/objectql/src/internal-fields.test.ts index 26829bf035..5b11b83239 100644 --- a/packages/objectql/src/internal-fields.test.ts +++ b/packages/objectql/src/internal-fields.test.ts @@ -261,8 +261,16 @@ describe('#7728: the `internal` field flag omits a value from the generic data p const updated = await ctx.engine.update('itest_api_key', { id: created.id, revoked: true }, { context: { isSystem: true }, } as any); - expect(updated.key).toBe(HASH); - expect(updated.revoked).toBe(true); + // [#16231] `update` declares its dispatch union now. This is the BY-ID + // path — the payload carries `id` and no `where` — so the answer is the + // post-write record; the count limb belongs to the predicate path. Pinned + // here rather than cast away, because "the by-id result is a RECORD that + // keeps the flagged field" is exactly what this case is about. + expect(typeof updated).toBe('object'); + expect(updated).not.toBeNull(); + const updatedRow = updated as Record; + expect(updatedRow.key).toBe(HASH); + expect(updatedRow.revoked).toBe(true); }); }); diff --git a/packages/objectql/src/multi-update-hook-key-divergence.test.ts b/packages/objectql/src/multi-update-hook-key-divergence.test.ts index 9abe179c3f..a6e24a732e 100644 --- a/packages/objectql/src/multi-update-hook-key-divergence.test.ts +++ b/packages/objectql/src/multi-update-hook-key-divergence.test.ts @@ -183,7 +183,15 @@ describe('[#14099] the mixed batch the card measured is refused', () => { it('names the object, the diverging key and the prescription', async () => { const { engine } = await mixedBatch(); - const err = await run(engine).catch((e) => e as MultiUpdateHookKeyDivergenceError); + // [#16231] The same two-armed `then` the case above uses, and now for a + // second reason: `update()` declares its result union, so a bare `.catch` + // types `err` as "the refusal OR whatever the write resolved" and every + // assertion below reads through that union. The refusal arm is the only one + // this case is about — an update that RESOLVED here is the defect. + const err = await run(engine).then( + () => { throw new Error('expected the batch to be refused'); }, + (e: unknown) => e as MultiUpdateHookKeyDivergenceError, + ); expect(err.object).toBe('task'); expect(err.keys).toEqual(['completed_at']); expect(err.rows).toBe(2); diff --git a/packages/objectql/src/plugin.integration.test.ts b/packages/objectql/src/plugin.integration.test.ts index c45b0fbb8a..250d97aa49 100644 --- a/packages/objectql/src/plugin.integration.test.ts +++ b/packages/objectql/src/plugin.integration.test.ts @@ -1529,7 +1529,12 @@ describe('ObjectQLPlugin - Metadata Service Integration', () => { findOne: async () => null, create: async (_o: string, d: any) => ({ id: 'rec-1', ...d }), update: async (_o: string, _i: any, d: any) => ({ id: _i, ...d }), - updateMany: async (_o: string, _ast: any, d: any) => { bulkUpdates.push({ ...d }); return [{ ...d }]; }, + // [#16231] `IDataDriver.updateMany` declares `Promise` — the + // affected-row count a predicate write resolves. This double answered an + // ARRAY of rows, a shape no driver produces and one the engine used to + // hand straight out under `Promise`; the assertions here read the + // captured payload, never the return, so the drift was invisible. + updateMany: async (_o: string, _ast: any, d: any) => { bulkUpdates.push({ ...d }); return 1; }, delete: async () => true, syncSchema: async () => {}, }; await kernel.use({ @@ -1604,7 +1609,12 @@ describe('ObjectQLPlugin - Metadata Service Integration', () => { findOne: async () => null, create: async (_o: string, d: any) => ({ id: 'rec-1', ...d }), update: async (_o: string, _i: any, d: any) => ({ id: _i, ...d }), - updateMany: async (_o: string, _ast: any, d: any) => { bulkUpdates.push({ ...d }); return [{ ...d }]; }, + // [#16231] `IDataDriver.updateMany` declares `Promise` — the + // affected-row count a predicate write resolves. This double answered an + // ARRAY of rows, a shape no driver produces and one the engine used to + // hand straight out under `Promise`; the assertions here read the + // captured payload, never the return, so the drift was invisible. + updateMany: async (_o: string, _ast: any, d: any) => { bulkUpdates.push({ ...d }); return 1; }, delete: async () => true, syncSchema: async () => {}, }; await kernel.use({ diff --git a/packages/objectql/src/search-companion-read-projection-conformance.test.ts b/packages/objectql/src/search-companion-read-projection-conformance.test.ts index cd1d666ec1..a0e55b1c36 100644 --- a/packages/objectql/src/search-companion-read-projection-conformance.test.ts +++ b/packages/objectql/src/search-companion-read-projection-conformance.test.ts @@ -352,8 +352,13 @@ describe.each([ it('door: the update response body', async () => { bindCompanionStamp(engine); const updated = await engine.update(CONTACT, { id: contactId, name: '张伟明' }); - expect(updated?.name).toBe('张伟明'); - expectNoCompanion(updated); + // [#16231] A by-id update answers the RECORD; the count limb is the + // predicate path's. Narrowed here so "the update response body" — the door + // this case is named for — is asserted as the row it is. + expect(typeof updated).toBe('object'); + const updatedRow = updated as Record | null; + expect(updatedRow?.name).toBe('张伟明'); + expectNoCompanion(updatedRow); }); it('door: records nested by expand', async () => { diff --git a/packages/objectql/src/verb-hook-result-shape.ts b/packages/objectql/src/verb-hook-result-shape.ts new file mode 100644 index 0000000000..99617e572d --- /dev/null +++ b/packages/objectql/src/verb-hook-result-shape.ts @@ -0,0 +1,367 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#16231] The other three `return hookContext.result` seams — `findOne`, + * `update` and `delete` — closed on the same terms `find()`'s was, now that + * each of them has a declaration worth guarding. + * + * ## Why this module could not have been written before + * + * `engine.ts` has FOUR `return hookContext.result` sites, one per hook-bearing + * verb. #15823 closed the `find()` one, and its own module note recorded why it + * could only close that one: `find()` declared `Promise` — a concrete + * container to violate — while `findOne`, `update` and `delete` all declared + * `Promise` and so carried nothing an `after*` handler could break. + * + * A guard cannot exist before a declaration worth guarding does. That is the + * whole framing of #16231, and the maintainer ruled it (option A, 2026-09-07, + * director seat summon #17, decision batch #2): the three declarations move off + * `any` onto what the verbs actually answer, and each seam is then guarded + * exactly as `find()`'s is. Options B (declare only, no enforcement) and C + * (record `any` as intended) were refused. + * + * ## What each verb declares, and where the shape comes from + * + * Not invented here — read off the driver contract each engine exit delegates + * to (`IDataDriver`, `packages/spec/src/contracts/data-driver.ts`) and off the + * dispatch ladder that chooses between them + * (`resolveEngineUpdateDispatch` / `resolveEngineDeleteDispatch`): + * + * findOne → `Record | null` + * ONE exit: `driver.findOne`, declared + * `Promise | null>`. The engine's own + * docblock has said the same in prose since #4419 — "Read the ONE + * record the query selects, or `null`" — and callers already + * branch on `if (!row)`. + * + * update → `Record | number | null` + * TWO exits. By-id (`driver.update`, + * `Promise | null>`) answers the + * post-write readback — or `null` when that readback leaves the + * caller's row scope. Predicate (`driver.updateMany`, + * `Promise`) answers the affected-row COUNT and names no + * row (#4639); `engine.ts` says so at the publish branch it feeds. + * + * delete → `boolean | number` + * TWO exits, same fork. By-id (`driver.delete`, + * `Promise`) answers whether the row was there — + * `metadata-protocol`'s `deleteData` already turns `false` into a + * 404. Predicate (`driver.deleteMany`, `Promise`) answers + * the affected count. + * + * ⚠️ The row's FIELD values stay erased (`Record`, not + * `Record`), and that is the precedent being extended rather + * than a softening of it. `find()` declares `Promise`: the CONTAINER is + * the contract and the rows inside it are `any`. Declaring the container is + * what makes a normalizer limb dead by type; declaring every field's value + * type is a different, much larger change that no ruling has asked for, and it + * was measured on this card's census as breaking a materially larger consumer + * set (every `new Date(row.some_column)` in the repo) for no gain the ruling + * names. `Record | null` is also the only spelling that can say + * "record or null" at all — `any | null` collapses to `any`. + * + * ## One predicate per verb, on the CONTAINER, and nothing cleverer + * + * SHAPING STAYS LEGAL, exactly as #15823 holds for `find()`. An `afterFind` + * handler may mutate the row in place, drop keys, or assign a DIFFERENT record + * built from it; an `afterUpdate` handler may reshape the record it is handed. + * So each check is on the shape of the container the verb declares and never on + * identity — comparing against the value the engine put there, freezing it or + * cloning it would each refuse a legitimate reshaping. + * + * What is refused is exactly one thing per verb: the answer stops being one of + * the shapes the declaration admits. + * + * ## `undefined` is refused everywhere, and that is a decision, not an oversight + * + * Same call #15823 made for `find()`, for the same reason and with the same + * cost. `undefined` is not one of the declared limbs on any of the three, and + * admitting it would leave a second hole beside the one being closed, in the + * same slot, indistinguishable from the outside. A read that answers no record + * answers `null`; a handler that wants to REFUSE an operation throws from the + * handler, which is how every other hook guard says no. + * + * ⚠️ For `delete` this is load-bearing in a way the others are not: `boolean | + * number` admits `false` and `0`, so a guard written as a truthiness check + * would refuse the two most ordinary answers there are ("the row was not + * there", "no rows matched"). The predicates below are `typeof` tests for that + * reason. + * + * ## Why `500`, and why REGISTERED ADR-0112 codes + * + * Both answers are `find-hook-result-shape.ts`'s, unchanged: the request was + * well-formed and authorized and a hook this deployment installed broke the + * engine's declared contract (a 5xx by definition, nothing for the caller to + * retry), and the value of the refusal is that a host can RECOGNISE it to find + * its own misbehaving handler — which an unregistered spelling cannot do, + * because it is demoted off `error.code` at every door + * (`resolveThrownHttpError`). A declared 5xx has its prose withheld at the HTTP + * doors, so the wire carries the code and the message reaches the HOOK'S + * AUTHOR in-process and in the server log, which is who it is addressed to. + * + * THREE codes rather than one shared code: the three declarations are three + * different contracts, a host branching on one of them is branching on a + * specific verb's answer shape, and ADR-0112's closed vocabulary is where that + * distinction belongs. `observed` carries WHICH wrong shape it was, per D3/D4, + * rather than growing the `code` vocabulary per shape. + * + * @see find-hook-result-shape.ts — the #15823 original this mirrors. + */ + +import { describeFindHookResult } from './find-hook-result-shape.js'; + +/** + * One word for what the handler left behind, shared with the `find()` refusal + * so the three verbs and their elder sibling describe a shape identically. + * + * Re-exported under a verb-neutral name rather than re-implemented: a second + * copy of `typeof`-with-special-cases is exactly the kind of near-duplicate + * that drifts (one of them learns about `Date`, the other does not) and then + * makes two refusals disagree about what they saw. + */ +export const describeHookResult = describeFindHookResult; + +/** A record — an object that is neither `null` nor an array. */ +function isRecordShape(value: unknown): boolean { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + +// --------------------------------------------------------------------------- +// findOne +// --------------------------------------------------------------------------- + +/** The wire code, registered in the spec's `ERROR_CODE_LEDGER`. */ +export const FIND_ONE_HOOK_RESULT_NOT_RECORD_CODE = 'FIND_ONE_HOOK_RESULT_NOT_RECORD' as const; + +/** `500` — a server-side handler broke a server-side contract. */ +export const FIND_ONE_HOOK_RESULT_NOT_RECORD_STATUS = 500 as const; + +/** Does this value satisfy `findOne`'s declared `Record | null`? */ +export function isFindOneResultShape(value: unknown): boolean { + return value === null || isRecordShape(value); +} + +/** + * The ADR-0112 envelope `findOne()` raises when its `afterFind` dispatch left + * `hookContext.result` outside `Record | null`. + * + * Thrown at the seam — immediately after `triggerHooks('afterFind', …)` and + * BEFORE `maskSecretFields` / `stripSearchCompanionFromRead`, both of which + * already assume a record or a nullish value — so the diagnosis names the + * handler that did it rather than surfacing as a `TypeError` at a call site + * that trusted the declaration. + */ +export class FindOneHookResultNotRecordError extends Error { + override readonly name = 'FindOneHookResultNotRecordError'; + readonly code = FIND_ONE_HOOK_RESULT_NOT_RECORD_CODE; + readonly status = FIND_ONE_HOOK_RESULT_NOT_RECORD_STATUS; + /** The object being read. */ + readonly object: string; + /** The hook event whose dispatch the replacement was observed after. */ + readonly event: string; + /** What the handler left behind — {@link describeHookResult}. */ + readonly observed: string; + /** The remedy half, addressed to the hook's author rather than to a user. */ + readonly developerMessage: string; + + constructor(info: { object: string; event: string; result: unknown }) { + const observed = describeHookResult(info.result); + super(refusalSentence(info.object, info.event, observed, 'findOne', 'a record or null')); + this.object = info.object; + this.event = info.event; + this.observed = observed; + this.developerMessage = + `'findOne()' declares 'Promise | null>' — the ONE record the query ` + + `selects, or 'null' — and its callers branch on 'if (!row)' rather than on a container ` + + `check. TWO things can put another shape here: a '${info.event}' handler that assigned ` + + `one, or a driver whose 'findOne' answered off its own contract ` + + `('Promise | null>'). A '${info.event}' handler may SHAPE that ` + + `record: mutate it in place, drop keys, ` + + `or assign a different RECORD built from it. Replacing it with something that is neither ` + + `a record nor 'null' is refused. To answer no record, assign 'null'. To REFUSE the read, ` + + `throw from the handler — that is the supported way for a '${info.event}' guard to say no. ` + + `To hand the caller a different structure, do it in the caller, not in the hook. Branch on ` + + `\`code === '${FIND_ONE_HOOK_RESULT_NOT_RECORD_CODE}'\` (ADR-0112) to detect this.`; + } +} + +// --------------------------------------------------------------------------- +// update +// --------------------------------------------------------------------------- + +/** The wire code, registered in the spec's `ERROR_CODE_LEDGER`. */ +export const UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE_CODE = 'UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE' as const; + +/** `500` — a server-side handler broke a server-side contract. */ +export const UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE_STATUS = 500 as const; + +/** + * Does this value satisfy `update`'s declared + * `Record | number | null`? + * + * `typeof value === 'number'` admits `0` deliberately — "the predicate matched + * no rows" is an ordinary answer, not a failure. + */ +export function isUpdateResultShape(value: unknown): boolean { + return value === null || typeof value === 'number' || isRecordShape(value); +} + +/** + * The ADR-0112 envelope `update()` raises when its `afterUpdate` dispatch left + * `hookContext.result` outside `Record | number | null`. + * + * Thrown at the seam — after the `afterUpdate` dispatch (including the per-row + * fan-out) and BEFORE `stripSearchCompanion`, which already assumes it is + * looking at either a record or a non-object it can skip. + */ +export class UpdateHookResultNotWriteShapeError extends Error { + override readonly name = 'UpdateHookResultNotWriteShapeError'; + readonly code = UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE_CODE; + readonly status = UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE_STATUS; + /** The object being written. */ + readonly object: string; + /** The hook event whose dispatch the replacement was observed after. */ + readonly event: string; + /** What the handler left behind — {@link describeHookResult}. */ + readonly observed: string; + /** The remedy half, addressed to the hook's author rather than to a user. */ + readonly developerMessage: string; + + constructor(info: { object: string; event: string; result: unknown }) { + const observed = describeHookResult(info.result); + super(refusalSentence( + info.object, info.event, observed, 'update', 'a record, an affected-row count or null', + )); + this.object = info.object; + this.event = info.event; + this.observed = observed; + this.developerMessage = + `'update()' declares 'Promise | number | null>' — the two answers its ` + + `two dispatch paths give. A BY-ID write resolves the post-write record (or 'null' when the ` + + `readback leaves the caller's row scope); a PREDICATE write resolves the affected-row COUNT ` + + // The predicate write's affected-count contract is stated in this module's + // header; the tracker id stays OUT of the runtime string, which reaches + // authors, operators and generated surfaces that cannot resolve one. + `and names no row. TWO things can put another shape here: a '${info.event}' handler that ` + + `assigned one, or a driver whose 'update' / 'updateMany' answered off its own contract ` + + `('Promise | null>' and 'Promise'). A '${info.event}' ` + + `handler may SHAPE what it is handed — mutate ` + + `the record in place, drop keys, assign a different RECORD — but replacing it with a shape ` + + `outside that union is refused, because the declaration is the contract. To REFUSE the ` + + `write, throw from the handler. Branch on ` + + `\`code === '${UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE_CODE}'\` (ADR-0112) to detect this.`; + } +} + +// --------------------------------------------------------------------------- +// delete +// --------------------------------------------------------------------------- + +/** The wire code, registered in the spec's `ERROR_CODE_LEDGER`. */ +export const DELETE_HOOK_RESULT_NOT_WRITE_SHAPE_CODE = 'DELETE_HOOK_RESULT_NOT_WRITE_SHAPE' as const; + +/** `500` — a server-side handler broke a server-side contract. */ +export const DELETE_HOOK_RESULT_NOT_WRITE_SHAPE_STATUS = 500 as const; + +/** + * Does this value satisfy `delete`'s declared `boolean | number`? + * + * ⚠️ `false` and `0` are the two most ORDINARY answers here — "the row was not + * there" and "no rows matched" — so the predicate is a pair of `typeof` tests + * and never a truthiness check. `metadata-protocol`'s `deleteData` turns the + * `false` into its 404, and would lose that answer to a lenient guard. + */ +export function isDeleteResultShape(value: unknown): boolean { + return typeof value === 'boolean' || typeof value === 'number'; +} + +/** + * The ADR-0112 envelope `delete()` raises when its `afterDelete` dispatch left + * `hookContext.result` outside `boolean | number`. + */ +export class DeleteHookResultNotWriteShapeError extends Error { + override readonly name = 'DeleteHookResultNotWriteShapeError'; + readonly code = DELETE_HOOK_RESULT_NOT_WRITE_SHAPE_CODE; + readonly status = DELETE_HOOK_RESULT_NOT_WRITE_SHAPE_STATUS; + /** The object being written. */ + readonly object: string; + /** The hook event whose dispatch the replacement was observed after. */ + readonly event: string; + /** What the handler left behind — {@link describeHookResult}. */ + readonly observed: string; + /** The remedy half, addressed to the hook's author rather than to a user. */ + readonly developerMessage: string; + + constructor(info: { object: string; event: string; result: unknown }) { + const observed = describeHookResult(info.result); + super(refusalSentence( + info.object, info.event, observed, 'delete', 'a boolean or an affected-row count', + )); + this.object = info.object; + this.event = info.event; + this.observed = observed; + this.developerMessage = + `'delete()' declares 'Promise' — the two answers its two dispatch paths ` + + `give. A BY-ID delete resolves whether the row was there ('false' is a real answer, and ` + + // ⛔ The metadata protocol is named without its quoted package specifier + // on purpose: `core-boundary.ratchet.test.ts` walks `core.ts`'s closure + // and flags a QUOTED forbidden package name anywhere in a file's text, + // import or not (ADR-0076 D2), and this module is inside that closure. + `the metadata protocol layer turns it into a 404); a PREDICATE delete resolves the ` + + `affected-row COUNT ('0' is a real answer). TWO things can put another shape here: an ` + + `'${info.event}' handler that assigned one, or a driver whose 'delete' / 'deleteMany' ` + + `answered off its own contract ('Promise' and 'Promise'). An ` + + `'${info.event}' handler that wants to ` + + `REFUSE a delete throws from the handler; a delete has no post-state to reshape, so ` + + `replacing 'ctx.result' with a record or an envelope is refused. Branch on ` + + `\`code === '${DELETE_HOOK_RESULT_NOT_WRITE_SHAPE_CODE}'\` (ADR-0112) to detect this.`; + } +} + +// --------------------------------------------------------------------------- +// The shared user-facing sentence +// --------------------------------------------------------------------------- + +/** + * The user-facing sentence, one composer for all three refusals. + * + * ⚠️ It names the SEAM and never a culprit, and that is a correction to the + * shape `find()`'s refusal could afford. On `find()` the value at the seam + * comes from `driver.find`, which every driver answers with an array, so + * "your handler replaced it" was true whenever the check fired. These three + * verbs have exits that can answer off-contract themselves — a `driver.update` + * double resolving `undefined` is the measured case, found by this very guard + * on four doubles in this repository — so a sentence blaming the handler would + * misattribute the fault on the most likely path. It states what is there + * after the dispatch; {@link FindOneHookResultNotRecordError.developerMessage} + * and its siblings name BOTH sources for the reader who has to go fix one. + * + * ⛔ It must not begin with a SQL verb — `@objectstack/rest`'s importer runs row + * errors through `sanitizeRowError`, whose SQL backstop replaces any message + * STARTING with `insert`/`update`/`delete` with generic text. That constraint + * bites here harder than it did on `find()`: two of these three refusals are + * ABOUT `update` and `delete`, so a sentence naming the verb first would be + * silently rewritten on exactly the path a REST caller reads. `Refusing the …` + * keeps the verb out of first position, the same shape + * `FindHookResultNotArrayError`, `DuplicateRecordError` and + * `MultiUpdateHookKeyDivergenceError` all record. + */ +function refusalSentence( + object: string, + event: string, + observed: string, + verb: string, + declared: string, +): string { + // `undefined` / `null` name themselves; everything else takes an article, and + // `object` is the one that needs `an`. + const what = + observed === 'undefined' || observed === 'null' + ? observed + : `${'aeiou'.includes(observed[0]) ? 'an' : 'a'} ${observed}`; + return ( + `Refusing the '${verb}' on '${object}': after the '${event}' dispatch 'ctx.result' is ` + + `${what}, and '${verb}()' answers ${declared}. Shaping what it answers is supported; ` + + `replacing it with another shape is not.` + ); +} diff --git a/packages/plugins/plugin-approvals/src/record-lock-schedule-run.integration.test.ts b/packages/plugins/plugin-approvals/src/record-lock-schedule-run.integration.test.ts index 957170b887..75dbf2446e 100644 --- a/packages/plugins/plugin-approvals/src/record-lock-schedule-run.integration.test.ts +++ b/packages/plugins/plugin-approvals/src/record-lock-schedule-run.integration.test.ts @@ -125,7 +125,7 @@ describe('an owning run and the approvals record lock (#3703 / #3712 / #3760)', await expect(writeAs(dataCtx)).resolves.toBeDefined(); const row = await engine.findOne('opportunity', { where: { id: oppId } }); - expect(row.amount).toBe(200); + expect(row!.amount).toBe(200); }); it('still blocks a DIFFERENT run', async () => { @@ -172,6 +172,6 @@ describe('an owning run and the approvals record lock (#3703 / #3712 / #3760)', await expect(writeAs(dataCtx)).resolves.toBeDefined(); const row = await engine.findOne('opportunity', { where: { id: oppId } }); - expect(row.amount).toBe(200); + expect(row!.amount).toBe(200); }); }); diff --git a/packages/plugins/plugin-auth/src/objectql-adapter.ts b/packages/plugins/plugin-auth/src/objectql-adapter.ts index 6be89f42f0..3107bd0c1a 100644 --- a/packages/plugins/plugin-auth/src/objectql-adapter.ts +++ b/packages/plugins/plugin-auth/src/objectql-adapter.ts @@ -981,7 +981,12 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { // first config edit, leaving a column that only LOOKS protected. liftClientSecretForWrite(objectName, patch); const result = await dataEngine.update(objectName, { ...patch, id: record.id }); - if (!result) return null; + // [#16231] The payload carries the resolved `id` and no `where`, so + // the engine dispatches `by-id` and answers the record or `null`. The + // `number` limb of the declared union is the predicate path's affected + // count, unreachable from here — refused rather than coerced, so this + // adapter never hands `normaliseLegacyDates` a count shaped as a row. + if (!result || typeof result === 'number') return null; const norm = normaliseLegacyDates(model, result); return (bridged ? remapKeys(norm, snakeToCamel) : norm) as T; }, @@ -1085,7 +1090,12 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { if (set) Object.assign(patch, bridged ? remapKeys(set, camelToSnake) : set); const result = await dataEngine.update(objectName, { ...patch, id: record.id }); - if (!result) return null; + // [#16231] The payload carries the resolved `id` and no `where`, so + // the engine dispatches `by-id` and answers the record or `null`. The + // `number` limb of the declared union is the predicate path's affected + // count, unreachable from here — refused rather than coerced, so this + // adapter never hands `normaliseLegacyDates` a count shaped as a row. + if (!result || typeof result === 'number') return null; const norm = normaliseLegacyDates(model, result); return (bridged ? remapKeys(norm, snakeToCamel) : norm) as T; }, diff --git a/packages/rest/src/import-business-timezone.test.ts b/packages/rest/src/import-business-timezone.test.ts index 0f4441aa94..7780a08ce3 100644 --- a/packages/rest/src/import-business-timezone.test.ts +++ b/packages/rest/src/import-business-timezone.test.ts @@ -447,12 +447,12 @@ describe('POST /data/:object/import — the round trip a customer actually runs' const original = await engine.findOne('shift', { where: { id: '1' } }); const reimported = await engine.findOne('shift', { where: { id: '2' } }); - expect(storedInstant(reimported.scanned_at)).toBe(storedInstant(original.scanned_at)); - expect(storedInstant(reimported.scanned_at)).toBe(CROSS_MONTH_UTC); + expect(storedInstant(reimported!.scanned_at)).toBe(storedInstant(original!.scanned_at)); + expect(storedInstant(reimported!.scanned_at)).toBe(CROSS_MONTH_UTC); // Stated as the report does: the row is still in the month it was exported // from. Under the process clock it stored 2026-08-01T13:00Z on this host. - expect(storedInstant(reimported.scanned_at)).not.toBe('2026-08-01T13:00:00.000Z'); - expect(String(reimported.due)).toContain('2026-08-01'); + expect(storedInstant(reimported!.scanned_at)).not.toBe('2026-08-01T13:00:00.000Z'); + expect(String(reimported!.due)).toContain('2026-08-01'); }); it('the same file imported by a UTC tenant is a different instant — the zone decides, not the host', async () => { @@ -468,6 +468,6 @@ describe('POST /data/:object/import — the round trip a customer actually runs' ); expect(jsonRes._json).toMatchObject({ total: 1, ok: 1, errors: 0, created: 1 }); const stored = await engine.findOne('shift', { where: { id: '2' } }); - expect(storedInstant(stored.scanned_at)).toBe('2026-08-01T06:00:00.000Z'); + expect(storedInstant(stored!.scanned_at)).toBe('2026-08-01T06:00:00.000Z'); }); }); diff --git a/packages/runtime/src/seed-loader.test.ts b/packages/runtime/src/seed-loader.test.ts index 51cda6f0e4..2d3c840669 100644 --- a/packages/runtime/src/seed-loader.test.ts +++ b/packages/runtime/src/seed-loader.test.ts @@ -70,7 +70,13 @@ function createMockEngine(data: Record = {}): IDataEngine { }), delete: vi.fn(async (_objectName: string, options?: any) => { assertEngineDeleteDispatch(options); - return { deleted: 1 }; + // [#16231] `IDataEngine.delete` declares its two dispatch answers now — + // whether the by-id row was there, or the predicate path's affected + // count. This double used to answer an invented envelope that no driver + // and no engine has ever produced; nothing in the loader reads it, which + // is precisely why `Promise` of `any` let the fiction stand. One row + // removed, spelled the way the predicate exit spells it. + return 1; }), count: vi.fn(async (objectName: string) => (store[objectName] || []).length), aggregate: vi.fn(async () => []), diff --git a/packages/services/service-automation/src/flow-field-expression-scale.integration.test.ts b/packages/services/service-automation/src/flow-field-expression-scale.integration.test.ts index 9e6b64b3fe..bb7b5616bc 100644 --- a/packages/services/service-automation/src/flow-field-expression-scale.integration.test.ts +++ b/packages/services/service-automation/src/flow-field-expression-scale.integration.test.ts @@ -127,7 +127,7 @@ describe('flow-computed money lands within its declared scale (#11060, oracle fo const row = await quoteByTitle('rounded'); expect(row, 'the quote row must persist').toBeTruthy(); // The PERSISTED value — not the expression result. - expect(row.total).toBe(126000); + expect(row!.total).toBe(126000); }); it('the assignment surface computes the same rounded value (config.assignments → interpolate)', async () => { diff --git a/packages/services/service-automation/src/paused-run-visibility.test.ts b/packages/services/service-automation/src/paused-run-visibility.test.ts index bde2672ed9..63d0b1252f 100644 --- a/packages/services/service-automation/src/paused-run-visibility.test.ts +++ b/packages/services/service-automation/src/paused-run-visibility.test.ts @@ -475,7 +475,7 @@ describe('#8050 cold boot over the same sqlite file (full stack)', () => { where: { id: runId }, context: { isSystem: true } as never, }); expect(row, 'the suspension row must survive the restart').toBeTruthy(); - expect(row.status).toBe('paused'); + expect(row!.status).toBe('paused'); // RED on main: `[]` for both listings, `null` for the detail. const listed = await second.automation.listRuns('showcase_budget_approval', { status: 'paused' }); diff --git a/packages/services/service-automation/src/runas-attribution-contract.test.ts b/packages/services/service-automation/src/runas-attribution-contract.test.ts index b03b18b595..f31b5ec148 100644 --- a/packages/services/service-automation/src/runas-attribution-contract.test.ts +++ b/packages/services/service-automation/src/runas-attribution-contract.test.ts @@ -211,12 +211,12 @@ describe("runAs:'system' attribution contract — elevation decides authorizatio const controlRow = await taskByTitle('control'); // (a) the elevated path: a `runAs:'system'` flow updates the row. - automation.registerFlow('elevated_touch', elevatedUpdateFlow('elevated_touch', String(elevatedRow.id)) as any); + automation.registerFlow('elevated_touch', elevatedUpdateFlow('elevated_touch', String(elevatedRow!.id)) as any); const res = await automation.execute('elevated_touch', { ...OPERATOR }); expect(res.success, `run failed: ${JSON.stringify(res)}`).toBe(true); // (b) the control: the same write, plain user context, no elevation. - await ql.update('crm_task', { id: controlRow.id, status: 'done' }, { context: { ...OPERATOR } }); + await ql.update('crm_task', { id: controlRow!.id, status: 'done' }, { context: { ...OPERATOR } }); const afterElevated = await taskByTitle('elevated'); const afterControl = await taskByTitle('control'); @@ -225,12 +225,12 @@ describe("runAs:'system' attribution contract — elevation decides authorizatio // its writes would land unattributed and lean on the actor label instead. // They do not: the operator is stamped, and byte-identically to the // unelevated write. - expect(afterElevated.updated_by, 'the elevated run must stamp the triggering operator').toBe('usr_operator'); - expect(afterElevated.updated_by, 'elevated attribution must equal the plain user path').toBe(afterControl.updated_by); + expect(afterElevated!.updated_by, 'the elevated run must stamp the triggering operator').toBe('usr_operator'); + expect(afterElevated!.updated_by, 'elevated attribution must equal the plain user path').toBe(afterControl!.updated_by); // The column MOVED off the creator — the assertion above is about this // update, not about the insert that seeded the row. - expect(afterElevated.created_by, 'the original creator is untouched').toBe('usr_creator'); - expect(afterElevated.status, 'the run must actually have written').toBe('done'); + expect(afterElevated!.created_by, 'the original creator is untouched').toBe('usr_creator'); + expect(afterElevated!.status, 'the run must actually have written').toBe('done'); // …and the envelope the audit writers read carries BOTH: elevation on // `isSystem` (authorization) and the operator on `userId` (attribution), @@ -254,7 +254,7 @@ describe("runAs:'system' attribution contract — elevation decides authorizatio // There is no operator to carry, so the user column stays NULL — ADR-0118 // D1 forbids a sentinel or pseudo-user standing in for one. - expect(row.created_by ?? null, 'a user-less run has no operator to stamp').toBeNull(); + expect(row!.created_by ?? null, 'a user-less run has no operator to stamp').toBeNull(); // …and the actor label is what keeps the write attributable anyway. This // is the half of the old prose that was TRUE — it was only ever true here. diff --git a/packages/services/service-automation/src/runas-system-stamping.integration.test.ts b/packages/services/service-automation/src/runas-system-stamping.integration.test.ts index 1671325617..0ec2975573 100644 --- a/packages/services/service-automation/src/runas-system-stamping.integration.test.ts +++ b/packages/services/service-automation/src/runas-system-stamping.integration.test.ts @@ -134,9 +134,9 @@ describe("runAs:'system' create_record stamps organization_id / owner_id / creat expect(row, 'the sweep must have created the row').toBeTruthy(); // The issue's step 2, inverted: the three platform columns are all // non-NULL, and each equals what the trigger context knew. - expect(row.created_by, 'created_by must be the triggering user').toBe('usr_admin'); - expect(row.owner_id, 'owner_id must be the acting user (the runAs:user default, restored)').toBe('usr_admin'); - expect(row.organization_id, "organization_id must be the trigger context's org").toBe('org_1'); + expect(row!.created_by, 'created_by must be the triggering user').toBe('usr_admin'); + expect(row!.owner_id, 'owner_id must be the acting user (the runAs:user default, restored)').toBe('usr_admin'); + expect(row!.organization_id, "organization_id must be the trigger context's org").toBe('org_1'); }); it("flow-authored ownership wins: an explicit `fields.owner_id` is never overwritten", async () => { @@ -153,10 +153,10 @@ describe("runAs:'system' create_record stamps organization_id / owner_id / creat // ADR-0073 D3 — flow logic sets ownership explicitly; the stamp is the // fill-only default underneath it, exactly like the security middleware's // own "empty means stamp" rule on the user path. - expect(row.owner_id).toBe('usr_assignee'); + expect(row!.owner_id).toBe('usr_assignee'); // Attribution is untouched by the ownership choice. - expect(row.created_by).toBe('usr_admin'); - expect(row.organization_id).toBe('org_1'); + expect(row!.created_by).toBe('usr_admin'); + expect(row!.organization_id).toBe('org_1'); }); it('a USER-LESS system run (schedule shape) stamps nothing — and that is the contract, not a gap', async () => { @@ -175,12 +175,12 @@ describe("runAs:'system' create_record stamps organization_id / owner_id / creat // banned alternative. ADR-0073's automation principal (a real identity for // these runs) is M2, gated on its first consumer. Provenance still names // the writer: the run's `svc:flow:*` actor label and flowRunId (#4366/#3712). - expect(row.created_by ?? null).toBeNull(); - expect(row.owner_id ?? null).toBeNull(); + expect(row!.created_by ?? null).toBeNull(); + expect(row!.owner_id ?? null).toBeNull(); // The schedule trigger supplies no org today, so there is nothing to // stamp; a schedule-run in an org-partitioned deployment needs the flow's // `fields` to place rows (or a future org-aware schedule binding). - expect(row.organization_id ?? null).toBeNull(); + expect(row!.organization_id ?? null).toBeNull(); }); it("REGRESSION: the runAs:'user' path is unchanged — audit + org stamps still land", async () => { @@ -192,8 +192,8 @@ describe("runAs:'system' create_record stamps organization_id / owner_id / creat expect(res.success, `run failed: ${JSON.stringify(res)}`).toBe(true); const row = await taskByTitle('renew D'); - expect(row.created_by).toBe('usr_admin'); - expect(row.organization_id).toBe('org_1'); + expect(row!.created_by).toBe('usr_admin'); + expect(row!.organization_id).toBe('org_1'); // (`owner_id` on the user path is the security middleware's stamp; the // full-security composition below covers it. This harness pins that the // audit + tenant machinery behave identically before and after the fix.) @@ -273,19 +273,19 @@ describe('the #5494 admission flip: row content, not caller, decides (real Secur expect(res.success, `run failed: ${JSON.stringify(res)}`).toBe(true); const row = await rowByTitle('flip A'); - expect(row.created_by).toBe('usr_member'); - expect(row.owner_id).toBe('usr_member'); - expect(row.organization_id).toBe('org_1'); + expect(row!.created_by).toBe('usr_member'); + expect(row!.owner_id).toBe('usr_member'); + expect(row!.organization_id).toBe('org_1'); // Step 3, inverted: the same member — no elevation, no transfer grant — // repairs and completes the record the sweep made for them. await expect( - ql.update('crm_task', { id: row.id, status: 'done' }, { context: { ...MEMBER_CTX } }), + ql.update('crm_task', { id: row!.id, status: 'done' }, { context: { ...MEMBER_CTX } }), ).resolves.toBeDefined(); - expect((await rowByTitle('flip A')).status).toBe('done'); + expect((await rowByTitle('flip A'))!.status).toBe('done'); await expect( - ql.delete('crm_task', { where: { id: row.id }, context: { ...MEMBER_CTX } }), + ql.delete('crm_task', { where: { id: row!.id }, context: { ...MEMBER_CTX } }), ).resolves.toBeDefined(); expect(await rowByTitle('flip A')).toBeFalsy(); }); @@ -298,7 +298,7 @@ describe('the #5494 admission flip: row content, not caller, decides (real Secur const res = await automation.execute('night_sweep', { event: 'schedule', params: {} } as any); expect(res.success).toBe(true); const row = await rowByTitle('flip B'); - expect(row.created_by ?? null).toBeNull(); + expect(row!.created_by ?? null).toBeNull(); // The SAME member context that succeeded above is denied here: the only // difference between the two attempts is the row's stamp columns — the @@ -323,10 +323,10 @@ describe('the #5494 admission flip: row content, not caller, decides (real Secur developerMessage: expect.stringContaining('(row-level security)'), }; await expect( - ql.update('crm_task', { id: row.id, status: 'done' }, { context: { ...MEMBER_CTX } }), + ql.update('crm_task', { id: row!.id, status: 'done' }, { context: { ...MEMBER_CTX } }), ).rejects.toMatchObject(rowLevelDenial); await expect( - ql.delete('crm_task', { where: { id: row.id }, context: { ...MEMBER_CTX } }), + ql.delete('crm_task', { where: { id: row!.id }, context: { ...MEMBER_CTX } }), ).rejects.toMatchObject(rowLevelDenial); }); }); diff --git a/packages/spec/src/api/error-code-ledger.zod.ts b/packages/spec/src/api/error-code-ledger.zod.ts index a6912719a2..e8b27b2226 100644 --- a/packages/spec/src/api/error-code-ledger.zod.ts +++ b/packages/spec/src/api/error-code-ledger.zod.ts @@ -677,10 +677,12 @@ export const ERROR_CODE_LEDGER = { // ADR-0119 D1/D4 fail-closed posture). Same #8087-gate family. 'ERR_TRANSACTION_UNSUPPORTED', // [#15823] an `afterFind` handler REPLACED `ctx.result` with something that - // is not an array, breaking the one `return hookContext.result` site in - // `engine.ts` that has a concrete declared shape to violate - // (`find(): Promise`; `findOne`/`update`/`delete` all declare - // `Promise`). Refused at the seam — immediately after the dispatch and + // is not an array, breaking `find()`'s declared `Promise`. It was the + // FIRST of the four `return hookContext.result` sites in `engine.ts` to be + // closed, and for a while the only one that could be: `findOne`/`update`/ + // `delete` declared `Promise` and so had nothing to violate. #16231 + // ruled that gap shut — all four verbs declare now, and the three refusals + // below are this one's twins. Refused at the seam — immediately after the dispatch and // ahead of `maskSecretFields` / `stripSearchCompanionFromRead`, which both // already assume the array — rather than surfacing as a `TypeError` at one // of ~140 call sites. SHAPING stays legal: mutating rows, dropping keys, @@ -693,6 +695,19 @@ export const ERROR_CODE_LEDGER = { // and the fault is a server-side extension's, not the caller's. // `FindHookResultNotArrayError`, `find-hook-result-shape.ts`. 'FIND_HOOK_RESULT_NOT_ARRAY', + // [#16231] The `findOne` twin of the refusal above, admitted once that verb + // DECLARED what it answers. `findOne(): Promise | null>` + // — the one record the query selects, or `null`, which its own docblock has + // said in prose since #4419 and which callers already branch on with + // `if (!row)`. An `afterFind` handler that replaces `ctx.result` with + // anything else (an envelope, an array, `undefined`) is refused at the same + // position `find()`'s is: after the dispatch, ahead of `maskSecretFields` + // and `stripSearchCompanionFromRead`, both of which already assume it. + // SHAPING stays legal — mutating the record, dropping keys, assigning a + // different RECORD are all untouched. Registered for the same reason as its + // sibling: a host has to RECOGNISE this to find its own misbehaving + // handler. `FindOneHookResultNotRecordError`, `verb-hook-result-shape.ts`. + 'FIND_ONE_HOOK_RESULT_NOT_RECORD', // [#14010] a hook declared `runAs: 'user'` and its trigger resolved NO user // (an `isSystem` plugin/service write, a system-elevated flow node), so its // `ctx.api` data operation has no identity to scope to and is REFUSED @@ -713,6 +728,25 @@ export const ERROR_CODE_LEDGER = { // decision. `MultiUpdateHookKeyDivergenceError`, // `multi-update-hook-key-divergence.ts`. 'MULTI_UPDATE_HOOK_KEY_DIVERGENCE', + // [#16231] The `delete` twin. `delete(): Promise` — the + // two answers its two dispatch paths give: whether the by-id row was there + // (`driver.delete`), or how many rows a predicate delete removed + // (`driver.deleteMany`, #4639). An `afterDelete` handler that replaces + // `ctx.result` with anything outside that union is refused. ⚠️ `false` and + // `0` are ORDINARY answers and are not refused — `metadata-protocol`'s + // `deleteData` turns the `false` into its 404. A handler that wants to + // refuse a delete throws. `DeleteHookResultNotWriteShapeError`, + // `verb-hook-result-shape.ts`. + 'DELETE_HOOK_RESULT_NOT_WRITE_SHAPE', + // [#16231] The `update` twin. + // `update(): Promise | number | null>` — the post-write + // record (or `null` when the readback leaves the caller's row scope) from + // the by-id exit, or the affected-row COUNT from the predicate exit + // (#4639), which names no row. An `afterUpdate` handler may SHAPE what it + // is handed; replacing it with a shape outside the union is refused, at the + // seam and ahead of `stripSearchCompanion` and the realtime publish. + // `UpdateHookResultNotWriteShapeError`, `verb-hook-result-shape.ts`. + 'UPDATE_HOOK_RESULT_NOT_WRITE_SHAPE', // [#14748] the ADR-0048 Phase 1 install-time namespace gate's refusal: a // package's `manifest.namespace` is already owned by an INSTALLED package // that is not a co-owner of it (ADR-0130 D1), so the install is refused up diff --git a/packages/spec/src/contracts/data-engine.test.ts b/packages/spec/src/contracts/data-engine.test.ts index 07299694c5..b44ad7fc5a 100644 --- a/packages/spec/src/contracts/data-engine.test.ts +++ b/packages/spec/src/contracts/data-engine.test.ts @@ -28,7 +28,14 @@ describe('Data Engine Contract', () => { findOne: async (_objectName, _query?) => null, insert: async (_objectName, data, _options?) => data, update: async (_objectName, data, _options?) => data, - delete: async (_objectName, _options?) => { return { deleted: 1 }; }, + // [#16231] `delete` declares `Promise[boolean | number]` — whether the + // by-id row was there, or how many rows a predicate delete removed. + // This fake used to answer `{ deleted: 1 }`, a shape NO driver and NO + // engine has ever produced; `Promise[any]` admitted it, and a contract + // test modelling a shape the contract does not have is the drift the + // declaration exists to stop. Spelled with square brackets in this + // comment only where a generic would otherwise be typed out. + delete: async (_objectName, _options?) => 1, count: async (_objectName, _query?) => 0, aggregate: async (_objectName, _query) => [], }; @@ -53,7 +60,7 @@ describe('Data Engine Contract', () => { return data; }, update: async (_obj, data) => data, - delete: async () => ({ deleted: 1 }), + delete: async () => 1, count: async () => store.length, aggregate: async () => [], }; @@ -92,7 +99,7 @@ describe('Data Engine Contract', () => { }, insert: async (_obj, data) => data, update: async (_obj, data) => data, - delete: async () => ({}), + delete: async () => true, count: async (_obj, _query, options) => { seen.push({ method: 'count', isSystem: options?.context?.isSystem }); return 0; @@ -123,7 +130,7 @@ describe('Data Engine Contract', () => { findOne: async () => null, insert: async (_obj, data) => data, update: async (_obj, data) => data, - delete: async () => ({}), + delete: async () => true, count: async () => 0, aggregate: async () => [], vectorFind: async (_objectName, _vector, options?) => { @@ -156,7 +163,7 @@ describe('Data Engine Contract', () => { findOne: async () => null, insert: async (_obj, data) => data, update: async (_obj, data) => data, - delete: async () => ({}), + delete: async () => true, count: async () => 0, aggregate: async () => [], execute: async (command, options?) => { @@ -169,6 +176,76 @@ describe('Data Engine Contract', () => { expect(result.raw).toBe(true); }); }); + /** + * [#16231] The three narrowed verb declarations are PINNED, not only guarded. + * + * The maintainer ruling (option A, 2026-09-07, director seat summon #17, + * decision batch #2) has two halves: `findOne` / `update` / `delete` DECLARE + * what they answer, and each hook seam GUARDS `hookContext.result` against + * that declaration. The guard half is pinned by + * `packages/objectql/src/engine-verb-hook-result-shape.test.ts`. The + * DECLARATION half was pinned nowhere: reverting these three members to + * `Promise[any]` while leaving the guards in place reddened nothing in the + * repository. Every consumer repair the census produced (`if (!row)`, + * `typeof x === 'number' ? … : …`, `result!.assignee`) compiles identically + * against `any` — `any` admits every property read and is assignable in both + * directions — so those repairs are evidence that a narrowing once happened, + * never that it is still in force. A declared contract nothing can fail is + * ADR-0049's enforce-or-remove target; these three cases close that. + * + * ## The channel, and why it cannot be vitest + * + * These are COMPILE facts. vitest strips types through esbuild without ever + * resolving them, so all three cases run green over a reverted declaration. + * The check that actually reads them is `check:test-typecheck` + * (`tsconfig.test.json`, #5286) — the gate whose absence had made eighteen + * `@ts-expect-error` pins across this repo phantom checks. + * + * ## How a revert is caught + * + * The mechanism is the one the `#12248` block below already records: every + * directive here is RESOLVED by tsc today, so widening a member back to + * `Promise[any]` does not make a case fail an assertion — it makes the + * directive UNUSED, and an unused `@ts-expect-error` is itself an error + * (TS2578) in a file whose debt ledger is exact. + * + * ⛔ No new engine double. Each case reads the DECLARED member type through + * `IDataEngine[…]`, so nothing here can drift from the contract by being + * modelled beside it. + */ + describe('the narrowed verb declarations are pinned, not just guarded (#16231)', () => { + type FindOneAnswer = Awaited>; + type UpdateAnswer = Awaited>; + type DeleteAnswer = Awaited>; + + it('findOne: an un-null-checked property read does not compile', () => { + // The null limb is the entire point of the declaration — `findOne` reads + // the ONE record the query selects, or nothing — and TS18047 here is the + // dominant signature of this card's 92-error consumer census. + // @ts-expect-error - 'row' is possibly 'null' + const readsWithoutChecking = (row: FindOneAnswer): unknown => row.id; + expect(typeof readsWithoutChecking).toBe('function'); + }); + + it('update: the answer cannot enter a record slot unnarrowed', () => { + // TWO dispatch exits: the by-id post-write record (or `null`), and the + // affected-row COUNT a predicate write resolves (#4639). A consumer that + // wants the record has to separate the count limb first, which is what + // `any` let every call site skip. + // @ts-expect-error - 'number | null' is not assignable to a record slot + const intoRecordSlot = (written: UpdateAnswer): Record => written; + expect(typeof intoRecordSlot).toBe('function'); + }); + + it('delete: the answer is not a row and has no field to read', () => { + // `boolean | number` — whether the by-id row was there, or how many rows + // a predicate delete removed. The `{ deleted: n }` envelope this card + // found in four test doubles is the shape an unread declaration admits. + // @ts-expect-error - property does not exist on 'boolean | number' + const readsARow = (removed: DeleteAnswer): unknown => removed.id; + expect(typeof readsARow).toBe('function'); + }); + }); /** * [#5126] `WriteObservabilityOptions` is the IN-PROCESS write-options diff --git a/packages/spec/src/contracts/data-engine.ts b/packages/spec/src/contracts/data-engine.ts index 991aa2038e..7907f0e294 100644 --- a/packages/spec/src/contracts/data-engine.ts +++ b/packages/spec/src/contracts/data-engine.ts @@ -273,10 +273,10 @@ export interface IDataEngine { * * [#6300] `query` is the author state (`z.input`), same as `find` above. */ - findOne(objectName: string, query?: EngineQueryOptions, options?: BaseEngineOptions): Promise; + findOne(objectName: string, query?: EngineQueryOptions, options?: BaseEngineOptions): Promise | null>; insert(objectName: string, data: any | any[], options?: DataEngineInsertOptions & WriteObservabilityOptions): Promise; - update(objectName: string, data: any, options?: EngineUpdateOptions & WriteObservabilityOptions): Promise; - delete(objectName: string, options?: EngineDeleteOptions): Promise; + update(objectName: string, data: any, options?: EngineUpdateOptions & WriteObservabilityOptions): Promise | number | null>; + delete(objectName: string, options?: EngineDeleteOptions): Promise; count(objectName: string, query?: EngineCountOptions, options?: BaseEngineOptions): Promise; aggregate(objectName: string, query: EngineAggregateOptions, options?: BaseEngineOptions): Promise; diff --git a/packages/spec/src/contracts/scoped-context.ts b/packages/spec/src/contracts/scoped-context.ts index 5c1878edc0..60d4c3b807 100644 --- a/packages/spec/src/contracts/scoped-context.ts +++ b/packages/spec/src/contracts/scoped-context.ts @@ -108,8 +108,9 @@ * here — the option VOCABULARY is `EngineQueryOptions`' to own and the engine's * to enforce (it rejects undeclared option keys since #4371); this interface's * job is the METHOD FACE. So the query bag is an object, the returns mirror - * `IDataEngine`'s (`Promise` / `Promise`), and nothing here claims - * to be the query schema. + * `IDataEngine`'s — `find` stays `Promise`, and since #16231 `findOne` + * and `update` carry the same declared answer shapes their `IDataEngine` + * counterparts do — and nothing here claims to be the query schema. */ import type { EngineTransactionInfo, EngineTransactionOptions } from './objectql-engine.js'; @@ -145,7 +146,7 @@ export interface IScopedObjectRepository { * nothing to do with what was asked, which no `if (!row)` can catch. When * any row genuinely will do, that is `find({ limit: 1 })`, which says so. */ - findOne(query?: Record): Promise; + findOne(query?: Record): Promise | null>; /** Count the records the query selects. */ count(query?: Record): Promise; @@ -161,7 +162,7 @@ export interface IScopedObjectRepository { * key out of the payload. The bulk form is `update(data, { where, multi: true })`; * there is no `updateMany`. */ - update(data: any, options?: Record): Promise; + update(data: any, options?: Record): Promise | number | null>; /** Update a single record by id — the id travels as the first argument. */ updateById(id: string | number, data: any): Promise;