From f31b19f199965466e7ed2d6ce036cdf050d02341 Mon Sep 17 00:00:00 2001 From: "Claude Fable 5.1" Date: Wed, 2 Sep 2026 06:19:37 +0000 Subject: [PATCH 1/5] wip(objectql): thread hook-write provenance into the readonlyWhen and runtime-owned strips Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68 --- packages/objectql/src/engine.ts | 123 ++++++++++++++-- .../objectql/src/validation/rule-validator.ts | 138 +++++++++++++++--- 2 files changed, 227 insertions(+), 34 deletions(-) diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 7eae1362b6..b857ca6be6 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -193,6 +193,7 @@ 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 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 // total over the MASTER's declared fields before it leaves this engine — the @@ -9576,19 +9577,62 @@ export class ObjectQL implements IObjectQLEngine { // is created once per CALL and shared by every row's context, before and // after — see `HookContext.dispatch`. const insertScope: Record = {}; + // ── [#14259] ARM the hook-write recording, one PER ROW ─────────────── + // + // The insert-side twin of the update path's single recording (#14088), + // and the reason it has to exist here at all: `stripRuntimeOwnedFields` + // below judges hook-vs-caller by `Object.is`, which cannot separate "the + // hook re-issued the record number the caller also submitted" from "the + // hook never touched the key". #6339 moved that judgement from a key SET + // to values on the argument that the key set made the contract true "only + // BY ACCIDENT" — and values are accidental in the identical way. + // + // PER ROW, not per call: a batch runs `beforeInsert` once per row against + // its own payload object, so one shared record would let a hook that + // stamps row 3 exempt row 4's caller-seeded value. One recording per row + // is what makes the answer mean "a hook assigned THIS key on THIS row". + // + // The two ends of the window are load-bearing, exactly as on the update + // path (the recorder module's own header carries the full argument): + // + // - ARMED HERE, over `defaultedData` — after the caller's payload has + // arrived, after `suppliedPerRow` snapshotted it, and after the + // engine's own defaulting (`applyFieldDefaults` / + // `initializeSummaryFields`) has run, so no engine default and no + // caller key can enter the record. A caller cannot execute an + // assignment; echoing a key, a value, a `null` or a `Proxy` back is + // not a `set` on this object. + // - SEALED immediately below, before the post-hook declared-field door + // reads the payload and long before `encryptSecretFields` writes to + // it. A recorder still armed for an engine-owned pass would report + // ENGINE writes as HOOK writes, which on a caller-forged secret column + // is precisely the escalation this is built to make impossible. + // + // Between those two points the only code that runs is the `beforeInsert` + // dispatch — server code, by definition. Writes through the recording + // land on the SAME row object, so a hook mutating `ctx.input.data.x` in + // place is mutating the engine's row exactly as it always has. + const rowHookWrites: Array = []; const rowHookContexts: HookContext[] = (isBatch ? (defaultedData as any[]) : [defaultedData]).map( - (row, rowIndex) => ({ - object, - event: 'beforeInsert', - input: { data: row, options: opCtx.options }, - dispatch: { mode: isBatch ? 'per-row' : 'record', index: rowIndex, scope: insertScope }, - session: this.buildSession(opCtx.context), - provenance: this.buildProvenance(opCtx.context), - user: this.buildUser(opCtx.context), - api: this.buildHookApi(opCtx.context), - transaction: opCtx.context?.transaction, - ql: this, - }), + (row, rowIndex) => { + const recording = + row !== null && typeof row === 'object' + ? recordHookPayloadWrites(row as Record) + : undefined; + rowHookWrites[rowIndex] = recording; + return { + object, + event: 'beforeInsert', + input: { data: recording?.payload ?? row, options: opCtx.options }, + dispatch: { mode: isBatch ? 'per-row' : 'record', index: rowIndex, scope: insertScope }, + session: this.buildSession(opCtx.context), + provenance: this.buildProvenance(opCtx.context), + user: this.buildUser(opCtx.context), + api: this.buildHookApi(opCtx.context), + transaction: opCtx.context?.transaction, + ql: this, + }; + }, ); // [#8682] A row the declared-field door refused runs NO hook. In // non-partial mode the throw above already returned, so this skip only @@ -9599,6 +9643,30 @@ export class ObjectQL implements IObjectQLEngine { if (undeclaredPerRow[i] !== undefined) continue; await this.triggerHooks('beforeInsert', rowHookContexts[i]); } + // ── [#14259] SEAL, before anything engine-owned reads or writes a row ── + // + // Sealing does two things and the write is wrong without either: it + // freezes each record before the engine's own passes can be mis-recorded + // as hook writes, and it puts the RAW row back in `input.data` so no + // recording view reaches a driver. + // + // Every row is sealed, including one the declared-field door culled: that + // row ran no hook, so its record is legitimately EMPTY, and restoring its + // raw payload keeps `rows[i]` a plain object on every branch rather than + // only on the live ones. + // + // `hookWrittenKeys` is `undefined` — not empty — for a row whose hook + // REPLACED the payload object (`ctx.input.data = { …ctx.input.data }`). + // `stripRuntimeOwnedFields` must read that as "fall back to the #6339 + // value test", never as "no hook wrote anything": a replacement's keys + // are mostly the CALLER's, so treating them as hook-owned would launder a + // caller-seeded record number into a platform write. + const rowHookWrittenKeys: Array | undefined> = []; + for (let i = 0; i < rowHookContexts.length; i++) { + const sealed = rowHookWrites[i]?.seal(rowHookContexts[i]!.input.data); + if (sealed) rowHookContexts[i]!.input.data = sealed.data as any; + rowHookWrittenKeys[i] = sealed?.hookWrittenKeys; + } // ── [#13657] The POST-hook half of the declared-field door ─────────── // @@ -9789,9 +9857,20 @@ export class ObjectQL implements IObjectQLEngine { // `origin/main`: `driverCreates 0` while the line said the write // was "COMMITTED WITHOUT IT". The card marked this half UNVERIFIED; // it reproduces, so the flag is threaded here too. + // [#14259] `hookWrittenKeys` — THIS row's sealed record, the other + // half of the same question `suppliedPerRow[i]` answers. #6339 + // handed values over instead of a key set because "the caller named + // this key" and "this key still holds the caller's value" are + // different facts; the record closes the case values cannot reach, + // where the hook wrote the value the caller also sent. The value + // test stays as the fallback for any row with no record. const stripped = stripRuntimeOwnedFields( schemaForValidation as any, rows[i], suppliedPerRow[i] ?? {}, this.logger, - { preserveAudit, strictReadonlyWrites: options?.strictReadonlyWrites === true }, + { + preserveAudit, + strictReadonlyWrites: options?.strictReadonlyWrites === true, + hookWrittenKeys: rowHookWrittenKeys[i], + }, ) as Record; if (stripped === rows[i]) continue; for (const k of Object.keys(rows[i])) { @@ -10965,7 +11044,18 @@ export class ObjectQL implements IObjectQLEngine { // is unchanged — a caller cannot make its own value look // hook-written (see `ReadonlyWhenStripOptions`) — and `isSystem` // is still NOT an exemption here, unlike the static strip below. - hookContext.input.data = stripReadonlyWhenFields(updateSchema as any, preRoWhen, priorRecord, this.logger, roWhenParent, { supplied: suppliedValues }) as any; + // + // [#14259] `hookWrittenKeys` — the SAME record sealed at the + // confluence above and consumed by the static strip below, now + // feeding the conditional one too. Threaded rather than + // re-derived: #9107 put this predicate in ONE shared function so + // the two `readonlyWhen` call sites could not fork, and a third + // derivation of "a hook wrote this key" would fork it against + // the static strip instead — which is the state #14088 left + // behind and this closes. The `supplied` snapshot is unchanged + // and still answers the caller-side half; the record only ever + // turns a STRIP into a KEEP, and only for a key a hook assigned. + hookContext.input.data = stripReadonlyWhenFields(updateSchema as any, preRoWhen, priorRecord, this.logger, roWhenParent, { supplied: suppliedValues, hookWrittenKeys }) as any; reportDroppedFields(preRoWhen, hookContext.input.data as Record, 'readonly_when'); // [#2948] Enforce STATIC `readonly` on the write path for // non-system callers (system writes legitimately set read-only @@ -11144,7 +11234,10 @@ export class ObjectQL implements IObjectQLEngine { // by-id branch above — "both call sites" is the #3106 / // #4441 shape that gets missed, and a bulk write must not // reach a different verdict about who wrote a key. - hookContext.input.data = stripReadonlyWhenFieldsMulti(updateSchema as any, preRoWhenMulti, priorRows, this.logger, parentForRow, { supplied: suppliedValues }) as any; + // [#14259] Which is why `hookWrittenKeys` is the SAME sealed + // set the by-id branch gets, from the one seal at the shared + // confluence — not a second recording armed on this branch. + hookContext.input.data = stripReadonlyWhenFieldsMulti(updateSchema as any, preRoWhenMulti, priorRows, this.logger, parentForRow, { supplied: suppliedValues, hookWrittenKeys }) as any; reportDroppedFields(preRoWhenMulti, hookContext.input.data as Record, 'readonly_when'); } // [#2948] Same static-`readonly` write guard on the bulk path — diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index 2aaaa242b0..b01fd9ab45 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -538,19 +538,22 @@ function readonlyWhenBindings( * caller's — the identical two-part test `stripReadonlyFields` applies: * * 1. the key is an OWN property of `supplied` (a key a hook ADDED is not), and - * 2. the payload still holds the caller's VALUE by `Object.is` (a key a hook - * OVERWROTE now carries a platform value, not a forgery). + * 2. a hook ASSIGNED the key (#14259 — by RECORD, see `hookWrittenKeys`), or + * failing a record, the payload still holds the caller's VALUE by + * `Object.is` (a key a hook OVERWROTE now carries a platform value, not a + * forgery). * * ⚠️ **This does not weaken the lock at the API boundary, and the reason is * that a caller cannot reach the exempt side of either test.** To be treated as - * hook-written, a value must differ from what arrived at engine entry — which - * only server code can arrange. A client that echoes the locked key back does - * not launder it: either no hook touched it (test 2 holds, it is stripped) or a - * hook overwrote it (the value that persists is the HOOK's, and the client's is - * gone regardless). What changed is who may write the column — the server's own - * trusted hooks — not whether a caller may. `isSystem` is deliberately still NOT - * an exemption here: a state lock that any system-context write could bypass - * would not be a state lock (#4889's frozen paid-invoice lines are built on it). + * hook-written, a key must have been ASSIGNED by server code after the caller's + * payload arrived (or, under the fallback, carry a value that differs from what + * arrived at engine entry — which again only server code can arrange). A client + * that echoes the locked key back does not launder it: no hook touched it, so + * neither test exempts it and it is stripped. What changed is who may write the + * column — the server's own trusted hooks — not whether a caller may. `isSystem` + * is deliberately still NOT an exemption here: a state lock that any + * system-context write could bypass would not be a state lock (#4889's frozen + * paid-invoice lines are built on it). * * ABSENT `supplied` means "treat the whole payload as caller-supplied" — the * pre-#9107 behaviour, and the fail-SAFE default: a call site that has no entry @@ -563,6 +566,28 @@ interface ReadonlyWhenStripOptions { * before the before-phase hooks. Omit to judge every key in the payload. */ supplied?: Readonly>; + /** + * [#14259] The keys the before-phase hook chain ACTUALLY ASSIGNED on this + * payload, recorded while the writes happened — `recordHookPayloadWrites`, + * the instrument #14088 built for the static `readonly` strip, threaded to + * its `readonlyWhen` sibling so the two cannot disagree about what + * "caller-supplied" means (which, between #14088 and this, they did). + * + * OPTIONAL, and absent means "this call cannot say", never "no hook wrote + * anything": a direct caller with no recording, and any write whose hook + * REPLACED the payload object, fall back to the `Object.is` test below + * exactly as before this option existed. + * + * ⚠️ It may only ever turn a STRIP into a KEEP, and only for a key a hook + * assigned. A caller cannot put a key in here — echoing a key, a value, a + * `null` or a `Proxy` back is not a `set` on the recorded object — so the + * #4889 lock is untouched: a caller-supplied value that NO hook wrote is + * still stripped, still warns with the same text, and still reports through + * `onFieldsDropped` / `strictReadonlyWrites`. Any future producer of this set + * owes the same proof, because a key in it is a key this strip stops + * defending. + */ + hookWrittenKeys?: ReadonlySet; } /** @@ -575,16 +600,36 @@ function isCallerSuppliedValue( data: Record, supplied: Readonly>, name: string, + hookWrittenKeys?: ReadonlySet, ): boolean { // Own-property, never `in`: a field name is `^[a-z_][a-z0-9_]*$`, which // admits `constructor` / `valueOf` — inherited from `Object.prototype` on any // plain snapshot, so `in` would call a hook stamp caller-supplied and strip // it (the trap #5591 names on the static strip, one function over). if (!Object.prototype.hasOwnProperty.call(supplied, name)) return false; + // [#14259] ...or a hook ASSIGNED it. Asked BEFORE the value comparison, in + // the same position and for the same reason as inside `stripReadonlyFields`: + // it answers by RECORD the question the comparison is only a proxy for. + // `Object.is` collapses "the hook deliberately wrote the value the caller + // also sent" into "the hook never touched the key", and those two demand + // opposite verdicts — the measured shape being a `beforeUpdate` hook that + // derives a `readonlyWhen`-locked field against a caller that round-tripped + // the whole record and echoed the same value back, whose derivation is then + // deleted. Not a `null` bug: `0`, `''`, `false` and a shared object reference + // collide identically, which is why the answer is provenance, not a sentinel. + if (hookWrittenKeys?.has(name)) return false; // ...and it must still BE the caller's value. `Object.is`, not `===`, on // purpose: `===` reports NaN !== NaN, which would read a caller-forged NaN as // "a hook rewrote it" and KEEP the forgery — the one input where the loose // operator inverts the verdict. + // + // [#14259] STAYS, and stays as the fallback for every key the record above + // has nothing to say about: a call site that passes no `hookWrittenKeys` gets + // byte-identical behaviour, and a hook that REPLACED the payload object + // leaves no record, so this test is what still separates its overwrite from a + // forgery. ⛔ Not "keep everything" — the fallback over-strips (the + // pre-existing defect), which is the only safe direction: reading a + // replacement's keys as hook-owned would launder a caller's forgery. return Object.is(data[name], supplied[name]); } @@ -632,7 +677,7 @@ export function stripReadonlyWhenFields( // is not this strip's business at all, so there is nothing to evaluate and // nothing to warn about — including the unbound-root LOCKED branch, whose // fail-CLOSED verdict exists to protect against an unjudgeable CALLER write. - if (!isCallerSuppliedValue(data, supplied, name)) continue; + if (!isCallerSuppliedValue(data, supplied, name, options?.hookWrittenKeys)) continue; if (isReadonlyWhenLocked(def, view.merged, view.previous, name, logger, parent)) { if (result === data) result = { ...data }; delete (result as Record)[name]; @@ -866,7 +911,12 @@ export function stripReadonlyWhenFieldsMulti( if (!def?.readonlyWhen || !(name in data)) continue; // [#9107] Same authorship gate as the single-id strip, asked before any row // is judged — a hook-written key is exempt for the whole batch, not per row. - if (!isCallerSuppliedValue(data, supplied, name)) continue; + // [#14259] ...off the SAME sealed record, too: the engine seals once at the + // shared post-hook confluence and hands the identical set to both branches, + // so a bulk write cannot reach a different verdict about who wrote a key + // than a by-id write does (the #3106 / #4441 divergence, which two separate + // derivations of "a hook wrote this" would reproduce exactly). + if (!isCallerSuppliedValue(data, supplied, name, options?.hookWrittenKeys)) continue; const lockedInSomeRow = rowViews().some((view, i) => isReadonlyWhenLocked( def, @@ -1300,25 +1350,63 @@ export function stripReadonlyFields( * `onFieldsDropped` / `strictReadonlyWrites`. What changed is exclusively the * case where the value being deleted was never the caller's. * + * ### ...and why VALUES were not enough either (#14259) + * + * #6339's own sentence is the finding, one iteration on: it argued that a key + * SET made the contract true "only BY ACCIDENT", and value equality is + * accidental in the identical way. `Object.is(row[name], supplied[name])` + * cannot separate *the hook re-issued the record number the caller happened to + * submit* from *the hook never touched the key*, and those demand opposite + * verdicts — so #6339's own measured row (a `beforeInsert` hook that re-issues + * or normalises `code`) still loses its write to the one caller who submitted + * the same value. `options.hookWrittenKeys` answers by RECORD what the + * comparison could only infer; the comparison stays as the fallback for every + * key no record covers. + * * KNOWN LIMIT, identical to the update side's and deliberately not papered over: - * the snapshot is SHALLOW, so a hook that mutates a caller-supplied object IN - * PLACE is indistinguishable from a hook that did nothing, and the field is - * still stripped. That fallback is the pre-#6339 behaviour, i.e. fail-safe; a - * hook meaning to own a runtime-owned column should ASSIGN to it. (An - * `autonumber` value is a scalar in every supported shape, so this limit is - * theoretical here in a way it is not for the update path's `json` columns.) + * without a record the snapshot is SHALLOW, so a hook that mutates a + * caller-supplied object IN PLACE is indistinguishable from a hook that did + * nothing, and the field is still stripped. That fallback is the pre-#6339 + * behaviour, i.e. fail-safe; a hook meaning to own a runtime-owned column + * should ASSIGN to it. (An `autonumber` value is a scalar in every supported + * shape, so this limit is theoretical here in a way it is not for the update + * path's `json` columns.) */ export function stripRuntimeOwnedFields( objectSchema: { name?: string; fields?: Record } | undefined | null, data: Record | undefined | null, supplied: Readonly>, logger?: EvaluateRulesOptions['logger'], - options?: { preserveAudit?: boolean; strictReadonlyWrites?: boolean }, + options?: { + preserveAudit?: boolean; + strictReadonlyWrites?: boolean; + /** + * [#14259] The keys the `beforeInsert` hook chain ACTUALLY ASSIGNED on THIS + * ROW, recorded while the writes happened (`recordHookPayloadWrites`, armed + * per row at `engine.insert`'s hook-context construction and sealed + * immediately after that row's dispatch). The insert-side twin of the + * option #14088 gave {@link stripReadonlyFields}, and read by the same + * rules: OPTIONAL, and absent means "this call cannot say", never "no hook + * wrote anything" — a direct caller, and any row whose hook REPLACED the + * payload object, fall back to the `Object.is` test below exactly as before + * this option existed. + * + * ⚠️ Per ROW, never per call: one recording is armed for each row of a + * batch, so a hook that stamps row 3 cannot exempt row 4's caller-seeded + * record number. And a caller cannot put a key in here — see the + * forgery-boundary note on the recorder — so #5503 is untouched: a + * caller-seeded record number that no hook assigned is still dropped, still + * warns with the same text, and still reports through `onFieldsDropped` / + * `strictReadonlyWrites`. + */ + hookWrittenKeys?: ReadonlySet; + }, ): Record | undefined | null { const fields = objectSchema?.fields; if (!fields || !data) return data; const preserveAudit = options?.preserveAudit === true; const strict = options?.strictReadonlyWrites === true; + const hookWrittenKeys = options?.hookWrittenKeys; let result = data; for (const [name, def] of Object.entries(fields)) { if (!isRuntimeOwnedField(def)) continue; @@ -1328,12 +1416,24 @@ export function stripRuntimeOwnedFields( // any plain snapshot, so `in` would call a hook stamp caller-supplied and // strip it. if (!Object.prototype.hasOwnProperty.call(supplied, name)) continue; // hook/middleware stamp — keep + // [#14259] ...or a hook ASSIGNED it on THIS row. Asked BEFORE the value + // comparison because it answers by RECORD the question the comparison is + // only a proxy for: `Object.is` reads "the hook re-issued the record number + // the caller also submitted" as "the hook never touched the key", and the + // two demand opposite verdicts. That is #6339's own argument against the + // key SET, applied to the values that replaced it. + if (hookWrittenKeys?.has(name)) continue; // the hook wrote this value — keep // [#6339] ...and it must still BE the caller's value. A hook that overwrote // this key wrote a PLATFORM value, and deleting that is what sent records // to the database holding a sequence number the hook had just replaced. // `Object.is`, not `===`: `===` reports NaN !== NaN, which would read a // caller-forged NaN as "a hook rewrote it" and KEEP the forgery — the one // input where the loose operator inverts the verdict. + // + // [#14259] STAYS as the fallback for every key the record above has nothing + // to say about — a call site passing no `hookWrittenKeys`, and a row whose + // hook replaced the payload object. ⛔ Not "keep everything": the fallback + // over-strips, which is the only safe direction. if (!Object.is((result as Record)[name], supplied[name])) continue; if (preserveAudit && isPreservableUnderAudit(name, def)) continue; // historical import reinstates it if (result === data) result = { ...data }; From 1341a5c35814e6dcf0922895061fee2d4da368a8 Mon Sep 17 00:00:00 2001 From: "Claude Fable 5.1" Date: Wed, 2 Sep 2026 06:39:59 +0000 Subject: [PATCH 2/5] test(objectql): pin the two sibling seams on hook-write provenance Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68 --- ...gine-hook-provenance-sibling-seams.test.ts | 636 ++++++++++++++++++ 1 file changed, 636 insertions(+) create mode 100644 packages/objectql/src/engine-hook-provenance-sibling-seams.test.ts diff --git a/packages/objectql/src/engine-hook-provenance-sibling-seams.test.ts b/packages/objectql/src/engine-hook-provenance-sibling-seams.test.ts new file mode 100644 index 0000000000..5b0a9022f2 --- /dev/null +++ b/packages/objectql/src/engine-hook-provenance-sibling-seams.test.ts @@ -0,0 +1,636 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #14259 — the two SIBLING seams of the strip #14088 repaired must decide +// hook-vs-caller by the same RECORD, not by `Object.is`. +// +// #14088 replaced `Object.is(payload[k], supplied[k])` inside +// `stripReadonlyFields` with a recording of the keys the before-phase hook +// chain actually assigned (`recordHookPayloadWrites`). Its argument was never +// about `null`: value equality cannot separate +// +// - the hook deliberately wrote the value the caller also sent, from +// - the hook never touched the key at all, +// +// and the two demand opposite verdicts. Two functions in the same file were +// left on the comparison that argument retired, and this suite pins both: +// +// 1. `isCallerSuppliedValue` — the shared predicate behind +// `stripReadonlyWhenFields` and `stripReadonlyWhenFieldsMulti`, whose own +// docblock says it is written to be textually parallel with the test inside +// `stripReadonlyFields` so the two "can never disagree about what +// caller-supplied means". Between #14088 and this, they did. +// 2. `stripRuntimeOwnedFields` — the INSERT-side twin. #6339's own prose is +// the finding: it argued a key SET made the contract true "only BY +// ACCIDENT" and moved to values, which is accidental in the identical way. +// +// ⛔ WHAT THIS SUITE IS NOT, and is written to fail if anyone reads it that +// way: it is NOT a relaxation of #3042 / #4889 / #5503, and it is NOT a `null` +// case. The DISCRIMINATOR PAIRS are the deliverable's proof — the same caller +// payload, byte for byte, on the same locked key, reaching OPPOSITE verdicts +// depending on whether a hook assigned it. A caller-supplied value that no hook +// wrote is still stripped, still warns with the same text, and still reports +// through `onFieldsDropped` / `strictReadonlyWrites`. That is what value +// equality cannot deliver and a record can. +// +// ⛔ And the forgery boundary is inherited unchanged: A CALLER-SUPPLIED VALUE +// MUST NEVER BECOME HOOK-OWNED. The insert-side recording this card arms is new +// (the update path's already existed), so it owes the same three properties, +// and they are pinned below: armed after the caller's payload has arrived, +// sealed before any engine-owned pass touches it, and recording that an +// assignment ran rather than anything about the payload's contents. + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectQL } from './engine.js'; + +function makeDriver() { + const stores = new Map>(); + const storeFor = (o: string) => { + let s = stores.get(o); + if (!s) { s = new Map(); stores.set(o, s); } + return s; + }; + const matches = (row: any, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + return Object.entries(where).every(([k, v]: [string, any]) => { + if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); + return row?.[k] === v; + }); + }; + let n = 0; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find(object: string, ast: any) { + return Array.from(storeFor(object).values()).filter((r) => matches(r, ast?.where)); + }, + async findOne(object: string, ast: any) { + for (const r of storeFor(object).values()) if (matches(r, ast?.where)) return r; + return null; + }, + async create(object: string, data: Record) { + n += 1; + const id = (data.id as string) ?? `r_${n}`; + const row = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const row = { ...s.get(id), ...data, id }; + s.set(id, row); + return row; + }, + async updateMany(object: string, ast: any, data: Record) { + const s = storeFor(object); + let count = 0; + for (const row of [...s.values()]) { + if (!matches(row, ast?.where)) continue; + s.set(row.id, { ...row, ...data, id: row.id }); + count += 1; + } + return count; + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count() { return 0; }, + async bulkCreate(object: string, rows: Record[]) { + const out = []; + for (const r of rows) out.push(await this.create(object, r, undefined)); + return out; + }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, storeFor }; +} + +const makeLogger = (sink: string[]) => { + const logger: any = { + warn: (m: string) => sink.push(String(m)), + debug() {}, info() {}, error() {}, trace() {}, fatal() {}, + child() { return logger; }, + }; + return logger; +}; + +// ───────────────────────────────────────────────────────────────────────────── +// SEAM 1 — `readonlyWhen`, UPDATE path +// ───────────────────────────────────────────────────────────────────────────── + +/** What the recompute hook derives. A client mirroring the formula sends this. */ +const DERIVED = '2026-11-14'; +/** What sits on the stored row before the write — the value a lost hook write leaves behind. */ +const STALE = '2026-01-01'; +/** What a caller forges. Never allowed to reach the row on a locked field. */ +const FORGED = '1999-01-01'; +/** A fault instant already on the stored row — the `null` collision's stale residue. */ +const FAULT_AT = '2026-08-01T09:00:00.000Z'; + +describe('seam 1 — the readonlyWhen strips read PROVENANCE, not value equality (#14259)', () => { + let engine: ObjectQL; + let storeFor: ReturnType['storeFor']; + let warns: string[]; + + beforeEach(async () => { + warns = []; + engine = new ObjectQL({ logger: makeLogger(warns) }); + const d = makeDriver(); + storeFor = d.storeFor; + engine.registerDriver(d.driver, true); + await engine.init(); + + // The #9107 downstream object, trimmed to the fields this card turns on. + // Three ALWAYS-locked columns so the collision can be shown on a string, on + // `null` and on `0` — proof the repair is provenance and not a sentinel — + // plus one STATE lock for the #4889 direction. + engine.registry.registerObject({ + name: 'prov_equipment', + fields: { + name: { type: 'text' }, + status: { type: 'text' }, + period_days: { type: 'number' }, + next_maintenance_date: { type: 'date', readonlyWhen: 'true' }, + last_fault_at: { type: 'datetime', readonlyWhen: 'true' }, + overdue_days: { type: 'number', readonlyWhen: 'true' }, + closed_note: { type: 'text', readonlyWhen: "record.status == 'closed'" }, + }, + // `packageId` is REQUIRED and passed on purpose: omitting it is what the + // TEST_DEBT ledger counts in this package, and a new test file may not + // add to a shrink-only ratchet. + } as any, 'test'); + + // The recompute hook, exactly #9107's shape: derive the locked columns + // whenever the write touches the cycle. It CLEARS the fault instant and + // ZEROES the overdue counter in the same pass, which is where the `null` + // and `0` collisions live. + engine.registerHook('beforeUpdate', async (ctx: any) => { + if (!Object.prototype.hasOwnProperty.call(ctx.input.data, 'period_days')) return; + ctx.input.data.next_maintenance_date = DERIVED; + ctx.input.data.last_fault_at = null; + ctx.input.data.overdue_days = 0; + }, { object: 'prov_equipment', priority: 50 }); + }); + + const seed = (id: string, over: Record = {}) => + storeFor('prov_equipment').set(id, { + id, name: 'Autoclave', status: 'open', period_days: 90, + next_maintenance_date: STALE, last_fault_at: FAULT_AT, overdue_days: 7, + closed_note: null, ...over, + }); + const eq = (id: string) => storeFor('prov_equipment').get(id); + + // ── THE DEFECT ──────────────────────────────────────────────────────────── + + it('THE DEFECT: a hook write the caller ECHOED now LANDS on a TRUE readonlyWhen field', async () => { + // The card's reproduction shape. A thick client (or a retried submit) + // mirrors the server's formula and sends the same date the hook is about to + // derive. `Object.is(DERIVED, DERIVED)` is true, so the pre-#14259 predicate + // read the hook's deliberate write as "the hook never touched the key", + // deleted it, and committed the new cycle beside the OLD maintenance date — + // the duplicate-plans loop #9107 measured, reopened on one input. + seed('eq_1'); + + await engine.update('prov_equipment', { + id: 'eq_1', period_days: 120, next_maintenance_date: DERIVED, + }); + + expect(eq('eq_1').period_days).toBe(120); + // The regression, stated as the value it must NOT be. + expect(eq('eq_1').next_maintenance_date).not.toBe(STALE); + expect(eq('eq_1').next_maintenance_date).toBe(DERIVED); + // The write landed, so nothing was dropped and nothing may be logged. + expect(warns).toEqual([]); + }); + + it('the same collision on `null` — the `Object.is(null, null)` case the file names', async () => { + // The hook CLEARS the fault instant; the caller's form round-trip submits + // the disabled input as `null`. Identical bytes on the key, and the stored + // row keeps a stale fault timestamp beside a freshly serviced machine. + seed('eq_2'); + + await engine.update('prov_equipment', { + id: 'eq_2', period_days: 120, last_fault_at: null, + }); + + expect(eq('eq_2').last_fault_at).not.toBe(FAULT_AT); + expect(eq('eq_2').last_fault_at).toBeNull(); + }); + + it('the same collision on `0` — so the repair cannot be a `null` sentinel', async () => { + // `Object.is(0, 0)` is true for exactly the reason `Object.is(null, null)` + // is. A fix that reads `null` specially leaves this one corrupt. (A plain + // `0`, never `-0`: `Object.is` separates those two, and leaning on that + // would be the same accident a third time.) + seed('eq_3'); + + await engine.update('prov_equipment', { + id: 'eq_3', period_days: 120, overdue_days: 0, + }); + + expect(eq('eq_3').overdue_days).toBe(0); + }); + + // ── ⛔ THE NEGATIVE CONTROL — the deliverable's proof ────────────────────── + + it('⛔ NEGATIVE CONTROL: the IDENTICAL caller payload with NO hook write is still STRIPPED', async () => { + // The one case that separates "provenance" from "stopped locking". The key + // and the value are byte-identical to THE DEFECT above — + // `next_maintenance_date: DERIVED` — but the payload omits `period_days`, + // so the hook returns without assigning and nobody authorised the write. + // The stored value must survive and the caller must be told. + seed('eq_4'); + + await engine.update('prov_equipment', { + id: 'eq_4', name: 'Renamed', next_maintenance_date: DERIVED, + }); + + expect(eq('eq_4').name).toBe('Renamed'); + expect(eq('eq_4').next_maintenance_date).toBe(STALE); + // The same warning text as before this card — the strip's documented line. + expect(warns.some((w) => + w.includes("Field 'next_maintenance_date' is read-only (readonlyWhen) — ignoring incoming change"), + )).toBe(true); + }); + + it('⛔ NEGATIVE CONTROL on `null`: an unauthorised clear is still stripped', async () => { + seed('eq_5'); + await engine.update('prov_equipment', { id: 'eq_5', name: 'R', last_fault_at: null }); + expect(eq('eq_5').last_fault_at).toBe(FAULT_AT); + expect(warns.some((w) => w.includes("Field 'last_fault_at' is read-only (readonlyWhen)"))).toBe(true); + }); + + it('⛔ NEGATIVE CONTROL: the drop still reaches onFieldsDropped as readonly_when', async () => { + seed('eq_6'); + const events: any[] = []; + + await engine.update( + 'prov_equipment', + { id: 'eq_6', next_maintenance_date: DERIVED }, + { onFieldsDropped: (e: any) => events.push(e) } as any, + ); + + expect(events.some((e) => e.reason === 'readonly_when' && e.fields.includes('next_maintenance_date'))).toBe(true); + expect(eq('eq_6').next_maintenance_date).toBe(STALE); + }); + + it('⛔ NEGATIVE CONTROL: strictReadonlyWrites still REFUSES the unauthorised write', async () => { + seed('eq_7'); + let refused: any; + + await engine.update( + 'prov_equipment', + { id: 'eq_7', next_maintenance_date: DERIVED }, + { strictReadonlyWrites: true } as any, + ).catch((e: unknown) => { refused = e; }); + + // The refusal ENVELOPE, not merely "it threw": a bare `toThrow()` would + // stay green against any unrelated failure on this path. + // ⚠️ `ReadonlyFieldRejectedError` carries `code` and `name` only — it has no + // `status` member (the HTTP mapping lives at the protocol layer), so + // asserting one here would pin a property this class does not have. + expect(refused).toBeDefined(); + expect(refused.name).toBe('ReadonlyFieldRejectedError'); + expect(refused.code).toBe('ERR_READONLY_FIELD_REJECTED'); + expect(refused.fields).toContain('next_maintenance_date'); + // And nothing was written. + expect(eq('eq_7').next_maintenance_date).toBe(STALE); + }); + + it('⛔ a hook that runs but writes some OTHER key confers nothing on this one', async () => { + // Provenance is per KEY, never "a hook ran on this write". + engine.registerHook('beforeUpdate', async (ctx: any) => { + ctx.input.data.name = 'rewritten-by-hook'; + }, { object: 'prov_equipment', priority: 60 }); + seed('eq_8'); + + await engine.update('prov_equipment', { id: 'eq_8', next_maintenance_date: FORGED }); + + expect(eq('eq_8').name).toBe('rewritten-by-hook'); + expect(eq('eq_8').next_maintenance_date).toBe(STALE); + }); + + // ── The #4889 lock: provenance leaves CALLER writes exactly where they were ─ + + it('#4889 UNCHANGED: a caller value on a TRUE STATE predicate is still stripped', async () => { + // The frozen-record class the triage ruling names. No hook assigns + // `closed_note`, so no record exempts it, and the state lock holds. + seed('eq_9', { status: 'closed' }); + + await engine.update('prov_equipment', { id: 'eq_9', closed_note: 'caller note' }); + + expect(eq('eq_9').closed_note).toBeNull(); + expect(warns.some((w) => w.includes("Field 'closed_note' is read-only (readonlyWhen)"))).toBe(true); + }); + + it('#4889 UNCHANGED: isSystem still does NOT exempt a caller value (Option B stays rejected)', async () => { + seed('eq_10', { status: 'closed' }); + await engine.update( + 'prov_equipment', { id: 'eq_10', closed_note: 'caller note' }, + { context: { isSystem: true } } as any, + ); + expect(eq('eq_10').closed_note).toBeNull(); + }); + + it("what persists is always the HOOK's value, never the caller's", async () => { + // The forgery boundary read as an outcome: a caller cannot launder its own + // value through the hook phase, because the hook's assignment is what stands + // on the key afterwards. + seed('eq_11'); + + await engine.update('prov_equipment', { + id: 'eq_11', period_days: 120, next_maintenance_date: FORGED, + }); + + expect(eq('eq_11').next_maintenance_date).not.toBe(FORGED); + expect(eq('eq_11').next_maintenance_date).toBe(DERIVED); + }); + + it('MEASURED-UNCHANGED: a hook writing a DIFFERENT value than the caller behaves as before', async () => { + // Pinned because the card asks for today's behaviour on this input to be + // measured before it moves — and it must NOT move. Pre-#14259 the value + // test already answered "not the caller's" here (`Object.is(DERIVED, + // FORGED)` is false) and kept the hook's write; under provenance the record + // answers the same way for a different reason. Same verdict, both routes, + // which is what makes the record a strictly narrower gate than the + // comparison it fronts. + seed('eq_12'); + await engine.update('prov_equipment', { + id: 'eq_12', period_days: 120, next_maintenance_date: FORGED, + }); + expect(eq('eq_12').next_maintenance_date).toBe(DERIVED); + expect(warns).toEqual([]); + }); + + // ── The BULK branch reaches the same verdict off the SAME sealed record ──── + + it('MULTI: the echoed hook write lands on the predicate branch too', async () => { + seed('eq_20', { status: 'active' }); + seed('eq_21', { status: 'active' }); + + await engine.update( + 'prov_equipment', + { period_days: 120, next_maintenance_date: DERIVED }, + { where: { status: 'active' }, multi: true } as any, + ); + + expect(eq('eq_20').next_maintenance_date).toBe(DERIVED); + expect(eq('eq_21').next_maintenance_date).toBe(DERIVED); + }); + + it('⛔ MULTI NEGATIVE CONTROL: the identical payload with no hook write is still stripped', async () => { + // #3106 / #4441 "both call sites": a bulk write must not reach a different + // verdict about who wrote a key than a by-id write does — in EITHER + // direction. This is the by-id negative control, run through + // `stripReadonlyWhenFieldsMulti`. + seed('eq_22', { status: 'active' }); + + await engine.update( + 'prov_equipment', + { name: 'Renamed', next_maintenance_date: DERIVED }, + { where: { status: 'active' }, multi: true } as any, + ); + + expect(eq('eq_22').name).toBe('Renamed'); + expect(eq('eq_22').next_maintenance_date).toBe(STALE); + expect(warns.some((w) => w.includes("Field 'next_maintenance_date' is read-only (readonlyWhen) in ≥1 matched row"))).toBe(true); + }); + + it('a hook that REPLACES ctx.input.data falls back to the value test, not to "keep everything"', async () => { + // The recorder's KNOWN LIMIT, inherited: a replacement's keys are mostly + // the CALLER's, so reading them as hook-owned would launder a forgery. With + // no attributable record the strip must behave exactly as it did before + // this card — i.e. over-strip. Fail-safe means "keep the old bug". + engine.registerHook('beforeUpdate', async (ctx: any) => { + ctx.input.data = { ...ctx.input.data, next_maintenance_date: DERIVED }; + }, { object: 'prov_equipment', priority: 60 }); + seed('eq_23'); + + await engine.update('prov_equipment', { + id: 'eq_23', name: 'R', next_maintenance_date: DERIVED, + }); + + expect(eq('eq_23').name).toBe('R'); + expect(eq('eq_23').next_maintenance_date).toBe(STALE); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// SEAM 2 — runtime-owned (`autonumber`), INSERT path +// ───────────────────────────────────────────────────────────────────────────── + +describe('seam 2 — the insert-side runtime-owned strip reads PROVENANCE (#14259)', () => { + let engine: ObjectQL; + let storeFor: ReturnType['storeFor']; + let warns: string[]; + let hookSaw: Array>; + + beforeEach(async () => { + warns = []; + hookSaw = []; + engine = new ObjectQL({ logger: makeLogger(warns) }); + const d = makeDriver(); + storeFor = d.storeFor; + engine.registerDriver(d.driver, true); + await engine.init(); + + engine.registry.registerObject({ + name: 'prov_ticket', + fields: { title: { type: 'text' }, code: { type: 'autonumber' } }, + } as any, 'test'); + + // #6339's own hook, unchanged: a `beforeInsert` that OWNS the record number + // — it re-issues or normalises it rather than letting the sequence decide. + // `title: 'no-hook'` short-circuits it, which is how a test tells "the hook + // did not fire" from "the hook fired and lost". + engine.registerHook('beforeInsert', async (ctx: any) => { + hookSaw.push({ ...(ctx.input.data as Record) }); + if (ctx.input.data.title === 'no-hook') return; + ctx.input.data.code = `HOOK-${String(ctx.input.data.title)}`; + }, { object: 'prov_ticket', priority: 50 }); + }); + + // ── THE DEFECT ──────────────────────────────────────────────────────────── + + it('THE DEFECT: a hook write the caller ECHOED now LANDS on a runtime-owned field', async () => { + // The whole-record POST: read a record, edit a field, submit everything + // back — so the payload necessarily echoes the record-number column it just + // read, and the hook re-issues the SAME number (an idempotent normalise, or + // a retried submit). `Object.is('HOOK-B', 'HOOK-B')` is true, so the + // pre-#14259 strip deleted the hook's deliberate write and the sequence + // value went to the database instead. + const row: any = await engine.insert('prov_ticket', { title: 'B', code: 'HOOK-B' }); + + // The regression, stated as the value it must NOT be. The sequence renders + // through the contract default `{0000}` since #6555 / #7262. + expect(row.code).not.toBe('0001'); + expect(row.code).toBe('HOOK-B'); + expect(warns).toEqual([]); + }); + + it('the echoing caller and the omitting caller now AGREE — the difference was the accident', async () => { + // #6339's own proof shape, re-run on the input its fix could not see. The + // two calls differ in nothing but whether the caller's payload happened to + // carry the value the hook was going to write. + const omitted: any = await engine.insert('prov_ticket', { title: 'X' }); + const echoed: any = await engine.insert('prov_ticket', { title: 'X', code: 'HOOK-X' }); + expect(omitted.code).toBe(echoed.code); + expect(echoed.code).toBe('HOOK-X'); + }); + + // ── ⛔ THE NEGATIVE CONTROL — the deliverable's proof ────────────────────── + + it('⛔ NEGATIVE CONTROL: the IDENTICAL caller payload with NO hook write is still STRIPPED', async () => { + // `title: 'no-hook'` makes the hook return without assigning, so the value + // standing on `code` is the caller's seed and #5503 takes it. Byte-identical + // key and value shape to THE DEFECT above; opposite verdict. + const row: any = await engine.insert('prov_ticket', { title: 'no-hook', code: 'HOOK-no-hook' }); + + expect(row.code).not.toBe('HOOK-no-hook'); + expect(row.code).toBe('0001'); + // The same warning text as before this card — contract of the text, not its + // wording, per #5503's own pin discipline. + expect(warns).toHaveLength(1); + expect(warns[0]).toContain("Field 'code' on 'prov_ticket'"); + expect(warns[0]).toContain('runtime-owned'); + expect(warns[0]).toContain('COMMITTED WITHOUT IT'); + expect(warns[0]).toContain('hook-written keys are not caller-supplied'); + }); + + it('⛔ NEGATIVE CONTROL: the drop still reaches onFieldsDropped as readonly', async () => { + const events: any[] = []; + await engine.insert( + 'prov_ticket', + { title: 'no-hook', code: 'CALLER-SEEDED' }, + { onFieldsDropped: (e: any) => events.push(e) } as any, + ); + expect(events.some((e) => e.reason === 'readonly' && e.fields.includes('code'))).toBe(true); + }); + + it('⛔ NEGATIVE CONTROL: strictReadonlyWrites still REFUSES the unauthorised seed', async () => { + let refused: any; + await engine.insert( + 'prov_ticket', + { title: 'no-hook', code: 'CALLER-SEEDED' }, + { strictReadonlyWrites: true } as any, + ).catch((e: unknown) => { refused = e; }); + + // The refusal ENVELOPE, not merely "it threw" (see the seam 1 twin for why + // `status` is not asserted: this class carries `code` and `name` only). + expect(refused).toBeDefined(); + expect(refused.name).toBe('ReadonlyFieldRejectedError'); + expect(refused.code).toBe('ERR_READONLY_FIELD_REJECTED'); + expect(refused.fields).toContain('code'); + expect(refused.operation).toBe('insert'); + // Refused BEFORE any driver dispatch. + expect(storeFor('prov_ticket').size).toBe(0); + }); + + it('⛔ a hook that runs but writes some OTHER key confers nothing on `code`', async () => { + engine.registerHook('beforeInsert', async (ctx: any) => { + ctx.input.data.title = `${String(ctx.input.data.title)}!`; + }, { object: 'prov_ticket', priority: 60 }); + + const row: any = await engine.insert('prov_ticket', { title: 'no-hook', code: 'CALLER-SEEDED' }); + + expect(row.title).toBe('no-hook!'); + expect(row.code).toBe('0001'); + }); + + // ── PER ROW, never per call ─────────────────────────────────────────────── + + it('a BATCH records per ROW: a hook stamping row 0 does not exempt row 1', async () => { + // The property a single shared recording would break, and the reason the + // insert path arms one recorder per row rather than one per call. + const rows: any = await engine.insert('prov_ticket', [ + { title: 'A', code: 'HOOK-A' }, + { title: 'no-hook', code: 'HOOK-A' }, + ] as any); + + expect(rows[0].code).toBe('HOOK-A'); + // Row 1's hook returned without assigning, so its caller seed is stripped + // and the sequence issues the number — even though the value is the very + // one row 0's hook legitimately wrote. + expect(rows[1].code).not.toBe('HOOK-A'); + // `0001`, not `0002`: `applyAutonumbers` fills only an EMPTY slot, and row + // 0 kept its hook-written code, so row 1 is the batch's first draw. Pinned + // at the measured value rather than at the row index — reading the counter + // as "one per row" would be a guess this very fix makes wrong. + expect(rows[1].code).toBe('0001'); + expect(warns).toHaveLength(1); + expect(warns[0]).toContain("Field 'code' on 'prov_ticket'"); + }); + + // ── The forgery boundary the NEW recording owes (card, verbatim) ─────────── + + it('FORGERY BOUNDARY: the recording is armed AFTER the caller payload arrives', async () => { + // "armed after the caller's payload has arrived": the hook sees the caller's + // own keys and values, and the caller's object is not the recorded one — so + // nothing the caller sent is in the record and the strip still judges + // against what the caller actually sent (`suppliedPerRow`, an explicit + // shallow copy taken ahead of the hooks, #6339's P3 invariant). + const payload: Record = { title: 'no-hook', code: 'CALLER-SEEDED' }; + const row: any = await engine.insert('prov_ticket', payload); + + expect(hookSaw[0]).toEqual({ title: 'no-hook', code: 'CALLER-SEEDED' }); + expect(hookSaw[0]).not.toBe(payload); + expect(payload).toEqual({ title: 'no-hook', code: 'CALLER-SEEDED' }); + // The caller's echo did not become hook-owned. + expect(row.code).toBe('0001'); + }); + + it('FORGERY BOUNDARY: the recording is SEALED before any engine-owned pass', async () => { + // "sealed before any engine-owned pass touches it". `applyAutonumbers` is + // the engine-owned writer of exactly this column and it runs AFTER the + // strip; if the recorder were still armed for it, the sequence value would + // register as a hook write. Read as an outcome: on the `no-hook` row the + // engine's own autonumber write does NOT exempt the caller's seed, so the + // strip still fires and still warns. + const row: any = await engine.insert('prov_ticket', { title: 'no-hook', code: 'CALLER-SEEDED' }); + expect(row.code).toBe('0001'); + expect(warns).toHaveLength(1); + }); + + it('FORGERY BOUNDARY: the record says an assignment RAN, not what the value was', async () => { + // "it records that an assignment ran rather than anything about the + // payload's contents". A hook that assigns the key the caller's OWN value, + // deliberately, is a hook write — that is the whole ruling — and a hook that + // assigns a different one is equally a hook write. Same verdict, two + // values: the record is blind to contents. + const same: any = await engine.insert('prov_ticket', { title: 'S', code: 'HOOK-S' }); + const diff: any = await engine.insert('prov_ticket', { title: 'D', code: 'CALLER-SEEDED' }); + expect(same.code).toBe('HOOK-S'); + expect(diff.code).toBe('HOOK-D'); + expect(warns).toEqual([]); + }); + + it('a hook that REPLACES ctx.input.data falls back to the value test, not to "keep everything"', async () => { + // The recorder's KNOWN LIMIT on the insert path. The replacement carries the + // caller's own `code`, and reading a replacement's keys as hook-owned would + // launder a caller-seeded record number into a platform write — so the + // fallback deliberately keeps the pre-#14259 over-strip. + engine.registerHook('beforeInsert', async (ctx: any) => { + if (ctx.input.data.title !== 'replace') return; + ctx.input.data = { ...ctx.input.data }; + }, { object: 'prov_ticket', priority: 60 }); + + const row: any = await engine.insert('prov_ticket', { title: 'replace', code: 'HOOK-replace' }); + + // The priority-50 hook assigned `HOOK-replace` on the RECORDING view; the + // priority-60 hook then replaced the object, so there is no attributable + // record and the value test judges the result. The caller sent the same + // value, so it is stripped — the old bug, kept on purpose. + expect(row.code).not.toBe('HOOK-replace'); + expect(row.code).toBe('0001'); + }); + + // ── The exemptions above this seam are untouched ────────────────────────── + + it('#5503 UNCHANGED: an isSystem caller still bypasses the whole pass', async () => { + const row: any = await engine.insert( + 'prov_ticket', { title: 'no-hook', code: 'SEEDED' }, + { context: { isSystem: true } } as any, + ); + expect(row.code).toBe('SEEDED'); + expect(warns).toEqual([]); + }); +}); From 43d8db273b0d09fc987dc75f2b933973850e79cd Mon Sep 17 00:00:00 2001 From: "Claude Fable 5.1" Date: Wed, 2 Sep 2026 06:42:47 +0000 Subject: [PATCH 3/5] chore(changeset): hook-write provenance for the two sibling strips Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68 --- .changeset/hook-provenance-sibling-seams.md | 47 +++++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 .changeset/hook-provenance-sibling-seams.md diff --git a/.changeset/hook-provenance-sibling-seams.md b/.changeset/hook-provenance-sibling-seams.md new file mode 100644 index 0000000000..88abc6648a --- /dev/null +++ b/.changeset/hook-provenance-sibling-seams.md @@ -0,0 +1,47 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): decide the `readonlyWhen` and insert-side runtime-owned strips by hook-write PROVENANCE, not `Object.is` (#14259) + +#14088 replaced `Object.is(payload[k], supplied[k])` inside `stripReadonlyFields` +with a recording of the keys the before-phase hook chain actually assigned +(`recordHookPayloadWrites`). Its argument was never about `null`: value equality +cannot separate *the hook deliberately wrote the value the caller also sent* from +*the hook never touched the key*, and those two demand opposite verdicts. Two +sibling seams in the same file were left on the comparison that argument retired, +and both are repaired here off the same instrument: + +- **`readonlyWhen`, update path.** `isCallerSuppliedValue` — the shared predicate + behind `stripReadonlyWhenFields` and `stripReadonlyWhenFieldsMulti` — now + consults the sealed record before the value test. A `beforeUpdate` hook that + derives a `readonlyWhen`-locked field lost its write to any caller that echoed + the same value back: the #9107 defect, surviving on the one input #9107's fix + cannot see. The engine threads the record already sealed at its post-hook + confluence into both call sites, so the by-id and bulk branches read one fact. +- **Runtime-owned fields, insert path.** `stripRuntimeOwnedFields` gains the same + optional `hookWrittenKeys`, and `engine.insert` now arms one recording **per + row** at hook-context construction and seals each immediately after that row's + `beforeInsert` chain. A `beforeInsert` hook that re-issues or normalises a + record number lost its write to any caller that submitted the same value — + #6339's own argument against the key SET, applied to the values that replaced + it. + +⛔ **Not a relaxation, and the accept set for callers does not move.** A +caller-supplied value that no hook assigned is still stripped, still warns with +the same text, and still reports through `onFieldsDropped` / +`strictReadonlyWrites`. `isSystem` remains NOT an exemption on the `readonlyWhen` +seam, so #4889's frozen paid-invoice lock is untouched. What changed is only the +EVIDENCE for the hook-write exemption that already existed — a record of which +keys were assigned, instead of an inference from the values afterwards. + +The forgery boundary is inherited verbatim: a caller-supplied value must never +become hook-owned. The new insert-side recording is armed after the caller's +payload has arrived and been snapshotted, sealed before any engine-owned pass +touches the row, and records that an assignment ran rather than anything about +the payload's contents — a caller cannot execute an assignment, so no key it +sends can enter the record. Per row, never per call, so a hook stamping one row +of a batch confers nothing on the next. A hook that REPLACES the payload object +leaves no attributable record and falls back to the pre-existing value test, +which over-strips: keeping the old bug is the only safe direction, because +reading a replacement's keys as hook-owned would launder a caller's forgery. From bea6331a223075e3f2b4f71505cb382cc0ee8d8f Mon Sep 17 00:00:00 2001 From: "Claude Fable 5.1" Date: Wed, 2 Sep 2026 07:00:57 +0000 Subject: [PATCH 4/5] fix(objectql): decide the insert-side runtime-owned strip by hook-write provenance Withholds the readonlyWhen sibling seam: threading the record into isCallerSuppliedValue was measured to let a caller value survive a TRUE readonlyWhen predicate (#9107 pin LOCK 3b), which #14259's fork clause routes to the decision inbox rather than to a unilateral choice. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68 --- .changeset/hook-provenance-sibling-seams.md | 63 +-- ...gine-hook-provenance-sibling-seams.test.ts | 382 +++--------------- packages/objectql/src/engine.ts | 18 +- .../objectql/src/validation/rule-validator.ts | 74 +--- 4 files changed, 109 insertions(+), 428 deletions(-) diff --git a/.changeset/hook-provenance-sibling-seams.md b/.changeset/hook-provenance-sibling-seams.md index 88abc6648a..962e825a17 100644 --- a/.changeset/hook-provenance-sibling-seams.md +++ b/.changeset/hook-provenance-sibling-seams.md @@ -2,46 +2,49 @@ "@objectstack/objectql": patch --- -fix(objectql): decide the `readonlyWhen` and insert-side runtime-owned strips by hook-write PROVENANCE, not `Object.is` (#14259) +fix(objectql): decide the insert-side runtime-owned strip by hook-write PROVENANCE, not `Object.is` (#14259) #14088 replaced `Object.is(payload[k], supplied[k])` inside `stripReadonlyFields` with a recording of the keys the before-phase hook chain actually assigned (`recordHookPayloadWrites`). Its argument was never about `null`: value equality cannot separate *the hook deliberately wrote the value the caller also sent* from -*the hook never touched the key*, and those two demand opposite verdicts. Two -sibling seams in the same file were left on the comparison that argument retired, -and both are repaired here off the same instrument: +*the hook never touched the key*, and those two demand opposite verdicts. -- **`readonlyWhen`, update path.** `isCallerSuppliedValue` — the shared predicate - behind `stripReadonlyWhenFields` and `stripReadonlyWhenFieldsMulti` — now - consults the sealed record before the value test. A `beforeUpdate` hook that - derives a `readonlyWhen`-locked field lost its write to any caller that echoed - the same value back: the #9107 defect, surviving on the one input #9107's fix - cannot see. The engine threads the record already sealed at its post-hook - confluence into both call sites, so the by-id and bulk branches read one fact. -- **Runtime-owned fields, insert path.** `stripRuntimeOwnedFields` gains the same - optional `hookWrittenKeys`, and `engine.insert` now arms one recording **per - row** at hook-context construction and seals each immediately after that row's - `beforeInsert` chain. A `beforeInsert` hook that re-issues or normalises a - record number lost its write to any caller that submitted the same value — - #6339's own argument against the key SET, applied to the values that replaced - it. +`stripRuntimeOwnedFields` — the INSERT-side twin — was left on the comparison +that argument retired, and #6339's own prose is the finding: it argued a key SET +made the contract true "only BY ACCIDENT" and moved to values, which is +accidental in the identical way. A `beforeInsert` hook that re-issues or +normalises a record number therefore still lost its write to any caller that +submitted the same value — the caller who omitted the key kept the hook's number, +the caller who echoed it got the sequence value, and the two differed in nothing +else. -⛔ **Not a relaxation, and the accept set for callers does not move.** A -caller-supplied value that no hook assigned is still stripped, still warns with -the same text, and still reports through `onFieldsDropped` / -`strictReadonlyWrites`. `isSystem` remains NOT an exemption on the `readonlyWhen` -seam, so #4889's frozen paid-invoice lock is untouched. What changed is only the -EVIDENCE for the hook-write exemption that already existed — a record of which -keys were assigned, instead of an inference from the values afterwards. +`engine.insert` now arms one recording **per row** at hook-context construction +and seals each immediately after that row's `beforeInsert` chain, and +`stripRuntimeOwnedFields` consults the sealed record before the value test. Per +row, never per call, so a hook stamping one row of a batch confers nothing on the +next. + +⛔ **Not a relaxation of #5503, and the accept set for callers does not move.** A +caller-seeded record number that no hook assigned is still stripped, still warns +with the same text, and still reports through `onFieldsDropped` / +`strictReadonlyWrites`; `isSystem` and `preserveAudit` are untouched. What +changed is only the EVIDENCE for the hook-write exemption that already existed — +a record of which keys were assigned, instead of an inference from the values +afterwards. The forgery boundary is inherited verbatim: a caller-supplied value must never become hook-owned. The new insert-side recording is armed after the caller's payload has arrived and been snapshotted, sealed before any engine-owned pass touches the row, and records that an assignment ran rather than anything about the payload's contents — a caller cannot execute an assignment, so no key it -sends can enter the record. Per row, never per call, so a hook stamping one row -of a batch confers nothing on the next. A hook that REPLACES the payload object -leaves no attributable record and falls back to the pre-existing value test, -which over-strips: keeping the old bug is the only safe direction, because -reading a replacement's keys as hook-owned would launder a caller's forgery. +sends can enter the record. A hook that REPLACES the payload object leaves no +attributable record and falls back to the pre-existing value test, which +over-strips: keeping the old bug is the only safe direction, because reading a +replacement's keys as hook-owned would launder a caller's forgery. + +The `readonlyWhen` sibling seam #14259 also names (`isCallerSuppliedValue`, +behind `stripReadonlyWhenFields` / `stripReadonlyWhenFieldsMulti`) is **not** +included: threading the record there was measured to let a caller's value survive +a TRUE `readonlyWhen` predicate, which is a maintainer decision rather than a +mechanical follow-through. Nothing about that seam's behaviour changes here. diff --git a/packages/objectql/src/engine-hook-provenance-sibling-seams.test.ts b/packages/objectql/src/engine-hook-provenance-sibling-seams.test.ts index 5b0a9022f2..00bb10276a 100644 --- a/packages/objectql/src/engine-hook-provenance-sibling-seams.test.ts +++ b/packages/objectql/src/engine-hook-provenance-sibling-seams.test.ts @@ -1,7 +1,7 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. // -// #14259 — the two SIBLING seams of the strip #14088 repaired must decide -// hook-vs-caller by the same RECORD, not by `Object.is`. +// #14259 — the INSERT-side runtime-owned strip must decide hook-vs-caller by +// the same RECORD `stripReadonlyFields` uses, not by `Object.is`. // // #14088 replaced `Object.is(payload[k], supplied[k])` inside // `stripReadonlyFields` with a recording of the keys the before-phase hook @@ -11,26 +11,32 @@ // - the hook deliberately wrote the value the caller also sent, from // - the hook never touched the key at all, // -// and the two demand opposite verdicts. Two functions in the same file were -// left on the comparison that argument retired, and this suite pins both: +// and the two demand opposite verdicts. `stripRuntimeOwnedFields` — the +// INSERT-side twin — was left on the comparison that argument retired, and +// #6339's own prose is the finding: it argued a key SET made the contract true +// "only BY ACCIDENT" and moved to VALUES, which is accidental in the identical +// way. So a `beforeInsert` hook that re-issues or normalises a record number +// still loses its write to the one caller that submitted the same value. // -// 1. `isCallerSuppliedValue` — the shared predicate behind -// `stripReadonlyWhenFields` and `stripReadonlyWhenFieldsMulti`, whose own -// docblock says it is written to be textually parallel with the test inside -// `stripReadonlyFields` so the two "can never disagree about what -// caller-supplied means". Between #14088 and this, they did. -// 2. `stripRuntimeOwnedFields` — the INSERT-side twin. #6339's own prose is -// the finding: it argued a key SET made the contract true "only BY -// ACCIDENT" and moved to values, which is accidental in the identical way. +// ⛔ THE SIBLING SEAM IS DELIBERATELY NOT HERE. #14259 named a second one — +// `isCallerSuppliedValue`, behind the two `readonlyWhen` strips — and it is +// WITHHELD pending a maintainer ruling, not forgotten. Measured on this branch: +// threading the record into that predicate turns the existing #9107 pin +// `LOCK 3b` red, because a hook spelled `ctx.input.data.x = ctx.input.data.x` +// is a `set` on the recorded object, so the CALLER's forged value becomes +// hook-owned and survives a TRUE `readonlyWhen` predicate (measured: +// `closed_note` committed `'1999-01-01'` where the lock had stripped it to +// `null`). That is the card's own fork clause — a caller value surviving a TRUE +// predicate — and it goes to the decision inbox, never resolved here. // // ⛔ WHAT THIS SUITE IS NOT, and is written to fail if anyone reads it that -// way: it is NOT a relaxation of #3042 / #4889 / #5503, and it is NOT a `null` -// case. The DISCRIMINATOR PAIRS are the deliverable's proof — the same caller -// payload, byte for byte, on the same locked key, reaching OPPOSITE verdicts -// depending on whether a hook assigned it. A caller-supplied value that no hook -// wrote is still stripped, still warns with the same text, and still reports -// through `onFieldsDropped` / `strictReadonlyWrites`. That is what value -// equality cannot deliver and a record can. +// way: it is NOT a relaxation of #5503. The DISCRIMINATOR PAIRS are the +// deliverable's proof — the same caller payload, byte for byte, on the same +// runtime-owned key, reaching OPPOSITE verdicts depending on whether a hook +// assigned it. A caller-seeded record number that no hook wrote is still +// stripped, still warns with the same text, and still reports through +// `onFieldsDropped` / `strictReadonlyWrites`. That is what value equality +// cannot deliver and a record can. // // ⛔ And the forgery boundary is inherited unchanged: A CALLER-SUPPLIED VALUE // MUST NEVER BECOME HOOK-OWNED. The insert-side recording this card arms is new @@ -113,307 +119,6 @@ const makeLogger = (sink: string[]) => { return logger; }; -// ───────────────────────────────────────────────────────────────────────────── -// SEAM 1 — `readonlyWhen`, UPDATE path -// ───────────────────────────────────────────────────────────────────────────── - -/** What the recompute hook derives. A client mirroring the formula sends this. */ -const DERIVED = '2026-11-14'; -/** What sits on the stored row before the write — the value a lost hook write leaves behind. */ -const STALE = '2026-01-01'; -/** What a caller forges. Never allowed to reach the row on a locked field. */ -const FORGED = '1999-01-01'; -/** A fault instant already on the stored row — the `null` collision's stale residue. */ -const FAULT_AT = '2026-08-01T09:00:00.000Z'; - -describe('seam 1 — the readonlyWhen strips read PROVENANCE, not value equality (#14259)', () => { - let engine: ObjectQL; - let storeFor: ReturnType['storeFor']; - let warns: string[]; - - beforeEach(async () => { - warns = []; - engine = new ObjectQL({ logger: makeLogger(warns) }); - const d = makeDriver(); - storeFor = d.storeFor; - engine.registerDriver(d.driver, true); - await engine.init(); - - // The #9107 downstream object, trimmed to the fields this card turns on. - // Three ALWAYS-locked columns so the collision can be shown on a string, on - // `null` and on `0` — proof the repair is provenance and not a sentinel — - // plus one STATE lock for the #4889 direction. - engine.registry.registerObject({ - name: 'prov_equipment', - fields: { - name: { type: 'text' }, - status: { type: 'text' }, - period_days: { type: 'number' }, - next_maintenance_date: { type: 'date', readonlyWhen: 'true' }, - last_fault_at: { type: 'datetime', readonlyWhen: 'true' }, - overdue_days: { type: 'number', readonlyWhen: 'true' }, - closed_note: { type: 'text', readonlyWhen: "record.status == 'closed'" }, - }, - // `packageId` is REQUIRED and passed on purpose: omitting it is what the - // TEST_DEBT ledger counts in this package, and a new test file may not - // add to a shrink-only ratchet. - } as any, 'test'); - - // The recompute hook, exactly #9107's shape: derive the locked columns - // whenever the write touches the cycle. It CLEARS the fault instant and - // ZEROES the overdue counter in the same pass, which is where the `null` - // and `0` collisions live. - engine.registerHook('beforeUpdate', async (ctx: any) => { - if (!Object.prototype.hasOwnProperty.call(ctx.input.data, 'period_days')) return; - ctx.input.data.next_maintenance_date = DERIVED; - ctx.input.data.last_fault_at = null; - ctx.input.data.overdue_days = 0; - }, { object: 'prov_equipment', priority: 50 }); - }); - - const seed = (id: string, over: Record = {}) => - storeFor('prov_equipment').set(id, { - id, name: 'Autoclave', status: 'open', period_days: 90, - next_maintenance_date: STALE, last_fault_at: FAULT_AT, overdue_days: 7, - closed_note: null, ...over, - }); - const eq = (id: string) => storeFor('prov_equipment').get(id); - - // ── THE DEFECT ──────────────────────────────────────────────────────────── - - it('THE DEFECT: a hook write the caller ECHOED now LANDS on a TRUE readonlyWhen field', async () => { - // The card's reproduction shape. A thick client (or a retried submit) - // mirrors the server's formula and sends the same date the hook is about to - // derive. `Object.is(DERIVED, DERIVED)` is true, so the pre-#14259 predicate - // read the hook's deliberate write as "the hook never touched the key", - // deleted it, and committed the new cycle beside the OLD maintenance date — - // the duplicate-plans loop #9107 measured, reopened on one input. - seed('eq_1'); - - await engine.update('prov_equipment', { - id: 'eq_1', period_days: 120, next_maintenance_date: DERIVED, - }); - - expect(eq('eq_1').period_days).toBe(120); - // The regression, stated as the value it must NOT be. - expect(eq('eq_1').next_maintenance_date).not.toBe(STALE); - expect(eq('eq_1').next_maintenance_date).toBe(DERIVED); - // The write landed, so nothing was dropped and nothing may be logged. - expect(warns).toEqual([]); - }); - - it('the same collision on `null` — the `Object.is(null, null)` case the file names', async () => { - // The hook CLEARS the fault instant; the caller's form round-trip submits - // the disabled input as `null`. Identical bytes on the key, and the stored - // row keeps a stale fault timestamp beside a freshly serviced machine. - seed('eq_2'); - - await engine.update('prov_equipment', { - id: 'eq_2', period_days: 120, last_fault_at: null, - }); - - expect(eq('eq_2').last_fault_at).not.toBe(FAULT_AT); - expect(eq('eq_2').last_fault_at).toBeNull(); - }); - - it('the same collision on `0` — so the repair cannot be a `null` sentinel', async () => { - // `Object.is(0, 0)` is true for exactly the reason `Object.is(null, null)` - // is. A fix that reads `null` specially leaves this one corrupt. (A plain - // `0`, never `-0`: `Object.is` separates those two, and leaning on that - // would be the same accident a third time.) - seed('eq_3'); - - await engine.update('prov_equipment', { - id: 'eq_3', period_days: 120, overdue_days: 0, - }); - - expect(eq('eq_3').overdue_days).toBe(0); - }); - - // ── ⛔ THE NEGATIVE CONTROL — the deliverable's proof ────────────────────── - - it('⛔ NEGATIVE CONTROL: the IDENTICAL caller payload with NO hook write is still STRIPPED', async () => { - // The one case that separates "provenance" from "stopped locking". The key - // and the value are byte-identical to THE DEFECT above — - // `next_maintenance_date: DERIVED` — but the payload omits `period_days`, - // so the hook returns without assigning and nobody authorised the write. - // The stored value must survive and the caller must be told. - seed('eq_4'); - - await engine.update('prov_equipment', { - id: 'eq_4', name: 'Renamed', next_maintenance_date: DERIVED, - }); - - expect(eq('eq_4').name).toBe('Renamed'); - expect(eq('eq_4').next_maintenance_date).toBe(STALE); - // The same warning text as before this card — the strip's documented line. - expect(warns.some((w) => - w.includes("Field 'next_maintenance_date' is read-only (readonlyWhen) — ignoring incoming change"), - )).toBe(true); - }); - - it('⛔ NEGATIVE CONTROL on `null`: an unauthorised clear is still stripped', async () => { - seed('eq_5'); - await engine.update('prov_equipment', { id: 'eq_5', name: 'R', last_fault_at: null }); - expect(eq('eq_5').last_fault_at).toBe(FAULT_AT); - expect(warns.some((w) => w.includes("Field 'last_fault_at' is read-only (readonlyWhen)"))).toBe(true); - }); - - it('⛔ NEGATIVE CONTROL: the drop still reaches onFieldsDropped as readonly_when', async () => { - seed('eq_6'); - const events: any[] = []; - - await engine.update( - 'prov_equipment', - { id: 'eq_6', next_maintenance_date: DERIVED }, - { onFieldsDropped: (e: any) => events.push(e) } as any, - ); - - expect(events.some((e) => e.reason === 'readonly_when' && e.fields.includes('next_maintenance_date'))).toBe(true); - expect(eq('eq_6').next_maintenance_date).toBe(STALE); - }); - - it('⛔ NEGATIVE CONTROL: strictReadonlyWrites still REFUSES the unauthorised write', async () => { - seed('eq_7'); - let refused: any; - - await engine.update( - 'prov_equipment', - { id: 'eq_7', next_maintenance_date: DERIVED }, - { strictReadonlyWrites: true } as any, - ).catch((e: unknown) => { refused = e; }); - - // The refusal ENVELOPE, not merely "it threw": a bare `toThrow()` would - // stay green against any unrelated failure on this path. - // ⚠️ `ReadonlyFieldRejectedError` carries `code` and `name` only — it has no - // `status` member (the HTTP mapping lives at the protocol layer), so - // asserting one here would pin a property this class does not have. - expect(refused).toBeDefined(); - expect(refused.name).toBe('ReadonlyFieldRejectedError'); - expect(refused.code).toBe('ERR_READONLY_FIELD_REJECTED'); - expect(refused.fields).toContain('next_maintenance_date'); - // And nothing was written. - expect(eq('eq_7').next_maintenance_date).toBe(STALE); - }); - - it('⛔ a hook that runs but writes some OTHER key confers nothing on this one', async () => { - // Provenance is per KEY, never "a hook ran on this write". - engine.registerHook('beforeUpdate', async (ctx: any) => { - ctx.input.data.name = 'rewritten-by-hook'; - }, { object: 'prov_equipment', priority: 60 }); - seed('eq_8'); - - await engine.update('prov_equipment', { id: 'eq_8', next_maintenance_date: FORGED }); - - expect(eq('eq_8').name).toBe('rewritten-by-hook'); - expect(eq('eq_8').next_maintenance_date).toBe(STALE); - }); - - // ── The #4889 lock: provenance leaves CALLER writes exactly where they were ─ - - it('#4889 UNCHANGED: a caller value on a TRUE STATE predicate is still stripped', async () => { - // The frozen-record class the triage ruling names. No hook assigns - // `closed_note`, so no record exempts it, and the state lock holds. - seed('eq_9', { status: 'closed' }); - - await engine.update('prov_equipment', { id: 'eq_9', closed_note: 'caller note' }); - - expect(eq('eq_9').closed_note).toBeNull(); - expect(warns.some((w) => w.includes("Field 'closed_note' is read-only (readonlyWhen)"))).toBe(true); - }); - - it('#4889 UNCHANGED: isSystem still does NOT exempt a caller value (Option B stays rejected)', async () => { - seed('eq_10', { status: 'closed' }); - await engine.update( - 'prov_equipment', { id: 'eq_10', closed_note: 'caller note' }, - { context: { isSystem: true } } as any, - ); - expect(eq('eq_10').closed_note).toBeNull(); - }); - - it("what persists is always the HOOK's value, never the caller's", async () => { - // The forgery boundary read as an outcome: a caller cannot launder its own - // value through the hook phase, because the hook's assignment is what stands - // on the key afterwards. - seed('eq_11'); - - await engine.update('prov_equipment', { - id: 'eq_11', period_days: 120, next_maintenance_date: FORGED, - }); - - expect(eq('eq_11').next_maintenance_date).not.toBe(FORGED); - expect(eq('eq_11').next_maintenance_date).toBe(DERIVED); - }); - - it('MEASURED-UNCHANGED: a hook writing a DIFFERENT value than the caller behaves as before', async () => { - // Pinned because the card asks for today's behaviour on this input to be - // measured before it moves — and it must NOT move. Pre-#14259 the value - // test already answered "not the caller's" here (`Object.is(DERIVED, - // FORGED)` is false) and kept the hook's write; under provenance the record - // answers the same way for a different reason. Same verdict, both routes, - // which is what makes the record a strictly narrower gate than the - // comparison it fronts. - seed('eq_12'); - await engine.update('prov_equipment', { - id: 'eq_12', period_days: 120, next_maintenance_date: FORGED, - }); - expect(eq('eq_12').next_maintenance_date).toBe(DERIVED); - expect(warns).toEqual([]); - }); - - // ── The BULK branch reaches the same verdict off the SAME sealed record ──── - - it('MULTI: the echoed hook write lands on the predicate branch too', async () => { - seed('eq_20', { status: 'active' }); - seed('eq_21', { status: 'active' }); - - await engine.update( - 'prov_equipment', - { period_days: 120, next_maintenance_date: DERIVED }, - { where: { status: 'active' }, multi: true } as any, - ); - - expect(eq('eq_20').next_maintenance_date).toBe(DERIVED); - expect(eq('eq_21').next_maintenance_date).toBe(DERIVED); - }); - - it('⛔ MULTI NEGATIVE CONTROL: the identical payload with no hook write is still stripped', async () => { - // #3106 / #4441 "both call sites": a bulk write must not reach a different - // verdict about who wrote a key than a by-id write does — in EITHER - // direction. This is the by-id negative control, run through - // `stripReadonlyWhenFieldsMulti`. - seed('eq_22', { status: 'active' }); - - await engine.update( - 'prov_equipment', - { name: 'Renamed', next_maintenance_date: DERIVED }, - { where: { status: 'active' }, multi: true } as any, - ); - - expect(eq('eq_22').name).toBe('Renamed'); - expect(eq('eq_22').next_maintenance_date).toBe(STALE); - expect(warns.some((w) => w.includes("Field 'next_maintenance_date' is read-only (readonlyWhen) in ≥1 matched row"))).toBe(true); - }); - - it('a hook that REPLACES ctx.input.data falls back to the value test, not to "keep everything"', async () => { - // The recorder's KNOWN LIMIT, inherited: a replacement's keys are mostly - // the CALLER's, so reading them as hook-owned would launder a forgery. With - // no attributable record the strip must behave exactly as it did before - // this card — i.e. over-strip. Fail-safe means "keep the old bug". - engine.registerHook('beforeUpdate', async (ctx: any) => { - ctx.input.data = { ...ctx.input.data, next_maintenance_date: DERIVED }; - }, { object: 'prov_equipment', priority: 60 }); - seed('eq_23'); - - await engine.update('prov_equipment', { - id: 'eq_23', name: 'R', next_maintenance_date: DERIVED, - }); - - expect(eq('eq_23').name).toBe('R'); - expect(eq('eq_23').next_maintenance_date).toBe(STALE); - }); -}); - // ───────────────────────────────────────────────────────────────────────────── // SEAM 2 — runtime-owned (`autonumber`), INSERT path // ───────────────────────────────────────────────────────────────────────────── @@ -623,6 +328,43 @@ describe('seam 2 — the insert-side runtime-owned strip reads PROVENANCE (#1425 expect(row.code).toBe('0001'); }); + // ── The measured consequence of "an assignment ran", stated out loud ────── + + it('MEASURED: a lone self-assigning hook leaves the CALLER value on the key', async () => { + // ⚠️ RECORDING BEHAVIOUR, NOT BLESSING IT. This is the direct consequence + // of the mechanism the card mandates: the record says an ASSIGNMENT RAN and + // is deliberately blind to the value, so `ctx.input.data.code = + // ctx.input.data.code` — which computes nothing — is a `set`, and the + // caller's seed becomes hook-owned and survives. `title: 'no-hook'` + // short-circuits the priority-50 hook so the self-assignment is the ONLY + // write to `code`, which is what makes the surviving value the caller's. + // + // Pinned so the consequence is visible rather than discovered later, and + // pinned rather than argued: it is the exact shape that forked the + // `readonlyWhen` seam out of this PR, whose #9107 pin `LOCK 3b` pins the + // OPPOSITE verdict for a STATE lock ("a hook that writes the caller value + // BACK is the caller value, and goes"). #14259's fork clause sends that one + // to the decision inbox; nothing here resolves it. + // + // Why the same mechanism ships on THIS seam: `stripRuntimeOwnedFields` + // guards a runtime-owned COLUMN (#5503) — the same class of protection + // #14088 already moved to provenance on the update side, and the class + // whose exemption `runtimeOwnedStripWarning` promises hook authors in prose + // — not a STATE lock whose whole purpose is that no caller write survives a + // TRUE predicate. INSERT is exempt from `readonlyWhen` entirely, so no lock + // of that class exists on this path to open. A ruling that self-assignment + // must NOT count would move this pin and #14088's seam together; that is a + // deliberate follow-up, not silent drift. + engine.registerHook('beforeInsert', async (ctx: any) => { + ctx.input.data.code = ctx.input.data.code; + }, { object: 'prov_ticket', priority: 60 }); + + const row: any = await engine.insert('prov_ticket', { title: 'no-hook', code: 'CALLER-SEEDED' }); + + expect(row.code).toBe('CALLER-SEEDED'); + expect(warns).toEqual([]); + }); + // ── The exemptions above this seam are untouched ────────────────────────── it('#5503 UNCHANGED: an isSystem caller still bypasses the whole pass', async () => { diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index b857ca6be6..c6b93eb5a8 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -11044,18 +11044,7 @@ export class ObjectQL implements IObjectQLEngine { // is unchanged — a caller cannot make its own value look // hook-written (see `ReadonlyWhenStripOptions`) — and `isSystem` // is still NOT an exemption here, unlike the static strip below. - // - // [#14259] `hookWrittenKeys` — the SAME record sealed at the - // confluence above and consumed by the static strip below, now - // feeding the conditional one too. Threaded rather than - // re-derived: #9107 put this predicate in ONE shared function so - // the two `readonlyWhen` call sites could not fork, and a third - // derivation of "a hook wrote this key" would fork it against - // the static strip instead — which is the state #14088 left - // behind and this closes. The `supplied` snapshot is unchanged - // and still answers the caller-side half; the record only ever - // turns a STRIP into a KEEP, and only for a key a hook assigned. - hookContext.input.data = stripReadonlyWhenFields(updateSchema as any, preRoWhen, priorRecord, this.logger, roWhenParent, { supplied: suppliedValues, hookWrittenKeys }) as any; + hookContext.input.data = stripReadonlyWhenFields(updateSchema as any, preRoWhen, priorRecord, this.logger, roWhenParent, { supplied: suppliedValues }) as any; reportDroppedFields(preRoWhen, hookContext.input.data as Record, 'readonly_when'); // [#2948] Enforce STATIC `readonly` on the write path for // non-system callers (system writes legitimately set read-only @@ -11234,10 +11223,7 @@ export class ObjectQL implements IObjectQLEngine { // by-id branch above — "both call sites" is the #3106 / // #4441 shape that gets missed, and a bulk write must not // reach a different verdict about who wrote a key. - // [#14259] Which is why `hookWrittenKeys` is the SAME sealed - // set the by-id branch gets, from the one seal at the shared - // confluence — not a second recording armed on this branch. - hookContext.input.data = stripReadonlyWhenFieldsMulti(updateSchema as any, preRoWhenMulti, priorRows, this.logger, parentForRow, { supplied: suppliedValues, hookWrittenKeys }) as any; + hookContext.input.data = stripReadonlyWhenFieldsMulti(updateSchema as any, preRoWhenMulti, priorRows, this.logger, parentForRow, { supplied: suppliedValues }) as any; reportDroppedFields(preRoWhenMulti, hookContext.input.data as Record, 'readonly_when'); } // [#2948] Same static-`readonly` write guard on the bulk path — diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index b01fd9ab45..a75f6c74d6 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -538,22 +538,19 @@ function readonlyWhenBindings( * caller's — the identical two-part test `stripReadonlyFields` applies: * * 1. the key is an OWN property of `supplied` (a key a hook ADDED is not), and - * 2. a hook ASSIGNED the key (#14259 — by RECORD, see `hookWrittenKeys`), or - * failing a record, the payload still holds the caller's VALUE by - * `Object.is` (a key a hook OVERWROTE now carries a platform value, not a - * forgery). + * 2. the payload still holds the caller's VALUE by `Object.is` (a key a hook + * OVERWROTE now carries a platform value, not a forgery). * * ⚠️ **This does not weaken the lock at the API boundary, and the reason is * that a caller cannot reach the exempt side of either test.** To be treated as - * hook-written, a key must have been ASSIGNED by server code after the caller's - * payload arrived (or, under the fallback, carry a value that differs from what - * arrived at engine entry — which again only server code can arrange). A client - * that echoes the locked key back does not launder it: no hook touched it, so - * neither test exempts it and it is stripped. What changed is who may write the - * column — the server's own trusted hooks — not whether a caller may. `isSystem` - * is deliberately still NOT an exemption here: a state lock that any - * system-context write could bypass would not be a state lock (#4889's frozen - * paid-invoice lines are built on it). + * hook-written, a value must differ from what arrived at engine entry — which + * only server code can arrange. A client that echoes the locked key back does + * not launder it: either no hook touched it (test 2 holds, it is stripped) or a + * hook overwrote it (the value that persists is the HOOK's, and the client's is + * gone regardless). What changed is who may write the column — the server's own + * trusted hooks — not whether a caller may. `isSystem` is deliberately still NOT + * an exemption here: a state lock that any system-context write could bypass + * would not be a state lock (#4889's frozen paid-invoice lines are built on it). * * ABSENT `supplied` means "treat the whole payload as caller-supplied" — the * pre-#9107 behaviour, and the fail-SAFE default: a call site that has no entry @@ -566,28 +563,6 @@ interface ReadonlyWhenStripOptions { * before the before-phase hooks. Omit to judge every key in the payload. */ supplied?: Readonly>; - /** - * [#14259] The keys the before-phase hook chain ACTUALLY ASSIGNED on this - * payload, recorded while the writes happened — `recordHookPayloadWrites`, - * the instrument #14088 built for the static `readonly` strip, threaded to - * its `readonlyWhen` sibling so the two cannot disagree about what - * "caller-supplied" means (which, between #14088 and this, they did). - * - * OPTIONAL, and absent means "this call cannot say", never "no hook wrote - * anything": a direct caller with no recording, and any write whose hook - * REPLACED the payload object, fall back to the `Object.is` test below - * exactly as before this option existed. - * - * ⚠️ It may only ever turn a STRIP into a KEEP, and only for a key a hook - * assigned. A caller cannot put a key in here — echoing a key, a value, a - * `null` or a `Proxy` back is not a `set` on the recorded object — so the - * #4889 lock is untouched: a caller-supplied value that NO hook wrote is - * still stripped, still warns with the same text, and still reports through - * `onFieldsDropped` / `strictReadonlyWrites`. Any future producer of this set - * owes the same proof, because a key in it is a key this strip stops - * defending. - */ - hookWrittenKeys?: ReadonlySet; } /** @@ -600,36 +575,16 @@ function isCallerSuppliedValue( data: Record, supplied: Readonly>, name: string, - hookWrittenKeys?: ReadonlySet, ): boolean { // Own-property, never `in`: a field name is `^[a-z_][a-z0-9_]*$`, which // admits `constructor` / `valueOf` — inherited from `Object.prototype` on any // plain snapshot, so `in` would call a hook stamp caller-supplied and strip // it (the trap #5591 names on the static strip, one function over). if (!Object.prototype.hasOwnProperty.call(supplied, name)) return false; - // [#14259] ...or a hook ASSIGNED it. Asked BEFORE the value comparison, in - // the same position and for the same reason as inside `stripReadonlyFields`: - // it answers by RECORD the question the comparison is only a proxy for. - // `Object.is` collapses "the hook deliberately wrote the value the caller - // also sent" into "the hook never touched the key", and those two demand - // opposite verdicts — the measured shape being a `beforeUpdate` hook that - // derives a `readonlyWhen`-locked field against a caller that round-tripped - // the whole record and echoed the same value back, whose derivation is then - // deleted. Not a `null` bug: `0`, `''`, `false` and a shared object reference - // collide identically, which is why the answer is provenance, not a sentinel. - if (hookWrittenKeys?.has(name)) return false; // ...and it must still BE the caller's value. `Object.is`, not `===`, on // purpose: `===` reports NaN !== NaN, which would read a caller-forged NaN as // "a hook rewrote it" and KEEP the forgery — the one input where the loose // operator inverts the verdict. - // - // [#14259] STAYS, and stays as the fallback for every key the record above - // has nothing to say about: a call site that passes no `hookWrittenKeys` gets - // byte-identical behaviour, and a hook that REPLACED the payload object - // leaves no record, so this test is what still separates its overwrite from a - // forgery. ⛔ Not "keep everything" — the fallback over-strips (the - // pre-existing defect), which is the only safe direction: reading a - // replacement's keys as hook-owned would launder a caller's forgery. return Object.is(data[name], supplied[name]); } @@ -677,7 +632,7 @@ export function stripReadonlyWhenFields( // is not this strip's business at all, so there is nothing to evaluate and // nothing to warn about — including the unbound-root LOCKED branch, whose // fail-CLOSED verdict exists to protect against an unjudgeable CALLER write. - if (!isCallerSuppliedValue(data, supplied, name, options?.hookWrittenKeys)) continue; + if (!isCallerSuppliedValue(data, supplied, name)) continue; if (isReadonlyWhenLocked(def, view.merged, view.previous, name, logger, parent)) { if (result === data) result = { ...data }; delete (result as Record)[name]; @@ -911,12 +866,7 @@ export function stripReadonlyWhenFieldsMulti( if (!def?.readonlyWhen || !(name in data)) continue; // [#9107] Same authorship gate as the single-id strip, asked before any row // is judged — a hook-written key is exempt for the whole batch, not per row. - // [#14259] ...off the SAME sealed record, too: the engine seals once at the - // shared post-hook confluence and hands the identical set to both branches, - // so a bulk write cannot reach a different verdict about who wrote a key - // than a by-id write does (the #3106 / #4441 divergence, which two separate - // derivations of "a hook wrote this" would reproduce exactly). - if (!isCallerSuppliedValue(data, supplied, name, options?.hookWrittenKeys)) continue; + if (!isCallerSuppliedValue(data, supplied, name)) continue; const lockedInSomeRow = rowViews().some((view, i) => isReadonlyWhenLocked( def, From 54d68d39399161a3715c63b75ecfabe2172dcf18 Mon Sep 17 00:00:00 2001 From: "Claude Fable 5.1" Date: Wed, 2 Sep 2026 07:25:30 +0000 Subject: [PATCH 5/5] fix(objectql): honour the caller's limit in the new pin's driver double; re-anchor the system-context census Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68 --- content/docs/permissions/system-context.mdx | 24 +++++++++---------- ...gine-hook-provenance-sibling-seams.test.ts | 7 +++++- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index a6fd7f3811..b8856cbf46 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:11049` | -| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:11217` | -| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9827` | +| 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:11128` | +| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:11296` | +| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9895` | | 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:9864`, `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:5761` | -| 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:3605`, `:3615`, `:3642` | +| 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:9943`, `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:5762` | +| 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:3606`, `:3616`, `:3643` | | 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:6459` | -| 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:11810` | -| 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:11739` | +| 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:6460` | +| 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:11889` | +| 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:11818` | ### 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:3412` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes | -| 63 | `objectql/src/engine.ts:14159` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag | +| 62 | `objectql/src/engine.ts:3413` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes | +| 63 | `objectql/src/engine.ts:14238` | 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:9810`–`9827` | +| "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:9878`–`9895` | | "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/packages/objectql/src/engine-hook-provenance-sibling-seams.test.ts b/packages/objectql/src/engine-hook-provenance-sibling-seams.test.ts index 00bb10276a..c0b3fd4908 100644 --- a/packages/objectql/src/engine-hook-provenance-sibling-seams.test.ts +++ b/packages/objectql/src/engine-hook-provenance-sibling-seams.test.ts @@ -67,7 +67,12 @@ function makeDriver() { name: 'memory', version: '0.0.0', supports: {}, async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, async find(object: string, ast: any) { - return Array.from(storeFor(object).values()).filter((r) => matches(r, ast?.where)); + const rows = Array.from(storeFor(object).values()).filter((r) => matches(r, ast?.where)); + // The caller's bound, applied AFTER the filter and by PRESENCE + // (`check:objectql-double-limit`): a double that silently ignores `limit` + // answers with more rows than the engine asked for, and a test written + // against it passes for a reason the real driver does not share. + return typeof ast?.limit === 'number' ? rows.slice(0, ast.limit) : rows; }, async findOne(object: string, ast: any) { for (const r of storeFor(object).values()) if (matches(r, ast?.where)) return r;