Skip to content

Commit dee4dd4

Browse files
os-muskclaude
andauthored
fix(objectql,spec): refuse a multi: true update whose per-row beforeUpdate hooks write divergent key sets (#14099) (#14734)
* wip: #14099 divergent key-set refusal + pins * wip: changeset * wip: census re-anchor * wip: regenerate spec reference docs for the new ledger code * test: honour the caller's bound in the new stub driver's find (check:objectql-double-limit) * refactor: one loop-scoped recording with per-row windows, so D3's payload identity pin holds unchanged * docs: re-anchor system-context census after the engine refactor * docs(objectql): name the row whose value actually survives the divergence blind spot The new module's "What is NOT covered" paragraph said the residue applies "the first row's value to every matched row", and the residue pin's own title said "row 1's value". Both are contradicted by the assertions directly beneath them: per-row rewrites accumulate onto ONE payload in dispatch order, so the LAST assignment to a key is what the single SET clause carries. The pins already measure it — `bulk-write-per-row-hooks.test.ts`'s D3 case reads `['stamped-2','stamped-2']`, and the residue pin reads `low` on the row whose own dispatch derived `high`. Prose only; no behaviour, no assertion and no exported symbol changes. The ruling's verbatim quotation is untouched — the correction is stated beside it, naming what the ruling said and what the engine does, because a docblock that names the wrong row sends the next author hunting for a per-row seam that does not exist. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 4368411 commit dee4dd4

11 files changed

Lines changed: 937 additions & 23 deletions
Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
---
2+
"@objectstack/objectql": minor
3+
"@objectstack/spec": minor
4+
---
5+
6+
fix(objectql,spec): refuse a `multi: true` update whose per-row `beforeUpdate` hooks write divergent key sets (#14099)
7+
8+
**BREAKING** accept-set narrowing on a published write path, shipped as `minor`
9+
under the repo's launch-window convention for breaking changes. A `multi: true`
10+
update that succeeds today is REFUSED when its `beforeUpdate` handlers assign
11+
different sets of payload keys to different matched rows.
12+
13+
**What it fixes.** `driver.updateMany` takes one `SET` clause for N rows, so a
14+
predicate update has exactly one payload (ADR-0058 Addendum II D3) — whatever a
15+
`beforeUpdate` handler writes for one row was applied to every matched row. The
16+
transition stamp is the shape this breaks, and it is the standard way to record
17+
when a record entered a state:
18+
19+
```ts
20+
// beforeUpdate — correct per record, silently wrong on a batch
21+
if (previous.status !== 'done' && next.status === 'done') patch.completed_at = now;
22+
```
23+
24+
Measured against published `17.2.0`: two rows, one open and one completed
25+
earlier, updated in a single `multi: true` call. The already-completed row's
26+
`completed_at` moved from `…:26.560Z` to `…:26.571Z`. It never transitioned,
27+
nothing errored, and the corrupted row is byte-for-byte indistinguishable from
28+
one genuinely completed late — so every on-time measure reading the column turns
29+
a compliant record into a breach, with no audit entry and nothing in the data
30+
that shows it happened. The whole class is exposed: `approved_at`, `closed_at`,
31+
`shipped_at`, `first_responded_at`.
32+
33+
**What changed.** The engine still dispatches the before phase once per matched
34+
row with that row's pre-image, and D3 still stands — the payload stays
35+
batch-scoped and the engine never splits its own write. It now also RECORDS,
36+
per row, the set of payload keys that row's hook chain assigned (the #14088
37+
provenance recorder, armed once more per row). If two rows disagree, the whole
38+
batch is refused before any write — not after the first row, not inside a
39+
transaction that then rolls back — with the ADR-0112 envelope
40+
`MULTI_UPDATE_HOOK_KEY_DIVERGENCE` (HTTP `400`,
41+
`MultiUpdateHookKeyDivergenceError`), naming the object, the diverging keys and
42+
the remedy. When every row's key set is identical the batch proceeds as one
43+
`updateMany`, exactly as before.
44+
45+
**The criterion is the key SET, never the values.** That is what keeps honest
46+
batches honest: objectql's own `sys_stamp_audit_update` builtin is registered on
47+
`'*'` and reads the clock inside the per-record stamp, so an ordinary bulk
48+
update writes `updated_at` on every row with different values. Every in-repo
49+
`beforeUpdate` payload rewrite was measured on a mixed batch before this shipped
50+
— the audit stamp (`['updated_at','updated_by']` on every row), plugin-pinyin's
51+
companion projection (`['__search']` on every row) and service-storage's
52+
copy-on-claim (`[]` on every row) — and all three are row-invariant, so none of
53+
them is refused.
54+
55+
**Migration — how to write a per-record rewrite on a batch.** Two supported
56+
routes, both available in this release:
57+
58+
1. **Route 2, from inside the handler.** Write the affected records with
59+
`ctx.api`, aimed with the per-row signals the hook sandbox now carries
60+
(`ctx.dispatch.mode === 'per-row'`, `ctx.input.id`, `ctx.input.options`),
61+
and leave the batch payload alone. ⚠️ Those signals are NOT in `17.2.0`
62+
they land in this same release, which is why the refusal and its
63+
prescription ship together rather than the refusal arriving first.
64+
2. **By-id updates from the caller.** Issue the updates per record when the
65+
value genuinely differs per record.
66+
67+
`objectstack-ai/hotcrm` and `objectstack-ai/duly` both carry hooks of this
68+
shape and should take route 1: `duly`'s `duly_task.completed_at` stamp is the
69+
measured instance, and hotcrm's `previous`-reading handlers are the same family.
70+
71+
**Known limit, carried openly rather than hidden.** A handler that writes the
72+
SAME key on every row but with a per-row VALUE (a per-row derived priority, say)
73+
still passes this test, and still applies the last dispatch's value to every
74+
matched row. That is D3's declared cost; the two routes above are the exit for
75+
it, and it is tracked as its own finding. ⛔ It is deliberately NOT closed by
76+
comparing values: a value comparison refuses honest audit-stamp batches
77+
non-deterministically (one clock read per row) and re-opens #14088's own
78+
`completed_at: null` row, where a hook that writes the value the caller also
79+
sent is indistinguishable from a hook that never touched the key.
80+
81+
<!-- adr-0087: not-required (no-migration-prescription) A runtime accept-set narrowing on the engine's predicate-update path: no authorable metadata key is removed, renamed or re-shaped, so there is no tombstone and nothing for `objectstack migrate meta` to rewrite. The affected artifact is HOOK BODY CODE, whose per-row intent no mechanical rewrite can infer — choosing between a `ctx.api` per-row write and by-id updates is an authoring decision. The refusal itself is the notification channel, raised at the write site with the object, the diverging keys and both routes in the envelope. -->

content/docs/permissions/system-context.mdx

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -109,18 +109,18 @@ that silently does not happen.
109109

110110
| # | Behaviour when `isSystem` | Package | What you get / what you lose | Anchor |
111111
|:--|:---|:---|:---|:---|
112-
| 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` |
113-
| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:11387` |
114-
| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9939` |
112+
| 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` |
113+
| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:11472` |
114+
| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:10024` |
115115
| 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` |
116-
| 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` |
117-
| 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` |
118-
| 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` |
116+
| 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` |
117+
| 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` |
118+
| 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` |
119119
| 25 | Engine-owned / append-only write guard bypassed | plugin-security | Get: generic writes to `managedBy` engine-owned objects | `system-write-guard.ts:96`, `:120` |
120120
| 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` |
121-
| 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` |
122-
| 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` |
123-
| 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` |
121+
| 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` |
122+
| 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` |
123+
| 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` |
124124

125125
### 3. Sharing (`plugin-sharing`)
126126

@@ -179,8 +179,8 @@ a reader tracing where elevation travels needs them.
179179

180180
| # | Site | Package | What it does |
181181
|:--|:---|:---|:---|
182-
| 62 | `objectql/src/engine.ts:3457` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes |
183-
| 63 | `objectql/src/engine.ts:14348` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag |
182+
| 62 | `objectql/src/engine.ts:3542` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes |
183+
| 63 | `objectql/src/engine.ts:14433` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag |
184184
| 64 | `plugin-reports/src/report-service.ts:556` | plugin-reports | Threads the flag into the engine call that runs a report |
185185
| 65 | `body-runner.ts:279` | runtime | Rebuilds an `ExecutionContext` from a hook session, carrying the flag across |
186186

@@ -195,7 +195,7 @@ assuming `isSystem` covers it is a documented source of bugs.
195195
|:---|:---|:---|
196196
| "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` |
197197
| "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) |
198-
| "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` |
198+
| "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` |
199199
| "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) |
200200
| "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` |
201201
| "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` |

content/docs/references/api/contract.mdx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ const result = ApiErrorSchema.parse(data);
2727

2828
| Property | Type | Required | Description |
2929
| :--- | :--- | :--- | :--- |
30-
| **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) |
30+
| **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) |
3131
| **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) |
3232
| **message** | `string` || Readable error message |
3333
| **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);
220220
* `METADATA_CONFLICT`
221221
* `METADATA_NOT_FOUND`
222222
* `METADATA_SCHEMA_INVALID`
223+
* `MULTI_UPDATE_HOOK_KEY_DIVERGENCE`
223224
* `NAMESPACE_PREFIX`
224225
* `NEEDS_PASSWORD`
225226
* `NODE_FAILURE`

