From b44e2f938722ff0f2c2b98fdbab99771f7116826 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 16:43:12 +0000 Subject: [PATCH 1/2] docs(spec): correct the `HookContext.input` contract table on `input.options` during `before*` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two `before` rows typed `input.options` as `DriverOptions`. Measured on the engine, that slot holds the CALLER's own engine options bag while `before*` runs — `where` and `multi` included — and the driver-facing keys are merged onto it only after the handlers return. Adds a positive pin in `hook-input-shape-contract.test.ts` for all four write shapes, so the property two break-glass guards depend on stops being prose only. Fixes #5997 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JTSZAjgtL3oR6YcpNDhW3T --- .changeset/hook-input-options-before-phase.md | 15 ++ .../src/hook-input-shape-contract.test.ts | 146 ++++++++++++++++++ packages/spec/src/data/hook.zod.ts | 52 +++++-- 3 files changed, 201 insertions(+), 12 deletions(-) create mode 100644 .changeset/hook-input-options-before-phase.md diff --git a/.changeset/hook-input-options-before-phase.md b/.changeset/hook-input-options-before-phase.md new file mode 100644 index 0000000000..d9bc59c30a --- /dev/null +++ b/.changeset/hook-input-options-before-phase.md @@ -0,0 +1,15 @@ +--- +"@objectstack/spec": patch +"@objectstack/objectql": patch +--- + +Correct the `HookContext.input` contract table on `input.options`: during +`before*` the slot holds the CALLER's engine options bag (`where` and `multi` +included), not `DriverOptions` — the engine merges the driver-facing keys onto +it only after the handlers return. The table's two `before` rows said +`DriverOptions`, a type that declares neither key, which reads as "a hook can +see no predicate at all"; the composed `ast` is what hooks cannot reach, while +the caller's raw predicate is right there and is an upper-bound approximation of +the row set (middleware only narrows) — the safe direction for the fail-closed +guards built on it. Pinned with a positive assertion in +`hook-input-shape-contract.test.ts`. diff --git a/packages/objectql/src/hook-input-shape-contract.test.ts b/packages/objectql/src/hook-input-shape-contract.test.ts index 95ef831a7e..1cca12bdba 100644 --- a/packages/objectql/src/hook-input-shape-contract.test.ts +++ b/packages/objectql/src/hook-input-shape-contract.test.ts @@ -54,6 +54,38 @@ * `input.ast`, so "no `ast` on the write paths" is a measurement, not an * assertion that would pass against an engine that had stopped setting `ast` * anywhere at all. + * + * ## [#5997] The other half — what a `before*` handler CAN reach + * + * Section 5 below is the POSITIVE twin of section 1, and it exists because the + * table's two `before` rows had the complementary defect: they typed + * `input.options` as `DriverOptions`, a type that declares neither `where` nor + * `multi`. The engine does not build that there — it hands `before*` the + * CALLER's own engine options bag (`EngineUpdateOptions` / + * `EngineDeleteOptions`), predicate included, and only merges the driver-facing + * keys onto it after the handlers return. Two shipped break-glass guards in + * `packages/plugins/plugin-auth` (the #5892 ban half and the #5941 delete half) + * resolve their target rows from exactly that slot, and objectql's own + * `isPredicateBulkWrite` (`hook-wrappers.ts`) reads `options.multi` off it — so + * the property was load-bearing while being prose only: nothing here asserted + * it, and `'ast' in input === false` above reads, on its own, as "a handler can + * see no predicate at all" — the false inference #5997 reported. + * + * Measured (the deletion test these assertions have to survive): rebuilding the + * `before*` slot into a STRIPPED `DriverOptions` — the shape the old table row + * described — turns exactly the four cases below red and leaves all eleven + * pre-existing cases in this file GREEN, including §2's + * `expect(seen[0].options).toBeDefined()`. That gap is why the pin is worth + * having: the suite as it stood could not tell the two shapes apart. + * + * The two statements are both true and must stay distinguishable: + * + * - the COMPOSED `ast` — the *effective* predicate, onto which the filters + * middleware may layer RLS / sharing narrowing — is NOT reachable (§1); + * - the CALLER's RAW `options.where` (and `options.multi`) IS (§5). + * + * Middleware only ever narrows, so the caller's predicate over-approximates + * the row set — the safe direction for a fail-closed guard. */ import { describe, it, expect } from 'vitest'; @@ -255,6 +287,113 @@ describe('[#5273] a metadata-declared hook reads the same shape', () => { }); }); +/* ──────────────────────────────────────────────────────────────────────────── + * 5. [#5997] `input.options` during `before*` IS the caller's bag — `where` + * and `multi` included. The positive twin of section 1. + * ──────────────────────────────────────────────────────────────────────────── */ + +describe("[#5997] `before*` reads the CALLER's options bag, predicate included", () => { + /** + * Every case asserts the same three things, and each one fails on a + * different way of getting this wrong: + * + * 1. `toBe(callerOptions)` — REFERENCE identity. The engine passes the + * caller's very object through; substituting a freshly built + * `DriverOptions` for it (which is what the old table row described) + * fails here even if the substitute happened to copy `where` across. + * 2. `options.where` deep-equals the predicate the caller passed. This is + * the read both plugin-auth guards actually perform, and it is what + * goes red if the slot is ever rebuilt into a stripped bag before the + * handlers run. + * 3. `'ast' in input === false` — restated per case so the pair reads + * together: the COMPOSED predicate stays unreachable while the RAW one + * is right there. Neither assertion is safe to read without the other. + */ + const assertCallerBag = ( + input: Record, + callerOptions: Record, + where: unknown, + ): void => { + expect(input.options).toBe(callerOptions); + expect((input.options as Record).where).toEqual(where); + // §1's claim, restated here so neither half can be read alone. + expect('ast' in input).toBe(false); + }; + + it('`beforeUpdate` (single id) carries the caller `where`', async () => { + const seen: Array> = []; + const { engine } = await boot(); + engine.registerHook('beforeUpdate', async (ctx: any) => { seen.push(ctx.input); }, { object: 'task' }); + + const [row] = await seedTasks(engine, [{ title: 'a', status: 'todo' }]); + const callerOptions = { where: { id: row.id } }; + await engine.update('task', { status: 'done' }, callerOptions as any); + + expect(seen).toHaveLength(1); + assertCallerBag(seen[0]!, callerOptions, { id: row.id }); + // A by-id write is not a batch: nothing sets `multi`. + expect((seen[0]!.options as any).multi).toBeUndefined(); + }); + + it('`beforeUpdate` (bulk) carries the caller `where` AND `multi`', async () => { + const seen: Array> = []; + const { engine } = await boot(); + engine.registerHook('beforeUpdate', async (ctx: any) => { seen.push(ctx.input); }, { object: 'task' }); + + await seedTasks(engine, [{ title: 'a', status: 'todo' }, { title: 'b', status: 'todo' }]); + const callerOptions = { multi: true, where: { status: 'todo' } }; + await engine.update('task', { status: 'done' }, callerOptions as any); + + expect(seen).toHaveLength(1); + assertCallerBag(seen[0]!, callerOptions, { status: 'todo' }); + // `multi` survives too — the guards branch on it to tell a batch from a + // by-id write when `input.id` is undefined for either reason. + expect((seen[0]!.options as any).multi).toBe(true); + // And the batch shape from §2 still holds on the same context. + expect(seen[0]!.id).toBeUndefined(); + }); + + it('`beforeDelete` (single id) carries the caller `where`', async () => { + const seen: Array> = []; + const { engine } = await boot(); + engine.registerHook('beforeDelete', async (ctx: any) => { seen.push(ctx.input); }, { object: 'task' }); + + const [row] = await seedTasks(engine, [{ title: 'a', status: 'todo' }]); + const callerOptions = { where: { id: row.id } }; + await engine.delete('task', callerOptions as any); + + expect(seen).toHaveLength(1); + assertCallerBag(seen[0]!, callerOptions, { id: row.id }); + expect((seen[0]!.options as any).multi).toBeUndefined(); + }); + + it('`beforeDelete` (bulk) carries an operator predicate verbatim, `multi` included', async () => { + const seen: Array> = []; + const { engine } = await boot(); + engine.registerHook('beforeDelete', async (ctx: any) => { seen.push(ctx.input); }, { object: 'task' }); + + const rows = await seedTasks(engine, [ + { title: 'a', status: 'todo' }, + { title: 'b', status: 'todo' }, + { title: 'c', status: 'keep' }, + ]); + // The `$in` shape #5941's guard is written against — a predicate that can + // sweep several administrators in one call. It must arrive UNPARSED, since + // the guard resolves it itself. + const doomed = [rows[0]!.id, rows[1]!.id]; + const callerOptions = { multi: true, where: { id: { $in: doomed } } }; + await engine.delete('task', callerOptions as any); + + expect(seen).toHaveLength(1); + assertCallerBag(seen[0]!, callerOptions, { id: { $in: doomed } }); + expect((seen[0]!.options as any).multi).toBe(true); + expect(seen[0]!.id).toBeUndefined(); + // The write really did run as a batch through that predicate — so the + // assertions above describe a live path, not an inert options bag. + expect(await engine.count('task', {} as any)).toBe(1); + }); +}); + /* ──────────────────────────────────────────────────────────────────────────── * Harness — a stub driver just wide enough for the dispatch paths above. * ──────────────────────────────────────────────────────────────────────────── */ @@ -276,6 +415,13 @@ function makeStubDriver(): any { if (!where || typeof where !== 'object') return true; for (const [k, v] of Object.entries(where)) { if (k.startsWith('$')) continue; + // `$in` (#5997): the batch shape the delete guard is written against, so + // its case exercises a predicate that really matches several rows rather + // than an options bag nothing consumes. + if (v && typeof v === 'object' && '$in' in (v as any)) { + if (!(v as any).$in.includes(row[k])) return false; + continue; + } const expected = v && typeof v === 'object' && '$eq' in (v as any) ? (v as any).$eq : v; if ((row[k] ?? null) !== (expected ?? null)) return false; } diff --git a/packages/spec/src/data/hook.zod.ts b/packages/spec/src/data/hook.zod.ts index 5448914087..53b64ecf0c 100644 --- a/packages/spec/src/data/hook.zod.ts +++ b/packages/spec/src/data/hook.zod.ts @@ -332,28 +332,56 @@ export const HookContextSchema = lazySchema(() => z.object({ * live there because only objectql can execute a dispatch, and spec must not * depend on it. * - * - find (also fires for findOne): { ast: QueryAST, options: DriverOptions } - * - insert (one context per row, batch inserts included): { data: Record, options: DriverOptions } - * - update (single id): { id: ID, data: Record, options: DriverOptions } - * - update (bulk, multi:true) — before: { id: undefined, data: Record, options: DriverOptions } + * - find (also fires for findOne): { ast: QueryAST, options: see PHASE below } + * - insert (one context per row, batch inserts included): { data: Record, options: see PHASE below } + * - update (single id): { id: ID, data: Record, options: see PHASE below } + * - update (bulk, multi:true) — before: { id: undefined, data: Record, options: EngineUpdateOptions } * - update (bulk, multi:true) — after, PER MATCHED ROW: { id: ID, data: Record, options: DriverOptions } - * - delete (single id): { id: ID, options: DriverOptions } - * - delete (bulk, multi:true) — before: { id: undefined, options: DriverOptions } + * - delete (single id): { id: ID, options: see PHASE below } + * - delete (bulk, multi:true) — before: { id: undefined, options: EngineDeleteOptions } * - delete (bulk, multi:true) — after, PER MATCHED ROW: { id: ID, options: DriverOptions } * + * PHASE — `input.options` is the one slot whose TYPE depends on when you read + * it, on every path above. The engine builds the context with the CALLER's + * own options bag (`EngineQueryOptions` / `DataEngineInsertOptions` / + * `EngineUpdateOptions` / `EngineDeleteOptions`) and only AFTER the `before*` + * handlers return — before the driver call — merges the driver-facing keys + * onto it (`buildDriverOptions`: transaction, tenantId, timezone, …). So a + * `before*` handler reads the CALLER's bag, `where` and `multi` included; an + * `after*` handler and the driver read the `DriverOptions` view. The merge is + * ADDITIVE — it spreads the caller's bag and adds keys, never strips one — so + * the widening is one-way and nothing a `before*` handler saw disappears. + * #5997 corrected the two `before` rows above, which had said `DriverOptions` + * (a type that declares no `where` and no `multi`): that is not what the + * engine builds there, and not what the two consumers named below read. + * Measured and pinned in the same contract test as the rest of this table. + * * A bulk (`multi: true`) update/delete fires the SAME `beforeUpdate`/ * `beforeDelete` events as a single-id write, ONCE for the whole batch; * there is no separate `*Many` event. `input.id` is present but `undefined` * there — binding it is precisely the test the engine dispatches on, so a * `before*` handler that sets it REROUTES the write onto the single-id path. * - * The row-scoping predicate is NOT reachable from `input` at all. It lives - * on the engine-internal `OperationContext.ast` (#2982) so that the filters + * The row-scoping predicate a bulk write EXECUTES is not reachable from + * `input` at all. It is the composed `ast`, which lives on the + * engine-internal `OperationContext.ast` (#2982) so that the filters * middleware composes onto it — RLS write policies, the sharing plugin's - * editable-rows filter — bind the driver call itself, where no handler can - * widen them. A bulk write therefore hands hooks no queryable predicate: - * scope the batch through `options.where` at the CALLER, or work per row on - * the `after*` events below. + * editable-rows filter — and binds the driver call itself, where no handler + * can widen it. What a `before*` handler CAN read is the strictly separate + * fact above: the caller's RAW predicate on `input.options.where` (with + * `input.options.multi`). The two differ by exactly the middleware's + * narrowing, and middleware only ever narrows, never widens — so treating + * the caller's predicate as the batch's row set is an UPPER-BOUND + * approximation. That is the safe direction for a fail-closed guard (it may + * refuse a write that would have touched fewer rows; it can never miss one + * that touches more) and the wrong direction for anything that needs the + * effective set exactly, which should work per row on the `after*` events + * below instead. Both of `plugin-auth`'s break-glass last-admin guards + * (#5892 ban half, #5941 delete half) are built on that upper bound, and + * objectql's own `isPredicateBulkWrite` (`hook-wrappers.ts`, #5038/#4775) + * reads `input.options.multi` from the same slot to tell a batch dispatch + * from a per-row one. Do not narrow `input.options` on the `before*` paths + * without re-reading all three. * * Since #5038 (ADR-0058's bulk-write addendum) the `after*` events on a bulk * write dispatch ONCE PER MATCHED ROW, each on a single-record-shaped From f2737e39f0e02d668ff1adfd255034d4568a8e38 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 17:30:40 +0000 Subject: [PATCH 2/2] test(objectql): drop the `as any` on the new `count` call (#4918 ratchet) `count(object, query?)` already infers the empty query; erasing it added a site to `check:query-options-erasure` for nothing. The positive control at the top of this file carries the same note. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JTSZAjgtL3oR6YcpNDhW3T --- packages/objectql/src/hook-input-shape-contract.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/objectql/src/hook-input-shape-contract.test.ts b/packages/objectql/src/hook-input-shape-contract.test.ts index 1cca12bdba..c9402ca324 100644 --- a/packages/objectql/src/hook-input-shape-contract.test.ts +++ b/packages/objectql/src/hook-input-shape-contract.test.ts @@ -390,7 +390,10 @@ describe("[#5997] `before*` reads the CALLER's options bag, predicate included", expect(seen[0]!.id).toBeUndefined(); // The write really did run as a batch through that predicate — so the // assertions above describe a live path, not an inert options bag. - expect(await engine.count('task', {} as any)).toBe(1); + // No `as any` on this one: `count(object, query?)` infers the empty query, + // and erasing it would add a site to the #4918 ratchet for nothing (the + // positive control at the top of this file carries the same note). + expect(await engine.count('task', {})).toBe(1); }); });