From d1ab7df727c09b55998ec08c677a98ce8269db08 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 05:13:12 +0000 Subject: [PATCH] =?UTF-8?q?feat(spec,objectql):=20hook=20=E6=B3=A8?= =?UTF-8?q?=E5=86=8C=E9=9D=A2=E6=96=B0=E5=A2=9E=20excludeObjects,=E5=8F=AF?= =?UTF-8?q?=E8=A1=A8=E8=BE=BE=E3=80=8C=E5=85=A8=E5=B1=80=E4=BD=86=E6=8E=92?= =?UTF-8?q?=E9=99=A4=E8=BF=99=E4=BA=9B=E5=AF=B9=E8=B1=A1=E3=80=8D=20(#5928?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 注册契约此前只有一种表达能力:`object` 允许列表(缺省 = 全局,`'*'` = 全集)。 允许列表与拒绝列表只在**封闭全集**上可互换,而对象全集在运行期是开放的 —— `/meta` PUT 成功后 `applyObjectRegistryMutation` 直接把新对象注册进引擎, `SchemaRegistry.registerObject` 不发任何事件,插件侧没有可订阅的通道来维护 一份枚举出来的名单。于是「全局但排除这几张平台表」只剩两条错路:知识留在 handler 里早退(注册面仍是全局,#5284/#5038 的按对象门对这些对象判真、白读 一遍行集),或把补集枚举进 `object`(名单在启动时冻结 —— 探针 D:此后新建 的对象静默不被覆盖,对审计插件是无声的合规倒退)。 ## 前提复核(origin/main,行号已漂移,以内容为准) 裁决评论(claude[bot] 5205548295)为唯一权威。逐处核对全部成立,仅行号变动: - `HookEntry` engine.ts:698(单据记 645/647) - `registerHook` options engine.ts:1147(单据记 1096) - `Registered hook` 日志 engine.ts:1182 - `triggerHooks` 按对象匹配 engine.ts:1415(单据记 1363) - `hasHooksFor` engine.ts:1442/1448(单据记 1396) - 契约面声明 spec/src/contracts/objectql-engine.ts:187-191(单据记 141) `excludeObjects` 亦非新造名词:rest-server.zod.ts:378 与 disaster-recovery.zod.ts:230 已用它表达「全部减去这些」。 ## 红线逐条落点 1. 单一匹配函数 —— 新增 `hookMatchesObject(entry, objectName)`, `triggerHooks` 与 `hasHooksFor` 两处手写实现同时收敛到它。 2. 性质 pin —— `hook-exclude-objects.test.ts` 以 allow×exclude 全矩阵断言 「门永远不得比派发更紧」(蕴含方向 `dispatched ⇒ gate open`,不是等价: `hasHooksFor` 故意不看 `skipAutomations`,更松是合法的)。 3. 拒绝语义(沿 #4281 / ADR-0078)—— `''` / `['']` / 任一空白成员拒绝; `'*'` 出现在排除面拒绝(减掉全集 = 永不触发)。均 throw 且消息给出改法。 `[]` 明确接受:它是「什么都不减」的诚实拼写,也是 `[...SKIP_OBJECTS]` 在源名单为空时的自然取值 —— #4281 拒绝 `object: []` 是因为 binder 会把它 **放大**成 `'*'`,这里没有任何东西改写该值。 4. 两个不做 —— 未加 `objectFilter` 谓词回调;未动 `packages/spec/src/data/hook.zod.ts` 的 authorable HookSchema (`check:authorable-surface` 绿即为证)。 5. 单 PR 跨两包,未拆两段(避免 spec 声明而引擎忽略的 declared≠enforced 窗口)。 6. 注册日志 `logger.debug('Registered hook', …)` 如实带上 `excludeObjects`。 7. 未做 #5860 的 plugin-audit 侧改动 —— 本单只交付表达能力本身。 8. changeset:spec + objectql 双包 minor。 ## 一处刻意的行为守恒 `hookMatchesObject` 的允许半边保留两份旧实现共用的**真值判断** (`if (entry.object)`),而非读起来更准的 `!== undefined`。二者仅在 `object: ''` 上不同:今天它注册的是**全局** hook(falsy ⇒ 不过滤)—— 即 #4281 的失败模式在它未覆盖的代码路径上的残留(该裁决只关了 schema 与 binder)。在本 PR 里翻转它,会把一个「对所有对象触发」的 hook 静默变成 「对任何对象都不触发」。故按原样保留并显式钉住,另行记录。 ## 验证读数 - `pnpm --filter @objectstack/objectql test` → 147 files / 2473 tests 全绿 - 新增 `hook-exclude-objects.test.ts` → 19/19 绿 - `pnpm --filter @objectstack/spec test` → 344 files / 8826 tests 全绿 - `pnpm --filter @objectstack/spec check:generated` → 10/10 up to date - 两包 `typecheck` 绿(spec 含 scripts + test 三道) - `check:spec-parsed-alias` / `check:nul-bytes` / `check:engine-double-contract` / `check:wildcard-fallthrough` / `check:startup-registry-verdict` / `check:durability-log-level` / `check:error-code-casing` / `check:adr-anchors` 全绿 - eslint 四个改动文件零告警 ## 逆向验证(方向先判后跑) R1 删掉共享匹配函数的**减法半边**(声明、日志、拒绝保留): 预判语义用例红、性质 pin **绿**;实测 6 红 13 绿 —— 两条真正的性质断言 如预判保持绿(它守的是门与派发的关系,不是排除面的存在),多出的 1 红是 `skipAutomations` 那条同时携带了一句排除断言,按它自己的理由红。 R2 让门比派发更紧(派发只读允许面,门读完整匹配): 预判性质 pin 红;实测正是它红,并打印出违例作用域: `gate TIGHTER than dispatch for {"excludeObjects":"account"} on 'account'`。 另记一条诚实读数:多注册组合那一条在 R2 下仍绿 —— 集合里只要有一个全局 entry,门对任何对象都是开的,violation 无从浮现。检出力在单 entry 矩阵, 已在测试内注明,不要把它读成有牙的断言。 Refs: #5860(被本单解锁)、#5846、#5284、#5038、#4281 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_011M7UwH25Unfi73UHim7ajY --- .../hook-exclude-objects-registration-face.md | 16 + packages/objectql/src/engine.ts | 184 ++++++++- .../objectql/src/hook-exclude-objects.test.ts | 354 ++++++++++++++++++ .../objectql-engine-hook-scope.test.ts | 97 +++++ .../spec/src/contracts/objectql-engine.ts | 29 +- 5 files changed, 659 insertions(+), 21 deletions(-) create mode 100644 .changeset/hook-exclude-objects-registration-face.md create mode 100644 packages/objectql/src/hook-exclude-objects.test.ts create mode 100644 packages/spec/src/contracts/objectql-engine-hook-scope.test.ts diff --git a/.changeset/hook-exclude-objects-registration-face.md b/.changeset/hook-exclude-objects-registration-face.md new file mode 100644 index 0000000000..2235f1cd21 --- /dev/null +++ b/.changeset/hook-exclude-objects-registration-face.md @@ -0,0 +1,16 @@ +--- +'@objectstack/objectql': minor +'@objectstack/spec': minor +--- + +A hook registration can now express "global, EXCEPT these objects" — `registerHook(event, handler, { excludeObjects })`. + +`registerHook` carried one scope face: `object`, an allow list (absent = global, `'*'` = every object). An allow list and a deny list are interchangeable only over a closed universe of object names, and this one is open — a successful `/meta` PUT registers new objects into a running engine, and `SchemaRegistry.registerObject` emits no event a plugin could subscribe to. So a registrant wanting "everything except these platform tables" had two options, both wrong: keep the skip list inside the handler as an early return, which leaves the registration global and makes the per-object gates (`hasHooksFor`, the bulk-write row-set read) answer "hooks apply" for objects the handler is about to skip; or enumerate the complement into `object`, which freezes the list at boot so an object created afterwards is silently not covered — a compliance regression for the audit plugin, and a silent one. + +`excludeObjects?: string | string[]` is the deny half, subtracted from whatever `object` admits: `matches = allowMatches && !excludeMatches`. Absent means subtract nothing, so every registration that compiled before still behaves identically. Declared on the registration rather than left to a predicate callback, so the scope stays static, printable — the `Registered hook` debug record now reports it — and introspectable by diagnostics. + +Two shapes are refused at registration, following the same reasoning as the empty-target ruling: an empty name (`''`, `['']`, or a blank member) would subtract nothing while reading as though it subtracted something, and `'*'` would subtract every object and leave a hook that can never fire (ADR-0078: no silently inert declaration). Both throw, naming the fix. `excludeObjects: []` is accepted — it is the honest spelling of "subtract nothing", and the natural value of a spread whose source list is empty. + +`triggerHooks` (dispatch) and `hasHooksFor` (the bulk-write gate) were two hand-written copies of one matching semantic; adding a second scope dimension to two copies is how they drift, so both now call one shared matcher. A property test pins the direction that matters — the gate is never tighter than the dispatch, since a looser gate costs a wasted query while a tighter one silently drops hooks that were going to fire. + +The authorable `HookSchema` is deliberately untouched: the consumer is plugin code registering in TypeScript, and no metadata author needs "global minus a list" today. The key stays off the authoring surface until real pull appears. diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 7858da326e..eeb9ec3fa2 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -698,6 +698,22 @@ export type HookHandler = (context: HookContext) => Promise | void; export interface HookEntry { handler: HookHandler; object?: string | string[]; // undefined = global hook + /** + * [#5928] Object name(s) SUBTRACTED from whatever `object` admits — the + * negative half of a registration's scope. Absent = subtract nothing. + * + * `object` alone can only say "these objects" (plus the `'*'` full set), and + * the two are interchangeable only over a CLOSED universe. This one is open: + * `applyObjectRegistryMutation` registers objects into a running engine on a + * successful `/meta` PUT, emitting no event a plugin could subscribe to, so a + * registrant that enumerated the complement of its skip list would keep a + * frozen list and silently stop covering every object registered after boot. + * For the audit plugin that is a compliance regression, and a silent one. + * + * Read only through {@link hookMatchesObject} — never re-implemented at a + * call site (see that function's note on the one-semantic rule). + */ + excludeObjects?: string | string[]; priority: number; packageId?: string; /** @@ -710,6 +726,122 @@ export interface HookEntry { hookName?: string; } +/** `object` / `excludeObjects` in either accepted spelling → a name list. */ +function hookTargetList(target: string | string[] | undefined): string[] { + if (target === undefined) return []; + return Array.isArray(target) ? target : [target]; +} + +/** + * [#5928] Does `entry` cover `objectName`? **The** answer — one function, both + * consumers. + * + * ## Why this is a function and not two `if`s + * + * `triggerHooks` (dispatch) and `hasHooksFor` (the #5038 / #5284 bulk-write + * gate that decides whether the matched row set is worth reading) used to carry + * this rule as two hand-written copies of one semantic. `hasHooksFor`'s own + * comment named the hazard that arrangement carries: a gate LOOSER than the + * dispatch only wastes a query, but a gate TIGHTER than the dispatch silently + * drops hooks that were going to fire. Two copies stay in agreement only for as + * long as everyone who edits one remembers the other, and #5928 adds a second + * dimension to the rule — exactly the edit that would have desynchronised them. + * So the copies are gone: both consumers call this, and the property test in + * `hook-exclude-objects.test.ts` pins the implication (`dispatched ⇒ gate open`) + * over the full allow × exclude matrix rather than trusting the structure. + * + * ## The semantic + * + * `matches = allowMatches && !excludeMatches`, where an absent `object` admits + * everything (global) and `'*'` in `object` does the same explicitly. + * `excludeObjects` names are matched literally — `'*'` is refused at + * registration, so it can never reach this as a subtract-everything wildcard. + * + * The allow half keeps the TRUTHINESS test both copies used verbatim, rather + * than the `!== undefined` that reads more precisely. They differ on exactly one + * input, `object: ''`, which today registers a GLOBAL hook (falsy ⇒ no filter) — + * #4281's failure mode surviving on the code path it never covered, since it + * closed the metadata path at the schema and the binder. Flipping it here would + * turn a hook that fires on everything into one that fires on nothing, silently, + * inside a PR about a different face of the contract. Out of scope by + * construction: preserved, pinned in `hook-exclude-objects.test.ts`, and filed + * separately. + */ +export function hookMatchesObject( + entry: Pick, + objectName: string, +): boolean { + // Allow half: absent = global; otherwise the wildcard or a literal name. + if (entry.object) { + const targets = hookTargetList(entry.object); + if (!targets.includes('*') && !targets.includes(objectName)) return false; + } + // Subtract half: any literal hit removes the object from the admitted set. + if (entry.excludeObjects) { + if (hookTargetList(entry.excludeObjects).includes(objectName)) return false; + } + return true; +} + +/** + * [#5928] Registration-time refusal for the `excludeObjects` face, following + * the sister ruling on empty hook targets (#4281 / #4001, ADR-0078 "no silently + * inert declaration"). + * + * Two shapes are refused, both statically decidable at the call site: + * + * - **`''` / `['']`** (or any blank member) — no object is named `''`, so the + * entry subtracts nothing while reading as though it subtracts something. + * Inert, and inert in the direction that quietly keeps a hook firing on + * objects its author believed they had excluded. + * - **`'*'` anywhere in the list** — subtracting the full set from any allow + * set leaves the empty set: a hook that can never fire, registered + * "successfully". That is ADR-0078's silently-inert declaration exactly, and + * the same never-fire shape #4281 refused when `['']` produced it from the + * other side. + * + * `[]` is deliberately ACCEPTED. It is the honest spelling of "subtract + * nothing" — identical in meaning to omitting the key, and the natural value of + * a spread (`excludeObjects: [...SKIP_OBJECTS]`) whose source list is empty. + * The refusal above is not "empty is bad"; it is "a name that matches nothing" + * and "a name that matches everything". #4281 refused `[]` on `object` for a + * reason that does not exist here: there, the binder WIDENED the empty target + * into `'*'`, so blank intent silently became the maximum blast radius. Nothing + * transforms this value. + * + * A throw, not a warn: `excludeObjects` ships in this release with no callers, + * so strictness costs no migration, and unlike the unknown-event branch above + * (where a custom driver dispatching its own events is a legitimate reading) + * neither refused shape has one. + */ +function assertValidHookExcludeObjects( + excludeObjects: string | string[] | undefined, + event: string, +): void { + if (excludeObjects === undefined) return; + const names = hookTargetList(excludeObjects); + for (const name of names) { + if (typeof name !== 'string' || name.trim().length === 0) { + throw new Error( + `[ObjectQL] Hook '${event}' declares an empty \`excludeObjects\` entry. ` + + 'No object is named \'\', so it would subtract nothing while reading as if it ' + + 'subtracted something — a hook still firing on objects it appears to exclude. ' + + 'Name the object(s) to exclude — `excludeObjects: [\'sys_audit_log\']` — or omit ' + + 'the option entirely (an empty array is accepted and subtracts nothing).', + ); + } + if (name === '*') { + throw new Error( + `[ObjectQL] Hook '${event}' excludes the wildcard '*', which subtracts every ` + + 'object and leaves a hook that can never fire (ADR-0078: no silently inert ' + + 'declaration). Exclude the object names you mean — ' + + '`excludeObjects: [\'sys_audit_log\']` — or, if the hook really should not be ' + + 'registered, do not register it.', + ); + } + } +} + /** Function registry entry — see `registerFunction`. */ export interface FunctionEntry { handler: HookHandler; @@ -1142,10 +1274,17 @@ export class ObjectQL implements IObjectQLEngine { * Register a hook * @param event The event name (e.g. 'beforeFind', 'afterInsert') * @param handler The handler function - * @param options Optional: target object(s) and priority + * @param options Optional: target object(s), objects to exclude, and priority */ registerHook(event: string, handler: HookHandler, options?: { object?: string | string[]; + /** + * [#5928] Object name(s) subtracted from what `object` admits — the way to + * say "global, except these". See {@link HookEntry.excludeObjects} for why + * an allow list cannot express it, and + * {@link assertValidHookExcludeObjects} for the two refused shapes. + */ + excludeObjects?: string | string[]; priority?: number; packageId?: string; /** Original metadata Hook definition (set by `bindHooksToEngine`). */ @@ -1153,6 +1292,9 @@ export class ObjectQL implements IObjectQLEngine { /** Stable name from metadata (set by `bindHooksToEngine`). */ hookName?: string; }) { + // [#5928] Refuse an exclusion face that subtracts nothing (`''`) or + // everything (`'*'`) before anything is registered or reported. + assertValidHookExcludeObjects(options?.excludeObjects, event); // [#3195] Guard against enum-vs-dispatch drift: a hook on an event the // engine never triggers would register "successfully" and then silently // never fire. Warn loudly rather than swallow it. Not a hard reject — a @@ -1172,6 +1314,7 @@ export class ObjectQL implements IObjectQLEngine { entries.push({ handler, object: options?.object, + excludeObjects: options?.excludeObjects, priority: options?.priority ?? 100, packageId: options?.packageId, meta: options?.meta, @@ -1179,7 +1322,10 @@ export class ObjectQL implements IObjectQLEngine { }); // Sort by priority (lower runs first) entries.sort((a, b) => a.priority - b.priority); - this.logger.debug('Registered hook', { event, object: options?.object, priority: options?.priority ?? 100, totalHandlers: entries.length }); + // The exclusion face is reported alongside the allow face: a registration's + // scope is BOTH halves (#5928), and a log that printed only `object` would + // describe a global hook for an entry that is global-minus-twenty-tables. + this.logger.debug('Registered hook', { event, object: options?.object, excludeObjects: options?.excludeObjects, priority: options?.priority ?? 100, totalHandlers: entries.length }); } /** @@ -1409,12 +1555,10 @@ export class ObjectQL implements IObjectQLEngine { (context.session as { skipAutomations?: boolean } | undefined)?.skipAutomations === true; for (const entry of entries) { - // Per-object matching - if (entry.object) { - const targets = Array.isArray(entry.object) ? entry.object : [entry.object]; - if (!targets.includes('*') && !targets.includes(context.object)) { - continue; // Skip non-matching hooks - } + // Per-object matching — allow face minus exclusion face, the one shared + // rule `hasHooksFor` also reads (#5928). + if (!hookMatchesObject(entry, context.object)) { + continue; // Skip non-matching hooks } if (skipAutomations && entry.meta) { this.logger.debug('Skipping metadata-bound hook (skipAutomations)', { event, hook: entry.hookName }); @@ -1427,26 +1571,26 @@ export class ObjectQL implements IObjectQLEngine { /** * [#5038] Would `triggerHooks(event, ctx)` reach ANY handler for `object`? * - * Mirrors the per-object filter in `triggerHooks` exactly (an entry with no - * `object` is global; an array or `'*'` widens it), because this answer gates - * a READ of the whole matched row set on the bulk write path. Getting it - * looser than the dispatch loop only costs a wasted query; getting it - * TIGHTER would silently drop hooks that were going to fire, so the two must - * be read together. + * Applies the SAME per-object rule the dispatch loop applies — literally the + * same function, {@link hookMatchesObject}, since #5928 — because this answer + * gates a READ of the whole matched row set on the bulk write path. Getting + * it looser than the dispatch loop only costs a wasted query; getting it + * TIGHTER would silently drop hooks that were going to fire. Until #5928 the + * two were separate hand-written copies of one semantic and this comment + * asked the reader to keep them in step; sharing the function is what + * actually keeps them in step, and the property test in + * `hook-exclude-objects.test.ts` pins the direction that matters. * * `session.skipAutomations` is deliberately NOT consulted: it suppresses only * metadata-bound entries, and code-registered hooks (audit, sharing) still * run, so the row set is still needed. Over-reading in that case is a cost, - * never a correctness loss. + * never a correctness loss — and it is the SAFE side of the asymmetry above, + * which is why the pin asserts an implication rather than an equality. */ private hasHooksFor(event: string, object: string): boolean { const entries = this.hooks.get(event); if (!entries || entries.length === 0) return false; - return entries.some((entry) => { - if (!entry.object) return true; - const targets = Array.isArray(entry.object) ? entry.object : [entry.object]; - return targets.includes('*') || targets.includes(object); - }); + return entries.some((entry) => hookMatchesObject(entry, object)); } /** diff --git a/packages/objectql/src/hook-exclude-objects.test.ts b/packages/objectql/src/hook-exclude-objects.test.ts new file mode 100644 index 0000000000..590f4a4907 --- /dev/null +++ b/packages/objectql/src/hook-exclude-objects.test.ts @@ -0,0 +1,354 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5928] The hook registration contract can express "global, EXCEPT these + * objects". + * + * ## What was missing + * + * `registerHook` options carried one scope face: `object`, an ALLOW list (absent + * = global, `'*'` = every object). An allow list and a deny list are + * interchangeable only over a CLOSED universe of object names, and this one is + * open — `applyObjectRegistryMutation` registers new objects into a running + * engine on a successful `/meta` PUT, and `SchemaRegistry.registerObject` emits + * no event a plugin could subscribe to. So a registrant that wanted "everything + * except these twenty platform tables" had exactly two options, both wrong: + * + * - keep the knowledge inside the handler as an early return, which leaves the + * registration GLOBAL — so the #5038 / #5284 per-object gates, which read the + * registration face, answer "yes, hooks apply" for objects the handler is + * about to skip, and the bulk-write path pays for a row-set read nobody uses; + * - enumerate the complement into `object`, which freezes the list at boot — + * the "probe D" scenario below, where an object created afterwards is + * silently NOT covered. For the audit plugin that is a compliance regression + * that reports nothing. + * + * `excludeObjects` is the third option: the deny half, declared on the + * registration where the engine reads it. + * + * ## What these tests pin + * + * 1. the matching matrix — allow face × exclusion face, both spellings; + * 2. probe D — under an exclusion face, an object nobody enumerated is covered + * BY DEFAULT, which is the whole reason this shape was chosen over + * enumerating the complement; + * 3. the two refused shapes (`''`/`['']`, `'*'`), following #4281's ruling on + * empty hook targets, plus the `[]` that is deliberately accepted; + * 4. **the property**: the `hasHooksFor` gate is never TIGHTER than the + * `triggerHooks` dispatch. Both now call one `hookMatchesObject`, so this is + * structurally true today — the test is what keeps it true, and it asserts + * the implication rather than an equality because the gate is allowed to be + * looser (it deliberately ignores `skipAutomations`). + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectQL, hookMatchesObject, type HookEntry } from './engine.js'; + +const EVENT = 'afterUpdate'; + +function makeLogger() { + return { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }; +} + +function makeEngine() { + return new ObjectQL({ logger: makeLogger() }); +} + +/** Dispatch `EVENT` for `object` and report which handler names ran. */ +async function dispatch(engine: ObjectQL, object: string): Promise { + const fired: string[] = []; + (engine as any)._firedSink = fired; + await engine.triggerHooks(EVENT, { object, event: EVENT, input: {} } as any); + return fired; +} + +/** Register a handler that records `name` into the engine's dispatch sink. */ +function register( + engine: ObjectQL, + name: string, + options: { object?: string | string[]; excludeObjects?: string | string[] }, +) { + engine.registerHook( + EVENT, + () => { + ((engine as any)._firedSink as string[]).push(name); + }, + options, + ); +} + +const gateOpen = (engine: ObjectQL, object: string): boolean => + (engine as any).hasHooksFor(EVENT, object); + +describe('[#5928] hook `excludeObjects` — matching semantics', () => { + it('global minus a list: fires on unlisted objects, not on excluded ones', async () => { + const engine = makeEngine(); + register(engine, 'audit', { excludeObjects: ['sys_audit_log', 'sys_job'] }); + + expect(await dispatch(engine, 'account')).toEqual(['audit']); + expect(await dispatch(engine, 'sys_audit_log')).toEqual([]); + expect(await dispatch(engine, 'sys_job')).toEqual([]); + }); + + it('explicit wildcard minus a list behaves the same as the implicit global', async () => { + const engine = makeEngine(); + register(engine, 'starred', { object: '*', excludeObjects: ['sys_audit_log'] }); + + expect(await dispatch(engine, 'account')).toEqual(['starred']); + expect(await dispatch(engine, 'sys_audit_log')).toEqual([]); + }); + + it('subtracts from a finite allow list too, not only from the global set', async () => { + const engine = makeEngine(); + register(engine, 'pair', { + object: ['account', 'contact'], + excludeObjects: ['contact'], + }); + + expect(await dispatch(engine, 'account')).toEqual(['pair']); + expect(await dispatch(engine, 'contact')).toEqual([]); + // Outside the allow list: the allow face already refused it. + expect(await dispatch(engine, 'lead')).toEqual([]); + }); + + it('accepts the bare-string spelling on both faces', async () => { + const engine = makeEngine(); + register(engine, 'single', { object: '*', excludeObjects: 'sys_audit_log' }); + + expect(await dispatch(engine, 'account')).toEqual(['single']); + expect(await dispatch(engine, 'sys_audit_log')).toEqual([]); + }); + + it('leaves the pre-existing allow-only registrations untouched', async () => { + const engine = makeEngine(); + register(engine, 'global', {}); + register(engine, 'allowOne', { object: 'account' }); + register(engine, 'allowMany', { object: ['account', 'contact'] }); + register(engine, 'wildcard', { object: '*' }); + + expect((await dispatch(engine, 'account')).sort()) + .toEqual(['allowMany', 'allowOne', 'global', 'wildcard']); + expect((await dispatch(engine, 'lead')).sort()).toEqual(['global', 'wildcard']); + }); + + it('`excludeObjects: []` is a no-op — the honest spelling of "subtract nothing"', async () => { + const engine = makeEngine(); + register(engine, 'spread', { excludeObjects: [] }); + + // The natural value of `excludeObjects: [...SKIP_OBJECTS]` when the source + // list is empty. Nothing transforms it, so it means what it says — unlike + // #4281's `object: []`, which the binder widened into the wildcard. + expect(await dispatch(engine, 'account')).toEqual(['spread']); + expect(await dispatch(engine, 'sys_audit_log')).toEqual(['spread']); + }); +}); + +describe('[#5928] probe D — an object registered after the hook was', () => { + /* + * The decisive comparison. `late_object` stands for an object a `/meta` PUT + * registers into a live engine after the plugin's `init()` ran — the case that + * ruled out enumerating the complement. + */ + it('is covered by default under an exclusion face', async () => { + const engine = makeEngine(); + register(engine, 'audit', { excludeObjects: ['sys_audit_log'] }); + + expect(await dispatch(engine, 'late_object')).toEqual(['audit']); + expect(gateOpen(engine, 'late_object')).toBe(true); + }); + + it('is silently NOT covered under an enumerated allow list', async () => { + const engine = makeEngine(); + // The same intent expressed as the complement, frozen at registration time. + register(engine, 'audit', { object: ['account', 'contact'] }); + + expect(await dispatch(engine, 'late_object')).toEqual([]); + expect(gateOpen(engine, 'late_object')).toBe(false); + }); +}); + +describe('[#5928] refused exclusion faces (#4281 / ADR-0078 lineage)', () => { + it('refuses the empty string — it subtracts nothing while reading as if it did', () => { + const engine = makeEngine(); + expect(() => engine.registerHook(EVENT, () => {}, { excludeObjects: '' })) + .toThrow(/empty `excludeObjects` entry/); + }); + + it("refuses `['']`, including as one member of an otherwise valid list", () => { + const engine = makeEngine(); + expect(() => engine.registerHook(EVENT, () => {}, { excludeObjects: [''] })) + .toThrow(/empty `excludeObjects` entry/); + expect(() => engine.registerHook(EVENT, () => {}, { excludeObjects: ['sys_job', ' '] })) + .toThrow(/empty `excludeObjects` entry/); + }); + + it("refuses `'*'` — subtracting every object leaves a hook that can never fire", () => { + const engine = makeEngine(); + expect(() => engine.registerHook(EVENT, () => {}, { excludeObjects: '*' })) + .toThrow(/can never fire/); + expect(() => engine.registerHook(EVENT, () => {}, { excludeObjects: ['sys_job', '*'] })) + .toThrow(/ADR-0078/); + }); + + it('registers nothing when it refuses', async () => { + const engine = makeEngine(); + expect(() => engine.registerHook(EVENT, () => {}, { excludeObjects: '*' })).toThrow(); + + // A refusal that still pushed the entry would leave exactly the inert + // registration the refusal exists to prevent. + expect(await dispatch(engine, 'account')).toEqual([]); + expect(gateOpen(engine, 'account')).toBe(false); + }); + + it('names the fix in the message rather than only the fault', () => { + const engine = makeEngine(); + const empty = (() => { + try { engine.registerHook(EVENT, () => {}, { excludeObjects: '' }); return ''; } + catch (e) { return (e as Error).message; } + })(); + expect(empty).toContain("excludeObjects: ['sys_audit_log']"); + expect(empty).toContain('an empty array is accepted'); + }); +}); + +describe('[#5928] registration log reports the exclusion face', () => { + it("carries `excludeObjects` in the 'Registered hook' debug record", () => { + const logger = makeLogger(); + const engine = new ObjectQL({ logger }); + engine.registerHook(EVENT, () => {}, { excludeObjects: ['sys_audit_log', 'sys_job'] }); + + const record = logger.debug.mock.calls.find((c) => c[0] === 'Registered hook'); + expect(record).toBeDefined(); + // A log printing only `object` would describe this entry as a plain global + // hook, which is precisely what it is not. + expect(record![1]).toMatchObject({ + event: EVENT, + excludeObjects: ['sys_audit_log', 'sys_job'], + }); + }); +}); + +/* + * ── The property: the gate is never tighter than the dispatch ─────────────── + * + * `hasHooksFor` gates a READ of the whole matched row set on the bulk write + * path (#5038, #5284). The asymmetry its comment records is the reason this is + * a property test and not a spot check: + * + * gate LOOSER than dispatch → a wasted query. Cost. + * gate TIGHTER than dispatch → hooks that were going to fire are silently + * dropped. Correctness, and silent. + * + * So the assertion is the implication `dispatched ⇒ gate open`, over every + * combination of the two scope faces — never an equality, which would forbid + * the legitimate looseness (`hasHooksFor` deliberately ignores + * `skipAutomations`). + */ +describe('[#5928] property: hasHooksFor is never tighter than triggerHooks', () => { + const SCOPES: Array<{ object?: string | string[]; excludeObjects?: string | string[] }> = [ + {}, + { object: '*' }, + { object: 'account' }, + { object: ['account', 'contact'] }, + { excludeObjects: 'account' }, + { excludeObjects: ['account', 'sys_job'] }, + { object: '*', excludeObjects: 'account' }, + { object: '*', excludeObjects: ['account', 'sys_job'] }, + { object: ['account', 'contact'], excludeObjects: 'contact' }, + { object: ['account', 'contact'], excludeObjects: ['account', 'contact'] }, + { object: 'account', excludeObjects: 'lead' }, + { excludeObjects: [] }, + ]; + const OBJECTS = ['account', 'contact', 'lead', 'sys_job', 'late_object']; + + it('holds for every single-entry scope × object pair', async () => { + for (const scope of SCOPES) { + for (const object of OBJECTS) { + const engine = makeEngine(); + register(engine, 'h', scope); + + const dispatched = (await dispatch(engine, object)).length > 0; + const open = gateOpen(engine, object); + + if (dispatched && !open) { + throw new Error( + `gate TIGHTER than dispatch for ${JSON.stringify(scope)} on '${object}': ` + + 'a hook fired that hasHooksFor said could not', + ); + } + // With one shared matcher and a single entry the two agree exactly; + // asserting that here would catch a drift the implication tolerates. + expect(open).toBe(dispatched); + } + } + }); + + /* + * Realism, not detection power. Measured against a deliberately broken build + * (gate applies the exclusion, dispatch reads the allow face only) the + * single-entry matrix above fails with the offending scope named, while THIS + * case still passes: once a global entry is in the set, the gate is open for + * every object, so no violation can surface. Kept because coexisting + * registrations are the real deployment shape — just do not read it as the + * assertion with teeth. + */ + it('holds for the whole scope set registered at once', async () => { + const engine = makeEngine(); + SCOPES.forEach((scope, i) => register(engine, `h${i}`, scope)); + + for (const object of OBJECTS) { + const dispatched = (await dispatch(engine, object)).length > 0; + expect(gateOpen(engine, object) || !dispatched).toBe(true); + } + }); + + it('holds when `skipAutomations` suppresses metadata-bound entries', async () => { + // The one legal direction of disagreement: the gate stays open while the + // dispatch suppresses. Loose, never tight. + const engine = makeEngine(); + engine.registerHook(EVENT, () => { throw new Error('must not run'); }, { + excludeObjects: ['sys_job'], + meta: { name: 'metadata_bound' }, + }); + + await engine.triggerHooks(EVENT, { + object: 'account', + event: EVENT, + input: {}, + session: { skipAutomations: true }, + } as any); + + expect(gateOpen(engine, 'account')).toBe(true); + expect(gateOpen(engine, 'sys_job')).toBe(false); + }); +}); + +describe('[#5928] hookMatchesObject — the one shared rule', () => { + it('is the function both consumers read', () => { + // Guards against a future edit re-introducing a hand-written copy: if this + // rule ever disagrees with either consumer, the property tests above fail. + const entry: Pick = { + object: '*', + excludeObjects: ['sys_job'], + }; + expect(hookMatchesObject(entry, 'account')).toBe(true); + expect(hookMatchesObject(entry, 'sys_job')).toBe(false); + expect(hookMatchesObject({}, 'anything')).toBe(true); + expect(hookMatchesObject({ object: [] }, 'account')).toBe(false); + }); + + it('preserves the pre-existing `object: \'\'` reading unchanged (out of scope)', async () => { + // Both replaced copies tested `object` for TRUTHINESS, so `''` registers a + // GLOBAL hook — #4281's "blank intent becomes the broadest blast radius", + // surviving on the code path that ruling did not cover (it closed the + // authorable schema and the binder). Flipping it inside this PR would + // silently convert a fires-on-everything hook into a fires-on-nothing one. + // Pinned as UNCHANGED, not as correct; filed separately. + expect(hookMatchesObject({ object: '' }, 'account')).toBe(true); + + const engine = makeEngine(); + register(engine, 'blank', { object: '' }); + expect(await dispatch(engine, 'account')).toEqual(['blank']); + expect(gateOpen(engine, 'account')).toBe(true); + }); +}); diff --git a/packages/spec/src/contracts/objectql-engine-hook-scope.test.ts b/packages/spec/src/contracts/objectql-engine-hook-scope.test.ts new file mode 100644 index 0000000000..4408385576 --- /dev/null +++ b/packages/spec/src/contracts/objectql-engine-hook-scope.test.ts @@ -0,0 +1,97 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5928] The `objectql` slot's hook-registration contract carries BOTH scope + * faces. + * + * `IObjectQLEngine.registerHook` is the declaration consumers outside + * `packages/objectql` program against — it is how a plugin knows what it may + * pass without reaching for `any`. Until #5928 it declared one face, `object`, + * an allow list; "global, except these objects" had no spelling, which is what + * blocked the audit plugin from moving its skip list out of the handler and onto + * the registration where the #5038 / #5284 per-object gates can read it. + * + * The declaration and the engine that implements it move in ONE PR on purpose: + * shipping this half alone would leave `packages/spec` advertising an option the + * engine ignores — a declared-but-unenforced window, the failure this repo keeps + * paying to close. + * + * The behaviour itself (`matches = allowMatches && !excludeMatches`, the refused + * shapes, the gate-vs-dispatch property) is pinned in + * `packages/objectql/src/hook-exclude-objects.test.ts`: only objectql can + * execute a dispatch, and spec must not depend on it. What is assertable HERE is + * the shape of the contract, so that is all this file claims. + */ + +import { describe, it, expect } from 'vitest'; +import type { IObjectQLEngine } from './objectql-engine'; + +/** + * Exact type equality. Written as a VALUE assertion below (`const x: Equals<…> = + * true`) rather than the bare `type _X = Expect<…>` alias the older contract + * tests use: an unused type alias is a `noUnusedLocals` error under + * `tsconfig.test.json`, which is why those files carry measured entries in + * `test-typecheck-debt.json`. That ledger is shrink-only, so a new file earns + * its way in by having no errors at all. + */ +type Equals = (() => G extends A ? 1 : 2) extends (() => G extends B ? 1 : 2) ? true : false; + +type RegisterHookOptions = NonNullable[2]>; + +describe('[#5928] IObjectQLEngine.registerHook scope faces', () => { + it('declares `excludeObjects` in both accepted spellings', () => { + // Fails to COMPILE if the declared type is anything else — `true` is not + // assignable to `false`. + const declared: Equals< + RegisterHookOptions['excludeObjects'], string | string[] | undefined + > = true; + // Mirrors `object` exactly — one vocabulary for both faces, so a caller + // never has to remember that one takes a list and the other a single name. + const mirrorsAllowFace: Equals< + RegisterHookOptions['excludeObjects'], RegisterHookOptions['object'] + > = true; + + expect(declared).toBe(true); + expect(mirrorsAllowFace).toBe(true); + }); + + it('keeps the option optional — an allow-only registration stays legal', () => { + // Every registration that compiled before #5928 still compiles: the face is + // additive, and absent means "subtract nothing". + const allowOnly: RegisterHookOptions = { object: 'account', priority: 50 }; + const global: RegisterHookOptions = {}; + expect(allowOnly.excludeObjects).toBeUndefined(); + expect(global.excludeObjects).toBeUndefined(); + }); + + it('admits the shape the audit plugin needs — global minus a static list', () => { + // The registration #5860 is blocked on: no `object`, so global, minus the + // platform tables the writer must not recurse into. + const auditScope: RegisterHookOptions = { + excludeObjects: ['sys_audit_log', 'sys_job', 'sys_metadata'], + packageId: 'plugin-audit', + }; + expect(auditScope.excludeObjects).toHaveLength(3); + + // …and the explicit-wildcard spelling of the same intent. + const starred: RegisterHookOptions = { object: '*', excludeObjects: 'sys_audit_log' }; + expect(starred.excludeObjects).toBe('sys_audit_log'); + }); + + it('is satisfiable by an implementation typed against the contract', () => { + // A structural check that the widened options do not break implementors: + // this is how a host or a test double declares the member. + const calls: Array<{ event: string; excludeObjects?: string | string[] }> = []; + const registerHook: IObjectQLEngine['registerHook'] = (event, _handler, options) => { + calls.push({ event, excludeObjects: options?.excludeObjects }); + }; + + registerHook('afterUpdate', () => {}, { excludeObjects: ['sys_audit_log'] }); + registerHook('afterInsert', () => {}); + + expect(calls).toEqual([ + { event: 'afterUpdate', excludeObjects: ['sys_audit_log'] }, + { event: 'afterInsert', excludeObjects: undefined }, + ]); + }); +}); diff --git a/packages/spec/src/contracts/objectql-engine.ts b/packages/spec/src/contracts/objectql-engine.ts index 928c1de04d..67c2319ce6 100644 --- a/packages/spec/src/contracts/objectql-engine.ts +++ b/packages/spec/src/contracts/objectql-engine.ts @@ -184,10 +184,37 @@ export interface IObjectQLEngine extends IDataEngine { executeAction(objectName: string, actionName: string, ctx: any): Promise; // ── Hook / middleware seams ────────────────────────────────────────── + /** + * Register a code-path hook. + * + * `object` is an ALLOW list (absent = global, `'*'` = every object); + * `excludeObjects` subtracts from whatever that admits, so the scope a + * registration expresses is + * `matches(entry, X) = allowMatches(entry, X) && !excludeMatches(entry, X)`. + * + * The subtraction half exists because an allow list cannot express "global, + * except these" over an OPEN universe (#5928): `/meta` PUT registers new + * objects into a live engine, so a registrant that enumerated the complement + * of its skip list would silently stop covering every object created after + * boot — the compliance-relevant direction of that failure is what ruled the + * enumerate-the-complement option out. Declared here rather than left to a + * predicate callback so the scope stays static, printable in the + * registration log, and introspectable. + * + * NOT mirrored onto the authorable `HookSchema` (`data/hook.zod.ts`): the + * consumer is plugin code, and no metadata author needs "global minus a + * list" today. + */ registerHook( event: string, handler: (context: any) => Promise | void, - options?: { object?: string | string[]; priority?: number; packageId?: string }, + options?: { + object?: string | string[]; + /** Object name(s) subtracted from `object`'s admitted set (#5928). */ + excludeObjects?: string | string[]; + priority?: number; + packageId?: string; + }, ): void; unregisterHooksByPackage(packageId: string): number; /**