content/docs/references/api/error-code-ledger.mdx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -336,6 +336,7 @@ const result = ErrorCode.parse(data);
336336
* `METADATA_CONFLICT`
337337
* `METADATA_NOT_FOUND`
338338
* `METADATA_SCHEMA_INVALID`
339+
* `MULTI_UPDATE_HOOK_KEY_DIVERGENCE`
339340
* `NAMESPACE_PREFIX`
340341
* `NEEDS_PASSWORD`
341342
* `NODE_FAILURE`

packages/objectql/src/bulk-write-per-row-hooks.test.ts

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -614,9 +614,19 @@ describe('[#5574 / D3] the payload stays BATCH-scoped, and that IS the merge rul
614614
});
615615

616616
it('a rewrite made on ONE row’s dispatch applies to the WHOLE batch', async () => {
617-
let firstOnly = true;
617+
// [#14099] The fixture — never the contract — changed here. It used to
618+
// stamp `owner` on the FIRST row only (`if (firstOnly) …`), which is now
619+
// REFUSED: writing a key for some matched rows and not others is precisely
620+
// the divergence D3's enforcement rejects before any write, because it can
621+
// only mean the handler is deciding per record. The contract this case
622+
// pins is untouched and is still the reason the refusal exists — one
623+
// `updateMany` carries one SET clause, so whichever dispatch produces a
624+
// value, EVERY row gets it. So the handler writes the same key on every
625+
// row, and the batch still carries ONE value: the last dispatch's.
626+
let dispatches = 0;
618627
const { engine } = await boot([hook('stamp', 'beforeUpdate', (ctx) => {
619-
if (firstOnly) { (ctx.input as any).data.owner = 'stamped'; firstOnly = false; }
628+
dispatches += 1;
629+
(ctx.input as any).data.owner = `stamped-${dispatches}`;
620630
})]);
621631
await seedTasks(engine, [
622632
{ title: 'a', status: 'todo', owner: 'u1' },
@@ -625,10 +635,12 @@ describe('[#5574 / D3] the payload stays BATCH-scoped, and that IS the merge rul
625635

626636
await engine.update('task', { status: 'done' }, { multi: true, where: { status: 'todo' } });
627637

628-
// Both rows got it, including the one whose dispatch did not make it. This
629-
// is the contract, not a leak: one `updateMany` carries one SET clause.
638+
// Both rows carry the SECOND dispatch's value, including the row whose own
639+
// dispatch produced `stamped-1`. That is the contract, not a leak — and it
640+
// is the residual hazard #14099's ruling names openly and does not close:
641+
// same key, per-row values still applies one row's value to all of them.
630642
const rows: any[] = await engine.find('task', {});
631-
expect(rows.map((r) => r.owner)).toEqual(['stamped', 'stamped']);
643+
expect(rows.map((r) => r.owner)).toEqual(['stamped-2', 'stamped-2']);
632644
});
633645

634646
it('rewrites ACCUMULATE in dispatch order, including a REPLACED payload', async () => {

0 commit comments

Comments
 (0)