From 745f9a2c17a3ccd52d80be4c602907739cb18eb3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 10:10:01 +0000 Subject: [PATCH 1/4] feat(objectql): dispatch before* hooks per matched row on a predicate bulk write (#5574, #5846) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `multi: true` update or delete now dispatches `beforeUpdate`/`beforeDelete` once per matched row, on a single-record-shaped context carrying that row's `id` and `previous` — the move #5038 made for the `after*` phase, held to the same yardstick. ADR-0058 Addendum II (ruling B, 2026-08-06) is the contract; `packages/spec/src/data/bulk-write-hook-conformance.ts` states it as D1-D7 and its `delivered` flags flip here. The harm: `ctx.previous` was never bound in the before phase of a predicate write, so every guard written as `if (ctx.previous?.locked) throw` passed silently on every batch — fail-open, and invisible. - D1/D2: one dispatch per matched row; `previous` is that row's pre-image, `result` stays absent, `input.options` is still the caller's bag. Zero matched rows is zero dispatches. - D3: the payload stays BATCH-scoped. Per-row contexts share THE payload object, rewrites apply to the whole batch and accumulate in dispatch order (including a REPLACED `input.data`). One updateMany, one affected count. - D4 + ADR-0058 Amendment II.1: `input.id` stops being a reroute lever, on the by-id path too. Clearing it converted a by-id write into a predicate write; rebinding moved the write to a row whose pre-image, readonlyWhen locks and validation rules were never evaluated. Both now reject with `HookTargetRebindError` (`ERR_HOOK_TARGET_REBIND`), naming the retired capability and its three replacements. - D6: one ceiling for both phases, checked before the FIRST dispatch. The engine's open-coded ceiling and message are replaced by the spec module's `resolveBulkPerRowHookBudget`. - D7: the matched row set is read ONCE and serves validation (#3106), the readonlyWhen strip (#3042) and both per-row dispatches. #5846 (a): `update()` reads its prior row BEFORE dispatching `beforeUpdate` and binds `previous` there, matching `delete()` since #5272, so both phases share one read. `sys_fetch_previous_update` is retired — its `!ctx.previous` guard is now permanently false. ADR-0049: `HookConditionLimitation` (both members), `isPredicateBulkWrite` and `predicateBulkWrite` are retired — a batch-scoped `before*` dispatch no longer exists, leaving them with neither producer nor reachable consumer. A `previous`-reading `before*` condition on a bulk write now evaluates as authored, per row. fix(plugin-auth): the last-administrator break-glass guard resolved its target set as "a scalar `input.id` if there is one, else the predicate", which was sound only while a predicate write's `before*` left `input.id` undefined. Under per-row dispatch a `multi` ban of every administrator arrived as N individually legitimate by-id bans and locked the environment out. `resolveTargetIds` now asks `options.multi` first. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013ZgeUyxzRnXzNCq8vizVoQ --- .changeset/bulk-write-before-hooks-per-row.md | 82 +++ .../last-admin-guard-per-row-dispatch.md | 25 + .../0058-expression-and-predicate-surface.md | 61 +- .../src/bulk-write-per-row-hooks.test.ts | 449 +++++++++++++ packages/objectql/src/core.ts | 5 +- .../engine-update-prior-read-scope.test.ts | 123 ++-- packages/objectql/src/engine.test.ts | 43 +- packages/objectql/src/engine.ts | 594 ++++++++++++------ .../src/hook-condition-bulk-previous.test.ts | 347 +++++----- .../src/hook-condition-fail-loud.test.ts | 124 ++-- .../src/hook-condition-merged-record.test.ts | 34 +- .../src/hook-condition-previous-scope.test.ts | 77 ++- .../src/hook-input-shape-contract.test.ts | 93 ++- .../objectql/src/hook-target-rebind-errors.ts | 136 ++++ packages/objectql/src/hook-wrappers.ts | 361 +++-------- packages/objectql/src/index.ts | 5 +- .../objectql/src/plugin.integration.test.ts | 16 +- packages/objectql/src/plugin.ts | 52 +- .../plugin-auth/src/last-admin-guard.test.ts | 20 + .../plugin-auth/src/last-admin-guard.ts | 88 ++- .../data/bulk-write-hook-conformance.test.ts | 44 +- .../src/data/bulk-write-hook-conformance.ts | 32 +- packages/spec/src/data/hook.zod.ts | 70 ++- scripts/adr-anchors.json | 3 +- 24 files changed, 1925 insertions(+), 959 deletions(-) create mode 100644 .changeset/bulk-write-before-hooks-per-row.md create mode 100644 .changeset/last-admin-guard-per-row-dispatch.md create mode 100644 packages/objectql/src/hook-target-rebind-errors.ts diff --git a/.changeset/bulk-write-before-hooks-per-row.md b/.changeset/bulk-write-before-hooks-per-row.md new file mode 100644 index 0000000000..8bce73aa15 --- /dev/null +++ b/.changeset/bulk-write-before-hooks-per-row.md @@ -0,0 +1,82 @@ +--- +"@objectstack/objectql": minor +"@objectstack/spec": patch +--- + +feat(objectql): dispatch `before*` hooks per matched row on a predicate bulk write (#5574, #5846) + +A `multi: true` update or delete now dispatches `beforeUpdate` / `beforeDelete` +**once per matched row**, on a single-record-shaped context carrying that row's +`id` and `previous` — the same move #5038 made for the `after*` phase, held to +the same yardstick. ADR-0058 Addendum II (maintainer ruling B, 2026-08-06) is +the contract; `packages/spec/src/data/bulk-write-hook-conformance.ts` states it +as clauses D1–D7, and its `delivered` flags flip with this change. + +**The harm this fixes.** `ctx.previous` was never bound in the before phase of a +predicate write, so every guard written the way guards are written — +`if (ctx.previous?.locked) throw` — passed **silently** on every batch. The +failure direction is fail-OPEN and the optional chaining that makes it silent is +exactly what an AI writes. One measured deployment had all 15 of its guard hooks +bypassed by a single batch edit, including writing `null` into a `readonly: true` +field that the single-id path refuses. + +**Two visible behaviour changes, both loud.** + +- **Guards now fire per row on predicate writes.** A `beforeUpdate` / + `beforeDelete` hook on an object targeted by a `multi: true` write runs N times + instead of once, each time with that row's `previous` bound. Zero matched rows + is zero dispatches. A hook that throws refuses the whole batch before anything + is written. The payload stays **batch-scoped** (D3): every per-row context + carries the one payload, so a rewrite applies to every matched row whichever + row's dispatch made it, rewrites accumulate in dispatch order, and no predicate + write is ever split into N single-row writes — one `updateMany`, one affected + count (#4639), one aggregate event. A rewrite *conditioned* on the row is + therefore out of contract: it widens to the whole batch rather than scoping + itself. Per-row `previous` is supplied so a guard can REFUSE, not so a rewrite + can be aimed. +- **The `input.id` reroute lever is retired and now refuses.** Clearing + `ctx.input.id` in a `beforeUpdate` handler used to convert a by-id write into a + predicate write over the caller's `where`; rebinding it moved the write to + another row (`delete()` honoured that by re-reading the pre-image). The + dispatch ladder is now resolved **before** the before phase — it has to be, + since per-row contexts are built from the matched row set — so a rebind + retargets nothing. Rather than ignore it (a silent no-op) or honour it (writing + a row whose pre-image, `readonlyWhen` locks and validation rules were never + evaluated), the write is rejected with `HookTargetRebindError` + (`ERR_HOOK_TARGET_REBIND`), whose message names the retired capability and the + three supported replacements. Recorded as ADR-0058 Amendment II.1. + +**Also in this change.** + +- **One read, reused (D7).** The matched row set is read ONCE per predicate + write, with the write's own composed AST, and serves per-row validation + (#3106), the `readonlyWhen` strip (#3042) and both per-row dispatches. +- **One ceiling, both phases (D6).** `MAX_BULK_PER_ROW_HOOK_ROWS` (10 000) now + governs `before*` as well as `after*`, checked **before the first dispatch**, so + an over-ceiling batch runs zero handlers and writes nothing — a refusal, never + a downgrade to one dispatch. The engine's open-coded ceiling and refusal + message are replaced by the spec module's `resolveBulkPerRowHookBudget`, so the + number and the wording have one definition again. +- **`update()` binds `previous` before the before phase (#5846 (a)).** The by-id + path reads its prior row ahead of the dispatch, matching `delete()`'s shape + since #5272, so both phases share one read. objectql's + `sys_fetch_previous_update` builtin is **retired**: it existed to bind + `previous` for the before phase behind `if (input.id && !ctx.previous)`, and + that guard is now permanently false. A by-id update on a kernel used to read + the same row three times; this removes one and makes the engine's read the + single producer. +- **`HookConditionLimitation` is retired** (ADR-0049 enforce-or-remove), with + `isPredicateBulkWrite` and the `predicateBulkWrite` flag. Both members + (`bulk_write_previous_unbound`, `bulk_write_stored_state_unavailable`) + described a batch-scoped `before*` dispatch that no longer exists, leaving them + with neither producer nor reachable consumer. A `previous`-reading `before*` + condition on a bulk write now **evaluates as authored**, per row, instead of + rejecting the batch. `HookConditionError` itself is unchanged — an unevaluable + condition still aborts the operation (#4775). + +**Migrating.** A handler that cleared or rebound `ctx.input.id` must instead +write through `ctx.api` / `ctx.ql` for the row it means, have the caller pass +`{ multi: true, where: … }`, or throw to refuse the write. A `beforeUpdate` hook +with side effects on an object that receives bulk writes should expect to run +per row; a batch-wide effect belongs in a payload rewrite, which is still +batch-scoped. diff --git a/.changeset/last-admin-guard-per-row-dispatch.md b/.changeset/last-admin-guard-per-row-dispatch.md new file mode 100644 index 0000000000..087f5332ae --- /dev/null +++ b/.changeset/last-admin-guard-per-row-dispatch.md @@ -0,0 +1,25 @@ +--- +"@objectstack/plugin-auth": patch +--- + +fix(plugin-auth): keep the last-administrator guard exact when `before*` hooks fire per row (#5574) + +The break-glass guard resolved a write's target set as "a scalar `input.id` if +there is one, otherwise the caller's predicate". That was sound only because a +predicate (`multi: true`) write's `before*` dispatch left `input.id` +present-but-**undefined**. ADR-0058 Addendum II makes the `before*` phase fire +once per MATCHED ROW, each context naming its own row — so read that way, a +`multi` ban of every administrator arrives as N separate by-id bans, each of +which is legitimately allowed (banning one admin out of three leaves two), and +the batch locked the environment out with no refusal anywhere. + +`resolveTargetIds` now asks `options.multi` FIRST: on a predicate write the +target set is the caller's predicate, whichever row the current dispatch names; +the id is consulted only when the write really is by-id. `input.options` is the +caller's bag during `before*` — `where` and `multi` included — and the contract +preserves that deliberately, so the discriminator the guard needs is unchanged. + +All eight guarded halves (#5892 ban, #5941 delete, #5978 standing) are covered +by the existing predicate cases, which went red on the engine change and are now +the pin that a population-scoped invariant survives being asked one row at a +time. diff --git a/docs/adr/0058-expression-and-predicate-surface.md b/docs/adr/0058-expression-and-predicate-surface.md index 1a97ce35c3..3900ca2b5b 100644 --- a/docs/adr/0058-expression-and-predicate-surface.md +++ b/docs/adr/0058-expression-and-predicate-surface.md @@ -289,9 +289,68 @@ > here: it is a live-behaviour question on a seam another card owns, and this > appendix will not presume the answer. The engine half and #5846 settle it > together, in one edit to one ordering, and record it as an amendment. +> **→ Settled in Amendment II.1 below.** > - **`scripts/adr-anchors.json`'s `hook-wrappers.ts` invariant still describes > the batch dispatch.** It is TRUE today and must move with the engine half, -> not before it. +> not before it. **→ Moved with #5574's engine half.** + +--- + +> **Amendment II.1 (2026-08, #5574 engine half + #5846 (a)) — the `input.id` +> reroute lever is RETIRED, and refused loudly.** +> _Settles the item Addendum II left open by name. Delivered in the same PR that +> delivered the per-row `before*` dispatch, because it is the same edit to the +> same ordering._ +> +> **The capability, stated exactly.** `update()` and `delete()` dispatched their +> `before*` event FIRST and only then read `hookContext.input.id` to choose the +> driver call. The id slot therefore doubled as a control lever: a handler +> assigning `ctx.input.id = undefined` on a by-id call converted the write into +> a PREDICATE write over the caller's `where`; a handler assigning a different +> id moved the write to another row (`delete()` supported that explicitly, by +> re-reading the pre-image for the new target — #5272). +> +> **Why it cannot survive the reorder.** A per-row `before*` context is BUILT +> from the matched row set, so the row set must be in hand before the first +> dispatch, so the branch that decides whether there IS a row set must be +> decided before that. #5846's (a) direction lands in the same edit: the by-id +> path reads its prior row ahead of the dispatch and binds `previous` there. By +> the time any handler runs, the target is settled — `previous`, the +> `readonlyWhen` strip and every validation rule have already been computed +> against the row the ladder chose. +> +> **The three options, and the choice.** *Ignore it* — the assignment retargets +> nothing and says nothing, which is the silent no-op D4 exists to abolish, and +> here the write still lands on the ORIGINAL row. *Honour it by re-resolving* — +> the write lands on a row whose pre-image was never read, whose `readonlyWhen` +> locks were never evaluated and whose rules were checked against a different +> record: silently weaker enforcement, aimed by a hook. *Refuse* — chosen. The +> write is rejected with `HookTargetRebindError` +> (`objectql/src/hook-target-rebind-errors.ts`, code `ERR_HOOK_TARGET_REBIND`, +> an `ERR_`-prefixed operational code on the error's own bag and deliberately +> NOT an ADR-0112 wire code, same reasoning as the budget refusal). The message +> NAMES the retired capability, so an author whose handler stopped working +> learns what changed instead of watching a write land somewhere unexpected. +> +> **Scope: both verbs, both by-id and per-row.** D4 already stated the per-row +> half. The by-id half is stated here, and it applies to `delete()` as well as +> `update()` — including the repoint `delete()` used to honour by re-reading. +> One rule beats two, and the delete-side re-read had no in-repo consumer (the +> premise was checked against `origin/main`: the only `ctx.input.id` assignment +> in the whole repository was one engine test forcing the fail-closed AST +> assertion, which is now the refusal's own pin). +> +> **What replaces it, for each thing it was used for.** Write a different row: +> `ctx.api` / `ctx.ql` for that row explicitly. Write many rows: have the caller +> pass `{ multi: true, where: … }`. Stop this write: throw from the handler — +> the supported way for a `before*` guard to refuse, and the one the per-row +> `previous` binding exists to enable. +> +> **One consequence priced with it.** `ENGINE_UPDATE_REJECT_MESSAGE` / +> `ENGINE_DELETE_REJECT_MESSAGE` used to be raised AFTER the before phase, so a +> handler binding `input.id` could convert a rejecting call into a by-id write. +> That is the same lever pointed the other way; with the ladder resolved first +> the refusal lands before any handler runs and before anything is read. --- diff --git a/packages/objectql/src/bulk-write-per-row-hooks.test.ts b/packages/objectql/src/bulk-write-per-row-hooks.test.ts index a5847b7a53..d3f6f27665 100644 --- a/packages/objectql/src/bulk-write-per-row-hooks.test.ts +++ b/packages/objectql/src/bulk-write-per-row-hooks.test.ts @@ -31,12 +31,23 @@ * written, never silently downgraded to one call; * 5. `onError` and the write's own return contract, both unchanged; * 6. the same contract on a bulk DELETE. + * + * [#5574] Section 7 extends every one of those to the `before*` phase, which + * ADR-0058 Addendum II brought under the same contract (clauses D1–D7). The + * cases live here rather than in a file of their own on purpose: "the before + * half diverged from the after half" is exactly the failure that would go + * unnoticed, and side-by-side is where it gets caught. */ import { describe, it, expect } from 'vitest'; import { ObjectQL } from './engine.js'; import { bindHooksToEngine } from './hook-binder.js'; +import { HookTargetRebindError, HOOK_TARGET_REBIND_ERROR_CODE } from './hook-target-rebind-errors.js'; import type { Hook, HookContext } from '@objectstack/spec/data'; +import { + BULK_PER_ROW_HOOK_LIMIT_ERROR_CODE, + MAX_BULK_PER_ROW_HOOK_ROWS, +} from '@objectstack/spec/data'; const TASK_FIELDS = { id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, @@ -443,6 +454,440 @@ describe('[#5038] a bulk delete fires afterDelete per row', () => { }); }); +/* ──────────────────────────────────────────────────────────────────────────── + * 7. [#5574] The BEFORE phase takes the same contract — ADR-0058 Addendum II + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#5574 / D1] a bulk write fires before-hooks once per matched row', () => { + it('N matched rows ⇒ N `beforeUpdate` dispatches, each naming its row', async () => { + const seen: string[] = []; + const { engine } = await boot([hook('per_row_before', 'beforeUpdate', (ctx) => { + seen.push(String((ctx.input as any).id)); + })]); + + const rows = await seedTasks(engine, [ + { title: 'a', status: 'todo' }, + { title: 'b', status: 'todo' }, + { title: 'c', status: 'todo' }, + ]); + + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + + expect(seen).toHaveLength(3); + expect(seen.sort()).toEqual(rows.map((r) => String(r.id)).sort()); + }); + + it('N doomed rows ⇒ N `beforeDelete` dispatches, each naming its row', async () => { + const seen: string[] = []; + const { engine } = await boot([hook('per_row_before', 'beforeDelete', (ctx) => { + seen.push(String((ctx.input as any).id)); + })]); + + const rows = await seedTasks(engine, [ + { title: 'a', status: 'stale' }, + { title: 'b', status: 'stale' }, + { title: 'c', status: 'live' }, + ]); + const doomed = rows.filter((r) => r.status === 'stale').map((r) => String(r.id)); + + await engine.delete('task', { multi: true, where: { status: 'stale' } } as any); + + expect(seen.sort()).toEqual(doomed.sort()); + }); + + it('zero matched rows ⇒ ZERO before dispatches', async () => { + // `[]` is meaningful and distinct from "no row set was read": a batch that + // matches nothing is not a record change, so there is nothing to dispatch + // over — not one dispatch standing for the empty set. + const seen: string[] = []; + const { engine } = await boot([hook('per_row_before', 'beforeUpdate', () => { seen.push('x'); })]); + await seedTasks(engine, [{ title: 'a', status: 'done' }]); + + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'nothing_matches' } } as any); + + expect(seen).toEqual([]); + }); + + it('is UNIFORM — the count never depends on the condition text', async () => { + // The same ruling the after phase got: a hook's firing COUNT must not + // depend on what its condition happens to say, because no author can infer + // that from any declaration. + const withPrevious: string[] = []; + const withoutPrevious: string[] = []; + const { engine } = await boot([ + hook('reads_previous', 'beforeUpdate', (ctx) => { withPrevious.push(String((ctx.input as any).id)); }, TRANSITION), + hook('reads_record', 'beforeUpdate', (ctx) => { withoutPrevious.push(String((ctx.input as any).id)); }, 'record.status == "done"'), + ]); + + await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + + expect(withPrevious).toHaveLength(2); + expect(withoutPrevious).toHaveLength(2); + }); + + it('a single-record write still fires exactly once', async () => { + const seen: string[] = []; + const { engine } = await boot([hook('per_row_before', 'beforeUpdate', () => { seen.push('x'); })]); + const row: any = await engine.insert('task', { title: 'a', status: 'todo' }); + + await engine.update('task', { status: 'done' }, { where: { id: row.id } } as any); + + expect(seen).toEqual(['x']); + }); +}); + +describe('[#5574 / D2] the per-row before context is the SINGLE-RECORD shape', () => { + it('binds THAT row’s `previous` — the measured harm of #5574, fixed', async () => { + // The failure this whole card exists for: `ctx.previous` was permanently + // undefined on a predicate write, so every guard written the way guards are + // written (`if (ctx.previous?.locked) throw`) passed SILENTLY. Fail-open, + // and invisible. + const seen: Array<{ id: string; prev: unknown }> = []; + const { engine } = await boot([hook('guard', 'beforeUpdate', (ctx) => { + seen.push({ id: String((ctx.input as any).id), prev: ctx.previous }); + })]); + + await seedTasks(engine, [ + { title: 'a', status: 'todo', owner: 'u1' }, + { title: 'b', status: 'todo', owner: 'u2' }, + ]); + + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + + expect(seen).toHaveLength(2); + for (const { id, prev } of seen) { + // Each context's `previous` is ITS row's pre-image — the pre-WRITE state, + // so `status` is still `todo` here. + expect((prev as any).id).toBe(id); + expect((prev as any).status).toBe('todo'); + } + expect(seen.map((s) => (s.prev as any).owner).sort()).toEqual(['u1', 'u2']); + }); + + it('a `previous`-reading GUARD can now refuse a bulk write per row', async () => { + // What per-row `previous` is FOR (D3 says so in as many words): so a guard + // can REFUSE, not so a rewrite can be aimed at one row. + const { engine } = await boot([hook('guard', 'beforeUpdate', (ctx) => { + if ((ctx.previous as any)?.status === 'locked') throw new Error('row is locked'); + })]); + + await seedTasks(engine, [ + { title: 'a', status: 'todo' }, + { title: 'b', status: 'locked' }, + ]); + + await expect( + engine.update('task', { status: 'done' }, { multi: true, where: {} } as any), + ).rejects.toThrow(/row is locked/); + + // The guard fired BEFORE the write, so nothing was written at all — the + // refusal covers the whole batch, not just the offending row. + expect(await engine.count('task', { where: { status: 'done' } } as any)).toBe(0); + }); + + it('carries NO `result` — the before phase has no post-state', async () => { + const seen: unknown[] = []; + const { engine } = await boot([hook('probe', 'beforeUpdate', (ctx) => { seen.push(ctx.result); })]); + await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); + + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + + expect(seen).toEqual([undefined, undefined]); + }); +}); + +describe('[#5574 / D3] the payload stays BATCH-scoped, and that IS the merge rule', () => { + it('every per-row context carries the SAME payload object, not a copy', async () => { + const payloads: unknown[] = []; + const { engine } = await boot([hook('probe', 'beforeUpdate', (ctx) => { + payloads.push((ctx.input as any).data); + })]); + await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); + + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + + expect(payloads).toHaveLength(2); + // Reference identity, deliberately. Copies are what a reconciliation step + // would need, and the ADR rejected reconciliation on measured evidence. + expect(payloads[0]).toBe(payloads[1]); + }); + + it('a rewrite made on ONE row’s dispatch applies to the WHOLE batch', async () => { + let firstOnly = true; + const { engine } = await boot([hook('stamp', 'beforeUpdate', (ctx) => { + if (firstOnly) { (ctx.input as any).data.owner = 'stamped'; firstOnly = false; } + })]); + await seedTasks(engine, [ + { title: 'a', status: 'todo', owner: 'u1' }, + { title: 'b', status: 'todo', owner: 'u2' }, + ]); + + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + + // Both rows got it, including the one whose dispatch did not make it. This + // is the contract, not a leak: one `updateMany` carries one SET clause. + const rows: any[] = await engine.find('task', {} as any); + expect(rows.map((r) => r.owner)).toEqual(['stamped', 'stamped']); + }); + + it('rewrites ACCUMULATE in dispatch order, including a REPLACED payload', async () => { + // Two spellings a handler can use, and both must accumulate: mutating the + // payload in place, and assigning a whole new object over `input.data`. + // The second is the one that would silently vanish with the row context + // that held it if the loop did not write it back. + const { engine } = await boot([hook('acc', 'beforeUpdate', (ctx) => { + const cur = (ctx.input as any).data as Record; + (ctx.input as any).data = { ...cur, title: `${String(cur.title ?? '')}+` }; + })]); + await seedTasks(engine, [ + { title: 'a', status: 'todo' }, + { title: 'b', status: 'todo' }, + { title: 'c', status: 'todo' }, + ]); + + await engine.update('task', { title: '', status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + + // Three dispatches, three appends, one payload — every row gets '+++'. + const rows: any[] = await engine.find('task', {} as any); + expect(rows.map((r) => r.title)).toEqual(['+++', '+++', '+++']); + }); + + it('never splits the write: one updateMany, one affected count (#4639)', async () => { + const { engine, driver } = await boot([hook('probe', 'beforeUpdate', () => {})]); + await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); + + driver.updateCalls = 0; + const affected = await engine.update( + 'task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any, + ); + + expect(affected).toBe(2); + // Not two single-row writes. A write that has learned to split itself + // cannot be un-split without breaking whoever came to depend on it. + expect(driver.updateCalls).toBe(0); + }); +}); + +describe('[#5574 / D4] `input.id` is not a reroute lever, and is refused loudly', () => { + it('REFUSES a per-row handler that rebinds `input.id`', async () => { + const { engine } = await boot([hook('rebind', 'beforeUpdate', (ctx) => { + (ctx.input as any).id = 'somewhere_else'; + })]); + await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); + + const err = await engine + .update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any) + .then(() => null, (e) => e); + + expect(err).toBeInstanceOf(HookTargetRebindError); + expect(err.code).toBe(HOOK_TARGET_REBIND_ERROR_CODE); + expect(err.path).toBe('per-row'); + expect(err.event).toBe('beforeUpdate'); + expect(err.observedId).toBe('somewhere_else'); + // Refused, not ignored — a silent no-op is the failure this family exists + // to abolish. And nothing was written. + expect(await engine.count('task', { where: { status: 'done' } } as any)).toBe(0); + }); + + it('REFUSES a by-id handler that clears `input.id` — the retired conversion', async () => { + const { engine } = await boot([hook('clear', 'beforeUpdate', (ctx) => { + (ctx.input as any).id = undefined; + })]); + const row: any = await engine.insert('task', { title: 'a', status: 'todo' }); + + const err = await engine + .update('task', { status: 'done' }, { where: { id: row.id } } as any) + .then(() => null, (e) => e); + + expect(err).toBeInstanceOf(HookTargetRebindError); + expect(err.path).toBe('by-id'); + expect(err.expectedId).toBe(row.id); + expect(err.message).toContain('RETIRED'); + expect((await engine.findOne('task', { where: { id: row.id } } as any) as any).status).toBe('todo'); + }); + + it('REFUSES a by-id `beforeDelete` handler that repoints the target', async () => { + // `delete()` used to HONOUR this, by re-reading the pre-image for the new + // target (#5272). ADR-0058 Amendment II.1 settles both verbs the same way: + // `previous` and the summary recompute were computed against the row the + // ladder chose, so honouring a repoint would delete a row none of that saw. + const { engine } = await boot([hook('repoint', 'beforeDelete', (ctx) => { + (ctx.input as any).id = 'other_row'; + })]); + const row: any = await engine.insert('task', { title: 'a', status: 'todo' }); + + const err = await engine + .delete('task', { where: { id: row.id } } as any) + .then(() => null, (e) => e); + + expect(err).toBeInstanceOf(HookTargetRebindError); + expect(err.event).toBe('beforeDelete'); + expect(err.path).toBe('by-id'); + expect(await engine.count('task', {} as any)).toBe(1); + }); + + it('leaves an untouched `input.id` alone — the refusal is not a trap', async () => { + // The negative control. A handler that reads the id, or writes back the + // SAME id, is doing nothing wrong and must not be refused. + const { engine } = await boot([hook('readonly', 'beforeUpdate', (ctx) => { + (ctx.input as any).id = (ctx.input as any).id; + })]); + await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); + + const affected = await engine.update( + 'task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any, + ); + expect(affected).toBe(2); + }); +}); + +describe('[#5574 / D5] equivalence with the single-id path', () => { + it('a read-only before-handler sees the same context on both shapes', async () => { + const shapeOf = (ctx: HookContext) => ({ + keys: Object.keys(ctx.input as any).sort(), + idIsRow: Boolean((ctx.input as any).id), + previousStatus: (ctx.previous as any)?.status, + data: (ctx.input as any).data, + result: ctx.result, + }); + + const bulkSeen: unknown[] = []; + const bulk = await boot([hook('probe', 'beforeUpdate', (ctx) => { bulkSeen.push(shapeOf(ctx)); })]); + await seedTasks(bulk.engine, [{ title: 'a', status: 'todo' }]); + await bulk.engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + + const singleSeen: unknown[] = []; + const single = await boot([hook('probe', 'beforeUpdate', (ctx) => { singleSeen.push(shapeOf(ctx)); })]); + const row: any = await single.engine.insert('task', { title: 'a', status: 'todo' }); + await single.engine.update('task', { status: 'done' }, { where: { id: row.id } } as any); + + // The declared differences (D5) are `input.options` carrying `multi`/`where`, + // the affected COUNT, the aggregate event and D4 — none of which this + // projection reads. Everything else must be indistinguishable. + expect(bulkSeen).toEqual(singleSeen); + }); +}); + +describe('[#5574 / D6] one ceiling, BOTH phases, checked before the first dispatch', () => { + const over = MAX_BULK_PER_ROW_HOOK_ROWS + 1; + + it('refuses an over-ceiling batch on the BEFORE phase — zero handlers, zero rows written', async () => { + const fired: string[] = []; + const { engine } = await boot([hook('per_row_before', 'beforeUpdate', () => { fired.push('x'); })]); + await seedTasks(engine, Array.from({ length: over }, (_, i) => ({ title: `t${i}`, status: 'todo' }))); + + const err = await engine + .update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any) + .then(() => null, (e) => e); + + expect(err.code).toBe(BULK_PER_ROW_HOOK_LIMIT_ERROR_CODE); + expect(err.event).toBe('beforeUpdate'); + expect(err.matched).toBe(over); + expect(err.limit).toBe(MAX_BULK_PER_ROW_HOOK_ROWS); + expect(err.message).toContain('Nothing was written'); + expect(err.message).toContain('NOT silently downgraded'); + // Refusal BEFORE the first dispatch — not after running `over` of them. + expect(fired).toEqual([]); + expect(await engine.count('task', { where: { status: 'todo' } } as any)).toBe(over); + }); + + it('refuses an over-ceiling bulk DELETE on the before phase too', async () => { + const fired: string[] = []; + const { engine } = await boot([hook('per_row_before', 'beforeDelete', () => { fired.push('x'); })]); + await seedTasks(engine, Array.from({ length: over }, (_, i) => ({ title: `t${i}`, status: 'stale' }))); + + const err = await engine + .delete('task', { multi: true, where: { status: 'stale' } } as any) + .then(() => null, (e) => e); + + expect(err.code).toBe(BULK_PER_ROW_HOOK_LIMIT_ERROR_CODE); + expect(err.event).toBe('beforeDelete'); + expect(fired).toEqual([]); + expect(await engine.count('task', {} as any)).toBe(over); + }); + + it('ADMITS a batch exactly AT the ceiling, in both phases', async () => { + // The boundary, stated on the admitted side too: an off-by-one here would + // refuse honest batches, which is the failure nobody reports as a bug. + const beforeFired: string[] = []; + const afterFired: string[] = []; + const { engine } = await boot([ + hook('pre', 'beforeUpdate', () => { beforeFired.push('x'); }), + hook('post', 'afterUpdate', () => { afterFired.push('x'); }), + ]); + const at = MAX_BULK_PER_ROW_HOOK_ROWS; + await seedTasks(engine, Array.from({ length: at }, (_, i) => ({ title: `t${i}`, status: 'todo' }))); + + const affected = await engine.update( + 'task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any, + ); + + expect(affected).toBe(at); + expect(beforeFired).toHaveLength(at); + expect(afterFired).toHaveLength(at); + }, 60_000); + + it('the engine ceiling IS the spec contract’s, not a second copy', async () => { + expect(ObjectQL.MAX_BULK_PER_ROW_HOOK_ROWS).toBe(MAX_BULK_PER_ROW_HOOK_ROWS); + }); +}); + +describe('[#5574 / D7] the matched row set is read ONCE, and serves everything', () => { + it('one `find` for a write whose object has BOTH a before- and an after-hook', async () => { + const { engine, driver } = await boot([ + hook('pre', 'beforeUpdate', () => {}), + hook('post', 'afterUpdate', () => {}), + ]); + await seedTasks(engine, [ + { title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }, + { title: 'c', status: 'todo' }, { title: 'd', status: 'todo' }, + ]); + + driver.findCalls.length = 0; + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + + // Four rows, eight dispatches, ONE query. The ruling forbids a second fetch + // in as many words. + expect(driver.findCalls).toHaveLength(1); + }); + + it('one `find` for a bulk DELETE with both phases hooked', async () => { + const { engine, driver } = await boot([ + hook('pre', 'beforeDelete', () => {}), + hook('post', 'afterDelete', () => {}), + ]); + await seedTasks(engine, [{ title: 'a', status: 'stale' }, { title: 'b', status: 'stale' }]); + + driver.findCalls.length = 0; + await engine.delete('task', { multi: true, where: { status: 'stale' } } as any); + + expect(driver.findCalls).toHaveLength(1); + }); + + it('does NOT read the row set when the object has NEITHER phase hooked', async () => { + // Still demand-driven: an object nobody hooks pays nothing for a contract + // it cannot observe. + const { engine, driver } = await boot([hook('elsewhere', 'beforeUpdate', () => {}, undefined, 'other')]); + await seedTasks(engine, [{ title: 'a', status: 'todo' }]); + + driver.findCalls.length = 0; + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + + expect(driver.findCalls).toHaveLength(0); + }); + + it('reads it once for a BEFORE-only hook — the phase can hold the gate open alone', async () => { + const { engine, driver } = await boot([hook('pre', 'beforeUpdate', () => {}, undefined, 'task')]); + await seedTasks(engine, [{ title: 'a', status: 'todo' }]); + + driver.findCalls.length = 0; + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + + expect(driver.findCalls).toHaveLength(1); + }); +}); + /* ──────────────────────────────────────────────────────────────────────────── * Harness * ──────────────────────────────────────────────────────────────────────────── */ @@ -489,6 +934,9 @@ function makeStubDriver(): any { name: 'memory', version: '0.0.0', supports: {}, /** Every `find` the engine issues, so a test can pin the read count. */ findCalls: [] as unknown[], + /** Single-row writes, so a test can pin that a predicate write is never + * split into N of them (#5574 D3). */ + updateCalls: 0, async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, async syncSchema() {}, async find(o: string, ast: any) { @@ -502,6 +950,7 @@ function makeStubDriver(): any { const row = { ...data, id }; storeFor(o).set(id, row); return row; }, async update(o: string, id: string, data: Record) { + d.updateCalls += 1; const s = storeFor(o); const cur = s.get(id); if (!cur) return null; const u = { ...cur, ...data, id }; s.set(id, u); return u; }, diff --git a/packages/objectql/src/core.ts b/packages/objectql/src/core.ts index 60f4a1403c..cef65bd443 100644 --- a/packages/objectql/src/core.ts +++ b/packages/objectql/src/core.ts @@ -57,7 +57,10 @@ export { applyInMemoryAggregation, bucketDateValue } from './in-memory-aggregati export { bindHooksToEngine } from './hook-binder.js'; export type { BindHooksOptions, BindHooksResult } from './hook-binder.js'; export { wrapDeclarativeHook, HookConditionError } from './hook-wrappers.js'; -export type { WrapDeclarativeOptions, HookConditionLimitation } from './hook-wrappers.js'; +// `HookConditionLimitation` was exported here until #5574 and is RETIRED — +// see the note above `HookConditionError` in `hook-wrappers.ts`. Its two members +// described a batch-scoped `before*` dispatch that no longer exists. +export type { WrapDeclarativeOptions } from './hook-wrappers.js'; // Validation export { ValidationError, validateRecord } from './validation/record-validator.js'; diff --git a/packages/objectql/src/engine-update-prior-read-scope.test.ts b/packages/objectql/src/engine-update-prior-read-scope.test.ts index 0de1b15fac..d1ec6a6315 100644 --- a/packages/objectql/src/engine-update-prior-read-scope.test.ts +++ b/packages/objectql/src/engine-update-prior-read-scope.test.ts @@ -21,13 +21,19 @@ * that read was only incidental: a deployment with no `afterUpdate` hook * anywhere left the old parent silently stale. * - * And the absent one: `beforeUpdate`. Unlike `delete()` (#5272), which reads the - * pre-image BEFORE dispatching `beforeDelete` and binds it there, `update()` - * dispatches `beforeUpdate` first and binds `hookContext.previous` only after - * the write — so a `beforeUpdate` hook observes `previous === undefined` - * whatever this gate decides. Two cases below measure that directly, so the - * "count before-hooks too" reflex is answered with evidence rather than - * symmetry. + * 4. [#5574 / #5846] a `beforeUpdate` hook on THIS object. This term was + * deliberately ABSENT until ADR-0058 Addendum II, and the reason it was + * absent is worth keeping: `update()` used to dispatch `beforeUpdate` + * FIRST and bind `hookContext.previous` only after the write, so a + * before-hook observed `previous === undefined` whatever this gate + * decided, and counting the event would have bought a read with no + * reader. The ruling reversed that ordering — the before phase is now a + * per-row reader of this very row set — so the reader exists and the term + * joins the gate. `update()` and `delete()` (#5272) finally agree. + * + * The cases below that used to measure the ABSENCE now measure the presence, + * and say so in place: the "count before-hooks too" reflex was answered with + * evidence before, and is answered with evidence now that the evidence changed. */ import { describe, it, expect, vi } from 'vitest'; @@ -284,28 +290,31 @@ describe('[#5284] the prior-row demand is asked per object', () => { * * `new ObjectQL()` carries no `ObjectQLPlugin` builtins, so the only producer * of `hookContext.previous` here is `update()` itself. That is exactly the - * subject: whether THIS gate's read can ever reach the before phase. + * subject: whether THIS gate's read reaches the before phase. + * + * ⚠️ This block asserted the INVERSE until #5574 — it was titled "`beforeUpdate` + * is not a reader of THIS read" and measured a before-hook seeing + * `previous === undefined` even with the row in hand. What made that true was + * dispatch ORDER, not any property of the gate, and ADR-0058 Addendum II + * reversed the order. The cases are kept in place, inverted, rather than + * deleted: the old readings are the receipts for why the gate was narrower than + * `delete()`'s for as long as it was (Prime Directive #13 — a reversed decision + * is a record). * - * A kernel-hosted engine has a second producer — `sys_fetch_previous_update` - * (`plugin.ts`, `object: '*'`, priority 5) makes its own `findOne` and assigns - * `previous` before any authored before-hook runs — so a `beforeUpdate` - * condition reading `previous` DOES evaluate there. That producer is untouched - * by this gate, which is why narrowing takes no binding away from anyone; the - * last case below drives its shape to show the two do not interfere. (That it - * is a duplicate read of the same row — a third one comes from plugin-audit's - * `captureBefore` — is filed as #5846, not fixed here.) + * A kernel-hosted engine used to have a SECOND producer, + * `sys_fetch_previous_update` (`plugin.ts`, `object: '*'`, priority 5), which + * made its own `findOne` before any authored before-hook ran. #5846 retired it: + * with the engine binding `previous` first, its `!ctx.previous` guard can no + * longer be true. The last case below drives that exact shape and pins the + * consequence — the hook-supplied fetch is now the redundant one. */ -describe('[#5284] `beforeUpdate` is not a reader of THIS read', () => { - it('observes nothing from the engine read even when the row IS in hand', async () => { - // Measured, not assumed: this object has an afterUpdate hook, so the prior - // row is fetched — and the beforeUpdate hook still sees nothing, because - // `update()` dispatches it BEFORE the read (it may still rewrite the very - // payload the read would be compared against) and binds - // `hookContext.previous` only after the write. - // - // This is why the gate does NOT count `beforeUpdate`: it would buy a read - // with no reader. `delete()` (#5272) counts `beforeDelete` because there - // the read genuinely precedes the dispatch and binds it. +describe('[#5284, inverted by #5574] `beforeUpdate` IS a reader of THIS read', () => { + it('observes the engine read — the row IS in hand, and now the before phase sees it', async () => { + // Measured, not assumed. This used to read `expect(beforeSeen).toEqual( + // [undefined])`: the prior row was fetched (this object has an afterUpdate + // hook) and the before-hook still saw nothing, because the dispatch + // preceded the read. Both phases now see the same pre-image from the same + // single read. const beforeSeen: Array = []; const afterSeen: Array = []; const { engine } = await boot([ @@ -316,42 +325,50 @@ describe('[#5284] `beforeUpdate` is not a reader of THIS read', () => { const row: any = await engine.insert('scope_task_a', { title: 'A', status: 'todo', done: false }); await engine.update('scope_task_a', { status: 'in_progress' }, { where: { id: row.id } } as any); - expect(beforeSeen).toEqual([undefined]); - expect(afterSeen).toEqual([{ id: row.id, title: 'A', status: 'todo', done: false }]); + const stored = { id: row.id, title: 'A', status: 'todo', done: false }; + expect(beforeSeen).toEqual([stored]); + expect(afterSeen).toEqual([stored]); }); - it('rejects a `previous.*` beforeUpdate condition whether or not the prior row was read', async () => { - // Both configurations reject identically, which is the point: on an engine - // with no other producer of `previous`, the verdict is decided by the update - // path's dispatch ORDER, not by this gate. So narrowing the gate creates no - // new rejection. (Under a kernel the builtin binds `previous` first and the - // same condition evaluates — see the block comment above.) - const withRead = await boot([ + it('EVALUATES a `previous.*` beforeUpdate condition instead of rejecting it', async () => { + // The inverse of what this case pinned before, and the measured harm #5574 + // is about, one path over: a legal, contract-shaped transition condition on + // a `beforeUpdate` hook used to REJECT the write (unevaluable ⇒ abort, + // #4775) on a bare engine, because nothing bound `previous` in the before + // phase. The gate now counts the event, so the read happens for the + // condition's sake and the condition evaluates as authored. + const withAfterHook = await boot([ { name: 'pre_cond', object: 'scope_task_a', events: ['beforeUpdate'], priority: 90, condition: 'previous.done != true && record.done == true', handler: () => {} } as unknown as Hook, - observer('post', 'scope_task_a', 'afterUpdate', []), // forces the prior read + observer('post', 'scope_task_a', 'afterUpdate', []), ]); - const rowA: any = await withRead.engine.insert('scope_task_a', { title: 'A', status: 'todo', done: false }); + const rowA: any = await withAfterHook.engine.insert('scope_task_a', { title: 'A', status: 'todo', done: false }); await expect( - withRead.engine.update('scope_task_a', { done: true }, { where: { id: rowA.id } } as any), - ).rejects.toThrow(/previous/); + withAfterHook.engine.update('scope_task_a', { done: true }, { where: { id: rowA.id } } as any), + ).resolves.toBeDefined(); - const withoutRead = await boot([ + // …and with NO after-hook anywhere, so the before-hook is the sole term + // holding the gate open. Same verdict — which is the whole point of adding + // the term rather than relying on some neighbouring consumer. + const beforeHookOnly = await boot([ { name: 'pre_cond', object: 'scope_task_a', events: ['beforeUpdate'], priority: 90, condition: 'previous.done != true && record.done == true', handler: () => {} } as unknown as Hook, ]); - const rowB: any = await withoutRead.engine.insert('scope_task_a', { title: 'A', status: 'todo', done: false }); + const rowB: any = await beforeHookOnly.engine.insert('scope_task_a', { title: 'A', status: 'todo', done: false }); await expect( - withoutRead.engine.update('scope_task_a', { done: true }, { where: { id: rowB.id } } as any), - ).rejects.toThrow(/previous/); + beforeHookOnly.engine.update('scope_task_a', { done: true }, { where: { id: rowB.id } } as any), + ).resolves.toBeDefined(); }); - it('leaves a `previous` supplied by an earlier before-hook alone — and still reads nothing', async () => { - // The kernel's `sys_fetch_previous_update` shape: a low-priority (= earlier) - // `beforeUpdate` hook that fetches the row itself and assigns - // `ctx.previous`. The narrowed gate must not fight it — neither by clobber- - // ing the binding (`priorRecord` is null, and the post-write assignment is - // guarded on it) nor by re-reading a row someone already has. + it('makes the retired builtin\'s own fetch the redundant one (#5846)', async () => { + // The kernel's `sys_fetch_previous_update` shape, replayed as an authored + // hook: a low-priority (= earlier) `beforeUpdate` hook that fetches the row + // itself behind `if (input.id && !ctx.previous)`. The engine now binds + // `previous` BEFORE any before-hook runs, so that guard is false and the + // hook's `findOne` never happens — which is exactly the argument for + // deleting the builtin rather than leaving it behind a guard that can no + // longer be true. Measured here as a read count, because "the guard is + // false now" is the kind of claim that rots silently. const supplied: Array = []; const { engine, reads } = await boot([ { name: 'fetch_previous', object: 'scope_task_a', events: ['beforeUpdate'], priority: 5, @@ -374,9 +391,11 @@ describe('[#5284] `beforeUpdate` is not a reader of THIS read', () => { const before = reads.findOne; await engine.update('scope_task_a', { done: true }, { where: { id: row.id } } as any); - // The condition evaluated (the handler ran) against the hook-supplied row… + // The condition evaluated (the handler ran) against the ENGINE-supplied row… expect(supplied).toEqual([{ id: row.id, title: 'A', status: 'todo', done: false }]); - // …and the engine added no read of its own: exactly the ONE the hook made. + // …and there was exactly ONE read for it: the engine's. The builtin-shaped + // hook's guard short-circuited, so it issued none — where before this + // change the count was one EACH. expect(reads.findOne - before).toBe(1); }); }); diff --git a/packages/objectql/src/engine.test.ts b/packages/objectql/src/engine.test.ts index b9cafe5c55..3143dda8bb 100644 --- a/packages/objectql/src/engine.test.ts +++ b/packages/objectql/src/engine.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { ObjectQL } from './engine'; import { SchemaRegistry } from './registry'; +import { HookTargetRebindError, HOOK_TARGET_REBIND_ERROR_CODE } from './hook-target-rebind-errors'; import type { IDataDriver } from '@objectstack/spec/contracts'; // Mock the SchemaRegistry to avoid side effects between tests. @@ -1086,19 +1087,43 @@ describe('ObjectQL Engine', () => { expect(ast.where).toEqual({ $and: [{ id: { $in: ['a', 'b'] } }, { owner_id: 'u1' }] }); }); - it('fails CLOSED (throws) if a hook clears the target id so the multi branch runs without a seeded ast', async () => { - // The only way to reach the multi branch with no seeded ast is a - // beforeUpdate hook clearing input.id after a truthy-id seed skip. - // The old `?? { object, where }` fallback would have silently - // rebuilt an UNSCOPED predicate; we now refuse it. + it('REFUSES a hook that clears the target id — the reroute lever is retired (#5574)', async () => { + // ⚠️ This case is the successor of "fails CLOSED (throws) if a hook + // clears the target id so the multi branch runs without a seeded + // ast". Clearing `input.id` in a `beforeUpdate` handler used to + // CONVERT a by-id update into a predicate update over the caller's + // `where` — the engine re-read `hookContext.input.id` to pick the + // branch — and the only thing standing between that and an + // UNSCOPED bulk write was #2982's fail-closed AST assertion, which + // fired because the by-id path had skipped the seed. + // + // ADR-0058 Addendum II resolves the dispatch ladder BEFORE the + // before phase (it has to: a per-row `before*` context is built + // from the matched row set), so there is no branch left to + // re-enter. The lever is refused by name rather than caught one + // layer down by a security backstop that was never about it. engine.registerHook('beforeUpdate', async (ctx: any) => { - ctx.input.id = undefined; // force the multi branch, no seeded ast + ctx.input.id = undefined; }); - await expect( - engine.update('task', { id: 't1', status: 'done' }, { multi: true } as any), - ).rejects.toThrow(/row-scoping AST was not seeded/); + const err = await engine + .update('task', { id: 't1', status: 'done' }, { multi: true } as any) + .then(() => null, (e) => e); + + expect(err).toBeInstanceOf(HookTargetRebindError); + expect(err.code).toBe(HOOK_TARGET_REBIND_ERROR_CODE); + expect(err.path).toBe('by-id'); + expect(err.event).toBe('beforeUpdate'); + expect(err.expectedId).toBe('t1'); + expect(err.observedId).toBeUndefined(); + // The retired capability is NAMED, so an author whose handler + // stopped working learns what changed instead of guessing. + expect(err.message).toContain('CLEARED'); + expect(err.message).toContain('RETIRED'); + expect(err.message).toContain('PREDICATE write'); + // Nothing was written, on either branch. expect((mockDriver as any).updateMany).not.toHaveBeenCalled(); + expect(mockDriver.update).not.toHaveBeenCalled(); }); }); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 1f87bbd4f2..74456a1b53 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -25,6 +25,10 @@ import { parseAutonumberFormat, renderAutonumber, missingFieldValues, isTenancyD // [#5158] Door 2's lowering sink — the SAME pair the protocol face (Door 1) // runs, so `FilterArray` has exactly one lowering in the product. import { isFilterAST, parseFilterAST, VALID_AST_OPERATORS } from '@objectstack/spec/data'; +// [#5574] D6, executable. The ceiling and the refusal message live in +// `packages/spec/src/data/bulk-write-hook-conformance.ts` so BOTH phases and +// both verbs enforce one definition; the engine raises, the contract decides. +import { MAX_BULK_PER_ROW_HOOK_ROWS, resolveBulkPerRowHookBudget } from '@objectstack/spec/data'; import { assertListComparandShapes } from './filter-comparand-shape.js'; // Seek pagination for the walks that must read EVERY row — the autonumber seed // scan is one (#6249). Shared with `summary-backfill` rather than re-rolled: @@ -63,6 +67,7 @@ import { type SummaryDescriptor, } from './summary-aggregate.js'; import { ReadonlyFieldRejectedError } from './readonly-strict-errors.js'; +import { HookTargetRebindError } from './hook-target-rebind-errors.js'; import { DriverConnectError, DatasourceUnavailableError, @@ -1662,35 +1667,133 @@ export class ObjectQL implements IObjectQLEngine { } /** - * [#5038] Ceiling on the matched-row set a predicate write fires per-row - * after-hooks over. + * [#5574] The per-row `before*` dispatch of a predicate (`multi: true`) + * write — ADR-0058 Addendum II, clauses D1–D4. * - * The consequence ADR-0058's addendum told this implementation to price: an - * after-hook that used to run once per batch now runs once per row, so a + * ## D1/D2 — one dispatch per matched row, on the SINGLE-RECORD shape + * + * `input.id` names the row, `previous` is that row's pre-image, and + * `input.options` is still the CALLER's bag (the PHASE rule in `hook.zod.ts` + * is unchanged: the `before*` phase reads the pre-merge view, `where` and + * `multi` visible). `result` stays ABSENT — the before phase has no + * post-state, and a value assigned to `ctx.result` here is overwritten by the + * write's own result before any `after*` handler could see it. + * + * Zero matched rows is zero dispatches, and `[]` is meaningful: a batch that + * changed nothing is not a record change. The caller checks that BEFORE + * calling in, so an empty row set never reaches this loop. + * + * ## D3 — the payload is BATCH-scoped, and that IS the merge rule + * + * Every per-row context carries THE payload, not a copy — deliberately + * unlike {@link buildPerRowAfterContexts}, which copies because an + * after-handler's mutation has nowhere legitimate to go. There is exactly one + * payload for a predicate update (`driver.updateMany` takes one SET clause + * for N rows), so: + * + * - a rewrite takes effect on the WHOLE batch, whichever row's dispatch made + * it, and rewrites ACCUMULATE across the N dispatches in dispatch order; + * - N post-hook payloads cannot diverge, so nothing is reconciled, no + * payload is discarded, and no predicate write is ever split into N + * single-row writes. + * + * Two ways a handler can write the payload and both must accumulate: mutating + * it in place (`ctx.input.data.x = 1`) needs no help, while REPLACING it + * (`ctx.input.data = {…}`) would otherwise be lost with the row context that + * held it. So each row reads the batch payload fresh and writes it back after + * its dispatch — that write-back is what makes "accumulate in dispatch order" + * true for both spellings rather than only the first. + * + * A rewrite CONDITIONED on the row (`ctx.previous`, `ctx.input.id`) is + * outside the contract: it does not scope itself to the row it was decided + * on, it widens to every matched row. Per-row `previous` is supplied so a + * guard can REFUSE the write (throw), not so a rewrite can be aimed. That is + * a contract statement, not an enforcement — no static rule can decide + * whether a rewrite is row-invariant — and the ADR names it as such rather + * than hiding it. + * + * ## D4 — `input.id` is not a reroute lever here + * + * On the old batch dispatch it was: `input.id` was present-but-`undefined`, + * and binding it moved the write onto the single-id branch. A per-row context + * arrives with `id` already bound and the dispatch already decided, so + * rebinding retargets nothing — and is refused rather than ignored. + */ + private async dispatchPerRowBeforeHooks( + object: string, + event: 'beforeUpdate' | 'beforeDelete', + rows: Record[], + batchCtx: HookContext, + ): Promise { + const schema = this._registry.getObject(object); + const carriesPayload = event === 'beforeUpdate'; + for (const row of rows) { + const rowId = (row as { id?: unknown }).id; + const options = (batchCtx.input as { options?: unknown }).options; + const rowCtx = { + ...batchCtx, + event, + // D3: THE payload, read fresh so a previous row's REPLACEMENT is what + // this row sees. Never a copy. + input: carriesPayload + ? { id: rowId, data: (batchCtx.input as { data?: unknown }).data, options } + : { id: rowId, options }, + previous: coerceBooleanFields(schema as any, row as any), + // D2: no post-state in the before phase. + result: undefined, + } as unknown as HookContext; + + await this.triggerHooks(event, rowCtx); + + // D3, the accumulate half — see the class doc above. + if (carriesPayload) { + (batchCtx.input as { data?: unknown }).data = (rowCtx.input as { data?: unknown }).data; + } + // D4. + const observed = (rowCtx.input as { id?: unknown }).id; + if (observed !== rowId) { + throw new HookTargetRebindError({ + object, event, path: 'per-row', expectedId: rowId, observedId: observed, + }); + } + } + } + + /** + * [#5038, one ceiling for both phases since #5574] Ceiling on the matched-row + * set a predicate write fires per-row hooks over. + * + * The consequence ADR-0058's addendum told this implementation to price: a + * hook that used to run once per batch now runs once per row, so a * notification hook sends N messages and a cache-invalidation hook runs N * times. Unbounded, a single `multi: true` update matching a whole table * turns into an unbounded fan-out of handler executions inside one write. * - * Exceeding it REJECTS the write, before `updateMany`/`deleteMany` runs, so - * nothing is written. The alternative — quietly falling back to firing once - * for the batch — is the silent degradation this whole family exists to - * abolish (#4649/#4775): the hooks would not fire for N-1 rows and nothing - * would say so. The rejection names the count, the ceiling and both routes - * out (narrow the predicate, or drop the after-hook). + * Exceeding it REJECTS the write, before the FIRST per-row dispatch and + * before `updateMany`/`deleteMany` runs, so nothing is written and no handler + * ran for a batch that was going to be refused anyway. The alternative — + * quietly falling back to firing once for the batch — is the silent + * degradation this whole family exists to abolish (#4649/#4775): the hooks + * would not fire for N-1 rows and nothing would say so. + * + * [#5574] The rule itself is `resolveBulkPerRowHookBudget` in + * `packages/spec/src/data/bulk-write-hook-conformance.ts` (D6), not a copy + * kept in step by a pin. This method is the RAISING half only: the contract + * is pure and total (no clock, no I/O, no throw), the engine turns a + * `refused` verdict into the thrown error. Splitting it that way is what lets + * `before*` and `after*`, update and delete, share one ceiling and one + * message with nothing to keep synchronised. */ private assertBulkPerRowHookBudget(object: string, event: string, matched: number): void { - if (matched <= ObjectQL.MAX_BULK_PER_ROW_HOOK_ROWS) return; - throw Object.assign( - new Error( - `Refusing the bulk write on '${object}': it matches ${matched} rows, and '${event}' hooks are ` + - `contracted to fire PER ROW on a predicate write (ADR-0058, bulk-write addendum), which is ` + - `over the ${ObjectQL.MAX_BULK_PER_ROW_HOOK_ROWS}-row ceiling for one write. Nothing was written. ` + - `Narrow the predicate so the batch matches fewer rows (paginate the write), or remove the ` + - `'${event}' hook from this object. The write is NOT silently downgraded to one hook call for ` + - `the batch — that would skip the hook for ${matched - 1} rows without saying so.`, - ), - { code: 'ERR_BULK_PER_ROW_HOOK_LIMIT', object, event, matched, limit: ObjectQL.MAX_BULK_PER_ROW_HOOK_ROWS }, - ); + const verdict = resolveBulkPerRowHookBudget({ object, event, matched }); + if (verdict.kind === 'ok') return; + throw Object.assign(new Error(verdict.message), { + code: verdict.code, + object: verdict.object, + event: verdict.event, + matched: verdict.matched, + limit: verdict.limit, + }); } // ======================================== @@ -4538,11 +4641,17 @@ export class ObjectQL implements IObjectQLEngine { private static readonly MAX_EXPAND_DEPTH = 3; private static readonly MAX_CASCADE_DEPTH = 10; /** - * [#5038] Most rows one predicate write may fire per-row after-hooks over. + * [#5038] Most rows one predicate write may fire per-row hooks over — in + * BOTH phases since #5574 (ADR-0058 Addendum II, D6). + * * Public so a test — and an operator reading a rejection — can name the same - * number the engine enforces. See `assertBulkPerRowHookBudget`. + * number the engine enforces. It is now a RE-EXPORT of the spec contract's + * `MAX_BULK_PER_ROW_HOOK_ROWS`, not a second literal: until #5574's engine + * half the same number was written down twice and only a pin in + * `bulk-write-hook-conformance.test.ts` kept the two agreeing. See + * `assertBulkPerRowHookBudget`. */ - public static readonly MAX_BULK_PER_ROW_HOOK_ROWS = 10_000; + public static readonly MAX_BULK_PER_ROW_HOOK_ROWS = MAX_BULK_PER_ROW_HOOK_ROWS; /** In-memory next-value cache per `object.field` for autonumber generation, * lazily seeded from the current max in the store. */ private readonly autonumberCounters = new Map(); @@ -6278,7 +6387,166 @@ export class ObjectQL implements IObjectQLEngine { transaction: opCtx.context?.transaction, ql: this }; - await this.triggerHooks('beforeUpdate', hookContext); + + // ──────────────────────────────────────────────────────────────────── + // [#5574 / #5846] The dispatch ladder is resolved BEFORE the before + // phase, and the before phase is dispatched PER MATCHED ROW. + // + // ADR-0058 Addendum II (ruling B, 2026-08-06) is what forces the + // reorder rather than merely permitting it: a per-row `before*` context + // is BUILT from the matched row set, so the row set has to be in hand + // before the first dispatch, so the branch that decides whether there IS + // a row set has to be decided before that. #5846's (a) direction lands + // in the same edit — the by-id path reads its prior row ahead of the + // dispatch and binds `previous` there, exactly as `delete()` has since + // #5272 — because the before phase becomes a real reader of that read on + // BOTH paths, which is the one thing #5284's gate comment said it was + // not. + // + // The lever that reorder retires is named and refused rather than + // silently dropped: see `HookTargetRebindError`. + // + // Keyed on the SAME falsy-`id` test the #2982 AST seed above uses, so + // seed, ladder and branch cannot disagree. + const isByIdWrite = Boolean(id); + const isPredicatePath = !isByIdWrite && Boolean(options?.multi) && typeof driver.updateMany === 'function'; + if (!isByIdWrite && !isPredicatePath) { + // [#5480] The `reject` verdict of resolveEngineUpdateDispatch. It + // used to be re-asked AFTER the before phase, because a hook could + // still bind the id and convert the call into a by-id write; with + // the ladder resolved first there is no such conversion, so the + // refusal lands where it costs least — before any handler runs and + // before anything is read. + throw new Error(ENGINE_UPDATE_REJECT_MESSAGE); + } + + const updateSchema = this._registry.getObject(object); + // Pre-update snapshot. Exposed to hooks via `hookContext.previous` in + // BOTH phases now (the HookContext contract documents `previous` for + // update/delete) and reused for object-level validation rules and the + // roll-up recompute. Fetched once, only for single-id updates, and only + // when something on THIS object actually consumes it — see + // `wantsPriorRecord` below. + let priorRecord: Record | null = null; + // [#5038] The matched rows a PREDICATE write fires its per-row + // `afterUpdate` contexts over. `[]` is meaningful and distinct from + // `null`: zero matched rows is zero record changes, hence zero hook + // calls. + let bulkPerRowRows: Record[] | null = null; + + if (isByIdWrite) { + // [#5284] Demand-driven, and the demand is asked PER OBJECT — see + // the long-form reasoning at the by-id branch below, which still + // holds for every term. What changed with #5574/#5846 is the one + // term that comment singled out as deliberately ABSENT: + // `beforeUpdate` is now a real reader of this read, because the + // read happens before the dispatch and binds `previous` for it. So + // it joins the gate, and the gate stops being narrower than + // `delete()`'s twin (#5272) for no reason anyone could state. + // + // This is not a NEW read where a kernel is concerned — it is the + // same one, moved and deduplicated. `sys_fetch_previous_update` + // (`plugin.ts`, `object: '*'`, priority 5) used to make its own + // `ql.findOne` on every by-id update to bind exactly this value; + // #5846 retires it, because the engine now binds `previous` before + // any authored before-hook runs. + const wantsPriorRecord = + needsPriorRecord(updateSchema as any) || + this.hasHooksFor('beforeUpdate', object) || + this.hasHooksFor('afterUpdate', object) || + this.getSummaryDescriptors(object).length > 0; + if (wantsPriorRecord) { + // `buildDriverOptions` is what carries the open transaction and + // the tenant scope onto a raw driver read — the same bag the + // post-phase write uses, built here because the write's own + // merge has not happened yet. `delete()`'s pre-image read does + // the same for the same reason. + const priorAst: QueryAST = { object, where: { id }, limit: 1 }; + const preOpts = this.buildDriverOptions(object, opCtx.context, hookContext.input.options as any); + priorRecord = await driver.findOne(object, priorAst, preOpts); + // Never fabricate: a row that is not there leaves `previous` + // UNBOUND rather than `{}`/`null`, so a condition reading it + // faults loudly instead of answering for a record nobody read + // (#4649/#4775) — `delete()`'s `bindPreImage` rule, verbatim. + if (priorRecord) hookContext.previous = coerceBooleanFields(updateSchema as any, priorRecord as any) as any; + } + await this.triggerHooks('beforeUpdate', hookContext); + // The retired lever, refused. Everything above — `previous`, and + // below it the `readonlyWhen` strip and every validation rule — was + // computed against the row the ladder chose. + if (hookContext.input.id !== id) { + throw new HookTargetRebindError({ + object, event: 'beforeUpdate', path: 'by-id', + expectedId: id, observedId: hookContext.input.id, + }); + } + } + + // [#3106/#3042/#5038/#5574] D7 — ONE read of the matched row set per + // predicate write, serving four consumers: per-row validation rules, + // the `readonlyWhen` strip, the per-row `before*` dispatch and the + // per-row `after*` dispatch. The ruling forbids a second fetch in as + // many words, so the read is a MEMO rather than a call at each + // consumer's site: the before phase needs it earliest (its contexts are + // built from it), the strip gate can only be asked of the POST-hook + // payload, and a memo is what lets both be true without the read + // happening twice or being hoisted past the gate that decides it is + // needed at all. + let priorRows: Record[] | null = null; + let priorRowsRead = false; + let readPriorRows: () => Promise[] | null> = async () => null; + + if (isPredicatePath) { + // [#2982] Consume the middleware-composed AST seeded above, so the + // injected row-scoping (RLS write filter, sharing's editable-rows + // filter) actually binds every read and write on this path — the + // per-row hook dispatch included, which is why the check moved here + // from the driver call. Fail CLOSED if it is somehow absent: + // rebuilding `{ object, where }` would silently drop every composed + // filter and reopen the unscoped-bulk-write hole this fix closed + // (AGENTS.md PD #12). + const ast = opCtx.ast; + if (!ast) { + throw new Error( + `[Security] Refusing bulk update on '${object}': row-scoping AST was not seeded ` + + `(the predicate branch was reached without the #2982 seed).`, + ); + } + const preOpts = this.buildDriverOptions(object, opCtx.context, hookContext.input.options as any); + readPriorRows = async () => { + if (!priorRowsRead) { + priorRowsRead = true; + priorRows = (await driver.find(object, ast, preOpts) as Record[]) ?? []; + } + return priorRows; + }; + + // The demand is uniform across hooks and asked PER OBJECT: it is + // NOT keyed on whether any condition mentions `previous`, which the + // ruling rejected explicitly as a hidden rule that makes a hook's + // firing count depend on its condition text. + const perRowBeforeHooks = this.hasHooksFor('beforeUpdate', object); + const perRowAfterHooks = this.hasHooksFor('afterUpdate', object); + if (perRowBeforeHooks || perRowAfterHooks) { + const rows = (await readPriorRows()) ?? []; + // [D6] ONE ceiling for BOTH phases, checked BEFORE the first + // per-row dispatch and before the driver call — so an + // over-ceiling batch runs zero handlers and writes nothing, + // rather than running 10 001 of them and then throwing. Named + // for the phase that will dispatch first, so the operator is + // told which hook to narrow or drop. + this.assertBulkPerRowHookBudget( + object, perRowBeforeHooks ? 'beforeUpdate' : 'afterUpdate', rows.length, + ); + if (perRowAfterHooks) bulkPerRowRows = rows; + // [D1] Zero matched rows is zero dispatches — a batch that + // changed nothing is not a record change. + if (perRowBeforeHooks && rows.length > 0) { + await this.dispatchPerRowBeforeHooks(object, 'beforeUpdate', rows, hookContext); + } + } + } + hookContext.input.options = this.buildDriverOptions(object, opCtx.context, hookContext.input.options as any); try { @@ -6291,41 +6559,18 @@ export class ObjectQL implements IObjectQLEngine { // driver returning a row from `updateMany` would silently reroute a // bulk write onto the per-record contract if we inferred it. let isPredicateWrite = false; - // Pre-update snapshot. Exposed to after-hooks via `hookContext.previous` - // (the HookContext contract documents `previous` for update/delete) and - // reused for object-level validation rules and the roll-up recompute. - // Fetched once, only for single-id updates, and only when something on - // THIS object actually consumes it — see `wantsPriorRecord` below. - // Binding `previous` is what makes record-change flow triggers work: - // their start-condition gate reads `previous.*` (e.g. `status == "done" - // && previous.status != "done"`), which fails when `previous` is absent. - // - // [#4784] It is ALSO what supplies the `previous` binding to a - // declarative hook `condition` (`hook-wrappers.ts`), which is how a - // TRANSITION is expressed there: `previous.done != true && - // record.done == true`. That needs NO second demand-driven fetch: the - // gate fetches whenever this object has an afterUpdate hook, and - // afterUpdate is the event whose context carries `previous`. Adding a - // "does the condition reference `previous`?" analysis on top would - // still be dead code — the demand is uniform across after-hooks, which - // #5038 records as a ruling (a hook's cost must not depend on its - // condition text). - let priorRecord: Record | null = null; - // [#5038] The matched rows a PREDICATE write fires its per-row - // `afterUpdate` contexts over — set only when this object actually - // has `afterUpdate` hooks, so a bulk write with none pays for no - // read and keeps its single (no-op) batch dispatch. `[]` is - // meaningful and distinct from `null`: zero matched rows is zero - // record changes, hence zero hook calls. - let bulkPerRowRows: Record[] | null = null; - const updateSchema = this._registry.getObject(object); const mediaValueShapeStrict = await this.mediaValueShapeStrictFor(updateSchema); const valueShapeStrict = await this.valueShapeStrictFor(updateSchema); const updateMsgCtx = this.validationMessageContext(object, opCtx.context); // [#4769] See the insert path — an update admits values on the same // terms, so it owes the same counterexample. const onAdmittedValueShapeViolation = this.admittedViolationSink(object); - if (hookContext.input.id) { + // [#5574] Branch on the LADDER, not on `hookContext.input.id`. The + // two used to be the same question asked twice, which is exactly + // what made the id slot a reroute lever; the ladder was resolved + // before the before phase above and a handler's attempt to move it + // has already been refused. + if (isByIdWrite) { // [#6435] The by-id half of #6262's strip — same defect, other // arm. Reaching this branch means the dispatch found a truthy // scalar id, but NOT necessarily in the payload: when `data.id` @@ -6448,36 +6693,26 @@ export class ObjectQL implements IObjectQLEngine { // a repointed child is only saved by some other object having // an afterUpdate hook. // - // `beforeUpdate` is deliberately NOT counted, and that is the one - // place this gate does NOT mirror `delete()`'s (#5272). The two - // paths order the read differently: `delete()` reads the pre-image - // BEFORE dispatching `beforeDelete` and binds it there, so a - // before-phase hook is a real reader of THAT read. `update()` - // dispatches `beforeUpdate` first (it may still rewrite the very - // payload this read would be compared against) and binds - // `hookContext.previous` only after the write — so no - // `beforeUpdate` hook can observe this row however the gate is - // written, and counting the event here would buy a read with no - // reader. + // [#5574 / #5846] `beforeUpdate` USED to be deliberately absent + // from this gate, and that was the one place it did not mirror + // `delete()`'s twin (#5272). The reason given was ordering: this + // path dispatched `beforeUpdate` first and bound + // `hookContext.previous` only after the write, so no + // `beforeUpdate` hook could observe this row however the gate was + // written, and counting the event would have bought a read with + // no reader. ADR-0058 Addendum II reversed the ordering, so the + // reader now exists — the read and the binding happen ABOVE, in + // the pre-phase, and `wantsPriorRecord` there counts + // `beforeUpdate` alongside `afterUpdate`. + // + // What that also retired: the `sys_fetch_previous_update` + // builtin (`plugin.ts`, `object: '*'`, priority 5) used to be the + // only producer of `previous` for the before phase, making its + // own `ql.findOne` on every by-id update. With the engine binding + // it first the builtin's `!ctx.previous` guard is permanently + // short-circuited, so #5846 removes it rather than leaving a + // second read behind a guard that can no longer be false. // - // What a kernel-hosted `beforeUpdate` hook DOES see comes from a - // different producer entirely: the `sys_fetch_previous_update` - // builtin (`plugin.ts`, `object: '*'`, priority 5) makes its own - // `findOne` and assigns `hookContext.previous` before any authored - // before-hook runs. That read is untouched by this gate — and it - // is why narrowing here cannot take a binding away from the before - // phase. It is also a duplicate of this one (plugin-audit's - // `captureBefore` makes a third): three reads of one row, filed - // as #5846 with the delete-side time-ordering (#5272) as the fix - // shape — not something to paper over by widening this gate. - const wantsPriorRecord = - needsPriorRecord(updateSchema as any) || - this.hasHooksFor('afterUpdate', object) || - this.getSummaryDescriptors(object).length > 0; - if (wantsPriorRecord) { - const priorAst: QueryAST = { object, where: { id: hookContext.input.id }, limit: 1 }; - priorRecord = await driver.findOne(object, priorAst, hookContext.input.options as any); - } // B2: drop writes to fields locked by a TRUE `readonlyWhen` — the // field is read-only for this record's state, so the incoming // change is ignored (the persisted value is kept). @@ -6544,7 +6779,7 @@ export class ObjectQL implements IObjectQLEngine { opCtx.data as Record, opCtx.context, updateMsgCtx, ); result = await driver.update(object, hookContext.input.id as string, hookContext.input.data as Record, hookContext.input.options as any); - } else if (options?.multi && driver.updateMany) { + } else { // [#6262] A bulk SET clause must not carry `id`. Reaching this // branch AT ALL means `resolveEngineUpdateDispatch` returned // `multi`, i.e. it found no scalar truthy id in EITHER source — @@ -6599,20 +6834,11 @@ export class ObjectQL implements IObjectQLEngine { await this.encryptSecretFields(object, hookContext.input.data as Record, opCtx.context, hookContext.input.options); normalizeMultiValueFields(updateSchema, hookContext.input.data as Record); validateRecord(updateSchema, hookContext.input.data as Record, 'update', { mediaValueShapeStrict, valueShapeStrict, messages: updateMsgCtx, onAdmittedValueShapeViolation }); - // [#2982] Consume the middleware-composed AST seeded above, so - // the injected row-scoping (RLS write filter, sharing's - // editable-rows filter) actually binds the driver operation. Fail - // CLOSED if it is somehow absent — rebuilding `{ object, where }` - // here would silently drop every composed filter and reopen the - // unscoped-bulk-write hole this fix closes (AGENTS.md PD #12: no - // lenient fallback that tolerates the broken invariant). - const ast = opCtx.ast; - if (!ast) { - throw new Error( - `[Security] Refusing bulk update on '${object}': row-scoping AST was not seeded ` + - `(a hook cleared the target id after the security filter was composed).`, - ); - } + // [#2982] The middleware-composed AST — asserted present and + // bound to the memoized row read in the pre-phase above, so the + // injected row-scoping (RLS write filter, sharing's + // editable-rows filter) binds every read AND the write. + const ast = opCtx.ast!; // [#3106] Validation rules, `requiredWhen` and per-option // `visibleWhen` are PER ROW on a bulk update, exactly like the // `readonlyWhen` strip below: one payload, N prior states. Read @@ -6623,27 +6849,18 @@ export class ObjectQL implements IObjectQLEngine { // it), so a rule-free schema still pays nothing here. const rulesNeedRows = needsPriorRecord(updateSchema as any); const payloadHasReadonlyWhen = hasReadonlyWhenInPayload(updateSchema as any, hookContext.input.data as Record); - // [#5038] The THIRD demand on that same read: after-hooks are - // contracted to fire PER ROW on a predicate write (ADR-0058, - // bulk-write addendum), and a per-row context needs the row's - // pre-image for `previous`. Folded into the existing gate on - // purpose — one `driver.find` serves validation, the - // `readonlyWhen` strip AND the hook dispatch, which is the - // issue's performance guardrail ("行集读取一次完成,求值批内复用") - // stated as code. The demand is uniform across after-hooks: it - // is NOT keyed on whether any condition mentions `previous`, - // which the ruling rejected explicitly as a hidden rule that - // makes a hook's firing count depend on its condition text. - const perRowAfterHooks = this.hasHooksFor('afterUpdate', object); - let priorRows: Record[] | null = null; - if (rulesNeedRows || payloadHasReadonlyWhen || perRowAfterHooks) { - priorRows = await driver.find(object, ast, hookContext.input.options as any) as Record[]; - } - if (perRowAfterHooks) { - // Refuse an unbounded fan-out BEFORE the write, so a batch - // over the ceiling changes nothing at all. - this.assertBulkPerRowHookBudget(object, 'afterUpdate', priorRows?.length ?? 0); - bulkPerRowRows = priorRows ?? []; + // [#5038/#5574] The hook phases are the third and fourth demands + // on that same read, and they were already resolved in the + // pre-phase above (they have to be — a per-row `before*` context + // is built from these very rows). What is left here is the + // strip's and the rules' own demand, asked of the POST-hook + // payload, which is why this call site survives at all. It is + // the SAME read: `readPriorRows` is a memo, so one `driver.find` + // serves validation, the `readonlyWhen` strip and BOTH hook + // phases — D7's one-read rule, and the issue's performance + // guardrail ("行集读取一次完成,求值批内复用"), stated as code. + if (rulesNeedRows || payloadHasReadonlyWhen) { + priorRows = await readPriorRows(); } // [#3042] Enforce conditional `readonlyWhen` on the bulk path too. // Unlike static `readonly` (below), a `readonlyWhen` lock is PER @@ -6736,15 +6953,9 @@ export class ObjectQL implements IObjectQLEngine { updateSchema, hookContext.input.data as Record, opCtx.data as Record, opCtx.context, updateMsgCtx, ); - result = await driver.updateMany(object, ast, hookContext.input.data as Record, hookContext.input.options as any); + // `updateMany` presence is part of the ladder verdict resolved above. + result = await driver.updateMany!(object, ast, hookContext.input.data as Record, hookContext.input.options as any); isPredicateWrite = true; - } else { - // [#5480] The `reject` verdict of resolveEngineUpdateDispatch, - // re-asked here because a beforeUpdate hook may have cleared the - // id since — the same shape delete()'s branch below carries, and - // the reason the wording lives in one exported constant either - // way. - throw new Error(ENGINE_UPDATE_REJECT_MESSAGE); } hookContext.event = 'afterUpdate'; @@ -7055,6 +7266,11 @@ export class ObjectQL implements IObjectQLEngine { // one that has nothing else to look at (its `input` carries an id and // no data), and the pre-image has to be taken before the row is gone // either way, so a single read serves both phases. + // + // [#5574] The same read now serves the PER-ROW `beforeDelete` dispatch on + // the predicate path — see the pre-phase below. `delete()` was already + // the right shape here; what changed is that the predicate branch grew + // the same discipline the by-id branch has had since #5272. const deleteSchema = this._registry.getObject(object); const wantsPreImage = this.hasHooksFor('beforeDelete', object) || @@ -7076,24 +7292,75 @@ export class ObjectQL implements IObjectQLEngine { hookContext.previous = row ? (coerceBooleanFields(deleteSchema as any, row as any) as any) : undefined; }; let priorRecord: Record | null = null; - if (id && wantsPreImage) { - priorRecord = await readPreImage(id); - bindPreImage(priorRecord); + // [#5038] Matched rows for the per-row `afterDelete` dispatch — see the + // twin in update(). A bulk delete is N record changes too, so a + // `record-after-delete` flow must see each deleted row rather than one + // context that names none of them. + let bulkPerRowRows: Record[] | null = null; + + // [#5574] The dispatch ladder, resolved BEFORE the before phase — see + // update()'s twin for the full reasoning. Keyed on the SAME falsy-`id` + // test the #2982 AST seed above uses. + const isByIdDelete = Boolean(id); + const isPredicatePath = !isByIdDelete && Boolean(options?.multi) && typeof driver.deleteMany === 'function'; + if (!isByIdDelete && !isPredicatePath) { + // [#4550] The `reject` verdict of resolveEngineDeleteDispatch. It used + // to be re-asked after the before phase because a hook could still bind + // the id; with the ladder resolved first there is no such conversion. + throw new Error(ENGINE_DELETE_REJECT_MESSAGE); } - await this.triggerHooks('beforeDelete', hookContext); - - // A `beforeDelete` hook may repoint the target id, or clear it (which - // #4550's re-asked dispatch verdict below already accounts for). The - // pre-image bound above describes the OLD id, so it must not ride into - // `afterDelete` — or into the summary recompute — as though it - // described the new target. A cleared id falls through to the predicate - // branch, whose batch-scoped dispatch must carry no single row's - // pre-image at all (`hook-wrappers` diagnoses that dispatch by the - // absence of both). - if (wantsPreImage && hookContext.input.id !== id) { - priorRecord = hookContext.input.id ? await readPreImage(hookContext.input.id) : null; - bindPreImage(priorRecord); + if (isByIdDelete) { + if (wantsPreImage) { + priorRecord = await readPreImage(id); + bindPreImage(priorRecord); + } + await this.triggerHooks('beforeDelete', hookContext); + // [#5574] The retired lever, refused. `previous` — and the summary + // recompute that rides it — were computed against the row the ladder + // chose, so a handler moving the id would delete a row none of that + // ever saw. It used to be honoured by RE-READING the pre-image for the + // new target (and, for a CLEARED id, by falling through to the + // predicate branch); ADR-0058 Addendum II's settlement of the same + // lever on `update()` applies here verbatim, and one rule beats two. + if (hookContext.input.id !== id) { + throw new HookTargetRebindError({ + object, event: 'beforeDelete', path: 'by-id', + expectedId: id, observedId: hookContext.input.id, + }); + } + } else { + // [#2982] Consume the middleware-composed AST seeded above so the + // injected row-scoping binds every read AND the delete. Fail CLOSED if + // it is absent rather than rebuilding an unscoped `{ object, where }` + // (AGENTS.md PD #12). + const ast = opCtx.ast; + if (!ast) { + throw new Error( + `[Security] Refusing bulk delete on '${object}': row-scoping AST was not seeded ` + + `(the predicate branch was reached without the #2982 seed).`, + ); + } + // [#5038/#5574] Read the doomed rows ONCE, before they are gone — the + // only moment their pre-image exists — and serve BOTH phases from it + // (D7). Gated on this object actually having delete-side hooks, so a + // bulk delete with none pays for no read. + const perRowBeforeHooks = this.hasHooksFor('beforeDelete', object); + const perRowAfterHooks = this.hasHooksFor('afterDelete', object); + if (perRowBeforeHooks || perRowAfterHooks) { + const preOpts = this.buildDriverOptions(object, opCtx.context, hookContext.input.options as any); + const doomed = (await driver.find(object, ast, preOpts) as Record[]) ?? []; + // [D6] One ceiling, both phases, BEFORE the first per-row dispatch + // and before the driver call. + this.assertBulkPerRowHookBudget( + object, perRowBeforeHooks ? 'beforeDelete' : 'afterDelete', doomed.length, + ); + if (perRowAfterHooks) bulkPerRowRows = doomed; + // [D1] Zero matched rows is zero dispatches. + if (perRowBeforeHooks && doomed.length > 0) { + await this.dispatchPerRowBeforeHooks(object, 'beforeDelete', doomed, hookContext); + } + } } hookContext.input.options = this.buildDriverOptions(object, opCtx.context, hookContext.input.options as any); @@ -7103,44 +7370,17 @@ export class ObjectQL implements IObjectQLEngine { // [#4639] See update()'s twin: recorded at the branch that chose the // driver call, not inferred later from a missing id. let isPredicateWrite = false; - // [#5038] Matched rows for the per-row `afterDelete` dispatch — see - // the twin in update(). A bulk delete is N record changes too, so a - // `record-after-delete` flow must see each deleted row rather than - // one context that names none of them. - let bulkPerRowRows: Record[] | null = null; - if (hookContext.input.id) { + if (isByIdDelete) { // Honor referential delete behavior (cascade/set_null/restrict) // for relations pointing at this record before removing it. await this.cascadeDeleteRelations(object, hookContext.input.id as string | number, opCtx.context); result = await driver.delete(object, hookContext.input.id as string, hookContext.input.options as any); - } else if (options?.multi && driver.deleteMany) { - // [#2982] Consume the middleware-composed AST seeded above so the - // injected row-scoping binds the bulk delete. Fail CLOSED if it - // is absent rather than rebuilding an unscoped `{ object, where }` - // (AGENTS.md PD #12). - const ast = opCtx.ast; - if (!ast) { - throw new Error( - `[Security] Refusing bulk delete on '${object}': row-scoping AST was not seeded ` + - `(a hook cleared the target id after the security filter was composed).`, - ); - } - // [#5038] Read the doomed rows ONCE, before they are gone — - // the only moment their pre-image exists. Gated on this object - // actually having `afterDelete` hooks, so a bulk delete with - // none pays for no read (this path did no read at all before). - if (this.hasHooksFor('afterDelete', object)) { - const doomed = await driver.find(object, ast, hookContext.input.options as any) as Record[]; - this.assertBulkPerRowHookBudget(object, 'afterDelete', doomed?.length ?? 0); - bulkPerRowRows = doomed ?? []; - } - result = await driver.deleteMany(object, ast, hookContext.input.options as any); - isPredicateWrite = true; } else { - // The `reject` verdict of resolveEngineDeleteDispatch, re-asked - // here because a beforeDelete hook may have cleared the id since - // (#4550 keeps the wording in one place either way). - throw new Error(ENGINE_DELETE_REJECT_MESSAGE); + // [#2982] The AST asserted present and already used for the + // pre-phase row read above. + // `deleteMany` presence is part of the ladder verdict resolved above. + result = await driver.deleteMany!(object, opCtx.ast!, hookContext.input.options as any); + isPredicateWrite = true; } hookContext.event = 'afterDelete'; diff --git a/packages/objectql/src/hook-condition-bulk-previous.test.ts b/packages/objectql/src/hook-condition-bulk-previous.test.ts index 5ff85c81a3..76706dfbb9 100644 --- a/packages/objectql/src/hook-condition-bulk-previous.test.ts +++ b/packages/objectql/src/hook-condition-bulk-previous.test.ts @@ -22,18 +22,25 @@ * exactly that dispatch, and its message no longer promises an expiry it would * now be breaking. * - * a. batch (`before*`) dispatch + a condition reading `previous` → the named - * `limitation`, the phase as the reason, and the after-type event as the - * first route out; + * a. RETIRED (#5574): the batch dispatch itself, and with it the whole + * diagnostic. A `previous`-reading `beforeUpdate` condition on a bulk + * write now evaluates PER ROW as authored, so the write succeeds; the + * `limitation` discriminator and the `predicateBulkWrite` flag are gone + * from `HookConditionError` under ADR-0049, and the pin that matters is + * that no dispatch lacking `previous` is produced at all; * b. the SAME hook on a single-record write → completely unchanged; - * c. a batch dispatch whose condition does NOT read `previous` → unchanged, - * including the plain typo report; - * d. the generic `No such key` / `Unknown variable` riddle is never the WHOLE - * story for (a) — including when the fault names some other key the same - * condition reads, which is what AST-based detection - * (`collectCelRootIdentifiers`) buys over reading cel-js's prose; - * e. RETIRED: the same condition on an `afterUpdate` hook, through the real - * engine, now evaluates per row and the bulk write SUCCEEDS. + * c. a condition that does NOT read `previous` → unchanged, including the + * plain typo report; + * d. RETIRED with (a): the AST-based `previous` detection that decided WHICH + * limitation to name; + * e. RETIRED by #5038: the same condition on an `afterUpdate` hook, through + * the real engine, evaluates per row and the bulk write SUCCEEDS. + * + * ⚠️ (a) and (d) are kept as inverted/annotated blocks rather than deleted: + * a reversed decision is a record (Prime Directive #13), and the diagnostic's + * two-year arc — #5037 stopgap, #5038 half-retirement, #5574 full — is the + * clearest available argument for fixing a producer instead of documenting a + * limitation. */ import { describe, it, expect } from 'vitest'; @@ -93,130 +100,113 @@ function singleCtx(data: Record): HookContext { } /* ──────────────────────────────────────────────────────────────────────────── - * a. The batch (`before*`) dispatch reading `previous` keeps the named limitation + * a. RETIRED — the batch dispatch, and the whole diagnostic that described it * ──────────────────────────────────────────────────────────────────────────── */ -describe('[#5038] the batch dispatch of a bulk write, whose condition reads `previous`', () => { +describe('[#5574] the batch dispatch is GONE, and so is its diagnostic', () => { const TRANSITION = 'previous.done != true && record.done == true'; - it('rejects with a machine-readable `limitation`, not just prose', async () => { - const ran: string[] = []; - const wrapped = wrapDeclarativeHook( - makeHook(TRANSITION), (async () => { ran.push('audited'); }) as any, { logger: silentLogger }, - ); + it('the bulk write #5037 and #5038 both rejected now SUCCEEDS, firing per row', async () => { + // ⚠️ The inverse of what this block asserted until #5574. It used to pin + // "rejects with a machine-readable `limitation`" — a legal, contract-shaped + // transition condition on a `beforeUpdate` hook aborted every bulk write, + // and the diagnostic's job was to explain that the platform, not the + // author, was at fault. + // + // ADR-0058 Addendum II removed the fault instead of explaining it: a + // predicate write dispatches `beforeUpdate` once per matched row, each + // carrying that row's `previous`, so the condition evaluates as authored. + const audited: string[] = []; + const engine = await bootEngine([{ + name: 'audit_task_completion', object: 'hook_task', events: ['beforeUpdate'], priority: 90, + condition: TRANSITION, + handler: (ctx: any) => { audited.push(String(ctx.input.id)); }, + } as unknown as Hook]); - const err = await wrapped(batchCtx({ done: true })).then(() => null, (e) => e); + await engine.insert('hook_task', { title: 'A', status: 'todo', done: false }); + await engine.insert('hook_task', { title: 'B', status: 'todo', done: false }); - expect(err).toBeInstanceOf(HookConditionError); - // The discriminator a caller branches on. Deliberately NOT `code`: ADR-0112 - // makes `error.code` a closed wire vocabulary and `rest-server.ts` promotes - // a thrown error's `.code` onto the envelope, so a `.code` here would mint - // an unregistered wire code by side effect. - expect(err.limitation).toBe('bulk_write_previous_unbound'); - expect((err as any).code).toBeUndefined(); - expect(err.predicateBulkWrite).toBe(true); - expect(err.reason).toBe('unevaluable'); - expect(err.hook).toBe('audit_task_completion'); - expect(err.condition).toBe(TRANSITION); - // …and the handler never ran (the gate is before it). - expect(ran).toEqual([]); + await expect( + engine.update('hook_task', { done: true }, { multi: true, where: { status: 'todo' } } as any), + ).resolves.toBeDefined(); + + // Per row, each naming its own row — the transition held for both. + expect(audited).toHaveLength(2); + expect(new Set(audited).size).toBe(2); + // And the write landed: fail-loud took no exception before, and there is + // nothing left to take an exception to now. + const rows: any[] = await engine.find('hook_task', {} as any); + expect(rows.every((r) => r.done === true)).toBe(true); }); - it('names the batch, the missing binding, and the PHASE as the reason', async () => { - const wrapped = wrapDeclarativeHook( - makeHook(TRANSITION), (async () => {}) as any, { logger: silentLogger }, - ); - const err = await wrapped(batchCtx({ done: true })).then(() => null, (e) => e); + it('does not fire for rows the transition did not happen on', async () => { + // The other half of "evaluates as authored": a transition is a transition, + // per row. Under the batch dispatch this distinction could not exist — + // there was one call and no row to judge. + const audited: string[] = []; + const engine = await bootEngine([{ + name: 'audit_task_completion', object: 'hook_task', events: ['beforeUpdate'], priority: 90, + condition: TRANSITION, + handler: (ctx: any) => { audited.push(String((ctx.previous as any).title)); }, + } as unknown as Hook]); - expect(err.message).toContain("Hook 'audit_task_completion'"); - expect(err.message).toContain('PREDICATE bulk write (multi: true)'); - expect(err.message).toContain('no single prior record to bind'); - expect(err.message).toContain("'beforeUpdate'"); - // The reason is now the phase, not a missing release. A `before*` hook may - // still rewrite the shared payload, so it cannot be per-row. - expect(err.message).toContain('before any row is written'); - expect(err.message).toContain('ADR-0058'); - }); + await engine.insert('hook_task', { title: 'A', status: 'todo', done: false }); + await engine.insert('hook_task', { title: 'already', status: 'todo', done: true }); - it('no longer promises an expiry that has already happened', async () => { - // #5037's message said "this rejection retires when #5038 lands". It has - // landed. Repeating that sentence would be a promise the platform is now - // breaking — an author would wait for a release that already shipped. - const wrapped = wrapDeclarativeHook( - makeHook(TRANSITION), (async () => {}) as any, { logger: silentLogger }, - ); - const err = await wrapped(batchCtx({ done: true })).then(() => null, (e) => e); + await engine.update('hook_task', { done: true }, { multi: true, where: { status: 'todo' } } as any); - expect(err.message).not.toContain('CURRENT-VERSION limitation'); - expect(err.message).not.toMatch(/retires when/); + expect(audited).toEqual(['A']); }); - it('leads with the after-type event — the route the contract just made real', async () => { - const wrapped = wrapDeclarativeHook( - makeHook(TRANSITION), (async () => {}) as any, { logger: silentLogger }, - ); - const err = await wrapped(batchCtx({ done: true })).then(() => null, (e) => e); + it('never dispatches a context that lacks `previous` — the producer is gone, not just the message', async () => { + // The load-bearing pin of the retirement. `HookConditionLimitation` was + // removed under ADR-0049 because it had no producer; that claim is only + // worth as much as this measurement. Every `beforeUpdate` context the + // engine dispatches on a predicate write must carry both `input.id` and + // `previous` — the two facts whose ABSENCE `isPredicateBulkWrite` used to + // key on. + const seen: HookContext[] = []; + const engine = await bootEngine([{ + name: 'observer', object: 'hook_task', events: ['beforeUpdate'], priority: 90, + handler: (ctx: any) => { seen.push({ id: ctx.input.id, previous: ctx.previous } as any); }, + } as unknown as Hook]); - // The first route out is the one per-row dispatch created: the same - // condition on the matching after-type event evaluates as authored. - expect(err.message).toContain("'afterUpdate'"); - expect(err.message).toContain('PER MATCHED ROW'); - // Single-record targeting still works and is still named. - expect(err.message).toContain('one record (update by id)'); - // Dropping `previous` is not free and the message must not present it as - // the fix: a transition silently becomes a state test. - expect(err.message).toContain('becomes a state test'); - }); + await engine.insert('hook_task', { title: 'A', status: 'todo', done: false }); + await engine.insert('hook_task', { title: 'B', status: 'todo', done: false }); - it('now DOES name the record-change flow trigger — the fact it was refused over changed', async () => { - // #5037 deliberately refused this route on measured evidence: the trigger - // subscribes to these same lifecycle hooks, so on a bulk write it fired once - // with the same unbound `previous` (#4862). #5038 fixed it at the producer, - // so an after-type record-change trigger rides the per-row dispatch. The - // message follows the fact rather than the other way round. - const wrapped = wrapDeclarativeHook( - makeHook(TRANSITION), (async () => {}) as any, { logger: silentLogger }, - ); - const err = await wrapped(batchCtx({ done: true })).then(() => null, (e) => e); + await engine.update('hook_task', { done: true }, { multi: true, where: { status: 'todo' } } as any); - expect(err.message).toContain('record-change flow trigger'); - expect(err.message).not.toContain('A record-change flow trigger is NOT a way around this'); + expect(seen).toHaveLength(2); + for (const ctx of seen) { + expect((ctx as any).id).toBeDefined(); + expect(ctx.previous).toBeDefined(); + } }); - it('names `beforeDelete` for a delete-shaped batch dispatch', async () => { + it('carries NO `limitation` / `predicateBulkWrite` even on a hand-fabricated batch context', async () => { + // The engine cannot build this context any more — but a stale double, a + // test helper or a future refactor could. If one ever does, the author gets + // the plain diagnosis, not a resurrected discriminator: the fields are gone + // from `HookConditionError` entirely, so reintroducing the branch means + // reintroducing the type, which is a visible edit rather than a quiet one. const wrapped = wrapDeclarativeHook( - makeHook('previous.done != true', { events: ['beforeDelete'] } as any), - (async () => {}) as any, { logger: silentLogger }, + makeHook(TRANSITION), (async () => {}) as any, { logger: silentLogger }, ); - const ctx = { - object: 'hook_task', event: 'beforeDelete', - input: { options: { multi: true } }, previous: undefined, ql: qlStub, - } as unknown as HookContext; - - const err = await wrapped(ctx).then(() => null, (e) => e); - expect(err.limitation).toBe('bulk_write_previous_unbound'); - expect(err.message).toContain("'afterDelete'"); - }); - it('still ABORTS the write — the rescoping is not an exemption', async () => { - const engine = await bootEngine([{ - name: 'audit_task_completion', object: 'hook_task', events: ['beforeUpdate'], priority: 90, - condition: TRANSITION, - handler: () => {}, - } as unknown as Hook]); - - await engine.insert('hook_task', { title: 'A', status: 'todo', done: false }); - await engine.insert('hook_task', { title: 'B', status: 'todo', done: false }); - - const err = await engine - .update('hook_task', { done: true }, { multi: true, where: { status: 'todo' } } as any) - .then(() => null, (e) => e); + const err = await wrapped(batchCtx({ done: true })).then(() => null, (e) => e); expect(err).toBeInstanceOf(HookConditionError); - expect(err.limitation).toBe('bulk_write_previous_unbound'); - // Fail loud takes no exception: nothing was written. - const rows: any[] = await engine.find('hook_task', {} as any); - expect(rows.every((r) => r.done === false)).toBe(true); + expect((err as any).limitation).toBeUndefined(); + expect((err as any).predicateBulkWrite).toBeUndefined(); + // Still no `.code`: ADR-0112 keeps `error.code` a closed wire vocabulary, + // and `rest-server.ts` promotes a thrown error's `.code` onto the envelope. + expect((err as any).code).toBeUndefined(); + // Fail-loud is untouched — an unevaluable condition still aborts (#4775). + expect(err.reason).toBe('unevaluable'); + expect(err.hook).toBe('audit_task_completion'); + // And the message is the plain one, with no batch prose left to maintain. + expect(err.message).not.toContain('PREDICATE bulk write'); + expect(err.message).not.toContain('PER MATCHED ROW'); }); }); @@ -310,106 +300,32 @@ describe('[#5038] a batch dispatch with no `previous` in the condition is unaffe expect(err.message).not.toContain('PREDICATE bulk write'); }); - it('keeps the DECLARED-but-unset field diagnosis on its own limitation name', async () => { - // Same root cause (no stored row in hand), different remedy, so it carries - // its own machine-readable name rather than being folded into `previous`. + it('a DECLARED-but-unset field reads as the ordinary unevaluable case now', async () => { + // ⚠️ This used to pin the SECOND limitation member, + // `bulk_write_stored_state_unavailable`: on a batch dispatch `record` was + // the bare payload, so a condition naming a declared field this write does + // not set was unevaluable through no fault of the author's, and the message + // said so. Retired with its twin (#5574) — a per-row `beforeUpdate` context + // merges the row's stored state into `record`, so the case it described no + // longer arises from a bulk write. What remains is the plain diagnosis, + // which is the right one for any OTHER way of reaching it. const wrapped = wrapDeclarativeHook( makeHook('record.archived == true'), (async () => {}) as any, { logger: silentLogger }, ); const err = await wrapped(batchCtx({ status: 'x' })).then(() => null, (e) => e); - expect(err.limitation).toBe('bulk_write_stored_state_unavailable'); - expect(err.predicateBulkWrite).toBe(true); - expect(err.message).toContain("'archived' IS declared on this object"); - // Same rescoping: the after-type event is where `record` holds the row's - // real state, so it is named here too. - expect(err.message).toContain("'afterUpdate'"); - }); -}); - -/* ──────────────────────────────────────────────────────────────────────────── - * d. The riddle is never the whole story for the `previous` case - * ──────────────────────────────────────────────────────────────────────────── */ - -describe('[#5038] the generic CEL fault is never the whole story on this path', () => { - it('does not leave the author with the bare `No such key` / unbound-root sentence', async () => { - const wrapped = wrapDeclarativeHook( - makeHook('previous.done != true && record.done == true'), - (async () => {}) as any, { logger: silentLogger }, - ); - - const err = await wrapped(batchCtx({ done: true })).then(() => null, (e) => e); - - // `describeCelFault`'s generic sentences — both of which read as "you wrote - // it wrong" — must not be what this author is left holding. - expect(err.message).not.toMatch(/which this object does not declare/); - expect(err.message).not.toMatch(/which is not bound for this operation/); - // The raw fault still travels as a FACT (`fault`, and the head's summary), - // it is simply no longer the diagnosis. - expect(err.fault).toMatch(/Unknown variable: previous|No such key: previous/); - }); - - it('names `previous` for a condition that also reads a declared-but-unset field', async () => { - // Both halves are unevaluable on a batch dispatch, for the same reason. The - // `previous` half is the one the author cannot work around by writing the - // condition differently, so it is the one the message leads with — reading - // the answer off the parsed AST rather than off whichever fault the - // evaluator happened to raise first is what keeps that true. - const wrapped = wrapDeclarativeHook( - makeHook('record.archived == true && previous.done != true'), - (async () => {}) as any, { logger: silentLogger }, - ); - - const err = await wrapped(batchCtx({ status: 'x' })).then(() => null, (e) => e); - - expect(err.limitation).toBe('bulk_write_previous_unbound'); - expect(err.message).toContain("The condition reads 'previous'"); - }); - - it('does NOT mistake a declared field spelled like the root for a `previous` reference', async () => { - // The AST reports ROOT identifiers; `previous_status` is a member name - // under `record`, so the root set is `{record}` and this write gets the - // declared-field diagnosis (whose remedy — reference only what this write - // sets — is the correct one here). A detector matching the source TEXT - // would answer `bulk_write_previous_unbound` and send the author looking - // for a `previous` reference that is not there. - const wrapped = wrapDeclarativeHook( - makeHook('record.previous_status == "todo"'), - (async () => {}) as any, { logger: silentLogger }, - ); - - const err = await wrapped(batchCtx({ status: 'x' })).then(() => null, (e) => e); - - expect(err.limitation).toBe('bulk_write_stored_state_unavailable'); - expect(err.message).toContain("'previous_status' IS declared on this object"); - expect(err.message).not.toContain("The condition reads 'previous'"); - }); - - it('keeps the typo sentence when the condition BOTH misspells a key and reads `previous`', async () => { - // The two halves have different owners: the typo is the author's, the batch - // limitation is the platform's. Reporting only one would send them back for - // a second round. - const wrapped = wrapDeclarativeHook( - makeHook('record.stauts == "x" && previous.done != true'), - (async () => {}) as any, { logger: silentLogger }, - ); - - const err = await wrapped(batchCtx({ status: 'x' })).then(() => null, (e) => e); - - expect(err.limitation).toBe('bulk_write_previous_unbound'); - if (err.missingKey === 'stauts') { - // The evaluator surfaced the typo → the author is told about BOTH. - expect(err.message).toContain("reads 'stauts', which this object does not declare"); - } - expect(err.message).toContain('PREDICATE bulk write (multi: true)'); + expect((err as any).limitation).toBeUndefined(); + expect((err as any).predicateBulkWrite).toBeUndefined(); + expect(err.reason).toBe('unevaluable'); }); it('is inert for a comprehension variable that happens to be named `previous`', async () => { - // `collectCelRootIdentifiers` reports comprehension bind variables as roots - // (its documented caveat), so this condition "reads previous" by that - // measure. It is a non-event: the expression binds its own variable and - // evaluates, so the answer is never consulted. + // Kept from the retired section (d): this case never depended on the + // batch-dispatch branch. `collectCelRootIdentifiers` reports comprehension + // bind variables as roots (its documented caveat), so this condition "reads + // previous" by that measure — and it is a non-event, because the expression + // binds its own variable and evaluates. const ran: string[] = []; const wrapped = wrapDeclarativeHook( makeHook('[1, 2].exists(previous, previous > 1)'), @@ -422,6 +338,31 @@ describe('[#5038] the generic CEL fault is never the whole story on this path', }); }); +/* ──────────────────────────────────────────────────────────────────────────── + * d. RETIRED with the branch it measured — the AST-based `previous` detection + * ──────────────────────────────────────────────────────────────────────────── */ + +/** + * ⛔ Six cases stood here until #5574 and are gone with their subject. They + * measured how the batch-dispatch diagnosis decided WHICH limitation to name: + * off the parsed CEL AST (`collectCelRootIdentifiers`) rather than off cel-js's + * fault prose, so a reworded upstream message could not silently turn the + * diagnosis back into a riddle — including the case where the fault named some + * OTHER key the same condition also read, and the deliberate non-match of + * `record.previous_status` (a member name, not the root). + * + * That detection was consulted ONLY on the error path of a batch dispatch. With + * no batch dispatch, there is no error path and nothing to decide, so keeping + * the cases would have meant keeping `conditionReadsPrevious` alive to be + * tested — a helper whose only caller was removed. The reasoning is preserved + * where a future diagnosis would look for it (`hook-wrappers.ts`, the retirement + * note), not here. + * + * One case did NOT depend on the branch and is kept, in section (c) above: a + * condition that reads a comprehension bind variable named `previous` + * evaluates fine and runs its handler. + */ + /* ──────────────────────────────────────────────────────────────────────────── * e. RETIRED — the after-type dispatch of the very same write now succeeds * ──────────────────────────────────────────────────────────────────────────── */ diff --git a/packages/objectql/src/hook-condition-fail-loud.test.ts b/packages/objectql/src/hook-condition-fail-loud.test.ts index c60f9e0b1a..0a5feee794 100644 --- a/packages/objectql/src/hook-condition-fail-loud.test.ts +++ b/packages/objectql/src/hook-condition-fail-loud.test.ts @@ -311,85 +311,31 @@ describe('[#4775] a condition fault never enters `onError`', () => { }); /* ──────────────────────────────────────────────────────────────────────────── - * 5. The predicate bulk write gets a DIAGNOSIS, not `No such key` (#4800 / B1) + * 5. RETIRED (#5574) — the predicate bulk write no longer NEEDS a diagnosis * ──────────────────────────────────────────────────────────────────────────── */ -describe('[#4775 / #4800 B1, rescoped by #5038] the BATCH dispatch of a predicate write gets its own diagnosis', () => { - // [#5038] The batch dispatch is now the `before*` phase only: after-hooks on - // a predicate write are dispatched once per matched row, on a - // single-record-shaped context, so they never land here. A `beforeUpdate` - // still fires ONCE for the whole batch — it may rewrite the shared payload, - // and there is one payload — so this diagnosis is its standing answer. - const bulkCtx = (data: Record) => makeCtx({ - event: 'beforeUpdate', - previous: undefined, - input: { data, options: { multi: true } }, - } as any); - - it('`previous` on a batch dispatch names the batch instead of saying "No such key"', async () => { - const wrapped = wrapDeclarativeHook( - makeHook('previous.done != true && record.done == true'), - (async () => {}) as any, { logger: silentLogger }, - ); - - const err = await wrapped(bulkCtx({ done: true })).then(() => null, (e) => e); - - expect(err).toBeInstanceOf(HookConditionError); - expect(err.predicateBulkWrite).toBe(true); - expect(err.message).toContain("Hook 'audit_hook'"); - expect(err.message).toContain('PREDICATE bulk write (multi: true)'); - expect(err.message).toContain('no single prior record to bind'); - expect(err.message).toContain('one record (update by id)'); - // The default riddle must NOT be the whole story the author gets. - expect(err.message).not.toMatch(/which this object does not declare/); - }); - - it('DOES point at the after-type event, and at a record-change flow trigger', async () => { - // #5037 refused both on measured evidence: the record-change trigger rides - // these same lifecycle hooks, so on a `multi: true` update it fired ONCE - // with `ctx.previous` undefined (#4862) — naming it would have made that - // message the next `declared ≠ delivered`. #5038 fixed it at the PRODUCER, - // so the per-row dispatch reaches the trigger and the route became real. - // The message follows the fact. - const wrapped = wrapDeclarativeHook( - makeHook('previous.done != true'), (async () => {}) as any, { logger: silentLogger }, - ); - const err = await wrapped(bulkCtx({ done: true })).then(() => null, (e) => e); - expect(err.message).toContain("'afterUpdate'"); - expect(err.message).toContain('record-change flow trigger'); - expect(err.message).not.toContain('A record-change flow trigger is NOT a way around this'); - }); - - it('a DECLARED field the bulk payload does not set gets the batch diagnosis too', async () => { - // Same root cause: no prior row in hand, so `record` is the bare payload - // and cannot be made total. `No such key: budget` alone reads as a typo, - // which would send the author to fix a field that is spelled correctly. - const wrapped = wrapDeclarativeHook( - makeHook('record.archived == true'), (async () => {}) as any, { logger: silentLogger }, - ); - const err = await wrapped(bulkCtx({ status: 'x' })).then(() => null, (e) => e); - - expect(err.predicateBulkWrite).toBe(true); - expect(err.message).toContain("'archived' IS declared on this object"); - expect(err.message).toContain('PREDICATE bulk write (multi: true)'); - }); - - it('an UNDECLARED key on a bulk write is still reported as a TYPO, not as the batch', async () => { - // The batch diagnosis would be actively wrong here — this one really is a - // misspelling, and it is misspelled on a single-record write too. - const wrapped = wrapDeclarativeHook( - makeHook('record.stauts == "x"'), (async () => {}) as any, { logger: silentLogger }, - ); - const err = await wrapped(bulkCtx({ status: 'x' })).then(() => null, (e) => e); - - expect(err.message).toContain("reads 'stauts', which this object does not declare"); - expect(err.message).not.toContain('PREDICATE bulk write'); - }); - - it('fail loud takes NO exception for the batch — the bulk write still fails', async () => { - // [#5038] On a `beforeUpdate` hook, which is the dispatch that is still - // batch-scoped. The same condition on `afterUpdate` now evaluates per row - // and the write SUCCEEDS — pinned in `hook-condition-bulk-previous.test.ts`. +describe('[#4800 B1 → #5038 → #5574] the batch-dispatch diagnosis is retired at the producer', () => { + // ⚠️ Six cases stood here and asserted the diagnosis's wording: that a + // `previous`-reading condition on a batch dispatch named the BATCH rather + // than saying "No such key", pointed at the after-type event and at a + // record-change flow trigger, and carried `predicateBulkWrite: true`. + // + // The arc is worth stating once, because it is the argument for fixing + // producers rather than writing better messages about them. #4800/B1 asked + // for a diagnosis instead of a riddle. #5037 shipped it with a machine- + // readable `limitation`, expiring when the contract landed. #5038 landed the + // per-row contract for `after*` and retired half of it. #5574 (ADR-0058 + // Addendum II) landed it for `before*` — and at that point there was no + // dispatch left without a bound `previous`, so the condition being diagnosed + // stopped existing and the diagnosis went with it under ADR-0049. + // + // What is pinned instead is the outcome: the write that used to fail now + // succeeds, per row, as authored. The full retirement pins (including that no + // dispatch lacking `previous` is produced at all) live in + // `hook-condition-bulk-previous.test.ts` §a. + + it('the bulk write this section existed to explain now SUCCEEDS', async () => { + // Verbatim the "fail loud takes NO exception for the batch" case, inverted. const engine = await bootEngine([{ name: 'bulk_breaker', object: 'hook_task', events: ['beforeUpdate'], priority: 90, condition: 'previous.done != true && record.done == true', @@ -401,7 +347,29 @@ describe('[#4775 / #4800 B1, rescoped by #5038] the BATCH dispatch of a predicat await expect( engine.update('hook_task', { done: true }, { multi: true, where: { status: 'todo' } } as any), - ).rejects.toThrow(/PREDICATE bulk write/); + ).resolves.toBeDefined(); + }); + + it('an UNDECLARED key on a bulk write is STILL reported as the typo it is', async () => { + // The half that was always the author's, and the half fail-loud is about. + // Retiring the platform-limitation branch must not soften this one — a + // misspelled key is misspelled on a bulk write exactly as on a single-row + // one, and it still ABORTS. + const engine = await bootEngine([{ + name: 'typo_hook', object: 'hook_task', events: ['beforeUpdate'], priority: 90, + condition: 'record.stauts == "x"', + handler: () => {}, + } as unknown as Hook]); + + await engine.insert('hook_task', { title: 'A', status: 'todo', done: false }); + + const err = await engine + .update('hook_task', { done: true }, { multi: true, where: { status: 'todo' } } as any) + .then(() => null, (e) => e); + + expect(err).toBeInstanceOf(HookConditionError); + expect(err.message).toContain("reads 'stauts', which this object does not declare"); + expect(err.message).not.toContain('PREDICATE bulk write'); }); it('a single-record write of the SAME hook still succeeds', async () => { diff --git a/packages/objectql/src/hook-condition-merged-record.test.ts b/packages/objectql/src/hook-condition-merged-record.test.ts index a4e7a2116d..fe162cf9ed 100644 --- a/packages/objectql/src/hook-condition-merged-record.test.ts +++ b/packages/objectql/src/hook-condition-merged-record.test.ts @@ -141,25 +141,37 @@ describe('[#4770] hook condition evaluates against stored ⊕ payload', () => { expect(calls).toEqual([]); }); - it('fabricates nothing when the prior row is not in hand (predicate bulk update)', async () => { + it('fabricates nothing when the prior row is not in hand (context with no `previous`)', async () => { const calls: string[] = []; const { logger } = captureLogger(); - // A `multi: true` update fetches no single prior row, so the persisted - // state is unknown. Defaulting `done` to null here would not materialise an - // absent value — it would contradict N stored rows. #4775: the condition is - // therefore unevaluable, and unevaluable now aborts the write. + // The invariant is unchanged and is the whole point of this file: with no + // stored row in hand, `record` is NOT made total. Defaulting `done` to null + // would not materialise an absent value — it would contradict whatever is + // actually stored. #4775: the condition is unevaluable, and unevaluable + // ABORTS the operation. + // + // ⚠️ The message this used to match (`/PREDICATE bulk write/`) is gone with + // #5574. This context — no id, `multi: true`, no `previous` — was the batch + // dispatch of a predicate write, and the engine no longer builds one: a + // predicate write dispatches `before*` per matched row with `previous` + // bound (ADR-0058 Addendum II). So the special diagnosis had no reachable + // input and was retired under ADR-0049; what a hand-fabricated context gets + // now is the plain one. const wrapped = wrapDeclarativeHook( makeHook('record.done == null', calls), (async () => { calls.push('ran'); }) as any, { logger }, ); - await expect( - wrapped(makeCtx({ - previous: undefined, - input: { data: { status: 'x' }, options: { multi: true } }, - } as any)), - ).rejects.toThrow(/PREDICATE bulk write/); + const err = await wrapped(makeCtx({ + previous: undefined, + input: { data: { status: 'x' }, options: { multi: true } }, + } as any)).then(() => null, (e) => e); + + expect(err).toBeInstanceOf(Error); + expect(err.reason).toBe('unevaluable'); + expect(err.message).not.toContain('PREDICATE bulk write'); + // Nothing was fabricated and the handler never ran — the gate is before it. expect(calls).toEqual([]); }); diff --git a/packages/objectql/src/hook-condition-previous-scope.test.ts b/packages/objectql/src/hook-condition-previous-scope.test.ts index 062a879504..d276c2af30 100644 --- a/packages/objectql/src/hook-condition-previous-scope.test.ts +++ b/packages/objectql/src/hook-condition-previous-scope.test.ts @@ -193,28 +193,36 @@ describe('[#4784] hook condition binds `previous` alongside `record`', () => { expect(conditionWarnings()).toEqual([]); }); - it('fabricates nothing on the BATCH dispatch of a bulk update — `previous` stays unbound', async () => { + it('fabricates nothing when `previous` is unbound — it stays unbound and the write aborts', async () => { const calls: string[] = []; const { logger } = captureLogger(); - // The `beforeUpdate` of a `multi: true` update fires ONCE for N matched - // rows; there is no single prior record. Binding `{}` or `null` would make - // `previous.done != true` answer for rows nobody read. #4775/B1: it stays - // unbound AND the write is rejected — with a diagnosis, not `No such key`. + // The invariant, unchanged: an absent `previous` is never bound to `{}` or + // `null`, because `previous.done != true` would then answer for a record + // nobody read. #4775: unevaluable ABORTS, so the fabrication is refused + // loudly rather than papered over. // - // [#5038] The AFTER dispatch of that same write is per row and DOES bind - // `previous` — the case below. + // ⚠️ What changed with #5574 is only which contexts can BE like this. This + // shape — no id, `multi: true`, no `previous` — was the batch dispatch of a + // predicate write, and the engine does not build one any more: it + // dispatches `before*` per matched row with the row's `previous` bound + // (ADR-0058 Addendum II), which is what makes the batch diagnosis this case + // used to match (`/PREDICATE bulk write/`) unreachable and retired. The + // per-row shape is the case below, and it now covers BOTH phases. const wrapped = wrapDeclarativeHook( makeHook(TRANSITION, ['beforeUpdate']), (async () => { calls.push('ran'); }) as any, { logger }, ); - await expect(wrapped(makeCtx({ + const err = await wrapped(makeCtx({ event: 'beforeUpdate', previous: undefined, input: { data: { done: true }, options: { multi: true } }, - } as any))).rejects.toThrow(/PREDICATE bulk write/); + } as any)).then(() => null, (e) => e); + expect(err).toBeInstanceOf(Error); + expect(err.reason).toBe('unevaluable'); + expect(err.message).not.toContain('PREDICATE bulk write'); expect(calls).toEqual([]); }); @@ -445,11 +453,17 @@ describe('[#4784] a condition that never mentions `previous` costs zero extra fe * true — and #5284 has since narrowed it, from "ANY object has an afterUpdate * hook" to "THIS object does (or its schema needs a prior row, or a roll-up * aggregates it)". Both pins still hold, and the first one carries more - * weight than it did: the object it drives has a `beforeUpdate` hook, which - * the narrowed gate deliberately does not count (a `beforeUpdate` hook is - * dispatched before the read and observes no `previous` on this path, so - * counting it would buy a read with no reader — see - * `engine-update-prior-read-scope.test.ts`, which measures exactly that). + * weight than it did. + * + * ⚠️ #5574 added a FOURTH term to that gate — a `beforeUpdate` hook on this + * object — because the before phase became a real reader of the prior row + * (it is now dispatched after the read, with `previous` bound). So the first + * pin below moved with it: an object whose only hook is a `beforeUpdate` + * DOES pay the read now. What the pin was actually protecting is untouched + * and is the second one: the cost never depends on the condition TEXT. A + * hook's firing count and a hook's read cost must both be inferable from its + * declaration, and "does the expression mention `previous`?" is not part of + * any declaration an author can see. */ async function bootWith(hooks: Hook[]) { const engine = new ObjectQL(); @@ -464,7 +478,16 @@ describe('[#4784] a condition that never mentions `previous` costs zero extra fe return { engine, reads: stub.reads }; } - it('reads no prior row at all for a before-hook condition over `record` only', async () => { + it('reads ONE prior row for a before-hook, whatever its condition says (#5574)', async () => { + // ⚠️ This pinned `0` until #5574 — a `beforeUpdate` hook bought no read, + // because the dispatch preceded the read and no before-handler could + // observe the row. ADR-0058 Addendum II reversed that ordering, so the + // event joined the gate and the cost is now ONE read, not zero. + // + // The number that matters is `1`, not `0`: the read is what binds + // `previous` for the before phase, and this is the same read the after + // phase and the validation rules use. A `2` here would mean the + // deduplication #5846 (a) is about had regressed. const { engine, reads } = await bootWith([{ name: 'record_only_guard', object: 'hook_task', @@ -478,8 +501,28 @@ describe('[#4784] a condition that never mentions `previous` costs zero extra fe const before = reads.findOne; await engine.update('hook_task', { status: 'in_progress' }, { where: { id: row.id } } as any); - // No afterUpdate hook, no prior-needing validation rule → nothing to fetch. - expect(reads.findOne - before).toBe(0); + expect(reads.findOne - before).toBe(1); + }); + + it('costs the SAME for a before-hook whose condition DOES read `previous`', async () => { + // The invariant the case above used to carry, restated where it still + // holds: the cost is a property of the DECLARATION (this object has a + // `beforeUpdate` hook), never of the condition text. The ruling rejected + // keying the read on "does the condition mention `previous`?" explicitly. + const { engine, reads } = await bootWith([{ + name: 'transition_guard', + object: 'hook_task', + events: ['beforeUpdate'], + priority: 100, + condition: TRANSITION, + handler: () => {}, + } as unknown as Hook]); + + const row: any = await engine.insert('hook_task', { title: 'A', status: 'todo', done: false }); + const before = reads.findOne; + await engine.update('hook_task', { done: true }, { where: { id: row.id } } as any); + + expect(reads.findOne - before).toBe(1); }); it('reads the SAME number of rows whether or not the condition mentions `previous`', async () => { diff --git a/packages/objectql/src/hook-input-shape-contract.test.ts b/packages/objectql/src/hook-input-shape-contract.test.ts index 7607c1e80e..2a11c8b2fe 100644 --- a/packages/objectql/src/hook-input-shape-contract.test.ts +++ b/packages/objectql/src/hook-input-shape-contract.test.ts @@ -133,9 +133,14 @@ describe('[#5273] a bulk write carries no `ast` on `input`', () => { await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); - expect(seen).toHaveLength(1); // before* fires ONCE for the whole batch - expect('ast' in seen[0]!).toBe(false); - expect(seen[0]!.ast).toBeUndefined(); + // [#5574] `before*` now fires once PER MATCHED ROW (D1), so the count is + // the row count — and EVERY per-row context must be free of `ast`, not + // just the first: the deleted claim would come back one row in otherwise. + expect(seen).toHaveLength(2); + for (const input of seen) { + expect('ast' in input).toBe(false); + expect(input.ast).toBeUndefined(); + } }); it('`beforeDelete` on a bulk write has no `ast` key', async () => { @@ -146,9 +151,11 @@ describe('[#5273] a bulk write carries no `ast` on `input`', () => { await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); await engine.delete('task', { multi: true, where: { status: 'todo' } } as any); - expect(seen).toHaveLength(1); - expect('ast' in seen[0]!).toBe(false); - expect(seen[0]!.ast).toBeUndefined(); + expect(seen).toHaveLength(2); + for (const input of seen) { + expect('ast' in input).toBe(false); + expect(input.ast).toBeUndefined(); + } }); }); @@ -156,22 +163,43 @@ describe('[#5273] a bulk write carries no `ast` on `input`', () => { * 2. `input.id` — present-but-undefined on the batch, bound per row after * ──────────────────────────────────────────────────────────────────────────── */ -describe('[#5273] `input.id` on a bulk write', () => { - it('`beforeUpdate` leaves `id` undefined (the key exists; nothing binds it)', async () => { +describe('[#5273, moved by #5574] `input.id` on a bulk write', () => { + it('`beforeUpdate` fires per matched row, each naming its own `id`', async () => { + // ⚠️ This case is the inverse of what it asserted until #5574. It used to + // read "`beforeUpdate` leaves `id` undefined (the key exists; nothing binds + // it)" — the batch dispatch's shape, and the thing that made `input.id` a + // REROUTE lever (binding it moved the write onto the single-id branch). + // ADR-0058 Addendum II (D1/D2) replaced that dispatch with one context per + // matched row, each carrying its own row's id; D4 retired the lever, and + // `hook-target-rebind-errors.ts` is the refusal that replaced it. const seen: Array> = []; const { engine } = await boot(); - engine.registerHook('beforeUpdate', async (ctx: any) => { seen.push(ctx.input); }, { object: 'task' }); + engine.registerHook('beforeUpdate', async (ctx: any) => { seen.push({ ...ctx.input }); }, { object: 'task' }); - await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); + const rows = await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); - // The engine builds `{ id, data, options }` with the shorthand `id`, so the - // KEY is there while the value is not. Documented as `{ id: undefined, … }` - // rather than "no id" because `'id' in input` answers true. - expect('id' in seen[0]!).toBe(true); - expect(seen[0]!.id).toBeUndefined(); - expect(seen[0]!.data).toEqual({ status: 'done' }); - expect(seen[0]!.options).toBeDefined(); + expect(seen).toHaveLength(2); + expect(seen.map((i) => i.id).sort()).toEqual(rows.map((r) => r.id).sort()); + // D3: the payload is BATCH-scoped — every per-row context carries THE + // payload, so a rewrite applies to the whole batch. + for (const input of seen) { + expect(input.data).toEqual({ status: 'done' }); + expect(input.options).toBeDefined(); + } + }); + + it('`beforeDelete` fires per matched row too, and carries no payload', async () => { + const seen: Array> = []; + const { engine } = await boot(); + engine.registerHook('beforeDelete', async (ctx: any) => { seen.push({ ...ctx.input }); }, { object: 'task' }); + + const rows = await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); + await engine.delete('task', { multi: true, where: { status: 'todo' } } as any); + + expect(seen).toHaveLength(2); + expect(seen.map((i) => i.id).sort()).toEqual(rows.map((r) => r.id).sort()); + for (const input of seen) expect('data' in input).toBe(false); }); it('`afterUpdate` fires per matched row, each naming its own `id`', async () => { @@ -344,13 +372,20 @@ describe("[#5997] `before*` reads the CALLER's options bag, predicate included", const callerOptions = { multi: true, where: { status: 'todo' } }; await engine.update('task', { status: 'done' }, callerOptions as any); - expect(seen).toHaveLength(1); - assertCallerBag(seen[0]!, callerOptions, { status: 'todo' }); - // `multi` survives too — the guards branch on it to tell a batch from a - // by-id write when `input.id` is undefined for either reason. - expect((seen[0]!.options as any).multi).toBe(true); - // And the batch shape from §2 still holds on the same context. - expect(seen[0]!.id).toBeUndefined(); + // [#5574] One context per matched row (D1) — and the PHASE rule is + // unchanged by that: `input.options` is still the CALLER's very bag on + // every one of them, `where` and `multi` visible. That is the whole point + // of asserting it per row rather than on the first: a per-row context that + // silently swapped in a `DriverOptions` view for rows 2..N would break + // both plugin-auth break-glass guards on exactly the batches they exist + // for, and `seen[0]` alone would never say so. + expect(seen).toHaveLength(2); + for (const input of seen) { + assertCallerBag(input, callerOptions, { status: 'todo' }); + expect((input.options as any).multi).toBe(true); + } + // The per-row shape from §2 holds on the same contexts: each names ITS row. + expect(new Set(seen.map((i) => i.id)).size).toBe(2); }); it('`beforeDelete` (single id) carries the caller `where`', async () => { @@ -384,10 +419,12 @@ describe("[#5997] `before*` reads the CALLER's options bag, predicate included", const callerOptions = { multi: true, where: { id: { $in: doomed } } }; await engine.delete('task', callerOptions as any); - expect(seen).toHaveLength(1); - assertCallerBag(seen[0]!, callerOptions, { id: { $in: doomed } }); - expect((seen[0]!.options as any).multi).toBe(true); - expect(seen[0]!.id).toBeUndefined(); + expect(seen).toHaveLength(2); + for (const input of seen) { + assertCallerBag(input, callerOptions, { id: { $in: doomed } }); + expect((input.options as any).multi).toBe(true); + } + expect(seen.map((i) => i.id).sort()).toEqual([...doomed].sort()); // The write really did run as a batch through that predicate — so the // assertions above describe a live path, not an inert options bag. // No `as any` on this one: `count(object, query?)` infers the empty query, diff --git a/packages/objectql/src/hook-target-rebind-errors.ts b/packages/objectql/src/hook-target-rebind-errors.ts new file mode 100644 index 0000000000..a27b43970a --- /dev/null +++ b/packages/objectql/src/hook-target-rebind-errors.ts @@ -0,0 +1,136 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5574 / #5846] The refusal that replaced a silent retarget: a `before*` hook + * rebinding — or clearing — `ctx.input.id` after the engine has already + * resolved which row(s) this write is about. + * + * ## What used to happen, and why it had to stop + * + * `update()` and `delete()` dispatched `beforeUpdate` / `beforeDelete` FIRST + * and only then read `hookContext.input.id` to choose the driver call. So the + * id slot doubled as a control lever: a handler that assigned `ctx.input.id = + * undefined` on a by-id call converted the write into a PREDICATE write over + * the caller's `where`, and a handler that assigned some OTHER id moved the + * write to a different row. + * + * ADR-0058 Addendum II (#5574 ruling B) reorders that seam. The per-row + * `before*` contract needs the matched row set in hand BEFORE the before phase + * — the row set is what the per-row contexts are BUILT from — so the dispatch + * ladder is resolved first, and #5846's (a) direction moves the by-id path's + * prior-row read ahead of the dispatch too, so `previous` is bound for the + * before phase. Both consequences point the same way: by the time a handler + * runs, the target is settled. `previous`, the `readonlyWhen` strip and every + * validation rule have already been computed against the row the ladder chose. + * + * That leaves the lever with exactly two possible meanings, and both are bad: + * + * - **Ignore it.** The handler's assignment retargets nothing and says + * nothing. A silent no-op is the failure shape this whole family exists to + * abolish (#4649 / #4775 / #5038) — and here the no-op is not even the worst + * of it, because the write still lands on the ORIGINAL row. + * - **Honour it by re-resolving.** The write would then land on a row whose + * pre-image was never read, whose `readonlyWhen` locks were never evaluated + * and whose validation rules were checked against a different record. + * Silently weaker enforcement, aimed by a hook. + * + * So the ruling picked the third option: REFUSE, loudly, naming the capability + * that was retired. Nothing is written; the handler learns its assignment was + * meaningful and is no longer honoured, instead of learning nothing. + * + * D4 states the predicate half of the same rule — a per-row context arrives + * with `id` ALREADY bound to its row, and rebinding it retargets nothing — so + * one error serves both paths. + * + * ## Why `code` is an `ERR_`-prefixed operational code, not a wire code + * + * Same reasoning as {@link BULK_PER_ROW_HOOK_LIMIT_ERROR_CODE} next door: + * ADR-0112 makes `error.code` a CLOSED vocabulary (`StandardErrorCode` ∪ + * `ERROR_CODE_LEDGER`) that `rest-server.ts` promotes onto the response + * envelope, so minting a member of it by side effect is the exact + * `declared ≠ enforced` shape that vocabulary exists to prevent. This code + * travels on the thrown error's own property bag; putting it on the wire is a + * ledger decision, not a property that happens to be named `code`. + */ +export const HOOK_TARGET_REBIND_ERROR_CODE = 'ERR_HOOK_TARGET_REBIND'; + +/** Which lifecycle seam observed the rebinding. */ +export type HookTargetRebindPath = + /** A by-id `update()` / `delete()` whose `before*` handler moved or cleared the id. */ + | 'by-id' + /** A per-row `before*` context on a predicate write (D4). */ + | 'per-row'; + +export class HookTargetRebindError extends Error { + override readonly name = 'HookTargetRebindError'; + readonly code = HOOK_TARGET_REBIND_ERROR_CODE; + /** The object being written. */ + readonly object: string; + /** The event whose dispatch the rebinding was observed after. */ + readonly event: string; + /** Which seam this is — see {@link HookTargetRebindPath}. */ + readonly path: HookTargetRebindPath; + /** The id the engine resolved and computed the write against. */ + readonly expectedId: unknown; + /** What `ctx.input.id` held when the dispatch returned. */ + readonly observedId: unknown; + + constructor(info: { + object: string; + event: string; + path: HookTargetRebindPath; + expectedId: unknown; + observedId: unknown; + }) { + super(buildMessage(info)); + this.object = info.object; + this.event = info.event; + this.path = info.path; + this.expectedId = info.expectedId; + this.observedId = info.observedId; + } +} + +const show = (v: unknown): string => + v === undefined ? 'undefined' : v === null ? 'null' : JSON.stringify(v) ?? String(v); + +function buildMessage(info: { + object: string; + event: string; + path: HookTargetRebindPath; + expectedId: unknown; + observedId: unknown; +}): string { + const { object, event, path, expectedId, observedId } = info; + const cleared = observedId === undefined || observedId === null || observedId === ''; + const head = + `Refusing the write on '${object}': a '${event}' handler ${cleared ? 'CLEARED' : 'REBOUND'} ` + + `'ctx.input.id' (${show(expectedId)} → ${show(observedId)}) after the engine had already resolved ` + + `the target. Nothing was written.`; + + // The capability being retired, named — the whole point of the refusal is + // that the author learns what stopped working rather than watching a write + // land somewhere unexpected. + const retired = + path === 'by-id' + ? cleared + ? ` The capability this used to have is RETIRED: clearing 'input.id' in a '${event}' handler ` + + `converted a by-id write into a PREDICATE write over the caller's 'where'. Since ADR-0058 ` + + `Addendum II (#5574 / #5846) the dispatch ladder is resolved BEFORE the before phase — the ` + + `predicate path has to read its matched rows first, to build one context per row — so there ` + + `is no ladder left to re-enter.` + : ` The capability this used to have is RETIRED: rebinding 'input.id' in a '${event}' handler ` + + `moved the write to another row. The engine now reads that row's pre-image, evaluates its ` + + `'readonlyWhen' locks and runs its validation rules BEFORE dispatching, so honouring a ` + + `rebind would write a row that none of those checks ever saw.` + : ` On a predicate write a '${event}' context arrives with 'id' ALREADY bound to its row and the ` + + `dispatch decided, so rebinding it retargets nothing (ADR-0058 Addendum II, D4). It is refused ` + + `rather than ignored, because a silent no-op is the failure this contract exists to abolish.`; + + const routes = + ` To write a DIFFERENT row, call 'ctx.api' / 'ctx.ql' for that row explicitly. To write MANY rows, ` + + `have the caller pass '{ multi: true, where: … }'. To stop this write, throw from the handler — ` + + `that is the supported way for a '${event}' guard to refuse.`; + + return head + retired + routes; +} diff --git a/packages/objectql/src/hook-wrappers.ts b/packages/objectql/src/hook-wrappers.ts index 80aa96ee4d..29fdb94274 100644 --- a/packages/objectql/src/hook-wrappers.ts +++ b/packages/objectql/src/hook-wrappers.ts @@ -17,7 +17,7 @@ import type { Hook, HookContext } from '@objectstack/spec/data'; import type { Expression } from '@objectstack/spec'; import type { Logger } from '@objectstack/spec/contracts'; import type { HookHandler } from './engine.js'; -import { ExpressionEngine, collectCelRootIdentifiers } from '@objectstack/formula'; +import { ExpressionEngine } from '@objectstack/formula'; import { noopHookMetricsRecorder, type HookMetricsRecorder, type HookMetricOutcome } from './hook-metrics.js'; import { materializeDeclaredFields } from './declared-fields.js'; import { describeCelFault, type CelFault } from './cel-fault.js'; @@ -103,54 +103,56 @@ const noopLogger: HookDiagnosticsLogger = { * mint a third set of semantics for the same word. */ /** - * Which platform limitation made the condition unevaluable, when a limitation - * — rather than the author — is what happened (#5037). - * - * The distinction this names is the whole point of the diagnostic: an - * undeclared key is the AUTHOR's to fix and stays fixed, while these two are - * the PLATFORM's. A caller that wants to tell "your hook is wrong" from "this - * event cannot do that" — a REST layer choosing a status, a test, a Studio - * surface — reads this field instead of matching on the message text. - * - * - `bulk_write_previous_unbound` — the condition names `previous` on a - * predicate (`multi: true`) write whose hook fires once for the whole - * batch, so there is no single prior record to bind; - * - `bulk_write_stored_state_unavailable` — the condition names a DECLARED - * field this write does not set, and `record` is the bare payload on such a - * write for the same reason (no single stored row to merge with). - * - * ## What #5038 retired, and what it did NOT - * - * #5037 shipped these as a stopgap for the whole bulk-write surface, expiring - * when #5038 landed the per-row contract. #5038 retires them for **after-type** - * hooks, which now receive one single-record-shaped context per matched row — - * `previous` bound, `record` the row's real state — so a transition condition - * on `afterUpdate`/`afterDelete` evaluates on a bulk write exactly as authored - * and never reaches this error. - * - * They stay reachable, and correct, for **before-type** hooks. A - * `beforeUpdate`/`beforeDelete` on a predicate write fires ONCE, before any row - * is touched, because it may still rewrite the payload — and one payload cannot - * be edited per row. That is not a version gap that a later release closes; it - * is what a batch-scoped event is. So the message no longer promises expiry: - * it names the phase as the reason and points at the after-type event, which - * per-row semantics made a route that actually works. - * - * ## Why this is not `error.code` - * - * Deliberately NOT named `code`. ADR-0112 makes `error.code` a CLOSED wire - * vocabulary — `StandardErrorCode` ∪ `ERROR_CODE_LEDGER`, both declared in - * `packages/spec/src/api/` — and `rest-server.ts` promotes a thrown error's - * `.code` straight onto the response envelope. Putting a `.code` here would - * therefore mint an unregistered wire code by side effect, which is the exact - * `declared ≠ enforced` shape this family exists to remove. If this ever needs - * to travel on the wire it goes through the ledger, as a decision, not as a - * property that happens to be named `code`. + * ⛔ RETIRED — `HookConditionLimitation` and its two members + * (`bulk_write_previous_unbound`, `bulk_write_stored_state_unavailable`), with + * the `isPredicateBulkWrite` predicate and the `predicateBulkWrite` flag that + * produced them. #5574's engine half, ADR-0049 enforce-or-remove. Do not + * reintroduce any of them. + * + * ## Why they existed + * + * #5037 shipped them for a real gap: on a predicate (`multi: true`) write the + * `before*` phase was dispatched ONCE for the whole batch, on a context with no + * `input.id` and no `previous`. A condition reading `previous` was therefore + * unevaluable, and since #4775 unevaluable ABORTS the write — so the author got + * a rejection that read like a typo report for an expression that was + * perfectly well formed. The `limitation` discriminator existed so a caller + * could tell "your hook is wrong" from "this event cannot do that" without + * matching on message text. + * + * #5038 retired them for the `after*` phase by making it per row. Addendum I + * kept them alive for `before*`, arguing the batch dispatch was not a version + * gap but what the phase IS. + * + * ## Why they are gone + * + * ADR-0058 Addendum II (#5574 ruling B) reversed that argument on measured + * evidence and made the `before*` phase per row too. Both ends of the + * declaration are now empty, and this is what that was verified against on the + * delivering tree: + * + * - **No producer.** `isPredicateBulkWrite`'s whole test was "no `input.id` + * and `options.multi`". Every context the engine now dispatches on a + * predicate write — both phases — arrives with `input.id` bound to its row, + * so the predicate answered `false` everywhere and the branch that set both + * members was unreachable. The batch-scoped `hookContext` still exists + * inside `update()`/`delete()`, but it is never handed to `triggerHooks`: + * the per-row loop runs whenever `hasHooksFor` is true, and when it is false + * no handler runs at all. So no handler can observe one. + * - **No reachable consumer.** Nothing outside this file and its own tests + * ever read `.limitation` or `.predicateBulkWrite`. + * + * A discriminator with neither is not a diagnostic, it is a promise the + * platform has stopped keeping — the exact `declared ≠ enforced` shape + * ADR-0049 exists to remove. What replaced it is not a better message but the + * absence of the condition it described: a `previous`-reading `before*` + * condition on a bulk write now EVALUATES, per row, as authored. The pins live + * in `hook-condition-bulk-previous.test.ts`. + * + * The `HookConditionError` that carried them is NOT retired — an unevaluable + * or uncompilable condition still aborts the operation (#4775). Only the + * bulk-write branch of its diagnosis is gone. */ -export type HookConditionLimitation = - | 'bulk_write_previous_unbound' - | 'bulk_write_stored_state_unavailable'; - export class HookConditionError extends Error { override readonly name = 'HookConditionError'; /** The hook whose declared condition could not be evaluated. */ @@ -166,13 +168,6 @@ export class HookConditionError extends Error { readonly fault: string; /** The key the expression read that the record does not carry, when known. */ readonly missingKey?: string; - /** True when the operation is a predicate (`multi: true`) bulk write, whose - * N matched rows have no single prior state to bind (#4800/B1). */ - readonly predicateBulkWrite?: boolean; - /** The current-version limitation behind the fault, when one is the cause - * rather than the authored expression (#5037). See - * {@link HookConditionLimitation}. */ - readonly limitation?: HookConditionLimitation; constructor(message: string, info: { hook: string; @@ -182,8 +177,6 @@ export class HookConditionError extends Error { reason: 'unevaluable' | 'uncompilable'; fault: string; missingKey?: string; - predicateBulkWrite?: boolean; - limitation?: HookConditionLimitation; }) { super(message); this.hook = info.hook; @@ -193,8 +186,6 @@ export class HookConditionError extends Error { this.reason = info.reason; this.fault = info.fault; this.missingKey = info.missingKey; - this.predicateBulkWrite = info.predicateBulkWrite; - this.limitation = info.limitation; } } @@ -262,11 +253,6 @@ export function wrapDeclarativeHook( if (expr.source && expr.source.trim()) { const source = expr.source; const check = ExpressionEngine.compile(expr); - // Read ONCE, off the parsed AST, whether this condition names `previous` - // at all (#5037) — see `conditionReadsPrevious`. Wrap time, not call - // time: the answer is a property of the source, and the call path is on - // every write of the object. - const readsPrevious = conditionReadsPrevious(source); if (check.ok) { conditionFn = (ctx: HookContext) => { // `previous` is passed through as-is: `undefined` means the binding @@ -280,7 +266,7 @@ export function wrapDeclarativeHook( const r = ExpressionEngine.evaluate(expr, { record: record ?? {}, previous }); if (!r.ok) { // [#4775] Fail LOUD. Not `false` — see `HookConditionError`. - throw unevaluableConditionError(meta, ctx, source, r.error, declaredFieldsFor(ctx), readsPrevious); + throw unevaluableConditionError(meta, ctx, source, r.error); } return Boolean(r.value); }; @@ -536,81 +522,26 @@ function isInsertEvent(event: unknown): boolean { } /** - * Is this context a BATCH-SCOPED dispatch of a predicate (`multi: true`) write - * — one hook call standing for N matched rows? - * - * Read off the same two facts the engine branches on (`input.id` absent + - * `options.multi`), which survive into the event context: `input.options` is - * rebuilt by `buildDriverOptions` as a COPY of the caller's bag, so `multi` is - * still there. - * - * ## Why the `id` test is the whole test (#5038) - * - * Since the per-row contract landed, a bulk write's AFTER-hooks are dispatched - * on one single-record-shaped context per matched row, each carrying that row's - * `input.id`, `previous` and `result`. Those contexts therefore answer `false` - * here — correctly, because they are not batch-scoped at all: nothing about - * them stands for N rows. What still answers `true` is the `beforeUpdate` / - * `beforeDelete` dispatch, which genuinely is one call for the whole batch (it - * may rewrite the shared payload, and there is only one payload to rewrite). - * - * So this predicate did not need a phase test bolted on: the id it looks for is - * exactly what per-row dispatch supplies and batch dispatch cannot. + * ⛔ RETIRED with the batch-dispatch diagnosis (#5574) — see the note above + * `HookConditionError`. Three helpers existed only to serve it and go with it: + * + * - `isPredicateBulkWrite` — "no `input.id` and `options.multi`", i.e. is this + * one hook call standing for N matched rows? No dispatch is batch-scoped + * any more, so it answered `false` everywhere. + * - `afterCounterpartEvent` — named the `after*` event a batch-scoped + * `before*` condition should move to. That was the route out of a + * limitation that no longer exists. + * - `isBeforeEvent` / `conditionReadsPrevious` — the phase test and the + * AST-based "does this condition name `previous`?" detection (#5037), both + * consulted only inside that branch. The AST detection is worth remembering + * rather than just deleting: it existed because deriving the diagnosis from + * cel-js's prose (`Unknown variable: previous`) made an author-facing + * message depend on an upstream library's wording. If a future diagnosis + * ever needs the same fact, `collectCelRootIdentifiers` + * (`@objectstack/formula`, the utility #4972's build gate uses) is the + * seam — with its documented caveat that a comprehension bind variable + * named `previous` reads as a reference. */ -function isPredicateBulkWrite(ctx: HookContext): boolean { - const input: any = ctx.input ?? {}; - if (!input || typeof input !== 'object') return false; - if (input.id !== undefined && input.id !== null && input.id !== '') return false; - const options: any = input.options; - return Boolean(options && typeof options === 'object' && options.multi); -} - -/** Hook events that fire BEFORE the write, once for the whole operation. */ -function isBeforeEvent(event: unknown): boolean { - return typeof event === 'string' && event.startsWith('before'); -} - -/** - * The after-type event a batch-scoped `before*` condition should move to, so - * the diagnostic can name it instead of describing one. - */ -function afterCounterpartEvent(event: unknown): string { - return typeof event === 'string' && event.startsWith('before') - ? `after${event.slice('before'.length)}` - : 'after* '; -} - -/** - * Does this condition NAME `previous` at all? Answered from the parsed CEL AST - * (#5037), not from the fault text a failed evaluation happened to produce. - * - * The diagnostic below has to tell "this hook reads the pre-write state, which - * a bulk write cannot supply in this version" from "this hook has a typo". - * Deriving that from cel-js's message (`Unknown variable: previous`) works - * today and is kept as the fallback, but it makes an author-facing diagnosis - * depend on an upstream library's prose: reword the fault and the batch silently - * goes back to reporting a riddle. The AST is the same fact stated by the - * expression itself, so the diagnosis holds whatever the evaluator says — and it - * is available even when the fault names something ELSE the same condition also - * reads. - * - * `collectCelRootIdentifiers` is the utility #4972's build gate already uses for - * "which roots does this expression reference". Its documented caveat: a - * comprehension bind variable (`[1,2].exists(previous, previous > 1)`) is - * reported as a root, so a hook that names its bind variable `previous` reads as - * a `previous` reference here. That false positive is inert by construction — - * this answer is consulted ONLY on the error path, and such an expression binds - * its own variable and evaluates fine, so it never reaches one. Paying for a - * comprehension-aware walk to remove an unreachable case would be the more - * expensive wrong call. - * - * A source that does not parse returns `false`: it never compiled, so it goes - * down the `uncompilableConditionError` path and never reaches this diagnosis. - */ -function conditionReadsPrevious(source: string): boolean { - const roots = collectCelRootIdentifiers(source); - return roots.ok ? roots.roots.includes('previous') : false; -} /** * The rejection a condition that CANNOT BE EVALUATED produces (#4775). @@ -619,148 +550,38 @@ function conditionReadsPrevious(source: string): boolean { * facts, same two explanatory sentences (both come out of the shared * `cel-fault.ts`), so an author who has met one message can read the other. * - * ## The predicate-bulk-write branch (#4800 / B1, rescoped by #5038) - * - * A `multi: true` write matches N rows. Its AFTER hooks now fire once PER ROW - * on a single-record-shaped context (ADR-0058's bulk-write addendum, - * implemented in `engine.ts`), so they never reach this branch: `previous` is - * that row's pre-image and `record` is that row's state, and the transition - * condition evaluates as authored. - * - * Its BEFORE hooks still fire ONCE for the whole batch — that is not a gap - * waiting on a release, it is what the phase means: a `before*` hook may - * rewrite the payload, and one `updateMany` carries one payload, so there is - * nothing per-row to give it. For that dispatch two things the condition may - * legitimately name are still not in hand: - * - * - `previous` — unbound, because there is no single prior record; - * - a DECLARED field the payload does not set — `record` is the payload - * alone here, since merging it with "the" stored row would mean picking one - * of N, and materialising declared fields to `null` would state something - * false about all N. - * - * Both are the SAME situation and neither is an author typo, so the default - * `No such key: previous` — which reads as "you misspelled something" — is - * actively misleading. This branch names the batch, says why the binding is - * missing, and gives the one route that actually works. It does NOT create an - * exception: the write still fails (maintainer's ruling on #4800 — fail loud, - * no exemptions), it just fails with a diagnosis instead of a riddle. - * - * An UNDECLARED key still routes to the ordinary typo message even on a bulk - * write: that one IS a typo, and saying "this is a batch" about it would send - * the author down the wrong path. When the condition reads `previous` AND - * misspells something, the author gets both sentences — the typo is theirs to - * fix, and the batch limitation is still waiting behind it. - * - * ## The rejection names a PHASE limit, not a version gap (#5038, ADR-0058) - * - * The 2026-08-04 ruling on #4800/#4862 settled what a bulk write MEANS: after - * hooks and record-change flow triggers evaluate and fire PER ROW. #5037 shipped - * this rejection as the rc-window stopgap for the gap between that contract and - * the engine, and said so — "this retires when #5038 lands". - * - * #5038 has landed, and the honest message changed with it. The rejection is no - * longer a placeholder for missing work; it is the standing answer for the one - * dispatch that is batch-scoped by nature, the `before*` phase. So the message - * no longer promises expiry (a promise it would now be breaking), and its FIRST - * route is the one the contract just made real: move the condition to the - * matching `after*` event, where it evaluates per row exactly as authored. - * - * ⚠️ One escape route named here was added on evidence and one was refused on - * evidence. A record-change flow trigger IS now a real route — it subscribes to - * these very lifecycle hooks (`trigger-record-change/src/record-change-trigger.ts` - * → `engine.registerHook`), which is precisely why the per-row contract reaches - * it: an `after*` record-change trigger receives the row's bound `previous` on a - * bulk write (#4862, closed by #5038). #5037 refused to name it because at that - * time it did not; the fact changed, so the message did. What is still NOT - * offered is "drop `previous` from the condition" as a fix — it unblocks the - * batch by silently turning a transition ("just became done") into a state test - * ("is done"), which fires on rows that were already done. It is named only with - * that cost attached. + * ## What used to branch here, and why nothing does now (#5574) + * + * Between #5037 and #5574 this function carried a second diagnosis for the + * BATCH dispatch of a predicate (`multi: true`) write: that dispatch had no + * `input.id` and no `previous`, so a `previous`-reading condition faulted + * through no fault of the author's, and the message said so — naming the + * limitation, the phase as the reason, and the matching `after*` event as the + * route out. + * + * ADR-0058 Addendum II removed the condition being diagnosed rather than the + * diagnosis: a predicate write dispatches `before*` once per matched row now, + * each context carrying that row's `id` and `previous`, so the very expression + * that used to abort the batch evaluates as authored. With no dispatch left + * that lacks a bound `previous`, the branch had no reachable input and its two + * `HookConditionLimitation` members had no producer — retired under ADR-0049, + * see the note above `HookConditionError`. + * + * What remains is the plain diagnosis, which was always the right answer for + * the case that is genuinely the author's: a condition naming a key the record + * does not carry. */ function unevaluableConditionError( meta: Hook, ctx: HookContext, source: string, error: CelFault, - declaredFields: Record | undefined, - readsPrevious = false, ): HookConditionError { - const { summary, missingKey, unknownVariable, detail } = describeCelFault(error, { + const { summary, missingKey, detail } = describeCelFault(error, { what: 'condition', undeclaredKeyFix: "fix the hook's condition, or declare the field", }); const head = `Hook '${meta.name}' could not evaluate its condition (${summary}) — operation aborted.`; - - if (isPredicateBulkWrite(ctx)) { - const declaredMissingKey = missingKey && declaredFields - && Object.prototype.hasOwnProperty.call(declaredFields, missingKey) - ? missingKey - : undefined; - // The typo sentence, kept alongside the batch diagnosis when the fault - // named a key this object does not declare: that half IS the author's. - const typoDetail = missingKey && !declaredMissingKey ? detail : ''; - // Does the condition read `previous`? The AST says so directly; the fault - // text (`Unknown variable: previous` — the ROOT is absent) is the fallback - // for a caller that did not pass the AST answer through. - const namesPrevious = readsPrevious || unknownVariable === 'previous'; - - // Which dispatch is this? A `before*` hook on a predicate write is the one - // that is batch-scoped by nature (#5038); anything else reaching here is a - // context that named no row despite the per-row contract, so it gets the - // same facts without the phase explanation. - const beforePhase = isBeforeEvent(ctx.event); - const afterEvent = afterCounterpartEvent(ctx.event); - const perRowSentence = beforePhase - ? ` A '${afterEvent}' hook on this same write does NOT have this problem: after-hooks on a` + - ` predicate write fire once PER MATCHED ROW, each with that row's 'previous' bound and` + - ` 'record' holding its real state (ADR-0058, bulk-write addendum; ruling on #4800/#4862).` + - ` If the condition is a TRANSITION rather than a guard on the incoming payload, move the hook` + - ` to '${afterEvent}' — or express it as an after-type record-change flow trigger, which rides` + - ` the same per-row dispatch.` - : ` After-hooks on a predicate write are dispatched once per matched row, each carrying that` + - ` row's id, 'previous' and state (ADR-0058, bulk-write addendum); this context carries no row id,` + - ` so it was dispatched for the batch.`; - - let limitation: HookConditionLimitation | undefined; - let bulkDetail: string | undefined; - if (namesPrevious) { - limitation = 'bulk_write_previous_unbound'; - bulkDetail = - ` The condition reads 'previous', but this is the ${beforePhase ? `'${ctx.event}'` : 'batch'} dispatch of a` + - ` PREDICATE bulk write (multi: true): it matches many rows and fires ONCE for the whole batch` + - ` — before any row is written, so it may still rewrite the shared payload — and one call has no` + - ` single prior record to bind.` + - perRowSentence + - ` Targeting the write at one record (update by id) also binds 'previous', so this very condition` + - ` evaluates as authored. Dropping 'previous' from the condition unblocks the batch too, but it` + - ` changes what the hook MEANS: a transition ("just became done") becomes a state test ("is done"),` + - ` which fires on rows that were already done.`; - } else if (declaredMissingKey) { - limitation = 'bulk_write_stored_state_unavailable'; - bulkDetail = - ` '${declaredMissingKey}' IS declared on this object, but this is the` + - ` ${beforePhase ? `'${ctx.event}'` : 'batch'} dispatch of a PREDICATE bulk write (multi: true):` + - ` the stored state of the matched rows is not in hand, so 'record' carries only this write's` + - ` payload.` + - perRowSentence + - ` Otherwise reference only fields this write sets, or target the write at one record (update by id).`; - } - if (bulkDetail !== undefined) { - return new HookConditionError(`${head}${typoDetail}${bulkDetail}`, { - hook: meta.name, - object: ctx.object, - event: ctx.event, - condition: source, - reason: 'unevaluable', - fault: summary, - ...(missingKey ? { missingKey } : {}), - predicateBulkWrite: true, - limitation, - }); - } - } - return new HookConditionError(`${head}${detail}`, { hook: meta.name, object: ctx.object, diff --git a/packages/objectql/src/index.ts b/packages/objectql/src/index.ts index f54841f5b9..340b22f410 100644 --- a/packages/objectql/src/index.ts +++ b/packages/objectql/src/index.ts @@ -123,7 +123,10 @@ export { applyInMemoryAggregation, bucketDateValue } from './in-memory-aggregati export { bindHooksToEngine } from './hook-binder.js'; export type { BindHooksOptions, BindHooksResult } from './hook-binder.js'; export { wrapDeclarativeHook, HookConditionError } from './hook-wrappers.js'; -export type { WrapDeclarativeOptions, HookConditionLimitation } from './hook-wrappers.js'; +// `HookConditionLimitation` was exported here until #5574 and is RETIRED — +// see the note above `HookConditionError` in `hook-wrappers.ts`. Its two members +// described a batch-scoped `before*` dispatch that no longer exists. +export type { WrapDeclarativeOptions } from './hook-wrappers.js'; // Export Validation export { ValidationError, validateRecord } from './validation/record-validator.js'; diff --git a/packages/objectql/src/plugin.integration.test.ts b/packages/objectql/src/plugin.integration.test.ts index 34b4b944e7..4391003e36 100644 --- a/packages/objectql/src/plugin.integration.test.ts +++ b/packages/objectql/src/plugin.integration.test.ts @@ -1597,7 +1597,7 @@ describe('ObjectQLPlugin - Metadata Service Integration', () => { return { objectql, bulkUpdates, getFindCalls: () => findCalls }; } - it('rejects the whole batch on a prior-free format rule, with no row fetch', async () => { + it('rejects the whole batch on a prior-free format rule (#5574: the row set IS read once)', async () => { const { objectql, bulkUpdates, getFindCalls } = await bootWithValidationCapture([], { name: 'vr_acct', label: 'VR Acct', datasource: 'vr-capture', fields: { email: { name: 'email', label: 'Email', type: 'text' } }, @@ -1609,7 +1609,19 @@ describe('ObjectQLPlugin - Metadata Service Integration', () => { { multi: true, where: { status: 'active' }, context: { userId: 'user-9' } }, )).rejects.toThrow(/valid email/); expect(bulkUpdates.length).toBe(0); // nothing written - expect(getFindCalls()).toBe(0); // format needs no prior state + // ⚠️ This asserted `0` until #5574 — "format needs no prior state", and + // nothing else on this object demanded the rows either. It is `1` now, + // and the extra read is not slack: this is a KERNEL, so objectql's + // `sys_stamp_audit_update` builtin is registered on `'*'` for + // `beforeUpdate`, and ADR-0058 Addendum II dispatches `before*` once per + // MATCHED ROW — which cannot be done without the matched rows. The ADR + // prices this consequence in as many words ("the demand becomes + // effectively universal"), bounded by the D6 ceiling. + // + // What the pin protects is unchanged and is why the number is asserted at + // all: ONE read, not one per row and not one per consumer. The format + // rule still rejects the whole batch before anything is written. + expect(getFindCalls()).toBe(1); }); it('evaluates a state_machine rule per matched row: one illegal row rejects, all-legal proceeds', async () => { diff --git a/packages/objectql/src/plugin.ts b/packages/objectql/src/plugin.ts index a332d82f6b..047c506d8f 100644 --- a/packages/objectql/src/plugin.ts +++ b/packages/objectql/src/plugin.ts @@ -895,31 +895,33 @@ export class ObjectQLPlugin implements Plugin { } }, }, - { - name: 'sys_fetch_previous_update', - object: '*', - events: ['beforeUpdate'], - priority: 5, - description: 'Auto-fetch the previous record for update hooks', - handler: async (hookCtx: any) => { - if (hookCtx.input?.id && !hookCtx.previous) { - try { - const existing = await this.ql!.findOne(hookCtx.object, { - where: { id: hookCtx.input.id }, - context: { - positions: [], - permissions: [], - isSystem: true, - ...(hookCtx.transaction ? { transaction: hookCtx.transaction } : {}), - } as any, - }); - if (existing) hookCtx.previous = existing; - } catch (_e) { - // Non-fatal: some objects may not support findOne - } - } - }, - }, + // ⛔ RETIRED — `sys_fetch_previous_update` (#5846 (a), delivered with + // #5574's engine half). Do not reintroduce it. + // + // It was registered here on `object: '*'` at priority 5, and on every + // by-id update it issued its own `ql.findOne` to bind + // `hookCtx.previous`, behind the guard `if (input.id && !ctx.previous)`. + // That guard is now PERMANENTLY FALSE: `update()` reads the prior row and + // binds `hookContext.previous` BEFORE dispatching `beforeUpdate` (the + // shape `delete()` has had since #5272), because ADR-0058 Addendum II + // makes the before phase a real reader of that row. A hook whose only + // statement is a guard that can no longer be true is not a safety net, + // it is a second read waiting to be rediscovered — so it goes, rather + // than being left in place "just in case". + // + // What it cost while it stood, measured on #5846: a single by-id update + // on a kernel with plugin-audit read the same row THREE times — this + // builtin, plugin-audit's `captureBefore`, and the engine's own gated + // read. Two of the three were engine reads through the full read pipeline + // (middleware, RLS, field masking) and neither consulted any demand gate. + // This change removes one and makes the engine's the single producer; + // `captureBefore`'s now-redundant read is the identity lane's follow-up. + // + // `sys_fetch_previous_delete` below is NOT retired by the same argument + // and must not be removed as a rider: `delete()` binds `previous` only + // when its own demand gate is true, so the builtin's guard is still + // reachable — see #5929, which is about that gate's `object: '*'` + // breadth, not about this ordering. { name: 'sys_fetch_previous_delete', object: '*', diff --git a/packages/plugins/plugin-auth/src/last-admin-guard.test.ts b/packages/plugins/plugin-auth/src/last-admin-guard.test.ts index fe50a80981..a3a151fd48 100644 --- a/packages/plugins/plugin-auth/src/last-admin-guard.test.ts +++ b/packages/plugins/plugin-auth/src/last-admin-guard.test.ts @@ -380,6 +380,26 @@ describe('[#5892] break-glass: the last unbanned administrator cannot be banned' // Predicate / bulk writes — the shape a by-id guard would miss // --------------------------------------------------------------------------- +/** + * ⚠️ [#5574] These predicate cases are load-bearing in a way they were not when + * they were written, and the reason is worth stating where it will be read. + * + * They used to exercise a BATCH dispatch: one `beforeUpdate` / `beforeDelete` + * call for the whole write, with `input.id` present-but-undefined, so the guard + * fell through to the caller's predicate and saw the whole doomed set. + * ADR-0058 Addendum II made the `before*` phase PER MATCHED ROW, so every + * dispatch now names one administrator — and a guard reading "there is an id, + * so this is a by-id write" approves each one on its own merits (banning one + * admin out of three is legitimate) while the batch bans all three. Measured: + * every case below went green-to-red on the engine change and was restored by + * making `options.multi` outrank the bound id in `resolveTargetIds`. + * + * So: these are not "the bulk variant of the by-id cases". They are the pin + * that a break-glass invariant over a POPULATION survives being asked one row + * at a time. Do not fold them into the by-id cases, and do not rewrite this + * guard to reason from `ctx.previous` — per-row `previous` is exactly the + * information that cannot see a batch. + */ describe('[#5892] the guard holds on predicate (multi) bans, not only by-id', () => { let engine: ObjectQL; diff --git a/packages/plugins/plugin-auth/src/last-admin-guard.ts b/packages/plugins/plugin-auth/src/last-admin-guard.ts index 8a75ac5939..c4e3ee1d11 100644 --- a/packages/plugins/plugin-auth/src/last-admin-guard.ts +++ b/packages/plugins/plugin-auth/src/last-admin-guard.ts @@ -186,24 +186,33 @@ * - **by-id** (`delete(obj, { where: { id } })` — what better-auth's adapter * emits, and what every cascade recursion re-enters with): `input.id` * carries the scalar id. - * - **predicate / `multi`**: `input.id` is present-but-undefined, and the - * CALLER's own options bag is still on `input.options`, predicate included - * — `delete()` only rebuilds that slot into `DriverOptions` *after* the - * `before*` hooks return. That is the same slot the ban half reads, and it - * does not contradict the `HookContextSchema.input` contract table - * (#5273 / #5899): what is unreachable from `input` is the composed - * `ast` — the *effective* predicate, onto which the filters middleware may - * add RLS / sharing scoping. Middleware can only NARROW it, so treating the - * caller's predicate as the doomed set over-approximates it, which is the - * fail-closed direction: this guard may refuse a delete that would have - * removed fewer rows, and can never miss one that removes more. + * - **predicate / `multi`**: the CALLER's own options bag is still on + * `input.options`, predicate and `multi` included — `delete()` only rebuilds + * that slot into `DriverOptions` *after* the `before*` hooks return. That is + * the same slot the ban half reads, and it does not contradict the + * `HookContextSchema.input` contract table (#5273 / #5899): what is + * unreachable from `input` is the composed `ast` — the *effective* + * predicate, onto which the filters middleware may add RLS / sharing + * scoping. Middleware can only NARROW it, so treating the caller's predicate + * as the doomed set over-approximates it, which is the fail-closed + * direction: this guard may refuse a delete that would have removed fewer + * rows, and can never miss one that removes more. + * + * ⚠️ [#5574] `input.id` USED to be present-but-undefined here, and this + * guard used to key on that. It no longer is: ADR-0058 Addendum II + * dispatches `before*` once per MATCHED ROW, each context naming its own + * row, so a predicate write is now indistinguishable from a by-id write by + * the id alone — and reading it that way approved a sweep of every + * administrator one legitimate-looking row at a time. `options.multi` is the + * discriminator, and `resolveTargetIds` asks it FIRST. See that function. * - `ctx.previous` (the engine's #5272 pre-image, and objectql's - * `sys_fetch_previous_delete` builtin — `object: '*'`, priority 5) is bound - * for the by-id shape ONLY; a batch dispatch names no single row, so it - * stays undefined there. The guard therefore never consumes it: it needs the - * target IDS, not a pre-image, and a `previous`-based implementation would - * be correct by-id and blind on exactly the bulk path that can sweep every - * administrator at once. + * `sys_fetch_previous_delete` builtin — `object: '*'`, priority 5) is now + * bound on BOTH shapes: by-id since #5272, and per matched row since #5574. + * The guard still never consumes it, and the reason is sharper than before: + * it needs the target IDS as a SET, and a `previous`-based implementation + * would see exactly one row per dispatch — correct for a single write and + * blind on exactly the bulk path that can sweep every administrator at + * once. * * ## Scope: the ENVIRONMENT, not each organization * @@ -800,15 +809,46 @@ export function registerLastAdminGuard( * guard having to refuse every bulk write on principle. When the resolution * itself cannot be completed (the read throws, or the match set overflows * `maxScan`) `scan` refuses loudly instead of guessing. + * + * ## [#5574] Why `multi` outranks a bound `input.id`, and why that is the + * whole fix + * + * This function used to read "is there an id? then that is the target set", + * and that was sound only because a predicate write's `before*` dispatch left + * `input.id` present-but-UNDEFINED. ADR-0058 Addendum II made the `before*` + * phase per row: a predicate write now dispatches once per MATCHED ROW, each + * context carrying that row's id. Read naively, every one of those dispatches + * looks like a by-id write of a single administrator — and a ban of ONE admin + * out of three is legitimately allowed, so a `multi` ban of ALL THREE passed + * as three separate approvals and locked the environment out. Measured, on + * this file's own #5892 / #5941 / #5978 cases. + * + * The distinction the guard actually needs is UNCHANGED and is the one the + * contract preserves on purpose: `input.options` is the CALLER's bag during + * `before*`, `multi` and `where` included (D2, and `hook.zod.ts`'s PHASE + * rule). So `multi` is asked FIRST — on a predicate write the target set is + * the caller's predicate, whichever row this particular dispatch happens to + * name — and the id is only the answer when the write really is by-id. + * + * This is the general shape of what per-row dispatch does to a guard that + * reasons about a write as a SET rather than about one record: the per-row + * view is strictly less information for that question, and the batch-scoped + * slot is where the question is still answerable. A guard here must never be + * rewritten to reason from `ctx.previous` alone for the same reason. + * + * Cost, named: on a legitimate predicate write this resolves the same set + * once per matched row. It is bounded by `maxScan` and it is the correct + * trade — the alternative is a break-glass guard that is exact and wrong. */ const resolveTargetIds = async ( op: GuardedOp, object: string, id: unknown, - options: { where?: unknown } | undefined, + options: { where?: unknown; multi?: unknown } | undefined, data?: Record, ): Promise> => { - const single = toId(id) ?? toId(data?.id); + const isPredicateWrite = options?.multi === true; + const single = isPredicateWrite ? undefined : (toId(id) ?? toId(data?.id)); if (single) return new Set([single]); const where = options?.where as EngineQueryOptions['where']; const rows = await scan(op, object, { @@ -861,7 +901,7 @@ export function registerLastAdminGuard( const enforce = async ( op: GuardedOp, input: - | { id?: unknown; data?: Record; options?: { where?: unknown } } + | { id?: unknown; data?: Record; options?: { where?: unknown; multi?: unknown } } | undefined, ): Promise => { const words = OP_WORDS[op]; @@ -930,7 +970,7 @@ export function registerLastAdminGuard( op: GuardedOp, table: string, input: - | { id?: unknown; data?: Record; options?: { where?: unknown } } + | { id?: unknown; data?: Record; options?: { where?: unknown; multi?: unknown } } | undefined, patch?: Record, ): Promise => { @@ -996,7 +1036,7 @@ export function registerLastAdminGuard( const guardBan = async (rawCtx: unknown): Promise => { const ctx = (rawCtx ?? {}) as { object?: string; - input?: { id?: unknown; data?: Record; options?: { where?: unknown } }; + input?: { id?: unknown; data?: Record; options?: { where?: unknown; multi?: unknown } }; }; if (ctx.object !== SystemObjectName.USER) return; @@ -1012,7 +1052,7 @@ export function registerLastAdminGuard( const guardDelete = async (rawCtx: unknown): Promise => { const ctx = (rawCtx ?? {}) as { object?: string; - input?: { id?: unknown; options?: { where?: unknown } }; + input?: { id?: unknown; options?: { where?: unknown; multi?: unknown } }; }; if (ctx.object !== SystemObjectName.USER) return; @@ -1031,7 +1071,7 @@ export function registerLastAdminGuard( const ctxOf = (rawCtx: unknown) => (rawCtx ?? {}) as { object?: string; - input?: { id?: unknown; data?: Record; options?: { where?: unknown } }; + input?: { id?: unknown; data?: Record; options?: { where?: unknown; multi?: unknown } }; }; const guardMemberUpdate = async (rawCtx: unknown): Promise => { diff --git a/packages/spec/src/data/bulk-write-hook-conformance.test.ts b/packages/spec/src/data/bulk-write-hook-conformance.test.ts index e4ca4e986c..64a9fd1701 100644 --- a/packages/spec/src/data/bulk-write-hook-conformance.test.ts +++ b/packages/spec/src/data/bulk-write-hook-conformance.test.ts @@ -116,31 +116,39 @@ describe('bulk-write hook dispatch contract — delivery status', () => { expect(byEvent('afterDelete')).toMatchObject({ delivered: true, engineDeliveryIssue: 5038 }); }); - it('records the before half as CONTRACTED but not yet delivered (#5574 engine half)', () => { - // ⚠️ This case is meant to go red exactly once. When the engine half of - // #5574 lands per-row `before*` dispatch, flip both `delivered` flags - // and this expectation in that PR — and move `hook.zod.ts`'s - // `HookContextSchema.input` shape table with it, since that table - // describes the engine as built and is pinned against a real dispatch - // in objectql. A green run here after the engine lands would mean the - // contract and the producer had drifted apart in the direction nobody - // checks (#5273). - expect(byEvent('beforeUpdate')).toMatchObject({ delivered: false, engineDeliveryIssue: 5574 }); - expect(byEvent('beforeDelete')).toMatchObject({ delivered: false, engineDeliveryIssue: 5574 }); + it('records the before half as DELIVERED by #5574\'s engine half', () => { + // ⚠️ This case was written to go red exactly once, and it did. Until + // #5574's engine half it read "CONTRACTED but not yet delivered" and + // asserted `delivered: false` on both `before*` entries — the honest + // face of a contract-first split. The engine landed per-row `before*` + // dispatch, so the flags flipped and this expectation flipped with + // them, in that same PR, alongside `hook.zod.ts`'s + // `HookContextSchema.input` shape table (which describes the engine as + // built and is pinned against a real dispatch in objectql). + expect(byEvent('beforeUpdate')).toMatchObject({ delivered: true, engineDeliveryIssue: 5574 }); + expect(byEvent('beforeDelete')).toMatchObject({ delivered: true, engineDeliveryIssue: 5574 }); + }); + it('has NOTHING undelivered — the contract-first split is closed', () => { + // The pin that replaces the inverted one above. It is the direction + // that keeps mattering: a clause added to this table later with + // `delivered: false` and no engine behind it fails here immediately, + // instead of standing unnoticed the way #5273's three keys did. const undelivered = BULK_WRITE_HOOK_DISPATCH_CONTRACT.filter((e) => !e.delivered); - expect(undelivered.map((e) => e.event)).toEqual(['beforeUpdate', 'beforeDelete']); - expect(undelivered.every((e) => e.phase === 'before')).toBe(true); + expect(undelivered.map((e) => e.event)).toEqual([]); + expect(BULK_WRITE_HOOK_DISPATCH_CONTRACT.every((e) => e.delivered)).toBe(true); }); }); describe('bulk-write per-row hook budget (D6)', () => { it('carries the ceiling objectql enforces today', () => { - // Receipt: `ObjectQL.MAX_BULK_PER_ROW_HOOK_ROWS = 10_000` - // (`packages/objectql/src/engine.ts`, #5038). Two definitions until the - // engine half reads this one; this number is what keeps them agreeing - // meanwhile, so changing it here without changing it there is the drift - // to catch. + // Receipt: `ObjectQL.MAX_BULK_PER_ROW_HOOK_ROWS` (`engine.ts`) is a + // RE-EXPORT of this constant since #5574's engine half, and + // `assertBulkPerRowHookBudget` raises what `resolveBulkPerRowHookBudget` + // decides — so there is one definition and nothing left to keep in + // step. The number is still pinned because it is a contract an + // operator reads out of a rejection message, not an implementation + // detail free to drift. expect(MAX_BULK_PER_ROW_HOOK_ROWS).toBe(10_000); }); diff --git a/packages/spec/src/data/bulk-write-hook-conformance.ts b/packages/spec/src/data/bulk-write-hook-conformance.ts index 3fbddadd2d..407f4d6c03 100644 --- a/packages/spec/src/data/bulk-write-hook-conformance.ts +++ b/packages/spec/src/data/bulk-write-hook-conformance.ts @@ -10,8 +10,9 @@ * #4862 / #5037 / #5038) settled the `after*` half and #5038 delivered it. * Addendum II (#5574's maintainer ruling B, 2026-08-06, landed by #6462) * extends the same per-row semantics to the `before*` half, and this module is - * that ruling's contract face — the spec half of a deliberate contract-first - * split, with the engine half tracked as #5574's engine card. + * that ruling's contract face. It landed as the spec half of a deliberate + * contract-first split; #5574's engine card delivered the producer, so every + * entry below now reads `delivered: true` and the split is closed. * * # Why a module and not one more paragraph of TSDoc * @@ -27,8 +28,13 @@ * mistaken for "delivered". So the two live apart and say different things. * {@link BULK_WRITE_HOOK_DISPATCH_CONTRACT} carries a `delivered` flag per * event, and `bulk-write-hook-conformance.test.ts` pins exactly which entries - * are still false — so the engine half cannot land without turning this file's - * pin red and flipping the flag in the same PR. + * are false — so the engine half could not land without turning this file's + * pin red and flipping the flag in the same PR. It did, and it did: the + * `before*` flags were flipped by #5574's engine half, and the pin now asserts + * that NOTHING is undelivered. The flag stays on the entries rather than being + * deleted with the gap it recorded, so the contract-first split remains + * readable as a decision (Prime Directive #13) and a future clause added the + * same way has a slot to be honest in. * * # The decision, in full * @@ -208,7 +214,7 @@ export const BULK_WRITE_HOOK_DISPATCH_CONTRACT: readonly BulkWriteHookDispatchCo phase: 'before', contextKeys: ['id', 'data', 'options', 'previous'], payloadScope: 'batch', - delivered: false, + delivered: true, engineDeliveryIssue: 5574, }, { @@ -216,7 +222,7 @@ export const BULK_WRITE_HOOK_DISPATCH_CONTRACT: readonly BulkWriteHookDispatchCo phase: 'before', contextKeys: ['id', 'options', 'previous'], payloadScope: 'none', - delivered: false, + delivered: true, engineDeliveryIssue: 5574, }, { @@ -247,13 +253,13 @@ export const BULK_WRITE_HOOK_DISPATCH_CONTRACT: readonly BulkWriteHookDispatchCo * a whole table becomes an unbounded fan-out of handler executions inside a * single write. * - * ⚠️ Two definitions today, one on purpose and only until the engine half - * lands: objectql holds the same literal as `ObjectQL.MAX_BULK_PER_ROW_HOOK_ROWS` - * (`engine.ts`, #5038) and open-codes the refusal. The engine half of #5574 - * replaces that static and that message with this module, at which point the - * ceiling has one definition again. Until then they agree because - * `bulk-write-hook-conformance.test.ts` pins the number, not because anything - * structural makes them. + * ONE definition, since #5574's engine half. `ObjectQL.MAX_BULK_PER_ROW_HOOK_ROWS` + * (`engine.ts`) is now a re-export of this constant and + * `ObjectQL.assertBulkPerRowHookBudget` is the raising half of + * {@link resolveBulkPerRowHookBudget} — the engine raises, the contract + * decides. Between #5038 and then the same literal and the same message were + * written down twice, agreeing only because a pin in + * `bulk-write-hook-conformance.test.ts` said so. */ export const MAX_BULK_PER_ROW_HOOK_ROWS = 10_000; diff --git a/packages/spec/src/data/hook.zod.ts b/packages/spec/src/data/hook.zod.ts index 4a0f2e4c04..23eb7438c4 100644 --- a/packages/spec/src/data/hook.zod.ts +++ b/packages/spec/src/data/hook.zod.ts @@ -335,10 +335,10 @@ export const HookContextSchema = lazySchema(() => z.object({ * - find (also fires for findOne): { ast: QueryAST, options: see PHASE below } * - insert (one context per row, batch inserts included): { data: Record, options: see PHASE below } * - update (single id): { id: ID, data: Record, options: see PHASE below } - * - update (bulk, multi:true) — before: { id: undefined, data: Record, options: EngineUpdateOptions } + * - update (bulk, multi:true) — before, PER MATCHED ROW: { id: ID, data: Record, options: EngineUpdateOptions } * - update (bulk, multi:true) — after, PER MATCHED ROW: { id: ID, data: Record, options: DriverOptions } * - delete (single id): { id: ID, options: see PHASE below } - * - delete (bulk, multi:true) — before: { id: undefined, options: EngineDeleteOptions } + * - delete (bulk, multi:true) — before, PER MATCHED ROW: { id: ID, options: EngineDeleteOptions } * - delete (bulk, multi:true) — after, PER MATCHED ROW: { id: ID, options: DriverOptions } * * PHASE — `input.options` is the one slot whose TYPE depends on when you read @@ -357,24 +357,36 @@ export const HookContextSchema = lazySchema(() => z.object({ * Measured and pinned in the same contract test as the rest of this table. * * A bulk (`multi: true`) update/delete fires the SAME `beforeUpdate`/ - * `beforeDelete` events as a single-id write, ONCE for the whole batch; - * there is no separate `*Many` event. `input.id` is present but `undefined` - * there — binding it is precisely the test the engine dispatches on, so a - * `before*` handler that sets it REROUTES the write onto the single-id path. + * `beforeDelete` events as a single-id write; there is no separate `*Many` + * event. Since #5574's engine half it fires them once PER MATCHED ROW, each + * on a single-record-shaped context carrying that row's `id` and `previous` + * — the same move #5038 made for the `after*` phase, held to the same + * yardstick. Zero matched rows is zero dispatches. The full clause set + * (D1–D7) with its budget ceiling is `data/bulk-write-hook-conformance.ts` + * (ADR-0058 Addendum II). * - * ⚠️ CONTRACTED TO CHANGE — the two `before` rows above and the paragraph - * just above them describe the engine as it is TODAY, which is what this - * table is for. #5574's maintainer ruling (2026-08-06, option B) extends the - * per-row bulk-write contract from the `after*` phase to the `before*` phase: - * a predicate write will dispatch `beforeUpdate`/`beforeDelete` once per - * matched row, each carrying that row's `previous` and `id`, over a payload - * that stays BATCH-scoped. The contract is stated — with its budget ceiling - * and its `delivered` flags — in `data/bulk-write-hook-conformance.ts` - * (ADR-0058 Addendum II); the engine half is #5574's engine card, and it - * moves these rows in the same PR that makes them false. Until then, a - * `before*` handler on a bulk write has NO `previous`: a guard written as - * `previous?.x` passes silently on every batch, which is the measured harm - * the ruling is about. + * Two things a reader of the rows above still has to know: + * + * - The PAYLOAD stays BATCH-scoped (D3). Every per-row `beforeUpdate` + * context carries THE one payload, not a copy — `driver.updateMany` takes + * one SET clause for N rows — so a rewrite applies to the whole batch + * whichever row's dispatch made it, and rewrites accumulate in dispatch + * order. A rewrite CONDITIONED on the row is therefore out of contract: + * it widens to every matched row instead of scoping itself. Per-row + * `previous` is supplied so a guard can REFUSE (throw), not so a rewrite + * can be aimed. + * - `input.id` is NOT a reroute lever (D4). It used to be: on the batch + * dispatch `input.id` was present-but-`undefined`, and binding it moved + * the write onto the single-id path. A per-row context arrives with `id` + * already bound and the dispatch already decided, so rebinding retargets + * nothing — and objectql REFUSES it (`HookTargetRebindError`) rather than + * ignoring it, on the by-id path too. A silent no-op is the failure this + * family exists to abolish. + * + * What the change fixed, recorded because the failure direction is the + * dangerous one: a `before*` handler on a bulk write used to have NO + * `previous`, so a guard written as `previous?.x` passed silently on every + * batch — fail-OPEN, and invisible. * * The row-scoping predicate a bulk write EXECUTES is not reachable from * `input` at all. It is the composed `ast`, which lives on the @@ -389,20 +401,22 @@ export const HookContextSchema = lazySchema(() => z.object({ * approximation. That is the safe direction for a fail-closed guard (it may * refuse a write that would have touched fewer rows; it can never miss one * that touches more) and the wrong direction for anything that needs the - * effective set exactly, which should work per row on the `after*` events - * below instead. Both of `plugin-auth`'s break-glass last-admin guards - * (#5892 ban half, #5941 delete half) are built on that upper bound, and - * objectql's own `isPredicateBulkWrite` (`hook-wrappers.ts`, #5038/#4775) - * reads `input.options.multi` from the same slot to tell a batch dispatch - * from a per-row one. Do not narrow `input.options` on the `before*` paths - * without re-reading all three. + * effective set exactly, which should work per row instead — on the + * `before*` events too, since #5574. Both of `plugin-auth`'s break-glass + * last-admin guards (#5892 ban half, #5941 delete half) are built on that + * upper bound, so do not narrow `input.options` on the `before*` paths + * without re-reading both. (objectql's `isPredicateBulkWrite` read the same + * slot to tell a batch dispatch from a per-row one; it is retired with the + * batch dispatch — `hook-wrappers.ts` records why.) * * Since #5038 (ADR-0058's bulk-write addendum) the `after*` events on a bulk * write dispatch ONCE PER MATCHED ROW, each on a single-record-shaped * context — `input.id` names that row, `previous` is its pre-image and * `result` its post-state — so a handler written for a single-id write needs - * no bulk-aware branch of its own. The batch-level context keeps the - * affected COUNT as `result` and is what the call itself resolves (#4639). + * no bulk-aware branch of its own. The `before*` events joined them in #5574, + * with `result` absent — the before phase has no post-state. The batch-level + * context keeps the affected COUNT as `result` and is what the call itself + * resolves (#4639); no handler is dispatched on it any more. */ input: z.record(z.string(), z.unknown()).describe('Mutable input parameters'), diff --git a/scripts/adr-anchors.json b/scripts/adr-anchors.json index d594136c7f..26cf535bb3 100644 --- a/scripts/adr-anchors.json +++ b/scripts/adr-anchors.json @@ -19,10 +19,11 @@ { "file": "packages/objectql/src/hook-wrappers.ts", "adrs": [ + "ADR-0049", "ADR-0058", "ADR-0112" ], - "invariant": "An unevaluable hook `condition` ABORTS the operation (ADR-0058's write-path addendum, #4775) — `onError` never sees it and cannot soften it back into a silent skip. On a predicate (`multi: true`) bulk write the rejection must name the LIMITATION, not the author. Since #5038 that rejection is scoped to the BATCH dispatch — the `before*` phase, which fires once for N rows because it may still rewrite the shared payload; after-hooks are dispatched per row with `previous` bound (ADR-0058's bulk-write addendum), so they never reach it. The message must therefore name the phase as the reason and point at the matching `after*` event, and must NOT promise an expiry that already happened. The discriminator is `limitation`, deliberately NOT `code` — ADR-0112 makes `error.code` a closed wire vocabulary and the REST layer promotes a thrown error's `.code` onto the envelope, so naming it `code` would mint an unregistered wire code by side effect." + "invariant": "An unevaluable hook `condition` ABORTS the operation (ADR-0058's write-path addendum, #4775) — `onError` never sees it and cannot soften it back into a silent skip. The BULK-WRITE branch of that diagnosis is RETIRED (#5574, ADR-0058 Addendum II): a predicate (`multi: true`) write dispatches `before*` once per matched row now, exactly as `after*` has since #5038, so every dispatch carries a bound `previous` and a `previous`-reading condition evaluates as authored instead of rejecting the batch. With no batch-scoped dispatch left, `isPredicateBulkWrite` had no true case, and `HookConditionLimitation` (`bulk_write_previous_unbound`, `bulk_write_stored_state_unavailable`) plus the `predicateBulkWrite` flag had neither producer nor reachable consumer — removed under ADR-0049 enforce-or-remove, with the reasoning pinned in the file. Do not reintroduce them: the honest signal that a dispatch cannot bind `previous` is that no such dispatch exists. What survives unchanged is the plain diagnosis for the case that IS the author's (a condition naming a key the record does not carry), and the rule that its discriminators are never `error.code` — ADR-0112 makes `error.code` a closed wire vocabulary and the REST layer promotes a thrown error's `.code` onto the envelope, so naming one `code` would mint an unregistered wire code by side effect." }, { "file": "packages/spec/src/data/bulk-write-hook-conformance.ts", From 55536844ae6ab6b987f19fb219e3786322d8a7ab Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 12:29:36 +0000 Subject: [PATCH 2/4] chore(objectql,scripts): satisfy the erasure ratchet and the ADR-0112 anchor (#5574) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The new bulk-write before-phase cases passed their engine options through `as any`. They are ordinary in-contract calls (`EngineUpdateOptions` / `EngineDeleteOptions` both declare `multi` and `where`), so they are typed instead: the test surface returns to its 263 ceiling rather than being raised for tests that never needed the erasure. - The `update()` restructure removed three `any`-erased engine option sites, so `packages/objectql/src/engine.ts` ratchets 12 → 9 in the baseline. - `hook-wrappers.ts` lost its ADR-0112 reference when the batch-write branch of the condition diagnosis was retired. The rule it anchored did NOT go with the branch — no discriminator on `HookConditionError` is ever `error.code`, for every field the class still carries — so it is restated where it now applies. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013ZgeUyxzRnXzNCq8vizVoQ --- .../src/bulk-write-per-row-hooks.test.ts | 60 +++++++++---------- packages/objectql/src/hook-wrappers.ts | 13 ++++ scripts/query-options-erasure-baseline.json | 2 +- 3 files changed, 44 insertions(+), 31 deletions(-) diff --git a/packages/objectql/src/bulk-write-per-row-hooks.test.ts b/packages/objectql/src/bulk-write-per-row-hooks.test.ts index d3f6f27665..3370b0c7d3 100644 --- a/packages/objectql/src/bulk-write-per-row-hooks.test.ts +++ b/packages/objectql/src/bulk-write-per-row-hooks.test.ts @@ -471,7 +471,7 @@ describe('[#5574 / D1] a bulk write fires before-hooks once per matched row', () { title: 'c', status: 'todo' }, ]); - await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } }); expect(seen).toHaveLength(3); expect(seen.sort()).toEqual(rows.map((r) => String(r.id)).sort()); @@ -490,7 +490,7 @@ describe('[#5574 / D1] a bulk write fires before-hooks once per matched row', () ]); const doomed = rows.filter((r) => r.status === 'stale').map((r) => String(r.id)); - await engine.delete('task', { multi: true, where: { status: 'stale' } } as any); + await engine.delete('task', { multi: true, where: { status: 'stale' } }); expect(seen.sort()).toEqual(doomed.sort()); }); @@ -503,7 +503,7 @@ describe('[#5574 / D1] a bulk write fires before-hooks once per matched row', () const { engine } = await boot([hook('per_row_before', 'beforeUpdate', () => { seen.push('x'); })]); await seedTasks(engine, [{ title: 'a', status: 'done' }]); - await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'nothing_matches' } } as any); + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'nothing_matches' } }); expect(seen).toEqual([]); }); @@ -520,7 +520,7 @@ describe('[#5574 / D1] a bulk write fires before-hooks once per matched row', () ]); await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); - await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } }); expect(withPrevious).toHaveLength(2); expect(withoutPrevious).toHaveLength(2); @@ -531,7 +531,7 @@ describe('[#5574 / D1] a bulk write fires before-hooks once per matched row', () const { engine } = await boot([hook('per_row_before', 'beforeUpdate', () => { seen.push('x'); })]); const row: any = await engine.insert('task', { title: 'a', status: 'todo' }); - await engine.update('task', { status: 'done' }, { where: { id: row.id } } as any); + await engine.update('task', { status: 'done' }, { where: { id: row.id } }); expect(seen).toEqual(['x']); }); @@ -553,7 +553,7 @@ describe('[#5574 / D2] the per-row before context is the SINGLE-RECORD shape', ( { title: 'b', status: 'todo', owner: 'u2' }, ]); - await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } }); expect(seen).toHaveLength(2); for (const { id, prev } of seen) { @@ -578,12 +578,12 @@ describe('[#5574 / D2] the per-row before context is the SINGLE-RECORD shape', ( ]); await expect( - engine.update('task', { status: 'done' }, { multi: true, where: {} } as any), + engine.update('task', { status: 'done' }, { multi: true, where: {} }), ).rejects.toThrow(/row is locked/); // The guard fired BEFORE the write, so nothing was written at all — the // refusal covers the whole batch, not just the offending row. - expect(await engine.count('task', { where: { status: 'done' } } as any)).toBe(0); + expect(await engine.count('task', { where: { status: 'done' } })).toBe(0); }); it('carries NO `result` — the before phase has no post-state', async () => { @@ -591,7 +591,7 @@ describe('[#5574 / D2] the per-row before context is the SINGLE-RECORD shape', ( const { engine } = await boot([hook('probe', 'beforeUpdate', (ctx) => { seen.push(ctx.result); })]); await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); - await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } }); expect(seen).toEqual([undefined, undefined]); }); @@ -605,7 +605,7 @@ describe('[#5574 / D3] the payload stays BATCH-scoped, and that IS the merge rul })]); await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); - await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } }); expect(payloads).toHaveLength(2); // Reference identity, deliberately. Copies are what a reconciliation step @@ -623,11 +623,11 @@ describe('[#5574 / D3] the payload stays BATCH-scoped, and that IS the merge rul { title: 'b', status: 'todo', owner: 'u2' }, ]); - await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } }); // Both rows got it, including the one whose dispatch did not make it. This // is the contract, not a leak: one `updateMany` carries one SET clause. - const rows: any[] = await engine.find('task', {} as any); + const rows: any[] = await engine.find('task', {}); expect(rows.map((r) => r.owner)).toEqual(['stamped', 'stamped']); }); @@ -646,10 +646,10 @@ describe('[#5574 / D3] the payload stays BATCH-scoped, and that IS the merge rul { title: 'c', status: 'todo' }, ]); - await engine.update('task', { title: '', status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + await engine.update('task', { title: '', status: 'done' }, { multi: true, where: { status: 'todo' } }); // Three dispatches, three appends, one payload — every row gets '+++'. - const rows: any[] = await engine.find('task', {} as any); + const rows: any[] = await engine.find('task', {}); expect(rows.map((r) => r.title)).toEqual(['+++', '+++', '+++']); }); @@ -677,7 +677,7 @@ describe('[#5574 / D4] `input.id` is not a reroute lever, and is refused loudly' await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); const err = await engine - .update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any) + .update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } }) .then(() => null, (e) => e); expect(err).toBeInstanceOf(HookTargetRebindError); @@ -687,7 +687,7 @@ describe('[#5574 / D4] `input.id` is not a reroute lever, and is refused loudly' expect(err.observedId).toBe('somewhere_else'); // Refused, not ignored — a silent no-op is the failure this family exists // to abolish. And nothing was written. - expect(await engine.count('task', { where: { status: 'done' } } as any)).toBe(0); + expect(await engine.count('task', { where: { status: 'done' } })).toBe(0); }); it('REFUSES a by-id handler that clears `input.id` — the retired conversion', async () => { @@ -697,14 +697,14 @@ describe('[#5574 / D4] `input.id` is not a reroute lever, and is refused loudly' const row: any = await engine.insert('task', { title: 'a', status: 'todo' }); const err = await engine - .update('task', { status: 'done' }, { where: { id: row.id } } as any) + .update('task', { status: 'done' }, { where: { id: row.id } }) .then(() => null, (e) => e); expect(err).toBeInstanceOf(HookTargetRebindError); expect(err.path).toBe('by-id'); expect(err.expectedId).toBe(row.id); expect(err.message).toContain('RETIRED'); - expect((await engine.findOne('task', { where: { id: row.id } } as any) as any).status).toBe('todo'); + expect((await engine.findOne('task', { where: { id: row.id } }) as any).status).toBe('todo'); }); it('REFUSES a by-id `beforeDelete` handler that repoints the target', async () => { @@ -718,13 +718,13 @@ describe('[#5574 / D4] `input.id` is not a reroute lever, and is refused loudly' const row: any = await engine.insert('task', { title: 'a', status: 'todo' }); const err = await engine - .delete('task', { where: { id: row.id } } as any) + .delete('task', { where: { id: row.id } }) .then(() => null, (e) => e); expect(err).toBeInstanceOf(HookTargetRebindError); expect(err.event).toBe('beforeDelete'); expect(err.path).toBe('by-id'); - expect(await engine.count('task', {} as any)).toBe(1); + expect(await engine.count('task', {})).toBe(1); }); it('leaves an untouched `input.id` alone — the refusal is not a trap', async () => { @@ -755,12 +755,12 @@ describe('[#5574 / D5] equivalence with the single-id path', () => { const bulkSeen: unknown[] = []; const bulk = await boot([hook('probe', 'beforeUpdate', (ctx) => { bulkSeen.push(shapeOf(ctx)); })]); await seedTasks(bulk.engine, [{ title: 'a', status: 'todo' }]); - await bulk.engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + await bulk.engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } }); const singleSeen: unknown[] = []; const single = await boot([hook('probe', 'beforeUpdate', (ctx) => { singleSeen.push(shapeOf(ctx)); })]); const row: any = await single.engine.insert('task', { title: 'a', status: 'todo' }); - await single.engine.update('task', { status: 'done' }, { where: { id: row.id } } as any); + await single.engine.update('task', { status: 'done' }, { where: { id: row.id } }); // The declared differences (D5) are `input.options` carrying `multi`/`where`, // the affected COUNT, the aggregate event and D4 — none of which this @@ -778,7 +778,7 @@ describe('[#5574 / D6] one ceiling, BOTH phases, checked before the first dispat await seedTasks(engine, Array.from({ length: over }, (_, i) => ({ title: `t${i}`, status: 'todo' }))); const err = await engine - .update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any) + .update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } }) .then(() => null, (e) => e); expect(err.code).toBe(BULK_PER_ROW_HOOK_LIMIT_ERROR_CODE); @@ -789,7 +789,7 @@ describe('[#5574 / D6] one ceiling, BOTH phases, checked before the first dispat expect(err.message).toContain('NOT silently downgraded'); // Refusal BEFORE the first dispatch — not after running `over` of them. expect(fired).toEqual([]); - expect(await engine.count('task', { where: { status: 'todo' } } as any)).toBe(over); + expect(await engine.count('task', { where: { status: 'todo' } })).toBe(over); }); it('refuses an over-ceiling bulk DELETE on the before phase too', async () => { @@ -798,13 +798,13 @@ describe('[#5574 / D6] one ceiling, BOTH phases, checked before the first dispat await seedTasks(engine, Array.from({ length: over }, (_, i) => ({ title: `t${i}`, status: 'stale' }))); const err = await engine - .delete('task', { multi: true, where: { status: 'stale' } } as any) + .delete('task', { multi: true, where: { status: 'stale' } }) .then(() => null, (e) => e); expect(err.code).toBe(BULK_PER_ROW_HOOK_LIMIT_ERROR_CODE); expect(err.event).toBe('beforeDelete'); expect(fired).toEqual([]); - expect(await engine.count('task', {} as any)).toBe(over); + expect(await engine.count('task', {})).toBe(over); }); it('ADMITS a batch exactly AT the ceiling, in both phases', async () => { @@ -845,7 +845,7 @@ describe('[#5574 / D7] the matched row set is read ONCE, and serves everything', ]); driver.findCalls.length = 0; - await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } }); // Four rows, eight dispatches, ONE query. The ruling forbids a second fetch // in as many words. @@ -860,7 +860,7 @@ describe('[#5574 / D7] the matched row set is read ONCE, and serves everything', await seedTasks(engine, [{ title: 'a', status: 'stale' }, { title: 'b', status: 'stale' }]); driver.findCalls.length = 0; - await engine.delete('task', { multi: true, where: { status: 'stale' } } as any); + await engine.delete('task', { multi: true, where: { status: 'stale' } }); expect(driver.findCalls).toHaveLength(1); }); @@ -872,7 +872,7 @@ describe('[#5574 / D7] the matched row set is read ONCE, and serves everything', await seedTasks(engine, [{ title: 'a', status: 'todo' }]); driver.findCalls.length = 0; - await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } }); expect(driver.findCalls).toHaveLength(0); }); @@ -882,7 +882,7 @@ describe('[#5574 / D7] the matched row set is read ONCE, and serves everything', await seedTasks(engine, [{ title: 'a', status: 'todo' }]); driver.findCalls.length = 0; - await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } } as any); + await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } }); expect(driver.findCalls).toHaveLength(1); }); diff --git a/packages/objectql/src/hook-wrappers.ts b/packages/objectql/src/hook-wrappers.ts index 29fdb94274..41982b9e44 100644 --- a/packages/objectql/src/hook-wrappers.ts +++ b/packages/objectql/src/hook-wrappers.ts @@ -152,6 +152,19 @@ const noopLogger: HookDiagnosticsLogger = { * The `HookConditionError` that carried them is NOT retired — an unevaluable * or uncompilable condition still aborts the operation (#4775). Only the * bulk-write branch of its diagnosis is gone. + * + * ## What did NOT change with them: no discriminator here is ever `error.code` + * + * `limitation` was deliberately not named `code`, and that rule OUTLIVES it — + * it governs every field this class carries now (`reason`, `fault`, + * `missingKey`) and any future one. ADR-0112 makes `error.code` a CLOSED wire + * vocabulary (`StandardErrorCode` ∪ `ERROR_CODE_LEDGER`, both declared in + * `packages/spec/src/api/`), and `rest-server.ts` promotes a thrown error's + * `.code` straight onto the response envelope. A `.code` added here would + * therefore mint an unregistered wire code by SIDE EFFECT — the exact + * `declared ≠ enforced` shape that vocabulary exists to prevent. If one of + * these facts ever needs to travel on the wire it goes through the ledger, as a + * decision, not as a property that happens to be named `code`. */ export class HookConditionError extends Error { override readonly name = 'HookConditionError'; diff --git a/scripts/query-options-erasure-baseline.json b/scripts/query-options-erasure-baseline.json index 890c716b9f..23f44f214a 100644 --- a/scripts/query-options-erasure-baseline.json +++ b/scripts/query-options-erasure-baseline.json @@ -36,7 +36,7 @@ "packages/metadata-protocol/src/protocol.ts": 6, "packages/metadata-protocol/src/seed-loader.ts": 3, "packages/metadata/src/loaders/database-loader.ts": 6, - "packages/objectql/src/engine.ts": 12, + "packages/objectql/src/engine.ts": 9, "packages/plugins/plugin-approvals/src/approval-service.ts": 10, "packages/plugins/plugin-approvals/src/approver-org-scope.ts": 2, "packages/plugins/plugin-approvals/src/lifecycle-hooks.ts": 4, From cad542c1101d1c7a775c6a95b9c6f55ce52a2e24 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 12:30:33 +0000 Subject: [PATCH 3/4] docs(objectql): record the measured #5929 position of sys_fetch_previous_delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The retirement note next door claimed the delete-side builtin's guard was still reachable. It is not, and the reason is worth handing to #5929 rather than leaving to be rediscovered: the builtin is a 'beforeDelete' hook on '*', so it holds open the very demand gate whose read then binds 'previous' before it runs — its only remaining effect is to make itself redundant. Retiring it stays #5929's card, since that card owns the gate's per-object honesty. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013ZgeUyxzRnXzNCq8vizVoQ --- packages/objectql/src/plugin.ts | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/packages/objectql/src/plugin.ts b/packages/objectql/src/plugin.ts index 047c506d8f..65f1287a58 100644 --- a/packages/objectql/src/plugin.ts +++ b/packages/objectql/src/plugin.ts @@ -917,11 +917,26 @@ export class ObjectQLPlugin implements Plugin { // This change removes one and makes the engine's the single producer; // `captureBefore`'s now-redundant read is the identity lane's follow-up. // - // `sys_fetch_previous_delete` below is NOT retired by the same argument - // and must not be removed as a rider: `delete()` binds `previous` only - // when its own demand gate is true, so the builtin's guard is still - // reachable — see #5929, which is about that gate's `object: '*'` - // breadth, not about this ordering. + // ⚠️ `sys_fetch_previous_delete` below is in the SAME position now, and is + // deliberately left standing because retiring it is #5929's card, not a + // rider on this one. Recording the measurement so that card does not have + // to rediscover it: + // + // `delete()` reads its pre-image when `wantsPreImage` is true, and that + // gate is `hasHooksFor('beforeDelete', object) || hasHooksFor( + // 'afterDelete', object) || summaries`. The builtin is itself a + // `beforeDelete` hook on `'*'`, so it makes the FIRST term true for every + // object — and then the engine's read binds `previous` before the builtin + // runs, so the builtin's own `!ctx.previous` guard is false and it issues + // no `findOne`. It is circular: the builtin's only remaining effect is to + // hold open the gate that makes it redundant. That circularity IS #5929 + // ("the delete-side per-object gate is always true"), and after this + // change its resolution is the same retirement performed above, with the + // same argument. + // + // The one shape where the guard can still be true — the engine read found + // nothing (the row is already gone) — is one where the builtin's read + // finds nothing either, so it changes no binding. { name: 'sys_fetch_previous_delete', object: '*', From a91ca24ed0b8f1805be7db0a591da6dbf637d614 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 14:35:55 +0000 Subject: [PATCH 4/4] refactor(objectql): split the delete-side by-id repoint out of #5574's settlement (#6752) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Amendment II.1 retired the `input.id` reroute lever, and the first cut applied the refusal to `delete()`'s by-id REPOINT as well — folding a behaviour removal into an ordering change. Split out; #5272's re-read is restored verbatim. The two verbs now answer a rebind differently, and the asymmetry is the point rather than a leftover. The case against honouring a rebind is that the write lands on a row whose pre-image, `readonlyWhen` locks and validation rules were never evaluated — and on `delete()` that is simply not true: #5272 already RE-RESOLVES the new target, re-reading its pre-image and rebinding `previous` before `afterDelete` or the summary recompute can see it. `update()` has no such mechanism and would have to grow one, which is the "silently pick re-resolution instead" the ruling forbids. So `update()` refuses and `delete()` keeps honouring, until the repoint is ruled on as its own question. A CLEARED id stays refused on both verbs, and that is not a discretionary choice: it worked by falling through to the predicate branch, and the ladder is now resolved before any handler runs because a per-row `before*` context is built from the matched row set. That is the capability the ruling names. - `engine.ts`: delete() by-id restores the repoint re-read; refuses only a clear. - `hook-target-rebind-errors.ts`, ADR-0058 Amendment II.1 (now a scope table), the changeset: state which cell answers what, and why the row is uneven. - `bulk-write-per-row-hooks.test.ts` §7 D4: the repoint case is inverted into a pin that the REPOINTED row is the one deleted and `afterDelete` sees its pre-image, so neither direction gets "tidied up" by a later reader. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013ZgeUyxzRnXzNCq8vizVoQ --- .changeset/bulk-write-before-hooks-per-row.md | 39 ++++++++++----- .../0058-expression-and-predicate-surface.md | 35 +++++++++++--- .../src/bulk-write-per-row-hooks.test.ts | 47 ++++++++++++++++--- packages/objectql/src/engine.ts | 36 ++++++++++---- .../objectql/src/hook-target-rebind-errors.ts | 21 +++++++++ 5 files changed, 143 insertions(+), 35 deletions(-) diff --git a/.changeset/bulk-write-before-hooks-per-row.md b/.changeset/bulk-write-before-hooks-per-row.md index 8bce73aa15..65ef1bc372 100644 --- a/.changeset/bulk-write-before-hooks-per-row.md +++ b/.changeset/bulk-write-before-hooks-per-row.md @@ -34,17 +34,29 @@ field that the single-id path refuses. therefore out of contract: it widens to the whole batch rather than scoping itself. Per-row `previous` is supplied so a guard can REFUSE, not so a rewrite can be aimed. -- **The `input.id` reroute lever is retired and now refuses.** Clearing - `ctx.input.id` in a `beforeUpdate` handler used to convert a by-id write into a - predicate write over the caller's `where`; rebinding it moved the write to - another row (`delete()` honoured that by re-reading the pre-image). The - dispatch ladder is now resolved **before** the before phase — it has to be, - since per-row contexts are built from the matched row set — so a rebind - retargets nothing. Rather than ignore it (a silent no-op) or honour it (writing - a row whose pre-image, `readonlyWhen` locks and validation rules were never - evaluated), the write is rejected with `HookTargetRebindError` +- **The `input.id` reroute lever is retired and now refuses.** The dispatch + ladder is resolved **before** the before phase — it has to be, since per-row + contexts are built from the matched row set — so the id slot can no longer + steer the write. Rather than ignore an assignment (a silent no-op) or honour + it blindly, the write is rejected with `HookTargetRebindError` (`ERR_HOOK_TARGET_REBIND`), whose message names the retired capability and the - three supported replacements. Recorded as ADR-0058 Amendment II.1. + three supported replacements. Recorded as ADR-0058 Amendment II.1. Precisely: + + | | CLEARED id | REBOUND to another id | + |---|---|---| + | `update()` by-id | refused | refused | + | `delete()` by-id | refused | **honoured, unchanged** (#5272's re-read) | + | either, per-row | refused (D4) | refused (D4) | + + Clearing is uniform because it worked by falling through to the predicate + branch, and that branch is now chosen before any handler runs. Rebinding is + not uniform, deliberately: the case against honouring it is that the write + lands on a row whose pre-image and rules were never evaluated, and on + `delete()` that is simply not true — #5272 already re-resolves the new target + before `afterDelete` or the summary recompute sees it. `update()` has no such + mechanism and building one would be the "silently pick re-resolution instead" + the ruling forbids. Retiring the delete-side repoint is its own question, + filed as #6752 rather than ridden in on an ordering change. **Also in this change.** @@ -74,9 +86,10 @@ field that the single-id path refuses. rejecting the batch. `HookConditionError` itself is unchanged — an unevaluable condition still aborts the operation (#4775). -**Migrating.** A handler that cleared or rebound `ctx.input.id` must instead -write through `ctx.api` / `ctx.ql` for the row it means, have the caller pass -`{ multi: true, where: … }`, or throw to refuse the write. A `beforeUpdate` hook +**Migrating.** A handler that cleared `ctx.input.id` — or rebound it on an +`update()` — must instead write through `ctx.api` / `ctx.ql` for the row it +means, have the caller pass `{ multi: true, where: … }`, or throw to refuse the +write. A `beforeDelete` handler that repoints the target is unaffected. A `beforeUpdate` hook with side effects on an object that receives bulk writes should expect to run per row; a batch-wide effect belongs in a payload rewrite, which is still batch-scoped. diff --git a/docs/adr/0058-expression-and-predicate-surface.md b/docs/adr/0058-expression-and-predicate-surface.md index 3900ca2b5b..7460847f25 100644 --- a/docs/adr/0058-expression-and-predicate-surface.md +++ b/docs/adr/0058-expression-and-predicate-surface.md @@ -332,13 +332,34 @@ > NAMES the retired capability, so an author whose handler stopped working > learns what changed instead of watching a write land somewhere unexpected. > -> **Scope: both verbs, both by-id and per-row.** D4 already stated the per-row -> half. The by-id half is stated here, and it applies to `delete()` as well as -> `update()` — including the repoint `delete()` used to honour by re-reading. -> One rule beats two, and the delete-side re-read had no in-repo consumer (the -> premise was checked against `origin/main`: the only `ctx.input.id` assignment -> in the whole repository was one engine test forcing the fail-closed AST -> assertion, which is now the refusal's own pin). +> **Scope, stated precisely, because the two verbs do NOT answer alike.** +> +> | | CLEARED id | REBOUND to another id | +> |---|---|---| +> | `update()` by-id | refused | refused | +> | `delete()` by-id | refused | **honoured** (#5272's re-read, unchanged) | +> | either, per-row | refused (D4) | refused (D4) | +> +> The CLEARED column is uniform because the ladder reorder leaves it no answer +> of its own: clearing worked by falling through to the predicate branch, and +> that branch is chosen before any handler runs. That is the capability this +> amendment retires, and it is the one the ruling names. +> +> The REBOUND column is not uniform, and the asymmetry is principled rather +> than an oversight. The case against honouring a rebind is that the write would +> land on a row whose pre-image, `readonlyWhen` locks and validation rules were +> never evaluated — and on `delete()` that is simply not true: #5272 already +> RE-RESOLVES the new target, re-reading its pre-image and rebinding `previous` +> before `afterDelete` or the summary recompute can see it. `update()` has no +> such mechanism and would have to grow one, which is the "silently pick +> re-resolution instead" this ruling forbids. So `update()` refuses and +> `delete()` keeps honouring, until the delete-side repoint is ruled on as its +> own question (#6752) — deliberately NOT folded in here as a rider on an +> ordering change. +> +> Premise for the retirement, checked against `origin/main`: the only +> `ctx.input.id` assignment in the whole repository was one engine test forcing +> the fail-closed AST assertion, which is now the refusal's own pin. > > **What replaces it, for each thing it was used for.** Write a different row: > `ctx.api` / `ctx.ql` for that row explicitly. Write many rows: have the caller diff --git a/packages/objectql/src/bulk-write-per-row-hooks.test.ts b/packages/objectql/src/bulk-write-per-row-hooks.test.ts index 3370b0c7d3..71ad6ae368 100644 --- a/packages/objectql/src/bulk-write-per-row-hooks.test.ts +++ b/packages/objectql/src/bulk-write-per-row-hooks.test.ts @@ -707,13 +707,12 @@ describe('[#5574 / D4] `input.id` is not a reroute lever, and is refused loudly' expect((await engine.findOne('task', { where: { id: row.id } }) as any).status).toBe('todo'); }); - it('REFUSES a by-id `beforeDelete` handler that repoints the target', async () => { - // `delete()` used to HONOUR this, by re-reading the pre-image for the new - // target (#5272). ADR-0058 Amendment II.1 settles both verbs the same way: - // `previous` and the summary recompute were computed against the row the - // ladder chose, so honouring a repoint would delete a row none of that saw. - const { engine } = await boot([hook('repoint', 'beforeDelete', (ctx) => { - (ctx.input as any).id = 'other_row'; + it('REFUSES a by-id `beforeDelete` handler that CLEARS the id', async () => { + // The clear is uniform across both verbs, because the ladder reorder leaves + // it no answer of its own: it used to work by falling through to the + // predicate branch, and that branch is now chosen before any handler runs. + const { engine } = await boot([hook('clear', 'beforeDelete', (ctx) => { + (ctx.input as any).id = undefined; })]); const row: any = await engine.insert('task', { title: 'a', status: 'todo' }); @@ -724,9 +723,43 @@ describe('[#5574 / D4] `input.id` is not a reroute lever, and is refused loudly' expect(err).toBeInstanceOf(HookTargetRebindError); expect(err.event).toBe('beforeDelete'); expect(err.path).toBe('by-id'); + expect(err.message).toContain('PREDICATE write'); expect(await engine.count('task', {})).toBe(1); }); + it('still HONOURS a by-id `beforeDelete` REPOINT — deliberately not retired here', async () => { + // ⚠️ The one place the two verbs answer a rebind differently, pinned so the + // asymmetry is a decision on the record rather than something a later + // reader "tidies up" in either direction. + // + // The case against honouring a rebind is that the write lands on a row + // whose pre-image and rules were never evaluated. On `delete()` that is not + // true: #5272 RE-RESOLVES the new target — re-reads its pre-image and + // rebinds `previous` — before `afterDelete` or the summary recompute can + // see it. `update()` has no such mechanism and refusing there is what keeps + // this PR from inventing one (the ruling forbids picking re-resolution + // silently). Retiring the delete-side repoint is its own question, filed as + // #6752. + const seen: unknown[] = []; + const { engine } = await boot([ + hook('repoint', 'beforeDelete', (ctx) => { + if ((ctx.previous as any)?.title === 'decoy') (ctx.input as any).id = String(target.id); + }), + hook('observe', 'afterDelete', (ctx) => { seen.push((ctx.previous as any)?.title); }, undefined, 'task', { priority: 200 }), + ]); + const decoy: any = await engine.insert('task', { title: 'decoy', status: 'todo' }); + const target: any = await engine.insert('task', { title: 'target', status: 'todo' }); + + await engine.delete('task', { where: { id: decoy.id } }); + + // The REPOINTED row is the one that went… + const left: any[] = await engine.find('task', {}); + expect(left.map((r) => r.title)).toEqual(['decoy']); + // …and `previous` was re-read for it, so the after phase describes the row + // actually deleted rather than the one the caller named. + expect(seen).toEqual(['target']); + }); + it('leaves an untouched `input.id` alone — the refusal is not a trap', async () => { // The negative control. A handler that reads the id, or writes back the // SAME id, is doing nothing wrong and must not be refused. diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 6a1c641e40..946bbeae3f 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -7318,14 +7318,34 @@ export class ObjectQL implements IObjectQLEngine { bindPreImage(priorRecord); } await this.triggerHooks('beforeDelete', hookContext); - // [#5574] The retired lever, refused. `previous` — and the summary - // recompute that rides it — were computed against the row the ladder - // chose, so a handler moving the id would delete a row none of that - // ever saw. It used to be honoured by RE-READING the pre-image for the - // new target (and, for a CLEARED id, by falling through to the - // predicate branch); ADR-0058 Addendum II's settlement of the same - // lever on `update()` applies here verbatim, and one rule beats two. - if (hookContext.input.id !== id) { + // A `beforeDelete` hook may still REPOINT the target id, and #5272's + // answer to that is unchanged: the pre-image bound above describes the + // OLD id, so it must not ride into `afterDelete` — or into the summary + // recompute — as though it described the new target. Re-read it. + // + // [#5574] What this PR does NOT do, deliberately: retire the repoint. + // The `update()` twin below refuses a rebind, and the asymmetry is + // principled rather than an oversight — `delete()` has a working + // RE-RESOLUTION for the new target (this block, delivered by #5272 with + // its own pins), so nothing stale reaches a consumer, while `update()` + // has none and would have to grow one. Building that is exactly the + // "silently pick re-resolution instead" the ruling forbids, so the two + // paths answer differently until the repoint itself is ruled on. Filed + // as #6752; do not fold it in as a rider here. + if (wantsPreImage && hookContext.input.id !== id && hookContext.input.id) { + priorRecord = await readPreImage(hookContext.input.id); + bindPreImage(priorRecord); + } + // CLEARING the id is a different question and this PR does settle it, + // because the ladder reorder leaves it no answer of its own: it used to + // convert the write into a PREDICATE delete over the caller's `where` + // by falling through to the branch below, and the ladder is now decided + // before any handler runs (a per-row `before*` context is built from the + // matched row set, so it must be). Ignoring it would delete the + // ORIGINAL row while the handler believes it cancelled the targeting; + // honouring it has nothing left to honour. Refused by name — ADR-0058 + // Amendment II.1, the capability the ruling names. + if (!hookContext.input.id) { throw new HookTargetRebindError({ object, event: 'beforeDelete', path: 'by-id', expectedId: id, observedId: hookContext.input.id, diff --git a/packages/objectql/src/hook-target-rebind-errors.ts b/packages/objectql/src/hook-target-rebind-errors.ts index a27b43970a..b2ce232877 100644 --- a/packages/objectql/src/hook-target-rebind-errors.ts +++ b/packages/objectql/src/hook-target-rebind-errors.ts @@ -42,6 +42,27 @@ * with `id` ALREADY bound to its row, and rebinding it retargets nothing — so * one error serves both paths. * + * ## What this error does NOT cover: `delete()`'s by-id REPOINT + * + * The two verbs answer a rebind differently on the by-id path, and the + * asymmetry is a scope decision rather than an oversight — read it before + * "fixing" it either way. + * + * `delete()` has had a working RE-RESOLUTION for a repointed target since + * #5272: when a `beforeDelete` handler moves `input.id`, the engine re-reads + * that row's pre-image and rebinds `previous`, so nothing stale reaches + * `afterDelete` or the summary recompute. The second bullet above simply is not + * true there. `update()` has no such mechanism, and building one would be the + * "silently pick re-resolution instead" the ruling forbids — so `update()` + * refuses a rebind and `delete()` keeps honouring one, until the repoint itself + * is ruled on as its own question (#6752). + * + * A CLEARED id is a different question and both verbs answer it the same way, + * because the ladder reorder leaves it no answer of its own: clearing used to + * convert the write into a PREDICATE write over the caller's `where`, and the + * ladder is now resolved before any handler runs. That is the capability the + * ruling names, and it is what this error is for on the `delete()` path. + * * ## Why `code` is an `ERR_`-prefixed operational code, not a wire code * * Same reasoning as {@link BULK_PER_ROW_HOOK_LIMIT_ERROR_CODE} next door: