diff --git a/.changeset/multi-update-hook-key-divergence-refusal.md b/.changeset/multi-update-hook-key-divergence-refusal.md new file mode 100644 index 0000000000..27efdcf7d5 --- /dev/null +++ b/.changeset/multi-update-hook-key-divergence-refusal.md @@ -0,0 +1,81 @@ +--- +"@objectstack/objectql": minor +"@objectstack/spec": minor +--- + +fix(objectql,spec): refuse a `multi: true` update whose per-row `beforeUpdate` hooks write divergent key sets (#14099) + +**BREAKING** accept-set narrowing on a published write path, shipped as `minor` +under the repo's launch-window convention for breaking changes. A `multi: true` +update that succeeds today is REFUSED when its `beforeUpdate` handlers assign +different sets of payload keys to different matched rows. + +**What it fixes.** `driver.updateMany` takes one `SET` clause for N rows, so a +predicate update has exactly one payload (ADR-0058 Addendum II D3) — whatever a +`beforeUpdate` handler writes for one row was applied to every matched row. The +transition stamp is the shape this breaks, and it is the standard way to record +when a record entered a state: + +```ts +// beforeUpdate — correct per record, silently wrong on a batch +if (previous.status !== 'done' && next.status === 'done') patch.completed_at = now; +``` + +Measured against published `17.2.0`: two rows, one open and one completed +earlier, updated in a single `multi: true` call. The already-completed row's +`completed_at` moved from `…:26.560Z` to `…:26.571Z`. It never transitioned, +nothing errored, and the corrupted row is byte-for-byte indistinguishable from +one genuinely completed late — so every on-time measure reading the column turns +a compliant record into a breach, with no audit entry and nothing in the data +that shows it happened. The whole class is exposed: `approved_at`, `closed_at`, +`shipped_at`, `first_responded_at`. + +**What changed.** The engine still dispatches the before phase once per matched +row with that row's pre-image, and D3 still stands — the payload stays +batch-scoped and the engine never splits its own write. It now also RECORDS, +per row, the set of payload keys that row's hook chain assigned (the #14088 +provenance recorder, armed once more per row). If two rows disagree, the whole +batch is refused before any write — not after the first row, not inside a +transaction that then rolls back — with the ADR-0112 envelope +`MULTI_UPDATE_HOOK_KEY_DIVERGENCE` (HTTP `400`, +`MultiUpdateHookKeyDivergenceError`), naming the object, the diverging keys and +the remedy. When every row's key set is identical the batch proceeds as one +`updateMany`, exactly as before. + +**The criterion is the key SET, never the values.** That is what keeps honest +batches honest: objectql's own `sys_stamp_audit_update` builtin is registered on +`'*'` and reads the clock inside the per-record stamp, so an ordinary bulk +update writes `updated_at` on every row with different values. Every in-repo +`beforeUpdate` payload rewrite was measured on a mixed batch before this shipped +— the audit stamp (`['updated_at','updated_by']` on every row), plugin-pinyin's +companion projection (`['__search']` on every row) and service-storage's +copy-on-claim (`[]` on every row) — and all three are row-invariant, so none of +them is refused. + +**Migration — how to write a per-record rewrite on a batch.** Two supported +routes, both available in this release: + +1. **Route 2, from inside the handler.** Write the affected records with + `ctx.api`, aimed with the per-row signals the hook sandbox now carries + (`ctx.dispatch.mode === 'per-row'`, `ctx.input.id`, `ctx.input.options`), + and leave the batch payload alone. ⚠️ Those signals are NOT in `17.2.0` — + they land in this same release, which is why the refusal and its + prescription ship together rather than the refusal arriving first. +2. **By-id updates from the caller.** Issue the updates per record when the + value genuinely differs per record. + +`objectstack-ai/hotcrm` and `objectstack-ai/duly` both carry hooks of this +shape and should take route 1: `duly`'s `duly_task.completed_at` stamp is the +measured instance, and hotcrm's `previous`-reading handlers are the same family. + +**Known limit, carried openly rather than hidden.** A handler that writes the +SAME key on every row but with a per-row VALUE (a per-row derived priority, say) +still passes this test, and still applies the last dispatch's value to every +matched row. That is D3's declared cost; the two routes above are the exit for +it, and it is tracked as its own finding. ⛔ It is deliberately NOT closed by +comparing values: a value comparison refuses honest audit-stamp batches +non-deterministically (one clock read per row) and re-opens #14088's own +`completed_at: null` row, where a hook that writes the value the caller also +sent is indistinguishable from a hook that never touched the key. + + diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index bce3f2ceb7..3f5b2606ac 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -109,18 +109,18 @@ that silently does not happen. | # | Behaviour when `isSystem` | Package | What you get / what you lose | Anchor | |:--|:---|:---|:---|:---| -| 18 | **`readonly` strip bypassed — UPDATE, single row** | objectql | Get: a `readonly` field CAN be written. Lose: the protection that stops a caller seeding e.g. `approval_status` | `objectql/src/engine.ts:11204` | -| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:11387` | -| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9939` | +| 18 | **`readonly` strip bypassed — UPDATE, single row** | objectql | Get: a `readonly` field CAN be written. Lose: the protection that stops a caller seeding e.g. `approval_status` | `objectql/src/engine.ts:11289` | +| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:11472` | +| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:10024` | | 21 | **`readonly` strip bypassed — INSERT (protocol ingress)** | metadata-protocol | `isSystem` is the **only** exemption here. `preserveAudit` is deliberately not read on this path (#6640) — a non-system historical import is still stripped on create | `metadata-protocol/src/protocol.ts:1746` | -| 22 | Strict-drop refusal never fires | objectql | Lose: a caller that opted into loud refusal gets **silence** — strict refuses exactly what the strip would have taken, and the strip took nothing | `objectql/src/engine.ts:9987`, `readonly-strict-errors.ts:66` | -| 23 | **Referential-integrity check skipped** | objectql | Get: writes proceed against unreachable/unresolvable targets. Lose: an `isSystem` caller can write a **dangling reference** | `objectql/src/engine.ts:5806` | -| 24 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `objectql/src/engine.ts:3650`, `:3660`, `:3687` | +| 22 | Strict-drop refusal never fires | objectql | Lose: a caller that opted into loud refusal gets **silence** — strict refuses exactly what the strip would have taken, and the strip took nothing | `objectql/src/engine.ts:10072`, `readonly-strict-errors.ts:66` | +| 23 | **Referential-integrity check skipped** | objectql | Get: writes proceed against unreachable/unresolvable targets. Lose: an `isSystem` caller can write a **dangling reference** | `objectql/src/engine.ts:5891` | +| 24 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `objectql/src/engine.ts:3735`, `:3745`, `:3772` | | 25 | Engine-owned / append-only write guard bypassed | plugin-security | Get: generic writes to `managedBy` engine-owned objects | `system-write-guard.ts:96`, `:120` | | 26 | Identity write guard bypassed (ADR-0092) | plugin-auth | Get: direct writes to identity tables through the generic data path | `identity-write-guard.ts:98` | -| 27 | Search-companion column **kept** in a read's rows when it was explicitly requested | objectql | Get: the internal companion column is readable. Lose: nothing for app code — this is the engine reading its own index | `objectql/src/engine.ts:6504` | -| 28 | Dependent-count disclosure on a blocked delete | objectql | Get: the count of blocking children. Nothing was elevated past the caller, so nothing is withheld | `objectql/src/engine.ts:11999` | -| 29 | Reference-cleanup log attributes the write to `'system'` | objectql | Get: an honest actor label instead of `anonymous` when the context carries neither `userId` nor `actor` | `objectql/src/engine.ts:11928` | +| 27 | Search-companion column **kept** in a read's rows when it was explicitly requested | objectql | Get: the internal companion column is readable. Lose: nothing for app code — this is the engine reading its own index | `objectql/src/engine.ts:6589` | +| 28 | Dependent-count disclosure on a blocked delete | objectql | Get: the count of blocking children. Nothing was elevated past the caller, so nothing is withheld | `objectql/src/engine.ts:12084` | +| 29 | Reference-cleanup log attributes the write to `'system'` | objectql | Get: an honest actor label instead of `anonymous` when the context carries neither `userId` nor `actor` | `objectql/src/engine.ts:12013` | ### 3. Sharing (`plugin-sharing`) @@ -179,8 +179,8 @@ a reader tracing where elevation travels needs them. | # | Site | Package | What it does | |:--|:---|:---|:---| -| 62 | `objectql/src/engine.ts:3457` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes | -| 63 | `objectql/src/engine.ts:14348` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag | +| 62 | `objectql/src/engine.ts:3542` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes | +| 63 | `objectql/src/engine.ts:14433` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag | | 64 | `plugin-reports/src/report-service.ts:556` | plugin-reports | Threads the flag into the engine call that runs a report | | 65 | `body-runner.ts:279` | runtime | Rebuilds an `ExecutionContext` from a hook session, carrying the flag across | @@ -195,7 +195,7 @@ assuming `isSystem` covers it is a documented source of bugs. |:---|:---|:---| | "It suppresses triggers / record-change automation" | **No.** Only `skipTriggers` does. A bare `{ isSystem: true }` on a seed write re-fired automation on freshly seeded rows and wedged first boot | `metadata-protocol/src/seed-loader.ts:1971` (rationale at `:1881`–`1883`, #3760), `flow.zod.ts:685` | | "It skips the state machine" | **No.** That is `skipStateMachine`, carried by seed replay and by `treatAsHistorical` imports | `objectql/src/engine.ts` FSM gate; see [State Machine](/docs/protocol/objectql/state-machine) | -| "It skips validation rules" | **No.** Field shape, `format`, `script` and the rest still run. The `readonly` strip runs *before* validation precisely so a discarded value is not judged | `objectql/src/engine.ts:9922`–`9939` | +| "It skips validation rules" | **No.** Field shape, `format`, `script` and the rest still run. The `readonly` strip runs *before* validation precisely so a discarded value is not judged | `objectql/src/engine.ts:10007`–`10024` | | "It preserves a supplied `updated_at` / `updated_by`" | **No.** That is `preserveAudit`, a separate opt-in — and an UPDATE-path exemption only | `field.zod.ts:1516` (#3493 / #6640) | | "It stamps `created_by`" | **No.** Audit stamping reads `userId` from the context. A user-less system write stamps nothing — that is today's behaviour, not an error | `runtime-identity.ts:280`–`281` | | "It bypasses every guard" | **No.** The last-admin guard applies to **every** context, `isSystem` included — the deprovision path that actually locks an org out is the system one | `last-admin-guard.ts:286` | diff --git a/content/docs/references/api/contract.mdx b/content/docs/references/api/contract.mdx index 88ad133f51..867fef07ac 100644 --- a/content/docs/references/api/contract.mdx +++ b/content/docs/references/api/contract.mdx @@ -27,7 +27,7 @@ const result = ApiErrorSchema.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **code** | `Enum<'VALIDATION_ERROR' \| 'INVALID_FIELD' \| 'MISSING_REQUIRED_FIELD' \| 'INVALID_FORMAT' \| 'VALUE_TOO_LONG' \| 'VALUE_TOO_SHORT' \| 'VALUE_OUT_OF_RANGE' \| … +291 more>` | ✅ | Error code (e.g. VALIDATION_ERROR; StandardErrorCode ∪ the ledger the serving side registers — ERROR_CODE_LEDGER for framework packages) | +| **code** | `Enum<'VALIDATION_ERROR' \| 'INVALID_FIELD' \| 'MISSING_REQUIRED_FIELD' \| 'INVALID_FORMAT' \| 'VALUE_TOO_LONG' \| 'VALUE_TOO_SHORT' \| 'VALUE_OUT_OF_RANGE' \| … +292 more>` | ✅ | Error code (e.g. VALIDATION_ERROR; StandardErrorCode ∪ the ledger the serving side registers — ERROR_CODE_LEDGER for framework packages) | | **declaredCode** | `string` | optional | The producer-declared code, verbatim, when it is not a member of the closed `code` vocabulary — the open, author-authored channel (app-specific spellings; ADR-0112) | | **message** | `string` | ✅ | Readable error message | | **userMessage** | `string` | optional | Producer-marked user-facing refusal text, verbatim. Present exactly when the producer opted in at throw time; consumers render it to end users and keep their generic substitution for anything unmarked. Status-agnostic; never replaces `message`. | @@ -220,6 +220,7 @@ const result = ApiErrorSchema.parse(data); * `METADATA_CONFLICT` * `METADATA_NOT_FOUND` * `METADATA_SCHEMA_INVALID` +* `MULTI_UPDATE_HOOK_KEY_DIVERGENCE` * `NAMESPACE_PREFIX` * `NEEDS_PASSWORD` * `NODE_FAILURE` diff --git a/content/docs/references/api/error-code-ledger.mdx b/content/docs/references/api/error-code-ledger.mdx index 5f419edcec..44312c13df 100644 --- a/content/docs/references/api/error-code-ledger.mdx +++ b/content/docs/references/api/error-code-ledger.mdx @@ -336,6 +336,7 @@ const result = ErrorCode.parse(data); * `METADATA_CONFLICT` * `METADATA_NOT_FOUND` * `METADATA_SCHEMA_INVALID` +* `MULTI_UPDATE_HOOK_KEY_DIVERGENCE` * `NAMESPACE_PREFIX` * `NEEDS_PASSWORD` * `NODE_FAILURE` 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 76550d9b1a..8a668c68c3 100644 --- a/packages/objectql/src/bulk-write-per-row-hooks.test.ts +++ b/packages/objectql/src/bulk-write-per-row-hooks.test.ts @@ -614,9 +614,19 @@ describe('[#5574 / D3] the payload stays BATCH-scoped, and that IS the merge rul }); it('a rewrite made on ONE row’s dispatch applies to the WHOLE batch', async () => { - let firstOnly = true; + // [#14099] The fixture — never the contract — changed here. It used to + // stamp `owner` on the FIRST row only (`if (firstOnly) …`), which is now + // REFUSED: writing a key for some matched rows and not others is precisely + // the divergence D3's enforcement rejects before any write, because it can + // only mean the handler is deciding per record. The contract this case + // pins is untouched and is still the reason the refusal exists — one + // `updateMany` carries one SET clause, so whichever dispatch produces a + // value, EVERY row gets it. So the handler writes the same key on every + // row, and the batch still carries ONE value: the last dispatch's. + let dispatches = 0; const { engine } = await boot([hook('stamp', 'beforeUpdate', (ctx) => { - if (firstOnly) { (ctx.input as any).data.owner = 'stamped'; firstOnly = false; } + dispatches += 1; + (ctx.input as any).data.owner = `stamped-${dispatches}`; })]); await seedTasks(engine, [ { title: 'a', status: 'todo', owner: 'u1' }, @@ -625,10 +635,12 @@ describe('[#5574 / D3] the payload stays BATCH-scoped, and that IS the merge rul 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. + // Both rows carry the SECOND dispatch's value, including the row whose own + // dispatch produced `stamped-1`. That is the contract, not a leak — and it + // is the residual hazard #14099's ruling names openly and does not close: + // same key, per-row values still applies one row's value to all of them. const rows: any[] = await engine.find('task', {}); - expect(rows.map((r) => r.owner)).toEqual(['stamped', 'stamped']); + expect(rows.map((r) => r.owner)).toEqual(['stamped-2', 'stamped-2']); }); it('rewrites ACCUMULATE in dispatch order, including a REPLACED payload', async () => { diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 37ab4cb63c..116d840815 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -199,6 +199,10 @@ import { evaluateValidationRules, needsPriorRecord, stripReadonlyWhenFields, str // SAME value. Armed and sealed in `update()`; the module owns the argument for // why neither end may move. import { recordHookPayloadWrites } from './hook-write-provenance.js'; +import { + divergingHookPayloadKeys, + MultiUpdateHookKeyDivergenceError, +} from './multi-update-hook-key-divergence.js'; import type { HookWriteRecording } from './hook-write-provenance.js'; import { resolveMasterDetailRelation } from './master-detail.js'; // [#6457] The master-detail header a `parent`-scoped predicate reads is made @@ -2928,10 +2932,27 @@ export class ObjectQL implements IObjectQLEngine { * 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. + * guard can REFUSE the write (throw), not so a rewrite can be aimed. + * + * ## D3, ENFORCED — divergent key sets refuse the batch [#14099] + * + * That last paragraph used to end "a contract statement, not an enforcement + * — no static rule can decide whether a rewrite is row-invariant". The second + * half is still true and the conclusion no longer follows: nothing STATIC can + * decide it, but the dispatch can MEASURE it. #14088's recorder is armed once + * more per row here, so each dispatch reports the set of payload keys THAT + * row's hook chain assigned; if two rows disagree, the batch is refused whole + * before any write ({@link MultiUpdateHookKeyDivergenceError}). + * + * Maintainer ruling of 2026-09-02, on the corruption `duly` measured against + * published 17.2.0 — a `completed_at` transition stamp moved an + * already-completed row's timestamp, silently, on a two-row batch. The + * criterion is the key SET and never the values, which is what lets the + * clock-reading audit stamp through: every row writes `updated_at`, so an + * honest batch is never refused non-deterministically. The full argument, + * both rejected value-comparison variants, and the blind spot the ruling + * carries openly (same key, per-row VALUES) live on + * `multi-update-hook-key-divergence.ts`. * * ## D4 — `input.id` is not a reroute lever here * @@ -2948,6 +2969,30 @@ export class ObjectQL implements IObjectQLEngine { ): Promise { const schema = this._registry.getObject(object); const carriesPayload = event === 'beforeUpdate'; + // ── [#14099] D3's ENFORCEMENT half: one observation window per row ────── + // + // The #14088 recorder, armed a SECOND time for this loop and NESTED over + // the batch-scoped recording `update()` already armed at its entry. The + // nesting is what makes both readings true at once: a write through this + // view lands on the outer recording's view, which lands on the real + // payload — so the outer record (the one the read-only strips read for + // provenance) still sees every hook write, while `closeWindow()` gives + // this loop what only it needs: the keys assigned by ONE row's chain. + // + // ⭐ ONE recording for the whole loop, not one per row, and that is a + // contract point rather than a saving: every per-row context must carry + // THE SAME payload object (D3, pinned by reference identity in + // `bulk-write-per-row-hooks.test.ts`). A fresh view per row would still be + // write-through onto one payload — no copy, nothing to reconcile — but it + // would hand each row a DIFFERENT object, which is a difference an author + // can observe and which this contract says is not there. + const batchPayloadAtEntry = (batchCtx.input as { data?: unknown }).data; + const hookWrites = + carriesPayload && batchPayloadAtEntry !== null && typeof batchPayloadAtEntry === 'object' + ? recordHookPayloadWrites(batchPayloadAtEntry as Record) + : undefined; + if (hookWrites) (batchCtx.input as { data?: unknown }).data = hookWrites.payload; + const perRowHookWrittenKeys: ReadonlySet[] = []; for (let index = 0; index < rows.length; index++) { const row = rows[index]; const rowId = (row as { id?: unknown }).id; @@ -2976,6 +3021,9 @@ export class ObjectQL implements IObjectQLEngine { // D3, the accumulate half — see the class doc above. if (carriesPayload) { (batchCtx.input as { data?: unknown }).data = (rowCtx.input as { data?: unknown }).data; + // [#14099] and the enforcement half: close THIS row's window, so the + // next row opens a fresh one. + if (hookWrites) perRowHookWrittenKeys.push(hookWrites.closeWindow()); } // D4. const observed = (rowCtx.input as { id?: unknown }).id; @@ -2985,6 +3033,43 @@ export class ObjectQL implements IObjectQLEngine { }); } } + + // [#14099] Close this loop's recording and put the underlying payload back + // in `batchCtx.input.data`, so no recording VIEW travels on to the outer + // seal, the strips or a driver. + // + // `hookWrittenKeys` is `undefined` exactly when a hook REPLACED the payload + // object somewhere in this batch (`hook-write-provenance.ts`'s KNOWN + // LIMIT). Then the whole batch ABSTAINS: the windows collected before the + // replacement describe writes the replacement discarded, so refusing on + // them would be a verdict about a payload that is no longer the one being + // written. Abstaining keeps the pre-#14099 behaviour for that shape, which + // is the same fail-safe direction #14088 chose for the same limit. + const sealedLoopWrites = hookWrites?.seal((batchCtx.input as { data?: unknown }).data); + if (sealedLoopWrites) { + (batchCtx.input as { data?: unknown }).data = sealedLoopWrites.data; + } + + // [#14099] The refusal, ruled 2026-09-02 (recommendation C). Placed after + // the loop and not inside it, for two reasons that are both about the + // envelope rather than about cost: the diverging set is `union` minus + // `intersection` over EVERY row, so the message names every offending key + // instead of the first pair to disagree, and it is order-independent — the + // same batch answers the same way whatever order the driver returned the + // matched rows in. + // + // ⭐ Still BEFORE any write, which is the load-bearing half. This method is + // called from `update()`'s predicate branch ahead of the outer hook-write + // seal, both readonly strips, `evaluateValidationRules` and every + // `driver.updateMany` — and it runs outside `update()`'s own `try`, so the + // envelope reaches the caller undecorated. Not "after the first row", not + // "inside a transaction that then rolls back": nothing was written. + if (sealedLoopWrites?.hookWrittenKeys !== undefined) { + const diverging = divergingHookPayloadKeys(perRowHookWrittenKeys); + if (diverging.length > 0) { + throw new MultiUpdateHookKeyDivergenceError(object, diverging, rows.length); + } + } } /** diff --git a/packages/objectql/src/hook-write-provenance.ts b/packages/objectql/src/hook-write-provenance.ts index 10fb16e6b0..81d7b30d1b 100644 --- a/packages/objectql/src/hook-write-provenance.ts +++ b/packages/objectql/src/hook-write-provenance.ts @@ -91,6 +91,31 @@ export interface HookWriteRecording { * exactly as it always has. */ readonly payload: Record; + /** + * [#14099] Close the current OBSERVATION WINDOW and open a fresh one: the + * keys assigned since the previous `closeWindow()` — or since arming, for + * the first call — and nothing before that. + * + * The cumulative record {@link HookWriteRecording.seal} returns is NOT + * affected, and that separation is the whole point. One recording answers + * two different questions on the predicate-update path, and the answers must + * not be the same set: + * + * - "did a HOOK assign this key at all?" — cumulative, what the read-only + * strips read for provenance (#14088, the reason this module exists); + * - "did THIS row's dispatch assign it?" — windowed, what ADR-0058 + * Addendum II D3's enforcement compares between rows (#14099). A batch of + * N rows closes N windows; a cumulative set could never tell two rows + * apart, because the second row writing the same key does not grow it. + * + * A window mirrors the cumulative record's own rule on removal: a key + * deleted during the window leaves it, so the window means "the keys holding + * a value this row's chain assigned", never "the names this row touched". + * + * After {@link HookWriteRecording.seal} the recording records nothing more, + * so this returns an empty set rather than a stale one. + */ + closeWindow(): ReadonlySet; /** * Close the recording and hand back the payload the rest of the write must * use. @@ -137,6 +162,12 @@ export interface SealedHookWrites { */ export function recordHookPayloadWrites(target: Record): HookWriteRecording { const written = new Set(); + // [#14099] The windowed twin of `written` — same entries, cleared at each + // `closeWindow()`. Kept beside the cumulative set rather than derived from + // it, because a derivation cannot exist: two rows writing the SAME key grow + // the cumulative set once, so a delta would report the second row as having + // written nothing, which is the exact false reading D3's enforcement turns on. + let windowWrites = new Set(); let sealed = false; const record = (key: string | symbol): void => { @@ -146,6 +177,7 @@ export function recordHookPayloadWrites(target: Record): HookWr // collide with a real key's provenance. if (sealed || typeof key !== 'string') return; written.add(key); + windowWrites.add(key); }; const payload = new Proxy(target, { @@ -173,13 +205,22 @@ export function recordHookPayloadWrites(target: Record): HookWr // records it again. Dropping it here keeps the set meaning "a hook // assigned the value standing on this key" rather than "a hook once // touched this name". - if (ok && typeof key === 'string') written.delete(key); + if (ok && typeof key === 'string') { + written.delete(key); + windowWrites.delete(key); + } return ok; }, }); return { payload, + closeWindow(): ReadonlySet { + if (sealed) return new Set(); + const closed = windowWrites; + windowWrites = new Set(); + return closed; + }, seal(current: unknown): SealedHookWrites { sealed = true; if (current !== payload) { diff --git a/packages/objectql/src/index.ts b/packages/objectql/src/index.ts index 23a82f0568..9a6dc785f3 100644 --- a/packages/objectql/src/index.ts +++ b/packages/objectql/src/index.ts @@ -110,6 +110,21 @@ export { ReadonlyFieldRejectedError } from './readonly-strict-errors.js'; // 'DUPLICATE_RECORD'` is the boundary-crossing identity, the class is the // in-process convenience. export { DuplicateRecordError, DUPLICATE_RECORD_CODE } from './duplicate-record-error.js'; +// [#14099] Thrown by `engine.update` when a `multi: true` batch's per-row +// `beforeUpdate` dispatches assigned DIFFERENT sets of payload keys — the +// enforcement half of ADR-0058 Addendum II D3. Exported for the same reason as +// its neighbour above: an application whose hook has to switch to a per-row +// `ctx.api` write (or to by-id updates) needs to NAME the condition, and +// `code === 'MULTI_UPDATE_HOOK_KEY_DIVERGENCE'` is the boundary-crossing +// identity. `divergingHookPayloadKeys` rides along because it is the whole +// criterion, pure and total, and a consumer reasoning about the refusal should +// be able to read it rather than re-derive it. +export { + MultiUpdateHookKeyDivergenceError, + MULTI_UPDATE_HOOK_KEY_DIVERGENCE_CODE, + MULTI_UPDATE_HOOK_KEY_DIVERGENCE_STATUS, + divergingHookPayloadKeys, +} from './multi-update-hook-key-divergence.js'; // Boot guard: thrown by `ObjectQL.init()` when a registered driver's connect() // fails (framework#3741). Hosts that boot the engine themselves can catch it to // render their own "database unreachable" message. diff --git a/packages/objectql/src/multi-update-hook-key-divergence.test.ts b/packages/objectql/src/multi-update-hook-key-divergence.test.ts new file mode 100644 index 0000000000..9abe179c3f --- /dev/null +++ b/packages/objectql/src/multi-update-hook-key-divergence.test.ts @@ -0,0 +1,462 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#14099] A `multi: true` update whose per-row `beforeUpdate` dispatches + * assigned DIFFERENT sets of payload keys is refused whole, before any write. + * + * The defect these pins retire was measured downstream against published + * 17.2.0: two `duly_task` rows — one open, one completed earlier — updated in + * one `multi: true` call with a `completed_at` transition stamp bound to + * `beforeUpdate`. The already-completed row's `completed_at` MOVED, from + * `…:26.560Z` to `…:26.571Z`, without transitioning and without an error. The + * corrupted row is byte-for-byte indistinguishable from one genuinely completed + * late, which is what turns a compliant record into a breach in every on-time + * measure reading the column. + * + * Maintainer ruling, 2026-09-02 (recommendation C): keep D3 — the payload stays + * batch-scoped and the engine never splits its own write — and ENFORCE it, by + * recording each row's assigned key set and refusing the batch when two rows + * disagree. ⛔ The criterion is the key SET and never the values; the three + * pins the ruling named are §1, §2 and §3 below, and §3 is the regression guard + * for the whole platform (if the audit-stamp-only batch ever moves, the refusal + * is over-firing on a hook registered in essentially every deployment). + */ + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from './engine.js'; +import { bindHooksToEngine } from './hook-binder.js'; +import { + MultiUpdateHookKeyDivergenceError, + MULTI_UPDATE_HOOK_KEY_DIVERGENCE_CODE, + MULTI_UPDATE_HOOK_KEY_DIVERGENCE_STATUS, + divergingHookPayloadKeys, +} from './multi-update-hook-key-divergence.js'; +import type { Hook, HookContext } from '@objectstack/spec/data'; + +const TASK_FIELDS = { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + status: { name: 'status', label: 'Status', type: 'text' as const }, + completed_at: { name: 'completed_at', label: 'Completed at', type: 'text' as const }, + priority: { name: 'priority', label: 'Priority', type: 'text' as const }, + updated_at: { name: 'updated_at', label: 'Updated at', type: 'text' as const }, +}; +const taskObject = { name: 'task', label: 'Task', fields: TASK_FIELDS }; +const silentLogger = { debug: () => {}, info: () => {}, warn: () => {}, error: () => {} }; + +/** The hook the card measured, and the shape the docs and the showcase teach. */ +const transitionStamp = (at: string) => (ctx: HookContext) => { + const prev = (ctx as any).previous as Record | undefined; + const data = (ctx.input as any).data as Record; + if (prev?.status !== 'done' && data.status === 'done') data.completed_at = at; +}; + +function hook(name: string, event: string, handler: (ctx: HookContext) => void): Hook { + return { name, object: 'task', events: [event], priority: 100, handler } as unknown as Hook; +} + +function makeStubDriver(): any { + const store = new Map>(); + let nextId = 0; + const matches = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) continue; + if (v && typeof v === 'object' && Array.isArray((v as any).$in)) { + if (!(v as any).$in.some((x: unknown) => x === row[k])) return false; + continue; + } + const expected = v && typeof v === 'object' && '$eq' in (v as any) ? (v as any).$eq : v; + if ((row[k] ?? null) !== (expected ?? null)) return false; + } + return true; + }; + const d: any = { + name: 'memory', version: '0.0.0', supports: {}, + store, + /** Single-row writes — a predicate write must never be split into N (D3). */ + updateCalls: 0, + /** Every payload `updateMany` was handed, so a pin can read the SET clause. */ + updateManyPayloads: [] as Record[], + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, async syncSchema() {}, + async find(_o: string, ast: any, opts?: any) { + // The caller's bound is applied AFTER the filter and by PRESENCE — a + // limit-blind double silently answers past a bound the engine set, which + // is what `check:objectql-double-limit` exists to keep out of new fakes. + const rows = Array.from(store.values()).filter((r) => matches(r, ast?.where)); + const limit = typeof ast?.limit === 'number' + ? ast.limit + : typeof opts?.limit === 'number' ? opts.limit : undefined; + return typeof limit === 'number' ? rows.slice(0, limit) : rows; + }, + async findOne(_o: string, ast: any) { + for (const r of store.values()) if (matches(r, ast?.where)) return r; + return null; + }, + async create(_o: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row = { ...data, id }; store.set(id, row); return row; + }, + async update(_o: string, id: string, data: Record) { + d.updateCalls += 1; + const cur = store.get(id); if (!cur) return null; + const u = { ...cur, ...data, id }; store.set(id, u); return u; + }, + async updateMany(_o: string, ast: any, data: Record) { + d.updateManyPayloads.push({ ...data }); + const rows = Array.from(store.values()).filter((r) => matches(r, ast?.where)); + for (const r of rows) store.set(r.id as string, { ...r, ...data, id: r.id }); + return rows.length; + }, + async delete(_o: string, id: string) { return store.delete(id); }, + async deleteMany(_o: string, ast: any) { + const rows = Array.from(store.values()).filter((r) => matches(r, ast?.where)); + for (const r of rows) store.delete(r.id as string); + return rows.length; + }, + async count(_o: string) { return store.size; }, + async bulkCreate(_o: string, rows: any[]) { return Promise.all(rows.map((r) => d.create(_o, r))); }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async upsert(_o: string, data: any) { return d.create(_o, data); }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return d; +} + +async function boot(hooks: Hook[]): Promise<{ engine: ObjectQL; driver: any }> { + const engine = new ObjectQL(); + const driver = makeStubDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(taskObject); + if (hooks.length > 0) { + bindHooksToEngine(engine, hooks, { packageId: 'app:test', logger: silentLogger }); + } + return { engine, driver }; +} + +/** The stored rows, as a stable, comparable snapshot. */ +const snapshot = (driver: any): string => + JSON.stringify( + [...(driver.store as Map>).values()] + .sort((a, b) => String(a.id).localeCompare(String(b.id))), + ); + +/* ──────────────────────────────────────────────────────────────────────────── + * 1. The card's own fixture — REFUSED, with the envelope naming `completed_at` + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#14099] the mixed batch the card measured is refused', () => { + async function mixedBatch() { + const { engine, driver } = await boot([ + hook('stamp_completion', 'beforeUpdate', transitionStamp('2026-09-01T00:00:26.571Z')), + ]); + await engine.insert('task', [ + { id: 'open', title: 'a', status: 'todo', completed_at: null }, + { id: 'already', title: 'b', status: 'done', completed_at: '2026-09-01T00:00:26.560Z' }, + ] as any); + return { engine, driver }; + } + + const run = (engine: ObjectQL) => + engine.update('task', { status: 'done' }, { + multi: true, where: { id: { $in: ['open', 'already'] } }, + } as any); + + it('refuses with the ADR-0112 envelope — asserted by `code` and `status`', async () => { + const { engine } = await mixedBatch(); + // ⛔ Never a bare `toThrow()`: an un-fixed engine throws nothing at all here, + // and a *different* refusal would satisfy one. The envelope is the pin. + const err = await run(engine).then( + () => { throw new Error('expected the batch to be refused'); }, + (e: unknown) => e as MultiUpdateHookKeyDivergenceError, + ); + expect(err).toBeInstanceOf(MultiUpdateHookKeyDivergenceError); + expect(err.code).toBe(MULTI_UPDATE_HOOK_KEY_DIVERGENCE_CODE); + expect(err.code).toBe('MULTI_UPDATE_HOOK_KEY_DIVERGENCE'); + expect(err.status).toBe(MULTI_UPDATE_HOOK_KEY_DIVERGENCE_STATUS); + expect(err.status).toBe(400); + }); + + it('names the object, the diverging key and the prescription', async () => { + const { engine } = await mixedBatch(); + const err = await run(engine).catch((e) => e as MultiUpdateHookKeyDivergenceError); + expect(err.object).toBe('task'); + expect(err.keys).toEqual(['completed_at']); + expect(err.rows).toBe(2); + // The message a user-facing surface renders names the object and the key… + expect(err.message).toContain("'task'"); + expect(err.message).toContain("'completed_at'"); + expect(err.message).toContain('Nothing was written'); + // …and the developer half carries both routes out, which is the whole point + // of refusing rather than corrupting. + expect(err.developerMessage).toContain('ctx.api'); + expect(err.developerMessage).toContain('by id'); + }); + + it('refuses BEFORE any write — no row changed, and no driver write ran', async () => { + const { engine, driver } = await mixedBatch(); + const before = snapshot(driver); + await run(engine).catch(() => {}); + // Not "after the first row", not "inside a transaction that then rolls + // back": the driver was never asked to write anything. + expect(driver.updateManyPayloads).toEqual([]); + expect(driver.updateCalls).toBe(0); + expect(snapshot(driver)).toBe(before); + // Specifically: the row the card watched still holds its ORIGINAL instant. + expect(driver.store.get('already').completed_at).toBe('2026-09-01T00:00:26.560Z'); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 2. An all-transition batch PROCEEDS as one `updateMany` + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#14099] an honest batch still proceeds as ONE updateMany (D3 intact)', () => { + it('every row transitions ⇒ one write, and every row is stamped', async () => { + const { engine, driver } = await boot([ + hook('stamp_completion', 'beforeUpdate', transitionStamp('2026-09-02T10:00:00.000Z')), + ]); + await engine.insert('task', [ + { id: 'a', title: 'a', status: 'todo', completed_at: null }, + { id: 'b', title: 'b', status: 'todo', completed_at: null }, + { id: 'c', title: 'c', status: 'todo', completed_at: null }, + ] as any); + + await engine.update('task', { status: 'done' }, { + multi: true, where: { status: 'todo' }, + } as any); + + // ONE `updateMany`, never split into N single-row writes. + expect(driver.updateManyPayloads).toHaveLength(1); + expect(driver.updateCalls).toBe(0); + expect(driver.updateManyPayloads[0].completed_at).toBe('2026-09-02T10:00:00.000Z'); + for (const id of ['a', 'b', 'c']) { + expect(driver.store.get(id).status).toBe('done'); + expect(driver.store.get(id).completed_at).toBe('2026-09-02T10:00:00.000Z'); + } + }); + + it('a batch of ONE row is never refused — there is nothing to diverge from', async () => { + const { engine, driver } = await boot([ + hook('stamp_completion', 'beforeUpdate', transitionStamp('2026-09-02T10:00:00.000Z')), + ]); + await engine.insert('task', [{ id: 'solo', title: 'a', status: 'done' }] as any); + await engine.update('task', { status: 'done' }, { + multi: true, where: { status: 'done' }, + } as any); + expect(driver.updateManyPayloads).toHaveLength(1); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 3. The audit-stamp-only batch is BYTE-IDENTICAL before and after + * + * The platform-wide regression guard. `sys_stamp_audit_update` is registered on + * `'*'`, so it runs in essentially every deployment: if the refusal ever fires + * on a batch whose only payload rewrite is that stamp, every bulk update on + * every ObjectStack install starts erroring. It reads the clock ONCE PER ROW, + * so the VALUES diverge across rows on a perfectly honest batch — which is + * precisely why the ruled criterion is the key SET. + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#14099] a row-invariant rewrite is never refused, however its values differ', () => { + /** The audit stamp's shape: same key on every row, a fresh clock read each time. */ + const perRowClockStamp = () => { + let tick = 0; + return (ctx: HookContext) => { + tick += 1; + (ctx.input as any).data.updated_at = `2026-09-02T10:00:00.${String(tick).padStart(3, '0')}Z`; + }; + }; + + it('same key, DIFFERENT value per row ⇒ proceeds (the values are not the criterion)', async () => { + const { engine, driver } = await boot([ + hook('audit_stamp', 'beforeUpdate', perRowClockStamp()), + ]); + await engine.insert('task', [ + { id: 'a', title: 'a', status: 'todo' }, + { id: 'b', title: 'b', status: 'done' }, + { id: 'c', title: 'c', status: 'blocked' }, + ] as any); + + await engine.update('task', { title: 'renamed' }, { + multi: true, where: {}, + } as any); + + expect(driver.updateManyPayloads).toHaveLength(1); + // D3, unchanged: the LAST dispatch's value is the one the batch carries. + expect(driver.updateManyPayloads[0].updated_at).toBe('2026-09-02T10:00:00.003Z'); + for (const id of ['a', 'b', 'c']) { + expect(driver.store.get(id).title).toBe('renamed'); + expect(driver.store.get(id).updated_at).toBe('2026-09-02T10:00:00.003Z'); + } + }); + + it('BYTE-IDENTICAL: the audit-stamp-only batch writes exactly what it wrote before #14099', async () => { + // Both engines run the same hook over the same rows; the pin is that the + // stored bytes are the same object graph either way, so "the refusal + // over-fires" cannot hide as a subtle payload difference. + const expected = JSON.stringify([ + { id: 'a', title: 'renamed', status: 'todo', updated_at: '2026-09-02T10:00:00.001Z' }, + { id: 'b', title: 'renamed', status: 'done', updated_at: '2026-09-02T10:00:00.001Z' }, + ]); + const { engine, driver } = await boot([ + hook('audit_stamp', 'beforeUpdate', (ctx) => { + (ctx.input as any).data.updated_at = '2026-09-02T10:00:00.001Z'; + }), + ]); + await engine.insert('task', [ + { id: 'a', title: 'a', status: 'todo' }, + { id: 'b', title: 'b', status: 'done' }, + ] as any); + await engine.update('task', { title: 'renamed' }, { multi: true, where: {} } as any); + expect(snapshot(driver)).toBe(expected); + }); + + it('a hook that writes NOTHING on any row is never refused', async () => { + const { engine, driver } = await boot([hook('inert', 'beforeUpdate', () => {})]); + await engine.insert('task', [ + { id: 'a', title: 'a', status: 'todo' }, + { id: 'b', title: 'b', status: 'done' }, + ] as any); + await engine.update('task', { title: 'x' }, { multi: true, where: {} } as any); + expect(driver.updateManyPayloads).toHaveLength(1); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 4. The blind spot the ruling carries OPENLY — pinned as intended, not fixed + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#14099] same key + per-row VALUES still passes — D3’s declared cost', () => { + it('a per-row derived value is NOT refused, and the LAST dispatch’s value reaches every row', async () => { + // ⚠️ This is the ruling's named blind spot, pinned so it cannot change by + // accident in either direction. It is filed as its own finding with a + // measured instance; ⛔ it is NOT widened into this card, and ⛔ the fix is + // NOT a value comparison — see `multi-update-hook-key-divergence.ts` for + // the two measurements that rejected one. + const { engine, driver } = await boot([ + hook('derive_priority', 'beforeUpdate', (ctx) => { + const prev = (ctx as any).previous as Record; + (ctx.input as any).data.priority = prev.status === 'blocked' ? 'high' : 'low'; + }), + ]); + await engine.insert('task', [ + { id: 'a', title: 'a', status: 'blocked' }, + { id: 'b', title: 'b', status: 'todo' }, + ] as any); + + await engine.update('task', { title: 'x' }, { multi: true, where: {} } as any); + + expect(driver.updateManyPayloads).toHaveLength(1); + // The LAST row's derivation wins and lands on both rows. Row `a` is + // `blocked` and should have been `high`; it is not. + expect(driver.store.get('a').priority).toBe('low'); + expect(driver.store.get('b').priority).toBe('low'); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 5. What the recorder cannot say, it does not say + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#14099] a hook that REPLACES the payload abstains rather than being refused', () => { + it('replacement ⇒ no attributable record ⇒ the batch is not refused', async () => { + // `hook-write-provenance.ts`'s KNOWN LIMIT: a replaced payload's keys are + // indistinguishable from the caller's, so the recording says "cannot say". + // Refusing on a measurement never taken would be a fabricated verdict, so + // the pre-#14099 behaviour stands — the same fail-safe direction #14088 + // chose for the same limit. + const { engine, driver } = await boot([ + hook('replaces', 'beforeUpdate', (ctx) => { + const prev = (ctx as any).previous as Record; + (ctx.input as any).data = prev.status === 'todo' + ? { ...(ctx.input as any).data, completed_at: 'x' } + : { ...(ctx.input as any).data }; + }), + ]); + await engine.insert('task', [ + { id: 'a', title: 'a', status: 'todo' }, + { id: 'b', title: 'b', status: 'done' }, + ] as any); + await engine.update('task', { title: 'x' }, { multi: true, where: {} } as any); + expect(driver.updateManyPayloads).toHaveLength(1); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 6. Neighbouring contracts, unchanged + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#14099] the refusal is scoped to the predicate UPDATE path', () => { + it('a by-id update with the same hook is untouched — one row, one payload', async () => { + const { engine, driver } = await boot([ + hook('stamp_completion', 'beforeUpdate', transitionStamp('2026-09-02T11:00:00.000Z')), + ]); + await engine.insert('task', [{ id: 'a', title: 'a', status: 'todo' }] as any); + await engine.update('task', { id: 'a', status: 'done' } as any); + expect(driver.store.get('a').completed_at).toBe('2026-09-02T11:00:00.000Z'); + }); + + it('a predicate DELETE is untouched — that event carries no payload at all', async () => { + const seen: unknown[] = []; + const { engine, driver } = await boot([ + hook('guard', 'beforeDelete', (ctx) => { seen.push((ctx.input as any).id); }), + ]); + await engine.insert('task', [ + { id: 'a', title: 'a', status: 'todo' }, + { id: 'b', title: 'b', status: 'done' }, + ] as any); + await engine.delete('task', { multi: true, where: {} } as any); + expect(seen).toHaveLength(2); + expect(driver.store.size).toBe(0); + }); +}); + +/* ──────────────────────────────────────────────────────────────────────────── + * 7. The criterion itself — pure, total, order-independent + * ──────────────────────────────────────────────────────────────────────────── */ + +describe('[#14099] divergingHookPayloadKeys', () => { + const S = (...k: string[]) => new Set(k); + + it('agreeing rows diverge on nothing', () => { + expect(divergingHookPayloadKeys([S('a', 'b'), S('b', 'a'), S('a', 'b')])).toEqual([]); + }); + + it('reports `union \\ intersection`, sorted — every offending key, not the first pair', () => { + expect(divergingHookPayloadKeys([S('x'), S('y'), S('x', 'y', 'z')])).toEqual(['x', 'y', 'z']); + expect(divergingHookPayloadKeys([S('shared', 'a'), S('shared')])).toEqual(['a']); + }); + + it('is ORDER-INDEPENDENT — the same batch answers the same whatever order the rows came in', () => { + const rows = [S('a'), S(), S('a', 'b')]; + const answer = divergingHookPayloadKeys(rows); + expect(divergingHookPayloadKeys([...rows].reverse())).toEqual(answer); + expect(divergingHookPayloadKeys([rows[1], rows[2], rows[0]])).toEqual(answer); + }); + + it('fewer than two recorded rows cannot diverge', () => { + expect(divergingHookPayloadKeys([])).toEqual([]); + expect(divergingHookPayloadKeys([S('a')])).toEqual([]); + }); + + it('rows with no writes at all agree with each other', () => { + // The abstention case — a hook REPLACED the payload, so the recording can + // say nothing — never reaches this function: the engine skips the + // comparison outright when its seal returns no record (pinned end-to-end in + // §5). ⛔ So an absent row must never be modelled here as an empty set; + // these are real windows that happen to be empty. + expect(divergingHookPayloadKeys([S(), S(), S()])).toEqual([]); + }); + + it('a key DELETED on one row and assigned on another diverges', () => { + // `recordHookPayloadWrites` drops a deleted key from the record, so this is + // the shape a `delete ctx.input.data.x` on some rows produces. + expect(divergingHookPayloadKeys([S('x'), S()])).toEqual(['x']); + }); +}); diff --git a/packages/objectql/src/multi-update-hook-key-divergence.ts b/packages/objectql/src/multi-update-hook-key-divergence.ts new file mode 100644 index 0000000000..0301b0d25b --- /dev/null +++ b/packages/objectql/src/multi-update-hook-key-divergence.ts @@ -0,0 +1,203 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#14099] The refusal that ends "one hook-mutated payload, applied to every + * matched row" — ADR-0058 Addendum II D3, ENFORCED rather than merely stated. + * + * ## The corruption this retires + * + * `driver.updateMany` takes ONE `SET` clause for N rows, so D3 makes the + * predicate write's payload batch-scoped and hands every per-row `before*` + * dispatch THE payload rather than a copy. The addendum names the residual + * hazard in as many words: a rewrite CONDITIONED on the row + * (`ctx.previous`, `ctx.input.id`) "does not scope itself to the row it was + * decided on, it widens to every matched row" — and until this module it said + * so as "a contract statement, not an enforcement". + * + * Measured downstream, against published 17.2.0: `duly_task` carries a + * `readonly` `completed_at` stamped by a `beforeUpdate` hook on the transition + * into `done`. Two rows — one open, one completed earlier — updated in one + * `multi: true` call. The already-done row's `completed_at` MOVED, from + * `…:26.560Z` to `…:26.571Z`. It did not transition. Nothing errored, and the + * corrupted row is byte-for-byte indistinguishable from one that really was + * completed late — which is what turns a compliant record into a breach in + * every on-time measure that reads the column. The whole class is exposed: + * `approved_at`, `closed_at`, `shipped_at`, `first_responded_at`. + * + * ## The criterion is the KEY SET, never the values + * + * Maintainer ruling of 2026-09-02 (recommendation C, on the decision batch that + * kept Addendum II D3 standing): + * + * > On a `multi: true` update the engine keeps dispatching the before-phase + * > hooks per row with the per-row pre-image and records, per row, the set of + * > payload keys the hook chain assigned. If the recorded key sets differ + * > between any two rows, the whole batch is refused before any write … If + * > every row's key set is identical, the batch proceeds as one `updateMany` + * > with the batch payload, exactly as D3 says. The criterion is the key set, + * > never the values, so the audit stamp's per-row clock reads cannot make an + * > honest batch non-deterministic. + * + * ⛔ Do not "tighten" this to compare VALUES. That variant was rejected on + * measurement twice over, and both measurements are recoverable only here: + * + * - objectql's own `sys_stamp_audit_update` builtin is registered on `'*'`, + * so it runs in essentially every deployment, and it reads the clock INSIDE + * the per-record stamp. Under per-row dispatch that is one clock read per + * row, so two rows either side of a millisecond boundary carry different + * `updated_at` VALUES on an entirely honest batch. A value comparison would + * refuse it non-deterministically — the failure direction nobody can debug. + * - A value comparison also re-breaks #14088's own row: a hook that + * deliberately writes the value the caller also sent (`completed_at: null` + * on a reopen, against a caller that round-tripped the record) is + * indistinguishable, by value, from a hook that never touched the key. That + * is the defect `hook-write-provenance.ts` exists to remove; a value test + * here would reintroduce it one seam over. + * + * The key set is what {@link recordHookPayloadWrites} already records, so this + * refusal adds no new instrument — it reads the #14088 recorder, armed per row. + * + * ## What is NOT covered, named rather than hidden + * + * A hook that writes the SAME key on every row but with per-row VALUES (a + * per-row derived priority, say) passes this test and still applies ONE row's + * value to every matched row. The ruling's own sentence says "the first row's + * value"; the engine's MEASURED behaviour is the LAST dispatch's, because the + * per-row rewrites accumulate onto one payload in dispatch order and the last + * assignment to a key is what the single `SET` clause carries. Pinned in both + * suites — `bulk-write-per-row-hooks.test.ts`'s D3 case reads + * `['stamped-2','stamped-2']`, and this module's own residue pin reads `low` + * on the row whose own dispatch derived `high`. Which row wins changes nothing + * about the ruling's verdict; it is corrected here because a docblock naming + * the wrong row sends the next author hunting for a per-row seam that does not + * exist. That is D3's cost by design; the ruling carries it openly and points + * at the prescription below as the exit. This module does not widen to cover + * it, and a future author reaching for a value comparison to close it must + * re-read the two measurements above first. + * + * ## Divergence is `union` minus `intersection` + * + * The diverging keys are every key some row's window holds and some other + * row's does not — order-independent by construction, so the envelope names + * the same keys whatever order the driver returned the matched rows in, and it + * names ALL of them rather than the first pair to disagree. + * + * ## When the recording cannot speak, the batch is not judged at all + * + * That decision does not live here, deliberately: it is the ENGINE that knows + * whether its recording survived the batch. A hook may REPLACE + * `ctx.input.data` rather than mutate it, and the replacement's keys are + * indistinguishable from the caller's (`hook-write-provenance.ts`'s KNOWN + * LIMIT) — so `seal` returns no record, and the engine skips this comparison + * entirely instead of feeding it windows that describe a payload the batch no + * longer writes. Refusing on a measurement that no longer applies would be a + * fabricated verdict; abstaining keeps the pre-#14099 behaviour for that + * shape, which is the same fail-safe direction #14088 chose for the same + * limit. This function therefore takes real key sets only, and reading a + * missing row as an empty set is not a case it can be handed. + */ + +/** + * The keys whose presence in the hook chain's writes DIFFERS across the rows + * of one predicate update, sorted; `[]` when every row agreed (which includes + * a batch of one row, and a batch where no hook wrote anything). + * + * Each entry is one row's OBSERVATION WINDOW — + * {@link HookWriteRecording.closeWindow}'s return, the keys that row's chain + * assigned. Pure and total: it never throws and never reads the engine. The + * engine raises; the contract decides. + */ +export function divergingHookPayloadKeys(perRow: readonly ReadonlySet[]): string[] { + if (perRow.length < 2) return []; + const union = new Set(); + for (const set of perRow) for (const key of set) union.add(key); + const diverging: string[] = []; + for (const key of union) { + if (!perRow.every((set) => set.has(key))) diverging.push(key); + } + return diverging.sort(); +} + +/** + * The wire code, registered in the spec's `ERROR_CODE_LEDGER` under + * `@objectstack/objectql`. + * + * ⚠️ A REGISTERED ADR-0112 code rather than the `ERR_`-prefixed operational + * kind its `HookTargetRebindError` neighbour uses, and the difference is not + * stylistic: the whole value of this refusal is that the application RECOGNISES + * it and takes the prescription. An unregistered spelling demotes off + * `error.code` at the dispatcher door and rides `declaredCode` instead, which + * is exactly the wrong place for the one code `duly` and `hotcrm` have to + * branch on. `FILE_FIELD_BULK_WRITE_REFUSED` — the same seam, the same shape of + * refusal, one predicate write over — made the same call for the same reason. + */ +export const MULTI_UPDATE_HOOK_KEY_DIVERGENCE_CODE = 'MULTI_UPDATE_HOOK_KEY_DIVERGENCE' as const; + +/** + * `400`, taken from `FileFieldBulkWriteError`'s reasoning verbatim because the + * verdict is the same one: the write is expressible and permitted, it just + * cannot be expressed as ONE payload, and the remedy belongs to the caller. + * Not `409` — nothing about the stored rows is in conflict, and nothing changes + * if the caller retries later. + */ +export const MULTI_UPDATE_HOOK_KEY_DIVERGENCE_STATUS = 400 as const; + +/** + * The ADR-0112 envelope a `multi: true` update raises when its per-row + * `beforeUpdate` dispatches assigned DIFFERENT sets of payload keys. + * + * Thrown from the per-row before-phase, which runs outside `update()`'s own + * `try` block, so it reaches the caller intact — and, decisively, BEFORE the + * payload is sealed, before both readonly strips, before validation and before + * any `driver.updateMany`. Nothing is written: not the first row, not a + * transaction that then rolls back. + */ +export class MultiUpdateHookKeyDivergenceError extends Error { + override readonly name = 'MultiUpdateHookKeyDivergenceError'; + readonly code = MULTI_UPDATE_HOOK_KEY_DIVERGENCE_CODE; + readonly status = MULTI_UPDATE_HOOK_KEY_DIVERGENCE_STATUS; + /** The object the refused batch targeted. */ + readonly object: string; + /** The keys whose presence differed across rows, sorted. */ + readonly keys: readonly string[]; + /** How many rows the predicate matched — the batch that was refused whole. */ + readonly rows: number; + /** The remedy half, addressed to the hook's author rather than to a user. */ + readonly developerMessage: string; + + constructor(object: string, keys: readonly string[], rows: number) { + super(buildMessage(object, keys, rows)); + this.object = object; + this.keys = [...keys]; + this.rows = rows; + this.developerMessage = + `A predicate update sends ONE 'SET' clause to the driver, so there is exactly one payload ` + + `for all ${rows} matched records (ADR-0058 Addendum II D3) — whatever a 'beforeUpdate' handler ` + + `writes for one row is applied to every row. The handler wrote ` + + `${keys.map((k) => `'${k}'`).join(', ')} for some of these records and not for others, which ` + + `means it is deciding per record; the engine refuses rather than applying one record's ` + + `derived value to all of them. Two supported ways to express it: write the affected records ` + + `from INSIDE the handler with 'ctx.api' (a per-row write, which the handler's own sandbox ` + + `signals — 'ctx.dispatch.mode', 'ctx.input.id' — let it aim), or have the caller issue the ` + + `updates by id. Branch on \`code === '${MULTI_UPDATE_HOOK_KEY_DIVERGENCE_CODE}'\` (ADR-0112) ` + + `to detect this.`; + } +} + +/** + * The user-facing sentence. + * + * ⛔ It must not begin with a SQL verb — `@objectstack/rest`'s importer runs + * row errors through `sanitizeRowError`, whose SQL backstop replaces any + * message STARTING with `insert`/`update`/`delete` with generic text. The same + * constraint `DuplicateRecordError`'s message records, measured there. + */ +function buildMessage(object: string, keys: readonly string[], rows: number): string { + const named = keys.map((k) => `'${k}'`).join(', '); + return ( + `Refusing a multi-record update on '${object}': its 'beforeUpdate' handlers wrote ${named} for ` + + `some of the ${rows} matched records and not for others, and a predicate update has one payload ` + + `for every record — so one record's value would have been written to all of them. Nothing was ` + + `written. Write those records individually, from inside the handler with 'ctx.api' or by id.` + ); +} diff --git a/packages/spec/src/api/error-code-ledger.zod.ts b/packages/spec/src/api/error-code-ledger.zod.ts index d72e9be969..2954a3f116 100644 --- a/packages/spec/src/api/error-code-ledger.zod.ts +++ b/packages/spec/src/api/error-code-ledger.zod.ts @@ -535,6 +535,19 @@ export const ERROR_CODE_LEDGER = { // been written (`TransactionUnsupportedError`, `transaction-errors.ts`; // ADR-0119 D1/D4 fail-closed posture). Same #8087-gate family. 'ERR_TRANSACTION_UNSUPPORTED', + // [#14099] a `multi: true` update whose per-row `beforeUpdate` dispatches + // assigned DIFFERENT sets of payload keys — a transition stamp + // (`completed_at` on the move into `done`) is the measured shape. One `SET` + // clause serves N rows (ADR-0058 Addendum II D3), so the engine refuses the + // batch whole, before any write, instead of applying one row's derived + // value to every matched row. Registered rather than left as an `ERR_` + // operational code precisely because the prescription is the application's + // to take: it branches on this to switch to a per-row `ctx.api` write or + // by-id updates. Not a VALIDATION_ERROR synonym — the payload and the + // predicate are both valid; the batch SHAPE cannot carry a per-row + // decision. `MultiUpdateHookKeyDivergenceError`, + // `multi-update-hook-key-divergence.ts`. + 'MULTI_UPDATE_HOOK_KEY_DIVERGENCE', // [#11142/#11230] a by-id update carried an `options.where.id` that is not // the bound payload `data.id` — a truthy scalar naming a DIFFERENT row // (#11142), or a non-scalar predicate over a row SET (#11230, which also