diff --git a/.changeset/provenance-stamp-per-row-dispatch.md b/.changeset/provenance-stamp-per-row-dispatch.md new file mode 100644 index 0000000000..80fb62496d --- /dev/null +++ b/.changeset/provenance-stamp-per-row-dispatch.md @@ -0,0 +1,14 @@ +--- +"@objectstack/plugin-email": patch +"@objectstack/plugin-sharing": patch +"@objectstack/plugin-webhooks": patch +--- + +The three provenance-stamp `beforeUpdate` hooks stop re-reading a row the engine has already read, and their contract now states what they actually do on a multi-row update. + +`sys_email_template`, `sys_sharing_rule` and `sys_webhook` each carry a hook that stamps `customized: true` when a non-system caller edits a package- or platform-seeded row — the half of seed-not-clobber that detects the admin edit. All three carried the same two comments, and both were assertions about runtime behaviour that runtime measurement falsifies: + +- **"multi-row updates (no single `input.id`) are not stamped."** Not true on any engine these packages ship against. A predicate (`multi: true`) update dispatches `beforeUpdate` once per matched row, and every per-row context arrives with `input.id` bound — so the `if (!id) return` guard answered "single write" on every row of a batch and declined nothing. The rows were being stamped all along. +- **"`previous` is not resolved before beforeUpdate hooks run — read the current row ourselves."** The engine binds `previous` before dispatching `beforeUpdate` on both write shapes, so each hook was issuing its own `find` for a row the engine had just read — on a bulk edit, one extra read **per matched row**. + +Observable behaviour is deliberately unchanged: the same rows are stamped, with the same values, and a bulk edit whose matched rows disagree on `managed_by` is still refused by the engine with `MULTI_UPDATE_HOOK_KEY_DIVERGENCE` (HTTP 400) rather than widening one row's stamp across the batch. What changes is the cost and the contract: the redundant per-row read is gone, and the header of each hook now describes the per-row dispatch, the single `SET` clause a predicate write shares, and why declining to stamp on a bulk edit was rejected — unstamped rows are exactly the ones the next boot's seeder overwrites. diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 9393cd0abd..5e1c8eb353 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -136,7 +136,7 @@ The largest single consumer — **17 of the 105 sites**. | 35 | `listShares()` skips the management gate | Get: full enumeration of who can see a record | `plugin-sharing/src/sharing-service.ts:1528` | | 36 | `sys_record_share` reads are **not** self-scoped | Get: tenant-wide share listing without `manage_sharing` | `sharing-plugin.ts:1189` | | 37 | Share-link policy `enabled` check bypassed; system callers re-enter under a system context | Get: link **creation** while the policy is off — resolution is **not** bypassed since #14033 (`publicSharing.enabled` is a standing policy held at every redemption): a link minted this way does not resolve until the block is enabled | `plugin-sharing/src/share-link-service.ts:459`, `:513`, `:517`, `:590`, `:620` | -| 38 | Sharing-rule provenance stamp skipped | Lose: the row is not marked as an admin customization — seeder / `defineRule` / boot reconcilers are "the package door" | `sharing-rule-provenance.ts:47` | +| 38 | Sharing-rule provenance stamp skipped | Lose: the row is not marked as an admin customization — seeder / `defineRule` / boot reconcilers are "the package door" | `sharing-rule-provenance.ts:66` | | 39 | Sharing-rule service write + delete paths return early | Lose: the manage-rules gate on the service surface, and the platform-global-rule delete guard | `sharing-rule-service.ts:278`, `:503` | ### 4. Approvals, reports, attachments, comments, knowledge @@ -167,7 +167,7 @@ The largest single consumer — **17 of the 105 sites**. | 56 | Activation write / authoring refusals do not fire | runtime | Get: activation artifacts writable and authorable without the activation-authoring capability | `activation-gate.ts:138`, `:189` | | 57 | Automation run-state read, flow-authoring write and unrelated-screen read all pass | runtime | Get: run state, flow writes and screen reads with no grant | `domains/automation.ts:254`, `:545`, `:635` | | 58 | Audience-binding suggestion recording skipped | plugin-security | Lose: install-time suggestions are not recorded for system callers | `suggested-audience-bindings.ts:703` | -| 59 | Email-template / webhook provenance stamps skipped | plugin-email, plugin-webhooks | Lose: the row is not marked as an admin customization | `email-template-provenance.ts:59`, `webhook-provenance.ts:50` | +| 59 | Email-template / webhook provenance stamps skipped | plugin-email, plugin-webhooks | Lose: the row is not marked as an admin customization | `email-template-provenance.ts:77`, `webhook-provenance.ts:68` | | 60 | **Automation flow data nodes re-add the `owner_id` stamp** (the one place row 2's gap is compensated inline) | service-automation | Get: a flow-authored INSERT under system elevation still lands owned, when the run resolved a user. Fill-only — flow-authored values win | `runtime-identity.ts:279`, called from `builtin/crud-nodes.ts:319` | | 61 | Inbox caller refusal names `isSystem` as what was carried | service-messaging | Get: nothing — the refusal still fires. The flag only shapes the diagnostic, because privilege is not an authorization subject | `inbox-caller.ts:148` | diff --git a/packages/plugins/plugin-email/src/bootstrap-declared-email-templates.test.ts b/packages/plugins/plugin-email/src/bootstrap-declared-email-templates.test.ts index 9ce7d7f087..12fb6002d3 100644 --- a/packages/plugins/plugin-email/src/bootstrap-declared-email-templates.test.ts +++ b/packages/plugins/plugin-email/src/bootstrap-declared-email-templates.test.ts @@ -69,7 +69,15 @@ class FakeEngine { } async update(name: string, data: any, opts?: any): Promise { const id = data?.id ?? opts?.where?.id; - const ctx = { input: { id, data }, session: opts?.context }; + // [#15302] `previous` is bound BEFORE the beforeUpdate dispatch, as the + // real engine binds it (#5574 / #5846 - by-id reads the prior row ahead of + // the dispatch; each per-row context of a predicate write carries its own). + // Withholding it here is what let this fake model a pre-#5574 engine, and + // a hook reading `ctx.previous` would have gone silently unstamped against + // a fake no production caller resembles. + const cond0 = opts?.where ?? (id ? { id } : undefined); + const previous = (this.rows[name] ?? []).filter((r) => this.matches(r, cond0)).map((r) => ({ ...r }))[0]; + const ctx = { input: { id, data }, previous, session: opts?.context }; for (const h of this.hooks) { if (h.event === 'beforeUpdate' && (!h.object || h.object === name)) { await h.handler(ctx); diff --git a/packages/plugins/plugin-email/src/email-template-provenance.per-row.test.ts b/packages/plugins/plugin-email/src/email-template-provenance.per-row.test.ts new file mode 100644 index 0000000000..d270c0e392 --- /dev/null +++ b/packages/plugins/plugin-email/src/email-template-provenance.per-row.test.ts @@ -0,0 +1,250 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#15302] The `sys_email_template` provenance stamp on a PREDICATE (`multi: true`) + * update, pinned against the REAL engine. + * + * This hook used to carry two comments that were assertions about runtime + * behaviour, and runtime measurement falsified both: + * + * 1. "multi-row updates (no single `input.id`) are not stamped" - false since + * #6966. Per-row `before*` dispatch binds `ctx.input.id` on EVERY context, + * so `if (!id) return` no longer detected a bulk write and the hook ran + * once per matched row regardless. + * 2. "`previous` is not resolved before beforeUpdate hooks run" - false since + * #5574 / #5846. The engine binds `previous` before dispatching + * `beforeUpdate` on both write shapes, so the hook's own `engine.find` was + * a second read of a row the engine had just read - once PER MATCHED ROW + * on a predicate write. + * + * A comment cannot be pinned, so what is pinned here is the behaviour each + * comment was wrong about. §3 is the read count, measured with a control that + * fires rather than asserted. + * + * ⚠️ The engine half of this suite resolves through `@objectstack/objectql`'s + * `exports` to `dist/` (this package aliases no objectql entry; the ledger in + * `scripts/check-test-source-alias.mjs` records that), so a stale objectql + * build makes these readings about the built artifact. The SUBJECT - + * `./email-template-provenance.js` - is a relative import read from source, which is what an + * ablation of this file's fix mutates. + */ + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { bindEmailTemplateProvenanceStamp } from './email-template-provenance.js'; + +const OBJECT = 'sys_email_template'; +const silentLogger = { debug() {}, info() {}, warn() {}, error() {} }; + +/** Minimal in-memory driver. `findCalls` is the instrument §3 reads. */ +function makeStubDriver(): any { + const store = new Map>(); + const matches = (row: any, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) continue; + const e: any = v && typeof v === 'object' && '$in' in (v as any) ? undefined : v; + if (v && typeof v === 'object' && '$in' in (v as any)) { + if (!(v as any).$in.includes(row[k])) return false; + continue; + } + const expected = e && typeof e === 'object' && '$eq' in (e as any) ? (e as any).$eq : e; + if ((row[k] ?? null) !== (expected ?? null)) return false; + } + return true; + }; + const d: any = { + name: 'memory', version: '0.0.0', supports: {}, + store, + /** Every `find` the engine (or a hook, through `engine.find`) issues. */ + findCalls: [] as unknown[], + /** One entry per `updateMany` - the ONE `SET` clause N rows share (D3). */ + updateManyPayloads: [] as Record[], + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, async syncSchema() {}, + async find(o: string, ast: any) { + d.findCalls.push({ object: o, where: ast?.where }); + const rows = [...store.values()].filter((r) => matches(r, ast?.where)); + // Hold the caller's bound, AFTER the filter and by PRESENCE + // (`check:objectql-double-limit`): §3's control passes `limit: 1`, so a + // limit-blind double would answer it with the whole table. + return typeof ast?.limit === 'number' ? rows.slice(0, ast.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) { + const row = { ...data }; store.set(String(row.id), row); return row; + }, + async update(_o: string, id: string, data: Record) { + const cur = store.get(id); if (!cur) return null; + const u = { ...cur, ...data, id }; store.set(id, u); return u; + }, + async upsert(o: string, data: any) { return this.create(o, data); }, + async delete(_o: string, id: string) { return store.delete(id); }, + async count(_o: string, ast: any) { return [...store.values()].filter((r) => matches(r, ast?.where)).length; }, + async bulkCreate(o: string, rows: any[]) { return Promise.all(rows.map((r) => this.create(o, r))); }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async updateMany(_o: string, ast: any, data: Record) { + d.updateManyPayloads.push({ ...data }); + const rows = [...store.values()].filter((r) => matches(r, ast?.where)); + for (const r of rows) store.set(String(r.id), { ...r, ...data, id: r.id }); + return rows.length; + }, + async deleteMany() { return 0; }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return d; +} + +const text = (name: string) => ({ name, label: name, type: 'text' as const }); + +/** + * @param extraHook registered on the SAME event/object/priority as the stamp, + * so the engine's own read pattern is identical across §3's three arms. + */ +async function boot(opts: { stamp: boolean; extraHook?: (engine: any) => void } = { stamp: true }) { + const engine: any = new ObjectQL(); + const driver = makeStubDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject({ + name: OBJECT, label: OBJECT, + fields: { + id: { ...text('id'), primaryKey: true }, + managed_by: text('managed_by'), + customized: { name: 'customized', label: 'customized', type: 'boolean' as const }, + subject: text('subject'), + }, + }); + if (opts.stamp) bindEmailTemplateProvenanceStamp(engine, silentLogger, OBJECT); + opts.extraHook?.(engine); + return { engine, driver }; +} + +const ROWS = (bManagedBy: string) => ([ + { id: 'a', managed_by: 'package', customized: false, subject: 'l' }, + { id: 'b', managed_by: bManagedBy, customized: false, subject: 'l' }, +]); + +const PAYLOAD = { subject: 'edited' }; +const WHERE = { multi: true, where: { id: { $in: ['a', 'b'] } } } as any; + +/* ── 1. every matched row is stamped ───────────────────────────────────── */ + +describe('[#15302] a predicate update stamps EVERY matched row', () => { + it('stamps both rows in ONE `SET` clause - the "no `input.id`" guard is gone', async () => { + const { engine, driver } = await boot(); + await engine.insert(OBJECT, ROWS('package')); + driver.updateManyPayloads.length = 0; + + await engine.update(OBJECT, { ...PAYLOAD }, WHERE); + + // Both matched rows carry the stamp: the documented "not stamped on + // multi-row updates" boundary never existed on this engine. + expect([...driver.store.values()].map((r: any) => [r.id, r.customized])) + .toEqual([['a', true], ['b', true]]); + // ADR-0058 Addendum II D3: N rows share ONE payload, hence one clause. + expect(driver.updateManyPayloads).toEqual([{ ...PAYLOAD, customized: true }]); + }); + + it('does not stamp when the pre-image is not package/platform managed', async () => { + // The control for the assertion above: the stamp is a decision about the + // ROW, so a run where no matched row qualifies must write no `customized`. + const { engine, driver } = await boot(); + await engine.insert(OBJECT, [ + { id: 'a', managed_by: 'admin', customized: false, subject: 'l' }, + { id: 'b', managed_by: 'user', customized: false, subject: 'l' }, + ]); + driver.updateManyPayloads.length = 0; + + await engine.update(OBJECT, { ...PAYLOAD }, WHERE); + + expect(driver.updateManyPayloads).toEqual([{ ...PAYLOAD }]); + expect([...driver.store.values()].map((r: any) => r.customized)).toEqual([false, false]); + }); + + it('does not stamp an isSystem write (the seeder door)', async () => { + const { engine, driver } = await boot(); + await engine.insert(OBJECT, ROWS('package')); + driver.updateManyPayloads.length = 0; + + await engine.update(OBJECT, { ...PAYLOAD }, { + ...WHERE, context: { isSystem: true, positions: [], permissions: [] }, + }); + + expect(driver.updateManyPayloads).toEqual([{ ...PAYLOAD }]); + }); +}); + +/* ── 2. divergent rows: the engine refuses, and nothing is written ─────── */ + +describe('[#15302] matched rows that disagree refuse the batch', () => { + it('refuses with the ADR-0112 envelope and writes nothing', async () => { + const { engine, driver } = await boot(); + await engine.insert(OBJECT, ROWS('user')); + driver.updateManyPayloads.length = 0; + + const err: any = await engine.update(OBJECT, { ...PAYLOAD }, WHERE).then( + () => { throw new Error('expected the batch to be refused'); }, + (e: any) => e, + ); + + // Read the envelope by FIELD (`code` + `status`, the minimum a rejection + // case asserts): a bare `toThrow()` would stay green against any error. + expect(err.code).toBe('MULTI_UPDATE_HOOK_KEY_DIVERGENCE'); + expect(err.status).toBe(400); + expect(err.keys).toEqual(['customized']); + expect(err.rows).toBe(2); + // The refusal is the SAFE side: no `SET` clause reached the driver and + // both rows are untouched. + expect(driver.updateManyPayloads).toEqual([]); + expect([...driver.store.values()].map((r: any) => [r.customized, r.subject])) + .toEqual([[false, 'l'], [false, 'l']]); + }); +}); + +/* ── 3. the read count, with a control that fires ─────────────────────── */ + +describe('[#15302] the stamp issues NO read of its own', () => { + /** Finds the engine issued on OBJECT during one predicate update. */ + async function findsDuringUpdate(opts: Parameters[0]): Promise { + const { engine, driver } = await boot(opts); + await engine.insert(OBJECT, ROWS('package')); + driver.findCalls.length = 0; + await engine.update(OBJECT, { ...PAYLOAD }, WHERE); + return driver.findCalls.filter((c: any) => c.object === OBJECT).length; + } + + it('adds zero finds, while the pre-#15302 shape adds one PER MATCHED ROW', async () => { + // Arm C - an inert hook, registered identically, so the engine's own reads + // (D7's single matched-row read) are held constant across the arms. + const inert = (engine: any) => engine.registerHook('beforeUpdate', async () => {}, + { object: OBJECT, packageId: 'pin:15302-inert', priority: 150 }); + const baseline = await findsDuringUpdate({ stamp: false, extraHook: inert }); + + // Arm A - the shipped stamp. + const shipped = await findsDuringUpdate({ stamp: true }); + + // Arm B - the CONTROL, and the "before" reading: a replica of the read + // this card deleted, one `engine.find` per dispatch keyed on the row's id. + const rereading = (engine: any) => engine.registerHook('beforeUpdate', + async (ctx: any) => { + await engine.find(OBJECT, { + where: { id: ctx?.input?.id }, + fields: ['id', 'managed_by', 'customized'], + limit: 1, + context: { isSystem: true, positions: [], permissions: [] }, + }); + }, { object: OBJECT, packageId: 'pin:15302-control', priority: 150 }); + const withControl = await findsDuringUpdate({ stamp: false, extraHook: rereading }); + + // The control FIRES: the instrument can see a per-row re-read, and it + // costs exactly one find per matched row (2 rows ⇒ +2). + expect(withControl - baseline).toBe(2); + // And the shipped stamp costs none of them. + expect(shipped).toBe(baseline); + }); +}); diff --git a/packages/plugins/plugin-email/src/email-template-provenance.ts b/packages/plugins/plugin-email/src/email-template-provenance.ts index eacbf98d47..8baf2f64da 100644 --- a/packages/plugins/plugin-email/src/email-template-provenance.ts +++ b/packages/plugins/plugin-email/src/email-template-provenance.ts @@ -22,25 +22,43 @@ * — so a caller can never forge/clear `customized`, while this hook's stamp * survives. * - * Known boundary: multi-row updates (no single `input.id`) are not stamped — - * every template-editing UI path updates by id. + * Multi-row updates ARE stamped, once per matched row. [#15302] Since #5574 + * the engine dispatches `beforeUpdate` PER MATCHED ROW of a predicate + * (`multi: true`) write, each context carrying that row's `id` and `previous` + * (the `HookContext` contract; ADR-0058 Addendum II D1-D7). The inference this + * hook used to draw - "no single `input.id` means a bulk write, decline" - + * therefore answered "single write" on every row of a batch and guarded + * nothing. It is deleted rather than re-expressed against the engine's + * `dispatch` marker: taking part in EVERY write shape is the intent, so there + * is no decision left for the marker to gate (#6966 asks it in + * `file-reference-lifecycle.ts` because that guard REFUSES; this one stamps). + * + * What an operator's bulk edit does, measured on the real engine rather than + * assumed: the payload is BATCH-scoped (D3 - `driver.updateMany` takes ONE + * `SET` clause for N rows). Matched rows that AGREE are all stamped in that one + * clause and the write lands. Matched rows that DISAGREE would stamp some and + * not others, and the engine refuses the whole batch + * (`MULTI_UPDATE_HOOK_KEY_DIVERGENCE`, 400) rather than widening one row's + * stamp to the rest - which is what makes a row-conditioned rewrite safe to + * leave here. + * + * Declining on a predicate write was weighed and REJECTED (#15302): the seeder + * skips only rows marked `customized`, so the rows left unstamped would be + * exactly the ones the next boot clobbers - discarding the operator edit this + * stamp exists to remember. */ interface MinimalEngine { - find(object: string, opts?: any): Promise; registerHook(event: string, handler: (ctx: any) => any, options?: Record): void; unregisterHooksByPackage(packageId: string): number; } interface MinimalLogger { info?: (msg: string, meta?: Record) => void; - warn?: (msg: string, meta?: Record) => void; } export const EMAIL_TEMPLATE_PROVENANCE_PACKAGE = 'plugin-email:template-provenance'; -const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const; - export function bindEmailTemplateProvenanceStamp( engine: MinimalEngine, logger?: MinimalLogger, @@ -57,30 +75,20 @@ export function bindEmailTemplateProvenanceStamp( // Seeder / boot reconcilers write with isSystem — the package door, not // an admin customization. if ((ctx?.session as any)?.isSystem) return; - const id = ctx?.input?.id ?? (ctx?.input?.data as any)?.id; - if (!id) return; // multi-row update — see boundary note above const data = ctx?.input?.data; if (!data || typeof data !== 'object') return; - try { - // `previous` is not resolved before beforeUpdate hooks run — read the - // current row ourselves (system ctx: this is a provenance check, not - // an authorization decision). - const rows = await engine.find(object, { - where: { id }, - fields: ['id', 'managed_by', 'customized'], - limit: 1, - context: SYSTEM_CTX, - }); - const row = Array.isArray(rows) ? rows[0] : undefined; - if (!row) return; - if ((row.managed_by === 'package' || row.managed_by === 'platform') && row.customized !== true) { - (data as any).customized = true; - } - } catch (err: any) { - logger?.warn?.('[email] template provenance stamp failed (edit proceeds unstamped)', { - id, - error: err?.message, - }); + // [#15302] The engine has ALREADY read this row. `ctx.previous` is + // its pre-image, bound before `beforeUpdate` runs on BOTH write shapes + // (#5574 / #5846: the by-id path reads it ahead of the dispatch, and + // every per-row context of a predicate write carries its own), and it + // is the published `HookContext` contract rather than an engine + // internal. This hook used to issue its own `engine.find` here - on a + // predicate write, one extra read PER MATCHED ROW of a row the engine + // had just read. + const previous = ctx?.previous as Record | undefined; + if (!previous || typeof previous !== 'object') return; + if ((previous.managed_by === 'package' || previous.managed_by === 'platform') && previous.customized !== true) { + (data as any).customized = true; } }, { object, packageId: EMAIL_TEMPLATE_PROVENANCE_PACKAGE, priority: 150 }, diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-provenance.per-row.test.ts b/packages/plugins/plugin-sharing/src/sharing-rule-provenance.per-row.test.ts new file mode 100644 index 0000000000..208aa181c8 --- /dev/null +++ b/packages/plugins/plugin-sharing/src/sharing-rule-provenance.per-row.test.ts @@ -0,0 +1,250 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#15302] The `sys_sharing_rule` provenance stamp on a PREDICATE (`multi: true`) + * update, pinned against the REAL engine. + * + * This hook used to carry two comments that were assertions about runtime + * behaviour, and runtime measurement falsified both: + * + * 1. "multi-row updates (no single `input.id`) are not stamped" - false since + * #6966. Per-row `before*` dispatch binds `ctx.input.id` on EVERY context, + * so `if (!id) return` no longer detected a bulk write and the hook ran + * once per matched row regardless. + * 2. "`previous` is not resolved before beforeUpdate hooks run" - false since + * #5574 / #5846. The engine binds `previous` before dispatching + * `beforeUpdate` on both write shapes, so the hook's own `engine.find` was + * a second read of a row the engine had just read - once PER MATCHED ROW + * on a predicate write. + * + * A comment cannot be pinned, so what is pinned here is the behaviour each + * comment was wrong about. §3 is the read count, measured with a control that + * fires rather than asserted. + * + * ⚠️ The engine half of this suite resolves through `@objectstack/objectql`'s + * `exports` to `dist/` (this package aliases no objectql entry; the ledger in + * `scripts/check-test-source-alias.mjs` records that), so a stale objectql + * build makes these readings about the built artifact. The SUBJECT - + * `./sharing-rule-provenance.js` - is a relative import read from source, which is what an + * ablation of this file's fix mutates. + */ + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { bindRuleProvenanceStamp } from './sharing-rule-provenance.js'; + +const OBJECT = 'sys_sharing_rule'; +const silentLogger = { debug() {}, info() {}, warn() {}, error() {} }; + +/** Minimal in-memory driver. `findCalls` is the instrument §3 reads. */ +function makeStubDriver(): any { + const store = new Map>(); + const matches = (row: any, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) continue; + const e: any = v && typeof v === 'object' && '$in' in (v as any) ? undefined : v; + if (v && typeof v === 'object' && '$in' in (v as any)) { + if (!(v as any).$in.includes(row[k])) return false; + continue; + } + const expected = e && typeof e === 'object' && '$eq' in (e as any) ? (e as any).$eq : e; + if ((row[k] ?? null) !== (expected ?? null)) return false; + } + return true; + }; + const d: any = { + name: 'memory', version: '0.0.0', supports: {}, + store, + /** Every `find` the engine (or a hook, through `engine.find`) issues. */ + findCalls: [] as unknown[], + /** One entry per `updateMany` - the ONE `SET` clause N rows share (D3). */ + updateManyPayloads: [] as Record[], + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, async syncSchema() {}, + async find(o: string, ast: any) { + d.findCalls.push({ object: o, where: ast?.where }); + const rows = [...store.values()].filter((r) => matches(r, ast?.where)); + // Hold the caller's bound, AFTER the filter and by PRESENCE + // (`check:objectql-double-limit`): §3's control passes `limit: 1`, so a + // limit-blind double would answer it with the whole table. + return typeof ast?.limit === 'number' ? rows.slice(0, ast.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) { + const row = { ...data }; store.set(String(row.id), row); return row; + }, + async update(_o: string, id: string, data: Record) { + const cur = store.get(id); if (!cur) return null; + const u = { ...cur, ...data, id }; store.set(id, u); return u; + }, + async upsert(o: string, data: any) { return this.create(o, data); }, + async delete(_o: string, id: string) { return store.delete(id); }, + async count(_o: string, ast: any) { return [...store.values()].filter((r) => matches(r, ast?.where)).length; }, + async bulkCreate(o: string, rows: any[]) { return Promise.all(rows.map((r) => this.create(o, r))); }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async updateMany(_o: string, ast: any, data: Record) { + d.updateManyPayloads.push({ ...data }); + const rows = [...store.values()].filter((r) => matches(r, ast?.where)); + for (const r of rows) store.set(String(r.id), { ...r, ...data, id: r.id }); + return rows.length; + }, + async deleteMany() { return 0; }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return d; +} + +const text = (name: string) => ({ name, label: name, type: 'text' as const }); + +/** + * @param extraHook registered on the SAME event/object/priority as the stamp, + * so the engine's own read pattern is identical across §3's three arms. + */ +async function boot(opts: { stamp: boolean; extraHook?: (engine: any) => void } = { stamp: true }) { + const engine: any = new ObjectQL(); + const driver = makeStubDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject({ + name: OBJECT, label: OBJECT, + fields: { + id: { ...text('id'), primaryKey: true }, + managed_by: text('managed_by'), + customized: { name: 'customized', label: 'customized', type: 'boolean' as const }, + label: text('label'), + }, + }); + if (opts.stamp) bindRuleProvenanceStamp(engine, silentLogger); + opts.extraHook?.(engine); + return { engine, driver }; +} + +const ROWS = (bManagedBy: string) => ([ + { id: 'a', managed_by: 'package', customized: false, label: 'l' }, + { id: 'b', managed_by: bManagedBy, customized: false, label: 'l' }, +]); + +const PAYLOAD = { label: 'edited' }; +const WHERE = { multi: true, where: { id: { $in: ['a', 'b'] } } } as any; + +/* ── 1. every matched row is stamped ───────────────────────────────────── */ + +describe('[#15302] a predicate update stamps EVERY matched row', () => { + it('stamps both rows in ONE `SET` clause - the "no `input.id`" guard is gone', async () => { + const { engine, driver } = await boot(); + await engine.insert(OBJECT, ROWS('package')); + driver.updateManyPayloads.length = 0; + + await engine.update(OBJECT, { ...PAYLOAD }, WHERE); + + // Both matched rows carry the stamp: the documented "not stamped on + // multi-row updates" boundary never existed on this engine. + expect([...driver.store.values()].map((r: any) => [r.id, r.customized])) + .toEqual([['a', true], ['b', true]]); + // ADR-0058 Addendum II D3: N rows share ONE payload, hence one clause. + expect(driver.updateManyPayloads).toEqual([{ ...PAYLOAD, customized: true }]); + }); + + it('does not stamp when the pre-image is not package/platform managed', async () => { + // The control for the assertion above: the stamp is a decision about the + // ROW, so a run where no matched row qualifies must write no `customized`. + const { engine, driver } = await boot(); + await engine.insert(OBJECT, [ + { id: 'a', managed_by: 'admin', customized: false, label: 'l' }, + { id: 'b', managed_by: 'user', customized: false, label: 'l' }, + ]); + driver.updateManyPayloads.length = 0; + + await engine.update(OBJECT, { ...PAYLOAD }, WHERE); + + expect(driver.updateManyPayloads).toEqual([{ ...PAYLOAD }]); + expect([...driver.store.values()].map((r: any) => r.customized)).toEqual([false, false]); + }); + + it('does not stamp an isSystem write (the seeder door)', async () => { + const { engine, driver } = await boot(); + await engine.insert(OBJECT, ROWS('package')); + driver.updateManyPayloads.length = 0; + + await engine.update(OBJECT, { ...PAYLOAD }, { + ...WHERE, context: { isSystem: true, positions: [], permissions: [] }, + }); + + expect(driver.updateManyPayloads).toEqual([{ ...PAYLOAD }]); + }); +}); + +/* ── 2. divergent rows: the engine refuses, and nothing is written ─────── */ + +describe('[#15302] matched rows that disagree refuse the batch', () => { + it('refuses with the ADR-0112 envelope and writes nothing', async () => { + const { engine, driver } = await boot(); + await engine.insert(OBJECT, ROWS('user')); + driver.updateManyPayloads.length = 0; + + const err: any = await engine.update(OBJECT, { ...PAYLOAD }, WHERE).then( + () => { throw new Error('expected the batch to be refused'); }, + (e: any) => e, + ); + + // Read the envelope by FIELD (`code` + `status`, the minimum a rejection + // case asserts): a bare `toThrow()` would stay green against any error. + expect(err.code).toBe('MULTI_UPDATE_HOOK_KEY_DIVERGENCE'); + expect(err.status).toBe(400); + expect(err.keys).toEqual(['customized']); + expect(err.rows).toBe(2); + // The refusal is the SAFE side: no `SET` clause reached the driver and + // both rows are untouched. + expect(driver.updateManyPayloads).toEqual([]); + expect([...driver.store.values()].map((r: any) => [r.customized, r.label])) + .toEqual([[false, 'l'], [false, 'l']]); + }); +}); + +/* ── 3. the read count, with a control that fires ─────────────────────── */ + +describe('[#15302] the stamp issues NO read of its own', () => { + /** Finds the engine issued on OBJECT during one predicate update. */ + async function findsDuringUpdate(opts: Parameters[0]): Promise { + const { engine, driver } = await boot(opts); + await engine.insert(OBJECT, ROWS('package')); + driver.findCalls.length = 0; + await engine.update(OBJECT, { ...PAYLOAD }, WHERE); + return driver.findCalls.filter((c: any) => c.object === OBJECT).length; + } + + it('adds zero finds, while the pre-#15302 shape adds one PER MATCHED ROW', async () => { + // Arm C - an inert hook, registered identically, so the engine's own reads + // (D7's single matched-row read) are held constant across the arms. + const inert = (engine: any) => engine.registerHook('beforeUpdate', async () => {}, + { object: OBJECT, packageId: 'pin:15302-inert', priority: 150 }); + const baseline = await findsDuringUpdate({ stamp: false, extraHook: inert }); + + // Arm A - the shipped stamp. + const shipped = await findsDuringUpdate({ stamp: true }); + + // Arm B - the CONTROL, and the "before" reading: a replica of the read + // this card deleted, one `engine.find` per dispatch keyed on the row's id. + const rereading = (engine: any) => engine.registerHook('beforeUpdate', + async (ctx: any) => { + await engine.find(OBJECT, { + where: { id: ctx?.input?.id }, + fields: ['id', 'managed_by', 'customized'], + limit: 1, + context: { isSystem: true, positions: [], permissions: [] }, + }); + }, { object: OBJECT, packageId: 'pin:15302-control', priority: 150 }); + const withControl = await findsDuringUpdate({ stamp: false, extraHook: rereading }); + + // The control FIRES: the instrument can see a per-row re-read, and it + // costs exactly one find per matched row (2 rows ⇒ +2). + expect(withControl - baseline).toBe(2); + // And the shipped stamp costs none of them. + expect(shipped).toBe(baseline); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-provenance.test.ts b/packages/plugins/plugin-sharing/src/sharing-rule-provenance.test.ts index 7c13b7c194..7415bbb1c7 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-provenance.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-provenance.test.ts @@ -74,9 +74,16 @@ function makeEngine() { }, /** Test helper: simulate an engine update passing through beforeUpdate hooks. */ async updateThroughHooks(o: string, id: string, data: Row, session: Row) { + // [#15302] `previous` is bound BEFORE the beforeUpdate dispatch, as the + // real engine binds it (#5574 / #5846 - by-id reads the prior row ahead of + // the dispatch; each per-row context of a predicate write carries its own). + // Withholding it here is what let this fake model a pre-#5574 engine, and + // a hook reading `ctx.previous` would have gone silently unstamped against + // a fake no production caller resembles. + const previous = ensure(o).filter((r) => r.id === id).map((r) => ({ ...r }))[0]; for (const h of hooks) { if (h.event === 'beforeUpdate' && h.options.object === o) { - await h.handler({ session, input: { id, data } }); + await h.handler({ session, input: { id, data }, previous }); } } return this.update(o, id, data); @@ -191,9 +198,21 @@ describe('provenance stamp hook (#2909 T1)', () => { expect(engine._tables.sys_sharing_rule.find((r) => r.name === 'admin_rule')!.customized).toBe(false); }); - it('ignores multi-row updates (no id) without crashing', async () => { + it('writes nothing when the dispatch carries no pre-image (it never guesses the row)', async () => { + // [#15302] This case used to read "ignores multi-row updates (no id)" and + // asserted a boundary that does not exist: per-row dispatch binds + // `input.id` on EVERY context, so "no id" never identified a bulk write - + // and the id is carried here to say so. What IS true, and what this pins: + // the stamp is a decision about the row's pre-image, so a dispatch the + // engine gave no `previous` writes nothing rather than re-reading the row. + // The per-row behaviour itself is pinned against the real engine in + // `sharing-rule-provenance.per-row.test.ts`. const hook = engine._hooks.find((h) => h.options.packageId === SHARING_RULE_PROVENANCE_PACKAGE)!; - await expect(hook.handler({ session: { userId: 'admin1' }, input: { data: { active: false } } })).resolves.toBeUndefined(); + const data: Row = { active: false }; + await expect( + hook.handler({ session: { userId: 'admin1' }, input: { id: 'srule_absent', data } }), + ).resolves.toBeUndefined(); + expect(data).toEqual({ active: false }); }); it('end-to-end: admin edit through hooks → next seed does not clobber', async () => { diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-provenance.ts b/packages/plugins/plugin-sharing/src/sharing-rule-provenance.ts index 7e11a6d57b..c4ce320094 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-provenance.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-provenance.ts @@ -22,22 +22,41 @@ * run — so a caller can never forge/clear `customized`, while this hook's * stamp survives. * - * Known boundary (recorded in the ADR): multi-row updates (no single - * `input.id`) are not stamped — every rule-editing UI path updates by id. + * Multi-row updates ARE stamped, once per matched row. [#15302] Since #5574 + * the engine dispatches `beforeUpdate` PER MATCHED ROW of a predicate + * (`multi: true`) write, each context carrying that row's `id` and `previous` + * (the `HookContext` contract; ADR-0058 Addendum II D1-D7). The inference this + * hook used to draw - "no single `input.id` means a bulk write, decline" - + * therefore answered "single write" on every row of a batch and guarded + * nothing. It is deleted rather than re-expressed against the engine's + * `dispatch` marker: taking part in EVERY write shape is the intent, so there + * is no decision left for the marker to gate (#6966 asks it in + * `file-reference-lifecycle.ts` because that guard REFUSES; this one stamps). + * + * What an operator's bulk edit does, measured on the real engine rather than + * assumed: the payload is BATCH-scoped (D3 - `driver.updateMany` takes ONE + * `SET` clause for N rows). Matched rows that AGREE are all stamped in that one + * clause and the write lands. Matched rows that DISAGREE would stamp some and + * not others, and the engine refuses the whole batch + * (`MULTI_UPDATE_HOOK_KEY_DIVERGENCE`, 400) rather than widening one row's + * stamp to the rest - which is what makes a row-conditioned rewrite safe to + * leave here. + * + * Declining on a predicate write was weighed and REJECTED (#15302): the seeder + * skips only rows marked `customized`, so the rows left unstamped would be + * exactly the ones the next boot clobbers - discarding the operator edit this + * stamp exists to remember. */ import type { OptionalSharingLogger } from './logger-shapes.js'; interface MinimalEngine { - find(object: string, opts?: any): Promise; registerHook(event: string, handler: (ctx: any) => any, options?: Record): void; unregisterHooksByPackage(packageId: string): number; } export const SHARING_RULE_PROVENANCE_PACKAGE = 'plugin-sharing:rule-provenance'; -const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const; - export function bindRuleProvenanceStamp(engine: MinimalEngine, logger?: OptionalSharingLogger): void { engine.registerHook( 'beforeUpdate', @@ -45,30 +64,20 @@ export function bindRuleProvenanceStamp(engine: MinimalEngine, logger?: Optional // Seeder / defineRule / boot reconcilers write with isSystem — those // are the package door, not an admin customization. if ((ctx?.session as any)?.isSystem) return; - const id = ctx?.input?.id ?? (ctx?.input?.data as any)?.id; - if (!id) return; // multi-row update — see boundary note above const data = ctx?.input?.data; if (!data || typeof data !== 'object') return; - try { - // `previous` is not resolved before beforeUpdate hooks run — read the - // current row ourselves (system ctx: this is a provenance check, not - // an authorization decision). - const rows = await engine.find('sys_sharing_rule', { - where: { id }, - fields: ['id', 'managed_by', 'customized'], - limit: 1, - context: SYSTEM_CTX, - }); - const row = Array.isArray(rows) ? rows[0] : undefined; - if (!row) return; - if ((row.managed_by === 'package' || row.managed_by === 'platform') && row.customized !== true) { - (data as any).customized = true; - } - } catch (err: any) { - logger?.warn?.('[sharing-rule] provenance stamp failed (edit proceeds unstamped)', { - id, - error: err?.message, - }); + // [#15302] The engine has ALREADY read this row. `ctx.previous` is + // its pre-image, bound before `beforeUpdate` runs on BOTH write shapes + // (#5574 / #5846: the by-id path reads it ahead of the dispatch, and + // every per-row context of a predicate write carries its own), and it + // is the published `HookContext` contract rather than an engine + // internal. This hook used to issue its own `engine.find` here - on a + // predicate write, one extra read PER MATCHED ROW of a row the engine + // had just read. + const previous = ctx?.previous as Record | undefined; + if (!previous || typeof previous !== 'object') return; + if ((previous.managed_by === 'package' || previous.managed_by === 'platform') && previous.customized !== true) { + (data as any).customized = true; } }, { object: 'sys_sharing_rule', packageId: SHARING_RULE_PROVENANCE_PACKAGE, priority: 150 }, diff --git a/packages/plugins/plugin-webhooks/src/bootstrap-declared-webhooks.test.ts b/packages/plugins/plugin-webhooks/src/bootstrap-declared-webhooks.test.ts index 4928c2f597..c43105b0b8 100644 --- a/packages/plugins/plugin-webhooks/src/bootstrap-declared-webhooks.test.ts +++ b/packages/plugins/plugin-webhooks/src/bootstrap-declared-webhooks.test.ts @@ -77,7 +77,15 @@ class FakeEngine { async update(name: string, data: any, opts?: any): Promise { // Run beforeUpdate hooks (the provenance stamp lives here). const id = data?.id ?? opts?.where?.id; - const ctx = { input: { id, data }, session: opts?.context }; + // [#15302] `previous` is bound BEFORE the beforeUpdate dispatch, as the + // real engine binds it (#5574 / #5846 - by-id reads the prior row ahead of + // the dispatch; each per-row context of a predicate write carries its own). + // Withholding it here is what let this fake model a pre-#5574 engine, and + // a hook reading `ctx.previous` would have gone silently unstamped against + // a fake no production caller resembles. + const cond0 = opts?.where ?? (id ? { id } : undefined); + const previous = (this.rows[name] ?? []).filter((r) => this.matches(r, cond0)).map((r) => ({ ...r }))[0]; + const ctx = { input: { id, data }, previous, session: opts?.context }; for (const h of this.hooks) { if (h.event === 'beforeUpdate' && (!h.object || h.object === name)) { await h.handler(ctx); diff --git a/packages/plugins/plugin-webhooks/src/webhook-provenance.per-row.test.ts b/packages/plugins/plugin-webhooks/src/webhook-provenance.per-row.test.ts new file mode 100644 index 0000000000..3e836c69b9 --- /dev/null +++ b/packages/plugins/plugin-webhooks/src/webhook-provenance.per-row.test.ts @@ -0,0 +1,250 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#15302] The `sys_webhook` provenance stamp on a PREDICATE (`multi: true`) + * update, pinned against the REAL engine. + * + * This hook used to carry two comments that were assertions about runtime + * behaviour, and runtime measurement falsified both: + * + * 1. "multi-row updates (no single `input.id`) are not stamped" - false since + * #6966. Per-row `before*` dispatch binds `ctx.input.id` on EVERY context, + * so `if (!id) return` no longer detected a bulk write and the hook ran + * once per matched row regardless. + * 2. "`previous` is not resolved before beforeUpdate hooks run" - false since + * #5574 / #5846. The engine binds `previous` before dispatching + * `beforeUpdate` on both write shapes, so the hook's own `engine.find` was + * a second read of a row the engine had just read - once PER MATCHED ROW + * on a predicate write. + * + * A comment cannot be pinned, so what is pinned here is the behaviour each + * comment was wrong about. §3 is the read count, measured with a control that + * fires rather than asserted. + * + * ⚠️ The engine half of this suite resolves through `@objectstack/objectql`'s + * `exports` to `dist/` (this package aliases no objectql entry; the ledger in + * `scripts/check-test-source-alias.mjs` records that), so a stale objectql + * build makes these readings about the built artifact. The SUBJECT - + * `./webhook-provenance.js` - is a relative import read from source, which is what an + * ablation of this file's fix mutates. + */ + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { bindWebhookProvenanceStamp } from './webhook-provenance.js'; + +const OBJECT = 'sys_webhook'; +const silentLogger = { debug() {}, info() {}, warn() {}, error() {} }; + +/** Minimal in-memory driver. `findCalls` is the instrument §3 reads. */ +function makeStubDriver(): any { + const store = new Map>(); + const matches = (row: any, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) continue; + const e: any = v && typeof v === 'object' && '$in' in (v as any) ? undefined : v; + if (v && typeof v === 'object' && '$in' in (v as any)) { + if (!(v as any).$in.includes(row[k])) return false; + continue; + } + const expected = e && typeof e === 'object' && '$eq' in (e as any) ? (e as any).$eq : e; + if ((row[k] ?? null) !== (expected ?? null)) return false; + } + return true; + }; + const d: any = { + name: 'memory', version: '0.0.0', supports: {}, + store, + /** Every `find` the engine (or a hook, through `engine.find`) issues. */ + findCalls: [] as unknown[], + /** One entry per `updateMany` - the ONE `SET` clause N rows share (D3). */ + updateManyPayloads: [] as Record[], + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, async syncSchema() {}, + async find(o: string, ast: any) { + d.findCalls.push({ object: o, where: ast?.where }); + const rows = [...store.values()].filter((r) => matches(r, ast?.where)); + // Hold the caller's bound, AFTER the filter and by PRESENCE + // (`check:objectql-double-limit`): §3's control passes `limit: 1`, so a + // limit-blind double would answer it with the whole table. + return typeof ast?.limit === 'number' ? rows.slice(0, ast.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) { + const row = { ...data }; store.set(String(row.id), row); return row; + }, + async update(_o: string, id: string, data: Record) { + const cur = store.get(id); if (!cur) return null; + const u = { ...cur, ...data, id }; store.set(id, u); return u; + }, + async upsert(o: string, data: any) { return this.create(o, data); }, + async delete(_o: string, id: string) { return store.delete(id); }, + async count(_o: string, ast: any) { return [...store.values()].filter((r) => matches(r, ast?.where)).length; }, + async bulkCreate(o: string, rows: any[]) { return Promise.all(rows.map((r) => this.create(o, r))); }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async updateMany(_o: string, ast: any, data: Record) { + d.updateManyPayloads.push({ ...data }); + const rows = [...store.values()].filter((r) => matches(r, ast?.where)); + for (const r of rows) store.set(String(r.id), { ...r, ...data, id: r.id }); + return rows.length; + }, + async deleteMany() { return 0; }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return d; +} + +const text = (name: string) => ({ name, label: name, type: 'text' as const }); + +/** + * @param extraHook registered on the SAME event/object/priority as the stamp, + * so the engine's own read pattern is identical across §3's three arms. + */ +async function boot(opts: { stamp: boolean; extraHook?: (engine: any) => void } = { stamp: true }) { + const engine: any = new ObjectQL(); + const driver = makeStubDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject({ + name: OBJECT, label: OBJECT, + fields: { + id: { ...text('id'), primaryKey: true }, + managed_by: text('managed_by'), + customized: { name: 'customized', label: 'customized', type: 'boolean' as const }, + label: text('label'), + }, + }); + if (opts.stamp) bindWebhookProvenanceStamp(engine, silentLogger); + opts.extraHook?.(engine); + return { engine, driver }; +} + +const ROWS = (bManagedBy: string) => ([ + { id: 'a', managed_by: 'package', customized: false, label: 'l' }, + { id: 'b', managed_by: bManagedBy, customized: false, label: 'l' }, +]); + +const PAYLOAD = { label: 'edited' }; +const WHERE = { multi: true, where: { id: { $in: ['a', 'b'] } } } as any; + +/* ── 1. every matched row is stamped ───────────────────────────────────── */ + +describe('[#15302] a predicate update stamps EVERY matched row', () => { + it('stamps both rows in ONE `SET` clause - the "no `input.id`" guard is gone', async () => { + const { engine, driver } = await boot(); + await engine.insert(OBJECT, ROWS('package')); + driver.updateManyPayloads.length = 0; + + await engine.update(OBJECT, { ...PAYLOAD }, WHERE); + + // Both matched rows carry the stamp: the documented "not stamped on + // multi-row updates" boundary never existed on this engine. + expect([...driver.store.values()].map((r: any) => [r.id, r.customized])) + .toEqual([['a', true], ['b', true]]); + // ADR-0058 Addendum II D3: N rows share ONE payload, hence one clause. + expect(driver.updateManyPayloads).toEqual([{ ...PAYLOAD, customized: true }]); + }); + + it('does not stamp when the pre-image is not package/platform managed', async () => { + // The control for the assertion above: the stamp is a decision about the + // ROW, so a run where no matched row qualifies must write no `customized`. + const { engine, driver } = await boot(); + await engine.insert(OBJECT, [ + { id: 'a', managed_by: 'admin', customized: false, label: 'l' }, + { id: 'b', managed_by: 'user', customized: false, label: 'l' }, + ]); + driver.updateManyPayloads.length = 0; + + await engine.update(OBJECT, { ...PAYLOAD }, WHERE); + + expect(driver.updateManyPayloads).toEqual([{ ...PAYLOAD }]); + expect([...driver.store.values()].map((r: any) => r.customized)).toEqual([false, false]); + }); + + it('does not stamp an isSystem write (the seeder door)', async () => { + const { engine, driver } = await boot(); + await engine.insert(OBJECT, ROWS('package')); + driver.updateManyPayloads.length = 0; + + await engine.update(OBJECT, { ...PAYLOAD }, { + ...WHERE, context: { isSystem: true, positions: [], permissions: [] }, + }); + + expect(driver.updateManyPayloads).toEqual([{ ...PAYLOAD }]); + }); +}); + +/* ── 2. divergent rows: the engine refuses, and nothing is written ─────── */ + +describe('[#15302] matched rows that disagree refuse the batch', () => { + it('refuses with the ADR-0112 envelope and writes nothing', async () => { + const { engine, driver } = await boot(); + await engine.insert(OBJECT, ROWS('user')); + driver.updateManyPayloads.length = 0; + + const err: any = await engine.update(OBJECT, { ...PAYLOAD }, WHERE).then( + () => { throw new Error('expected the batch to be refused'); }, + (e: any) => e, + ); + + // Read the envelope by FIELD (`code` + `status`, the minimum a rejection + // case asserts): a bare `toThrow()` would stay green against any error. + expect(err.code).toBe('MULTI_UPDATE_HOOK_KEY_DIVERGENCE'); + expect(err.status).toBe(400); + expect(err.keys).toEqual(['customized']); + expect(err.rows).toBe(2); + // The refusal is the SAFE side: no `SET` clause reached the driver and + // both rows are untouched. + expect(driver.updateManyPayloads).toEqual([]); + expect([...driver.store.values()].map((r: any) => [r.customized, r.label])) + .toEqual([[false, 'l'], [false, 'l']]); + }); +}); + +/* ── 3. the read count, with a control that fires ─────────────────────── */ + +describe('[#15302] the stamp issues NO read of its own', () => { + /** Finds the engine issued on OBJECT during one predicate update. */ + async function findsDuringUpdate(opts: Parameters[0]): Promise { + const { engine, driver } = await boot(opts); + await engine.insert(OBJECT, ROWS('package')); + driver.findCalls.length = 0; + await engine.update(OBJECT, { ...PAYLOAD }, WHERE); + return driver.findCalls.filter((c: any) => c.object === OBJECT).length; + } + + it('adds zero finds, while the pre-#15302 shape adds one PER MATCHED ROW', async () => { + // Arm C - an inert hook, registered identically, so the engine's own reads + // (D7's single matched-row read) are held constant across the arms. + const inert = (engine: any) => engine.registerHook('beforeUpdate', async () => {}, + { object: OBJECT, packageId: 'pin:15302-inert', priority: 150 }); + const baseline = await findsDuringUpdate({ stamp: false, extraHook: inert }); + + // Arm A - the shipped stamp. + const shipped = await findsDuringUpdate({ stamp: true }); + + // Arm B - the CONTROL, and the "before" reading: a replica of the read + // this card deleted, one `engine.find` per dispatch keyed on the row's id. + const rereading = (engine: any) => engine.registerHook('beforeUpdate', + async (ctx: any) => { + await engine.find(OBJECT, { + where: { id: ctx?.input?.id }, + fields: ['id', 'managed_by', 'customized'], + limit: 1, + context: { isSystem: true, positions: [], permissions: [] }, + }); + }, { object: OBJECT, packageId: 'pin:15302-control', priority: 150 }); + const withControl = await findsDuringUpdate({ stamp: false, extraHook: rereading }); + + // The control FIRES: the instrument can see a per-row re-read, and it + // costs exactly one find per matched row (2 rows ⇒ +2). + expect(withControl - baseline).toBe(2); + // And the shipped stamp costs none of them. + expect(shipped).toBe(baseline); + }); +}); diff --git a/packages/plugins/plugin-webhooks/src/webhook-provenance.ts b/packages/plugins/plugin-webhooks/src/webhook-provenance.ts index 41c01023dc..06e6fe8de8 100644 --- a/packages/plugins/plugin-webhooks/src/webhook-provenance.ts +++ b/packages/plugins/plugin-webhooks/src/webhook-provenance.ts @@ -21,25 +21,43 @@ * — so a caller can never forge/clear `customized`, while this hook's stamp * survives. * - * Known boundary: multi-row updates (no single `input.id`) are not stamped — - * every webhook-editing UI path updates by id. + * Multi-row updates ARE stamped, once per matched row. [#15302] Since #5574 + * the engine dispatches `beforeUpdate` PER MATCHED ROW of a predicate + * (`multi: true`) write, each context carrying that row's `id` and `previous` + * (the `HookContext` contract; ADR-0058 Addendum II D1-D7). The inference this + * hook used to draw - "no single `input.id` means a bulk write, decline" - + * therefore answered "single write" on every row of a batch and guarded + * nothing. It is deleted rather than re-expressed against the engine's + * `dispatch` marker: taking part in EVERY write shape is the intent, so there + * is no decision left for the marker to gate (#6966 asks it in + * `file-reference-lifecycle.ts` because that guard REFUSES; this one stamps). + * + * What an operator's bulk edit does, measured on the real engine rather than + * assumed: the payload is BATCH-scoped (D3 - `driver.updateMany` takes ONE + * `SET` clause for N rows). Matched rows that AGREE are all stamped in that one + * clause and the write lands. Matched rows that DISAGREE would stamp some and + * not others, and the engine refuses the whole batch + * (`MULTI_UPDATE_HOOK_KEY_DIVERGENCE`, 400) rather than widening one row's + * stamp to the rest - which is what makes a row-conditioned rewrite safe to + * leave here. + * + * Declining on a predicate write was weighed and REJECTED (#15302): the seeder + * skips only rows marked `customized`, so the rows left unstamped would be + * exactly the ones the next boot clobbers - discarding the operator edit this + * stamp exists to remember. */ interface MinimalEngine { - find(object: string, opts?: any): Promise; registerHook(event: string, handler: (ctx: any) => any, options?: Record): void; unregisterHooksByPackage(packageId: string): number; } interface MinimalLogger { info?: (msg: string, meta?: Record) => void; - warn?: (msg: string, meta?: Record) => void; } export const WEBHOOK_PROVENANCE_PACKAGE = 'plugin-webhooks:provenance'; -const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const; - export function bindWebhookProvenanceStamp(engine: MinimalEngine, logger?: MinimalLogger): void { if (typeof engine?.registerHook !== 'function') return; engine.registerHook( @@ -48,30 +66,20 @@ export function bindWebhookProvenanceStamp(engine: MinimalEngine, logger?: Minim // Seeder / boot reconcilers write with isSystem — the package door, not // an admin customization. if ((ctx?.session as any)?.isSystem) return; - const id = ctx?.input?.id ?? (ctx?.input?.data as any)?.id; - if (!id) return; // multi-row update — see boundary note above const data = ctx?.input?.data; if (!data || typeof data !== 'object') return; - try { - // `previous` is not resolved before beforeUpdate hooks run — read the - // current row ourselves (system ctx: this is a provenance check, not - // an authorization decision). - const rows = await engine.find('sys_webhook', { - where: { id }, - fields: ['id', 'managed_by', 'customized'], - limit: 1, - context: SYSTEM_CTX, - }); - const row = Array.isArray(rows) ? rows[0] : undefined; - if (!row) return; - if ((row.managed_by === 'package' || row.managed_by === 'platform') && row.customized !== true) { - (data as any).customized = true; - } - } catch (err: any) { - logger?.warn?.('[webhook] provenance stamp failed (edit proceeds unstamped)', { - id, - error: err?.message, - }); + // [#15302] The engine has ALREADY read this row. `ctx.previous` is + // its pre-image, bound before `beforeUpdate` runs on BOTH write shapes + // (#5574 / #5846: the by-id path reads it ahead of the dispatch, and + // every per-row context of a predicate write carries its own), and it + // is the published `HookContext` contract rather than an engine + // internal. This hook used to issue its own `engine.find` here - on a + // predicate write, one extra read PER MATCHED ROW of a row the engine + // had just read. + const previous = ctx?.previous as Record | undefined; + if (!previous || typeof previous !== 'object') return; + if ((previous.managed_by === 'package' || previous.managed_by === 'platform') && previous.customized !== true) { + (data as any).customized = true; } }, { object: 'sys_webhook', packageId: WEBHOOK_PROVENANCE_PACKAGE, priority: 150 },